diff options
author | Tom Lane | 2018-09-19 16:43:51 +0000 |
---|---|---|
committer | Tom Lane | 2018-09-19 16:43:51 +0000 |
commit | 265ac024572b639eb315eab1b68faf45bcc8f4f2 (patch) | |
tree | 4f358df99aa73ee4b5b3bd5c34d27d1a12364bd4 | |
parent | f547035a0e1bfb338cd2113976d7f384854a64e3 (diff) |
Don't ignore locktable-full failures in StandbyAcquireAccessExclusiveLock.
Commit 37c54863c removed the code in StandbyAcquireAccessExclusiveLock
that checked the return value of LockAcquireExtended. That created a
bug, because it's still passing reportMemoryError = false to
LockAcquireExtended, meaning that LOCKACQUIRE_NOT_AVAIL will be returned
if we're out of shared memory for the lock table.
In such a situation, the startup process would believe it had acquired an
exclusive lock even though it hadn't, with potentially dire consequences.
To fix, just drop the use of reportMemoryError = false, which allows us
to simplify the call into a plain LockAcquire(). It's unclear that the
locktable-full situation arises often enough that it's worth having a
better recovery method than crash-and-restart. (I strongly suspect that
the only reason the code path existed at all was that it was relatively
simple to do in the pre-37c54863c implementation. But now it's not.)
LockAcquireExtended's reportMemoryError parameter is now dead code and
could be removed. I refrained from doing so, however, because there
was some interest in resurrecting the behavior if we do get reports of
locktable-full failures in the field. Also, it seems unwise to remove
the parameter concurrently with shipping commit f868a8143, which added a
parameter; if there are any third-party callers of LockAcquireExtended,
we want them to get a wrong-number-of-parameters compile error rather
than a possibly-silent misinterpretation of its last parameter.
Back-patch to 9.6 where the bug was introduced.
Discussion: https://postgr.es/m/6202.1536359835@sss.pgh.pa.us
-rw-r--r-- | src/backend/storage/ipc/standby.c | 3 | ||||
-rw-r--r-- | src/backend/storage/lmgr/lock.c | 12 |
2 files changed, 8 insertions, 7 deletions
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index f50c6883068..5073c1cc308 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -664,8 +664,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid) SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid); - (void) LockAcquireExtended(&locktag, AccessExclusiveLock, true, false, - false, NULL); + (void) LockAcquire(&locktag, AccessExclusiveLock, true, false); } static void diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index f7e947af936..98a2f366b4d 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -693,11 +693,13 @@ LockAcquire(const LOCKTAG *locktag, /* * LockAcquireExtended - allows us to specify additional options * - * reportMemoryError specifies whether a lock request that fills the - * lock table should generate an ERROR or not. This allows a priority - * caller to note that the lock table is full and then begin taking - * extreme action to reduce the number of other lock holders before - * retrying the action. + * reportMemoryError specifies whether a lock request that fills the lock + * table should generate an ERROR or not. Passing "false" allows the caller + * to attempt to recover from lock-table-full situations, perhaps by forcibly + * cancelling other lock holders and then retrying. Note, however, that the + * return code for that is LOCKACQUIRE_NOT_AVAIL, so that it's unsafe to use + * in combination with dontWait = true, as the cause of failure couldn't be + * distinguished. * * If locallockp isn't NULL, *locallockp receives a pointer to the LOCALLOCK * table entry if a lock is successfully acquired, or NULL if not. |