On Wed, Mar 11, 2020 at 2:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 6:39 PM Robert Haas <robertmh...@gmail.com> wrote:
> >
> > On Fri, Mar 6, 2020 at 11:27 PM Dilip Kumar <dilipbal...@gmail.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.
I have just written a WIP patch for relation extension lock where
instead of incrementing and decrementing the counter in
LockRelationForExtension and UnlockRelationForExtension respectively.
We can just set and reset the flag in LockAcquireExtended and
LockRelease. So this patch appears simple to me as we are not
involving the transaction APIs to set and reset the flag. However, we
need to add an extra check as you have already mentioned. I think we
could measure the performance and see whether it has any impact or
not?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From e49e483646f14a2e626190d5ef98f628668d025c Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Thu, 12 Mar 2020 13:16:45 +0530
Subject: [PATCH v5] WIP-Add assert check for relation extension lock
Add assert to check that 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
same lock.
---
src/backend/storage/lmgr/lock.c | 43 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 56dba09..f572dab 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -170,6 +170,15 @@ typedef struct TwoPhaseLockRecord
*/
static int FastPathLocalUseCount = 0;
+/*
+ * Flag is set if the relation extension lock is currently held by this backend.
+ * We need this flag so that we can ensure that while holding the relation
+ * extension lock we are not trying to acquire any other heavy weight lock.
+ * Basically, that will ensuring that the proc holding relation extension lock
+ * can not wait for any another lock which can lead to a deadlock.
+ */
+static bool IsRelationExtensionLockHeld = false;
+
/* Macros for manipulating proc->fpLockBits */
#define FAST_PATH_BITS_PER_SLOT 3
#define FAST_PATH_LOCKNUMBER_OFFSET 1
@@ -841,6 +850,14 @@ LockAcquireExtended(const LOCKTAG *locktag,
}
/*
+ * We should not try to acquire any other heavyweight lock if we are already
+ * holding the relation extension lock. If we are trying to hold the same
+ * relation extension lock then it should have been already granted so we
+ * will not come here.
+ */
+ Assert(!IsRelationExtensionLockHeld);
+
+ /*
* Prepare to emit a WAL record if acquisition of this lock needs to be
* replayed in a standby server.
*
@@ -900,6 +917,11 @@ LockAcquireExtended(const LOCKTAG *locktag,
locallock->lock = NULL;
locallock->proclock = NULL;
GrantLockLocal(locallock, owner);
+
+ /* Set the flag that we acquired the relation extension lock. */
+ if (locktag->locktag_type == LOCKTAG_RELATION_EXTEND)
+ IsRelationExtensionLockHeld = true;
+
return LOCKACQUIRE_OK;
}
}
@@ -1100,6 +1122,10 @@ LockAcquireExtended(const LOCKTAG *locktag,
locktag->locktag_field2);
}
+ /* Set the flag that we acquired the relation extension lock. */
+ if (locktag->locktag_type == LOCKTAG_RELATION_EXTEND)
+ IsRelationExtensionLockHeld = true;
+
return LOCKACQUIRE_OK;
}
@@ -1999,6 +2025,13 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
if (released)
{
RemoveLocalLock(locallock);
+
+ /*
+ * Reset the flag if we have released the relation extension lock.
+ */
+ if (locktag->locktag_type == LOCKTAG_RELATION_EXTEND)
+ IsRelationExtensionLockHeld = false;
+
return true;
}
}
@@ -2072,6 +2105,10 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
LWLockRelease(partitionLock);
RemoveLocalLock(locallock);
+
+ /* Reset the flag if we have released the relation extension lock. */
+ if (locktag->locktag_type == LOCKTAG_RELATION_EXTEND)
+ IsRelationExtensionLockHeld = false;
return true;
}
@@ -2347,6 +2384,12 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks)
LWLockRelease(partitionLock);
} /* loop over partitions */
+ /*
+ * We have released all the lock so reset the relation extension lock held
+ * flag.
+ */
+ IsRelationExtensionLockHeld = false;
+
#ifdef LOCK_DEBUG
if (*(lockMethodTable->trace_flag))
elog(LOG, "LockReleaseAll done");
--
1.8.3.1
