Relax lock level for setting PGPROC->statusFlags

alvherre · alvherre · commit 27838981be9d · 2020-11-18T13:24:22.000-03:00
We don't actually need a lock to set PGPROC->statusFlags itself; what we do need is a shared lock on either XidGenLock or ProcArrayLock in order to ensure MyProc->pgxactoff keeps still while we modify the mirror array in ProcGlobal->statusFlags. Some places were using an exclusive lock for that, which is excessive. Relax those to use shared lock only. procarray.c has a couple of places with somewhat brittle assumptions about PGPROC changes: ProcArrayEndTransaction uses only shared lock, so it's permissible to change MyProc only. On the other hand, ProcArrayEndTransactionInternal also changes other procs, so it must hold exclusive lock. Add asserts to ensure those assumptions continue to hold. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/20201117155501.GA13805@alvherre.pgsql
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
@@ -1741,7 +1741,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 		 * MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId()
 		 * might appear to go backwards, which is probably Not Good.
 		 */
-		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+		LWLockAcquire(ProcArrayLock, LW_SHARED);
 		MyProc->statusFlags |= PROC_IN_VACUUM;
 		if (params->is_wraparound)
 			MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
@@ -181,7 +181,7 @@ StartupDecodingContext(List *output_plugin_options,
 	 */
 	if (!IsTransactionOrTransactionBlock())
 	{
-		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+		LWLockAcquire(ProcArrayLock, LW_SHARED);
 		MyProc->statusFlags |= PROC_IN_LOGICAL_DECODING;
 		ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 		LWLockRelease(ProcArrayLock);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
@@ -527,7 +527,7 @@ ReplicationSlotRelease(void)
 	MyReplicationSlot = NULL;
 
 	/* might not have been set when we've been a plain slot */
-	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	LWLockAcquire(ProcArrayLock, LW_SHARED);
 	MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING;
 	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 	LWLockRelease(ProcArrayLock);
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
@@ -662,6 +662,8 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
 		/* avoid unnecessarily dirtying shared cachelines */
 		if (proc->statusFlags & PROC_VACUUM_STATE_MASK)
 		{
+			/* Only safe to change my own flags with just share lock */
+			Assert(proc == MyProc);
 			Assert(!LWLockHeldByMe(ProcArrayLock));
 			LWLockAcquire(ProcArrayLock, LW_SHARED);
 			Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]);
@@ -682,7 +684,11 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
 {
 	size_t		pgxactoff = proc->pgxactoff;
 
-	Assert(LWLockHeldByMe(ProcArrayLock));
+	/*
+	 * Note: we need exclusive lock here because we're going to
+	 * change other processes' PGPROC entries.
+	 */
+	Assert(LWLockHeldByMeInMode(ProcArrayLock, LW_EXCLUSIVE));
 	Assert(TransactionIdIsValid(ProcGlobal->xids[pgxactoff]));
 	Assert(ProcGlobal->xids[pgxactoff] == proc->xid);
 
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
@@ -623,7 +623,7 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
 					 * because that flag is set at process start and never
 					 * reset.  There is logic elsewhere to avoid canceling an
 					 * autovacuum that is working to prevent XID wraparound
-					 * problems (which needs to read a different vacuumFlag
+					 * problems (which needs to read a different statusFlags
 					 * bit), but we don't do that here to avoid grabbing
 					 * ProcArrayLock.
 					 */
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
@@ -98,6 +98,11 @@ typedef enum
  * The semaphore and lock-activity fields in a prepared-xact PGPROC are unused,
  * but its myProcLocks[] lists are valid.
  *
+ * We allow many fields of this struct to be accessed without locks, such as
+ * statusFlags or delayChkpt. However, keep in mind that writing mirrored ones
+ * (see below) requires holding ProcArrayLock or XidGenLock in at least shared
+ * mode, so that pgxactoff does not change concurrently.
+ *
  * Mirrored fields:
  *
  * Some fields in PGPROC (see "mirrored in ..." comment) are mirrored into an

Original file line number	Diff line number	Diff line change
`@@ -181,7 +181,7 @@ StartupDecodingContext(List *output_plugin_options,`
`181`	`181`	`*/`
`182`	`182`	`if (!IsTransactionOrTransactionBlock())`
`183`	`183`	`{`
`184`		`- LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);`
	`184`	`+ LWLockAcquire(ProcArrayLock, LW_SHARED);`
`185`	`185`	`MyProc->statusFlags \|= PROC_IN_LOGICAL_DECODING;`
`186`	`186`	`ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;`
`187`	`187`	`LWLockRelease(ProcArrayLock);`
Original file line number	Diff line number	Diff line change
`@@ -98,6 +98,11 @@ typedef enum`
`98`	`98`	`* The semaphore and lock-activity fields in a prepared-xact PGPROC are unused,`
`99`	`99`	`* but its myProcLocks[] lists are valid.`
`100`	`100`	`*`
	`101`	`+ * We allow many fields of this struct to be accessed without locks, such as`
	`102`	`+ * statusFlags or delayChkpt. However, keep in mind that writing mirrored ones`
	`103`	`+ * (see below) requires holding ProcArrayLock or XidGenLock in at least shared`
	`104`	`+ * mode, so that pgxactoff does not change concurrently.`
	`105`	`+ *`
`101`	`106`	`* Mirrored fields:`
`102`	`107`	`*`
`103`	`108`	`* Some fields in PGPROC (see "mirrored in ..." comment) are mirrored into an`