mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't ignore locktable-full failures in StandbyAcquireAccessExclusiveLock.

Browse Source 
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
This commit is contained in:
Tom Lane 2018-09-19 12:43:51 -04:00
parent df8b5f3eb8
commit c84f83975e
2 changed files with 8 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/backend/storage/ipc/standby.c
View File

@ -661,8 +661,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid); SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid);
(void) LockAcquireExtended(&locktag, AccessExclusiveLock, true, false, (void) LockAcquire(&locktag, AccessExclusiveLock, true, false);
false, NULL);
} }
static void static void

12
src/backend/storage/lmgr/lock.c
View File

@ -693,11 +693,13 @@ LockAcquire(const LOCKTAG *locktag,
/* /*
* LockAcquireExtended - allows us to specify additional options * LockAcquireExtended - allows us to specify additional options
* *
* reportMemoryError specifies whether a lock request that fills the * reportMemoryError specifies whether a lock request that fills the lock
* lock table should generate an ERROR or not. This allows a priority * table should generate an ERROR or not. Passing "false" allows the caller
* caller to note that the lock table is full and then begin taking * to attempt to recover from lock-table-full situations, perhaps by forcibly
* extreme action to reduce the number of other lock holders before * cancelling other lock holders and then retrying. Note, however, that the
* retrying the action. * 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 * If locallockp isn't NULL, *locallockp receives a pointer to the LOCALLOCK
* table entry if a lock is successfully acquired, or NULL if not. * table entry if a lock is successfully acquired, or NULL if not.