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