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

On Tue, Mar 10, 2020 at 8:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> Please find the updated patch (summary of the changes)
> - Instead of searching the lock hash table for assert, it maintains a counter.
> - Also, handled the case where we can acquire the relation extension
> lock while holding the relation extension lock on the same relation.
> - Handled the error case.
>
> In addition to that prepared a WIP patch for handling the PageLock.
> First, I thought that we can use the same counter for the PageLock and
> the RelationExtensionLock because in assert we just need to check
> whether we are trying to acquire any other heavyweight lock while
> holding any of these locks.  But, the exceptional case where we
> allowed to acquire a relation extension lock while holding any of
> these locks is a bit different.  Because, if we are holding a relation
> extension lock then we allowed to acquire the relation extension lock
> on the same relation but it can not be any other relation otherwise it
> can create a cycle.  But, the same is not true with the PageLock,
> i.e. while holding the PageLock you can acquire the relation extension
> lock on any relation and that will be safe because the relation
> extension lock guarantee that, it will never create the cycle.
> However, I agree that we don't have any such cases where we want to
> acquire a relation extension lock on the different relations while
> holding the PageLock.
>

Right, today, we don't have such cases where after acquiring relation
extension or page lock for a particular relation, we need to acquire
any of those for other relation and I am not able to offhand think of
many cases where we might have such a need in the future.  The one
theoretical possibility is to include fork_num in the lock tag while
acquiring extension lock for fsm/vm, but that will also have the same
relation.  Similarly one might say it is valid to acquire extension
lock in share mode after we have acquired it exclusive mode.  I am not
sure how much futuristic we want to make these Asserts.

I feel we should cover the current possible cases (which I think will
make the asserts more strict then required) and if there is a need to
relax them in the future for any particular use case, then we will
consider those.  In general, if we consider the way Mahendra has
written a patch which is to find the entry via the local hash table to
check for an Assert condition, then it will be a bit easier to extend
the checks if required in future as that way we have more information
about the particular lock. However, it will make the check more
expensive which might be okay considering that it is only for Assert
enabled builds.

One minor comment:
/*
+ * We should not acquire any other lock if we are already holding the
+ * relation extension lock.  Only exception is that if we are trying to
+ * acquire the relation extension lock then we can hold the relation
+ * extension on the same relation.
+ */
+ Assert(!IsRelExtLockHeld() ||
+    ((locktag->locktag_type == LOCKTAG_RELATION_EXTEND) && found));

I think you don't need the second part of the check because if we have
found the lock in the local lock table, we would return before this
check.  I think it will catch the case where if we have an extension
lock on one relation, then it won't allow us to acquire it on another
relation. OTOH, it will also not allow cases where backend has
relation extension lock in Exclusive mode and it tries to acquire it
in Shared mode. So, not sure if it is a good idea.

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