Fix race with synchronous_standby_names at startup

synchronous_standby_names cannot be reloaded safely by backends, and the
checkpointer is in charge of updating a state in shared memory if the
GUC is enabled in WalSndCtl, to let the backends know if they should
wait or not for a given LSN.  This provides a strict control on the
timing of the waiting queues if the GUC is enabled or disabled, then
reloaded.  The checkpointer is also in charge of waking up the backends
that could be waiting for a LSN when the GUC is disabled.

This logic had a race condition at startup, where it would be possible
for backends to not wait for a LSN even if synchronous_standby_names is
enabled.  This would cause visibility issues with transactions that we
should be waiting for but they were not.  The problem lasts until the
checkpointer does its initial update of the shared memory state when it
loads synchronous_standby_names.

In order to take care of this problem, the shared memory state in
WalSndCtl is extended to detect if it has been initialized by the
checkpointer, and not only check if synchronous_standby_names is
defined.  In WalSndCtlData, sync_standbys_defined is renamed to
sync_standbys_status, a bits8 able to know about two states:
- If the shared memory state has been initialized.  This flag is set by
the checkpointer at startup once, and never removed.
- If synchronous_standby_names is known as defined in the shared memory
state.  This is the same as the previous sync_standbys_defined in
WalSndCtl.

This method gives a way for backends to decide what they should do until
the shared memory area is initialized, and they now ultimately fall back
to a check on the GUC value in this case, which is the best thing that
can be done.

Fortunately, SyncRepUpdateSyncStandbysDefined() is called immediately by
the checkpointer when this process starts, so the window is very narrow.
It is possible to enlarge the problematic window by making the
checkpointer wait at the beginning of SyncRepUpdateSyncStandbysDefined()
with a hardcoded sleep for example, and doing so has showed that a 2PC
visibility test is indeed failing.  On machines slow enough, this bug
would cause spurious failures.

In 17~, we have looked at the possibility of adding an injection point
to have a reproducible test, but as the problematic window happens at
early startup, we would need to invent a way to make an injection point
optionally persistent across restarts when attached, something that
would be fine for this case as it would involve the checkpointer.  This
issue is quite old, and can be reproduced on all the stable branches.

Author: Melnikov Maksim <[email protected]>
Co-authored-by: Michael Paquier <[email protected]>
Discussion: https://postgr.es/m/163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru
Backpatch-through: 13

diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index ce163b99e95f88360205a2eb0c1c563e46e0b7eb..d478ab16eae6017028c83fb8ffd4df81496ee14a 100644 (file)
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -163,16 +163,23 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
     * sync replication standby names defined.
     *
     * Since this routine gets called every commit time, it's important to
-    * exit quickly if sync replication is not requested. So we check
-    * WalSndCtl->sync_standbys_defined flag without the lock and exit
-    * immediately if it's false. If it's true, we need to check it again
-    * later while holding the lock, to check the flag and operate the sync
-    * rep queue atomically. This is necessary to avoid the race condition
-    * described in SyncRepUpdateSyncStandbysDefined(). On the other hand, if
-    * it's false, the lock is not necessary because we don't touch the queue.
+    * exit quickly if sync replication is not requested.
+    *
+    * We check WalSndCtl->sync_standbys_status flag without the lock and exit
+    * immediately if SYNC_STANDBY_INIT is set (the checkpointer has
+    * initialized this data) but SYNC_STANDBY_DEFINED is missing (no sync
+    * replication requested).
+    *
+    * If SYNC_STANDBY_DEFINED is set, we need to check the status again later
+    * while holding the lock, to check the flag and operate the sync rep
+    * queue atomically.  This is necessary to avoid the race condition
+    * described in SyncRepUpdateSyncStandbysDefined().  On the other hand, if
+    * SYNC_STANDBY_DEFINED is not set, the lock is not necessary because we
+    * don't touch the queue.
     */
    if (!SyncRepRequested() ||
-       !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+       ((((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_status) &
+        (SYNC_STANDBY_INIT | SYNC_STANDBY_DEFINED)) == SYNC_STANDBY_INIT)
        return;
 
    /* Cap the level for anything other than commit to remote flush only. */
@@ -188,16 +195,52 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
    Assert(MyProc->syncRepState == SYNC_REP_NOT_WAITING);
 
    /*
-    * We don't wait for sync rep if WalSndCtl->sync_standbys_defined is not
-    * set.  See SyncRepUpdateSyncStandbysDefined.
+    * We don't wait for sync rep if SYNC_STANDBY_DEFINED is not set.  See
+    * SyncRepUpdateSyncStandbysDefined().
     *
     * Also check that the standby hasn't already replied. Unlikely race
     * condition but we'll be fetching that cache line anyway so it's likely
     * to be a low cost check.
+    *
+    * If the sync standby data has not been initialized yet
+    * (SYNC_STANDBY_INIT is not set), fall back to a check based on the LSN,
+    * then do a direct GUC check.
     */
-   if (!WalSndCtl->sync_standbys_defined ||
-       lsn <= WalSndCtl->lsn[mode])
+   if (WalSndCtl->sync_standbys_status & SYNC_STANDBY_INIT)
+   {
+       if ((WalSndCtl->sync_standbys_status & SYNC_STANDBY_DEFINED) == 0 ||
+           lsn <= WalSndCtl->lsn[mode])
+       {
+           LWLockRelease(SyncRepLock);
+           return;
+       }
+   }
+   else if (lsn <= WalSndCtl->lsn[mode])
+   {
+       /*
+        * The LSN is older than what we need to wait for.  The sync standby
+        * data has not been initialized yet, but we are OK to not wait
+        * because we know that there is no point in doing so based on the
+        * LSN.
+        */
+       LWLockRelease(SyncRepLock);
+       return;
+   }
+   else if (!SyncStandbysDefined())
    {
+       /*
+        * If we are here, the sync standby data has not been initialized yet,
+        * and the LSN is newer than what need to wait for, so we have fallen
+        * back to the best thing we could do in this case: a check on
+        * SyncStandbysDefined() to see if the GUC is set or not.
+        *
+        * When the GUC has a value, we wait until the checkpointer updates
+        * the status data because we cannot be sure yet if we should wait or
+        * not. Here, the GUC has *no* value, we are sure that there is no
+        * point to wait; this matters for example when initializing a
+        * cluster, where we should never wait, and no sync standbys is the
+        * default behavior.
+        */
        LWLockRelease(SyncRepLock);
        return;
    }
@@ -938,7 +981,7 @@ SyncRepWakeQueue(bool all, int mode)
 
 /*
  * The checkpointer calls this as needed to update the shared
- * sync_standbys_defined flag, so that backends don't remain permanently wedged
+ * sync_standbys_status flag, so that backends don't remain permanently wedged
  * if synchronous_standby_names is unset.  It's safe to check the current value
  * without the lock, because it's only ever updated by one process.  But we
  * must take the lock to change it.
@@ -948,7 +991,8 @@ SyncRepUpdateSyncStandbysDefined(void)
 {
    bool        sync_standbys_defined = SyncStandbysDefined();
 
-   if (sync_standbys_defined != WalSndCtl->sync_standbys_defined)
+   if (sync_standbys_defined !=
+       ((WalSndCtl->sync_standbys_status & SYNC_STANDBY_DEFINED) != 0))
    {
        LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
 
@@ -972,7 +1016,30 @@ SyncRepUpdateSyncStandbysDefined(void)
         * backend that hasn't yet reloaded its config might go to sleep on
         * the queue (and never wake up).  This prevents that.
         */
-       WalSndCtl->sync_standbys_defined = sync_standbys_defined;
+       WalSndCtl->sync_standbys_status = SYNC_STANDBY_INIT |
+           (sync_standbys_defined ? SYNC_STANDBY_DEFINED : 0);
+
+       LWLockRelease(SyncRepLock);
+   }
+   else if ((WalSndCtl->sync_standbys_status & SYNC_STANDBY_INIT) == 0)
+   {
+       LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
+
+       /*
+        * Note that there is no need to wake up the queues here.  We would
+        * reach this path only if SyncStandbysDefined() returns false, or it
+        * would mean that some backends are waiting with the GUC set.  See
+        * SyncRepWaitForLSN().
+        */
+       Assert(!SyncStandbysDefined());
+
+       /*
+        * Even if there is no sync standby defined, let the readers of this
+        * information know that the sync standby data has been initialized.
+        * This can just be done once, hence the previous check on
+        * SYNC_STANDBY_INIT to avoid useless work.
+        */
+       WalSndCtl->sync_standbys_status |= SYNC_STANDBY_INIT;
 
        LWLockRelease(SyncRepLock);
    }

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 269914bce2892ad61d9ce8cff2c39ca9e1aa8606..4bd4bb7aa0d25d25a000e8415fde59119a8a420b 100644 (file)
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1504,13 +1504,13 @@ WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId
     * When skipping empty transactions in synchronous replication, we send a
     * keepalive message to avoid delaying such transactions.
     *
-    * It is okay to check sync_standbys_defined flag without lock here as in
-    * the worst case we will just send an extra keepalive message when it is
+    * It is okay to check sync_standbys_status without lock here as in the
+    * worst case we will just send an extra keepalive message when it is
     * really not required.
     */
    if (skipped_xact &&
        SyncRepRequested() &&
-       ((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+       (((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_status & SYNC_STANDBY_DEFINED))
    {
        WalSndKeepalive(false, lsn);
 

diff --git a/src/include/replication/walsender_private.h b/src/include/replication/walsender_private.h
index c14888e493fd83ae0cfb6b71ffef76a01f8746b6..3f09a89187179a94581753c8062c38bfc0124fbd 100644 (file)
--- a/src/include/replication/walsender_private.h
+++ b/src/include/replication/walsender_private.h
@@ -98,15 +98,30 @@ typedef struct
    XLogRecPtr  lsn[NUM_SYNC_REP_WAIT_MODE];
 
    /*
-    * Are any sync standbys defined?  Waiting backends can't reload the
-    * config file safely, so checkpointer updates this value as needed.
-    * Protected by SyncRepLock.
+    * Status of data related to the synchronous standbys.  Waiting backends
+    * can't reload the config file safely, so checkpointer updates this value
+    * as needed. Protected by SyncRepLock.
     */
-   bool        sync_standbys_defined;
+   bits8       sync_standbys_status;
 
    WalSnd      walsnds[FLEXIBLE_ARRAY_MEMBER];
 } WalSndCtlData;
 
+/* Flags for WalSndCtlData->sync_standbys_status */
+
+/*
+ * Is the synchronous standby data initialized from the GUC?  This is set the
+ * first time synchronous_standby_names is processed by the checkpointer.
+ */
+#define SYNC_STANDBY_INIT          (1 << 0)
+
+/*
+ * Is the synchronous standby data defined?  This is set when
+ * synchronous_standby_names has some data, after being processed by the
+ * checkpointer.
+ */
+#define SYNC_STANDBY_DEFINED       (1 << 1)
+
 extern PGDLLIMPORT WalSndCtlData *WalSndCtl;