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