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

On Wed, Mar 11, 2020 at 2:23 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Mar 10, 2020 at 8:53 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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 have fixed this in the attached patch set.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com