Relax lock level for setting PGPROC->statusFlags

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
index d89ba3031f9a834fe40a00e693c6a55b9a5d032b..395e75f768fd6c631d05d322a6c511a281ddecbb 100644 (file)
--- 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
index f1f4df7d70f286ad4d4a06efda0a82fbd133a7a8..4324e3265657300b4919ea6b5d3c7a4702d4414b 100644 (file)
--- 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
index 5ed955ba0bf4a1768c08e3888e53b6949c34f9db..9c7cf13d4d9c212f468aa9867fc498de2d128002 100644 (file)
--- 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
index e2cb6e990d1cc089b6f00d2d3031db7dacb977fd..94edb24b22d33f694b20712a9455f23312664d50 100644 (file)
--- 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
index cb7a8f0fd28cdbb01e7f3f033d94ca0a5e898bab..f7ed6968ca938bf5c172b617bef268610445334a 100644 (file)
--- 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
index 1067f58f51b52b9f4607d4b252d468975ca20312..00bb244340a3e35f188182e764bd94c76ee67ebf 100644 (file)
--- 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