Re: [COMMITTERS] pgsql: Remove O(N^2) performance issue with multiple SAVEPOINTs.

On 19.07.2011 23:08, Simon Riggs wrote:
> On Tue, Jul 19, 2011 at 8:49 PM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com>  wrote:
>> On 19.07.2011 19:22, Simon Riggs wrote:
>>>
>>> Remove O(N^2) performance issue with multiple SAVEPOINTs.
>>> Subtransaction locks now released en masse at main commit, rather than
>>> repeatedly re-scanning for locks as we ascend the nested transaction tree.
>>> Split transaction state TBLOCK_SUBEND into two states, TBLOCK_SUBCOMMIT
>>> and TBLOCK_SUBRELEASE to allow the commit path to be optimised using
>>> the existing code in ResourceOwnerRelease() which appears to have been
>>> intended for this usage, judging from comments therein.
>>
>> CommitSubTransaction(true) does this:
>>
>> ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_LOCKS, true,
>> isTopLevel /* == true */);
>> ...
>> ResourceOwnerDelete(s->curTransactionOwner);
>>
>> Because isTopLevel is passed as true, ResourceOwnerRelease() doesn't release
>> or transfer the locks belonging to the resource owner. After the call, they
>> still point to s->curTransactionOwner. Then, the resource owner is deleted.
>> After those two calls, the locks still have pointers to the now-pfree'd
>> ResourceOwner object. Looking at lock.c, we apparently never dereference
>> LOCALLOCKOWNER.owner field. Nevertheless, a dangling pointer like that seems
>> like a recipe for trouble. After releasing all subtransactions, we still
>> fire deferred triggers, for example, which can do arbitrarily complex
>> things. For example, you might allocate new resource owners, which if you're
>> really unlucky might get allocated at the same address as the
>> already-pfree'd resource owner. I'm not sure what would happen then, but it
>> can't be good.
>>
>> Instead of leaving the locks dangling to an already-destroyed resource
>> owner, how about assigning all locks directly to the top-level resource
>> owner in one sweep? That'd still be much better than the old way of
>> recursively reassigning them up the subtransaction tree, one level at a
>> time.
>
> Yes, I did see what the code was doing.
>
> My feeling was the code was specifically written that way, just never
> used. So I wired it up to be used the way intended. Have a look at
> ResourceOwnerReleaseInternal()... not code I wrote or touched on this
> patch.

The way ResourceOwnerReleaseIntenal(isTopLevel==true) works in the case 
of a genuine top-level commit doesn't have this problem, because the 
sub-resource owners are not deleted until TopTransactionResourceOwner 
has been processed, and all the locks released. In fact, before this 
patch I think an "Assert(!isTopLevel || owner == 
TopTransactionResourceOwner)" would've be in order in 
ResourceOwnerRelease(). Or it could've done "bool isTopLevel = (owner == 
TopTransactionResoureOwner)" in the beginning instead of having 
isTopLevel as an argument.

> You might persuade me to do it another way, but I can't see how to
> make that way work. Your case seems a stretch.

You get coincidences with memory allocations surprisingly often, because 
things tend to get allocated and free'd in chunks of certain sizes. It's 
also pretty fragile in the face of future development. It's not hard to 
imagine someone adding code in lock.c to dereference the pointer.

> Not sure why you mention it now,>7 weeks after review.

Because I only just spotted it.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com