Restore lock level to update statusFlags

Reverts 27838981be9d (some comments are kept).  Per discussion, it does
not seem safe to relax the lock level used for this; in order for it to
be safe, there would have to be memory barriers between the point we set
the flag and the point we set the trasaction Xid, which perhaps would
not be so bad; but there would also have to be barriers at the readers'
side, which from a performance perspective might be bad.

Now maybe this analysis is wrong and it *is* safe for some reason, but
proof of that is not trivial.

Discussion: https://postgr.es/m/20201118190928[email protected]

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 395e75f768fd6c631d05d322a6c511a281ddecbb..f1112111de86d0bf5c5c8ed4b0903265524524e9 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1712,13 +1712,6 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
    /* Begin a transaction for vacuuming this relation */
    StartTransactionCommand();
 
-   /*
-    * Need to acquire a snapshot to prevent pg_subtrans from being truncated,
-    * cutoff xids in local memory wrapping around, and to have updated xmin
-    * horizons.
-    */
-   PushActiveSnapshot(GetTransactionSnapshot());
-
    if (!(params->options & VACOPT_FULL))
    {
        /*
@@ -1739,9 +1732,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
         * Note: these flags remain set until CommitTransaction or
         * AbortTransaction.  We don't want to clear them until we reset
         * MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId()
-        * might appear to go backwards, which is probably Not Good.
+        * might appear to go backwards, which is probably Not Good.  (We also
+        * set PROC_IN_VACUUM *before* taking our own snapshot, so that our
+        * xmin doesn't become visible ahead of setting the flag.)
         */
-       LWLockAcquire(ProcArrayLock, LW_SHARED);
+       LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
        MyProc->statusFlags |= PROC_IN_VACUUM;
        if (params->is_wraparound)
            MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
@@ -1749,6 +1744,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
        LWLockRelease(ProcArrayLock);
    }
 
+   /*
+    * Need to acquire a snapshot to prevent pg_subtrans from being truncated,
+    * cutoff xids in local memory wrapping around, and to have updated xmin
+    * horizons.
+    */
+   PushActiveSnapshot(GetTransactionSnapshot());
+
    /*
     * Check for user-requested abort.  Note we want this to be inside a
     * transaction, so xact.c doesn't issue useless WARNING.

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 4324e3265657300b4919ea6b5d3c7a4702d4414b..f1f4df7d70f286ad4d4a06efda0a82fbd133a7a8 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_SHARED);
+       LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
        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 94edb24b22d33f694b20712a9455f23312664d50..ee912b9d5e40d1bf0e9d71b2ed965ebaa523d02d 100644 (file)
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -662,10 +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);
+           LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
            Assert(proc->statusFlags == ProcGlobal->statusFlags[proc->pgxactoff]);
            proc->statusFlags &= ~PROC_VACUUM_STATE_MASK;
            ProcGlobal->statusFlags[proc->pgxactoff] = proc->statusFlags;
@@ -685,8 +683,8 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
    size_t      pgxactoff = proc->pgxactoff;
 
    /*
-    * Note: we need exclusive lock here because we're going to
-    * change other processes' PGPROC entries.
+    * 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]));

diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e75f6e8178254b1f7d637c8e4d7d59fb439a8318..e77f76ae8a18ab240cc898e1fe496a015cbbe8e0 100644 (file)
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -102,9 +102,9 @@ typedef enum
  * 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.
+ * delayChkpt and isBackgroundWorker. 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:
  *