On Wed, Mar 11, 2020 at 2:23 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Mar 10, 2020 at 8:53 AM Dilip Kumar <dilipbal...@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 have fixed this in the attached patch set. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
v4-0001-Add-assert-to-check-that-we-should-not-acquire-an.patch
Description: Binary data
v4-0003-Conflict-Extension-Page-lock-in-group-member.patch
Description: Binary data
v4-0002-WIP-Extend-the-patch-for-handling-PageLock.patch
Description: Binary data