Re: PATCH: optimized DROP of multiple tables within a transaction

On 20.12.2012 12:33, Andres Freund wrote:
> On 2012-12-20 02:54:48 +0100, Tomas Vondra wrote:
>> On 19.12.2012 02:18, Andres Freund wrote:
>>> On 2012-12-17 00:31:00 +0100, Tomas Vondra wrote:
>>>
>>> I think except of the temp buffer issue mentioned below its ready.
>>>
>>>> -DropRelFileNodeAllBuffers(RelFileNodeBackend rnode)
>>>> +DropRelFileNodeAllBuffers(RelFileNodeBackend * rnodes, int nnodes)
>>>>  {
>>>> -    int            i;
>>>> +    int         i, j;
>>>> +
>>>> +    /* sort the list of rnodes */
>>>> +    pg_qsort(rnodes, nnodes, sizeof(RelFileNodeBackend), rnode_comparator);
>>>>
>>>>      /* If it's a local relation, it's localbuf.c's problem. */
>>>> -    if (RelFileNodeBackendIsTemp(rnode))
>>>> +    for (i = 0; i < nnodes; i++)
>>>>      {
>>>> -        if (rnode.backend == MyBackendId)
>>>> -            DropRelFileNodeAllLocalBuffers(rnode.node);
>>>> -        return;
>>>> +        if (RelFileNodeBackendIsTemp(rnodes[i]))
>>>> +        {
>>>> +            if (rnodes[i].backend == MyBackendId)
>>>> +                DropRelFileNodeAllLocalBuffers(rnodes[i].node);
>>>> +        }
>>>>      }
>>>
>>> While you deal with local buffers here you don't anymore in the big loop
>>> over shared buffers. That wasn't needed earlier since we just returned
>>> after noticing we have local relation, but thats not the case anymore.
>>
>> Hmm, but that would require us to handle the temp relations explicitly
>> wherever we call DropRelFileNodeAllBuffers. Currently there are two such
>> places - smgrdounlink() and smgrdounlinkall().
>>
>> By placing it into DropRelFileNodeAllBuffers() this code is shared and I
>> think it's a good thing.
>>
>> But that does not mean the code is perfect - it was based on the
>> assumption that if there's a mix of temp and regular relations, the temp
>> relations will be handled in the first part and the rest in the second one.
>>
>> Maybe it'd be better to improve it so that the temp relations are
>> removed from the array after the first part (and skip the second one if
>> there are no remaining relations).
>
> The problem is that afaik without the backend-local part a temporary
> relation could match the same relfilenode as a "full" relation which
> would have pretty bad consequences.

Attached is a patch with fixed handling of temporary relations. I've
chosen to keep the logic in DropRelFileNodeAllBuffers and rather do a
local copy without the local relations.

regards
Tomas