Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

On Tue, Mar 10, 2020 at 6:39 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Mar 6, 2020 at 11:27 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > I think instead of the flag we need to keep the counter because we can
> > acquire the same relation extension lock multiple times. So
> > basically, every time we acquire the lock we can increment the counter
> > and while releasing we can decrement it. During an error path, I
> > think it is fine to set it to 0 in CommitTransaction/AbortTransaction.
> > But, I am not sure that we can set to 0 or decrement it in
> > AbortSubTransaction because we are not sure whether we have acquired
> > the lock under this subtransaction or not.
>
> I think that CommitTransaction, AbortTransaction, and friends have
> *zero* business touching this. I think the counter - or flag - should
> track whether we've got a PROCLOCK entry for a relation extension
> lock. We either do, or we do not, and that does not change because of
> anything have to do with the transaction state. It changes because
> somebody calls LockRelease() or LockReleaseAll().
>

Do we want to have a special check in the LockRelease() to identify
whether we are releasing relation extension lock? If not, then how we
will identify that relation extension is released and we can reset it
during subtransaction abort due to error? During success paths, we
know when we have released RelationExtension or Page Lock (via
UnlockRelationForExtension or UnlockPage). During the top-level
transaction end, we know when we have released all the locks, so that
will imply that RelationExtension and or Page locks must have been
released by that time.

If we have no other choice, then I see a few downsides of adding a
special check in the LockRelease() call:

1. Instead of resetting/decrement the variable from specific APIs like
UnlockRelationForExtension or UnlockPage, we need to have it in
LockRelease. It will also look odd, if set variable in
LockRelationForExtension, but don't reset in the
UnlockRelationForExtension variant. Now, maybe we can allow to reset
it at both places if it is a flag, but not if it is a counter
variable.

2. One can argue that adding extra instructions in a generic path
(like LockRelease) is not a good idea, especially if those are for an
Assert. I understand this won't add anything which we can measure by
standard benchmarks.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com