From: Maksim.Melnikov <m.melnikov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kirill Reshke <reshkekirill@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: sync_standbys_defined read/write race on startup
Date: Wed, 9 Apr 2025 18:21:50 +0300
Message-ID: <69541672-0fa2-41ea-af52-6e6110bf7235@postgrespro.ru> (raw)
In-Reply-To: <Z_YltcZNm1cCRwYj@paquier.xyz>
References: <163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ru>
<Z-R79EJVYfRy-EpW@paquier.xyz>
<cf922d84-95dd-4628-bf1e-087535f95932@postgrespro.ru>
<Z_YltcZNm1cCRwYj@paquier.xyz>
Hi Michael, thanks for your answer.
Test 041_checkpoint_at_promote.pl is really good example
of using injection points, but anyway, I still don't see
how injection points can help us here. In failed test case
we started postgres, immediately open psql connection and commit
prepared transaction. Postgres, on start, before accepting
connections, fork checkpointer and checkpointer initialize
shared parameter sync_standbys_defined in CheckpointerMain,
before process loop. So, in most attempts, we can't call
injection_points_attach in psql before
macro INJECTION_POINT() code will be executed in
checkpointer(I mean INJECTION_POINT() code instead sleep()),
bacause checkpointer process is forking before database
system is ready to accept connections.
The manual execution of checkpoint can't help us too.
P.S.
sorry for style errors, I am new in postgres, so can miss
some rules. Anyway, to avoid any community rules mistakes,
attached patch with correct naming.
Thanks.
Best regards,
Maksim Melnikov
Attachments:v4-0001-Fix-sync_standbys_defined-race-on-startup.patch (text/x-patch, 6.6K) download
From 8804867a70c9c1c946683b9b9c455da2d1504aa0 Mon Sep 17 00:00:00 2001
From: Maksim Melnikov <m.melnikov@postgrespro.ru>
Date: Wed, 26 Mar 2025 16:44:09 +0300
Subject: [PATCH v4] Fix sync_standbys_defined race on startup.
WalSndCtl->sync_standbys_defined, can be updated
by checkpointer process only and in backends it
can be read. So, on postgres startup, we have race
between checkpointer process and backends, that check
sync_standbys_defined for sync replications.
To avoid this, now we are working with sync_standbys_defined
not as bool field, but as bitmask, where 7th bit check
standbys_defined or not, 8th bit check
WalSndCtlData->sync_standbys_defined field has been
initialized by checkpointer. So if in synchronous replication functions
was detected that sync_standbys_defined is not initialized,
check SyncStandbysDefined() value.
---
src/backend/replication/syncrep.c | 32 +++++++++++++++++----
src/backend/replication/walsender.c | 2 +-
src/include/replication/walsender_private.h | 18 +++++++++++-
3 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.cindex d75e3968035..c049e82286a 100644--- a/src/backend/replication/syncrep.c+++ b/src/backend/replication/syncrep.c@@ -85,6 +85,7 @@
#include "tcop/tcopprot.h"
#include "utils/guc_hooks.h"
#include "utils/ps_status.h"
+#include "utils/injection_point.h"
/* User-settable parameters for sync rep */
char *SyncRepStandbyNames;
@@ -148,6 +149,7 @@ void
SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
{
int mode;
+ bool sync_standbys_defined;
/*
* This should be called while holding interrupts during a transaction
@@ -170,7 +172,7 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
* it's false, the lock is not necessary because we don't touch the queue.
*/
if (!SyncRepRequested() ||
- !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)+ IS_SYNCSTANDBYS_INITIALIZED_ONLY(((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined))
return;
/* Cap the level for anything other than commit to remote flush only. */
@@ -184,6 +186,8 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
Assert(MyProc->syncRepState == SYNC_REP_NOT_WAITING);
+ sync_standbys_defined = WalSndCtl->sync_standbys_defined & SYNCSTANDBYSINITIALIZED+ ? (WalSndCtl->sync_standbys_defined & SYNCSTANDBYSDEFINED) : SyncStandbysDefined();
/*
* We don't wait for sync rep if WalSndCtl->sync_standbys_defined is not
@@ -193,7 +197,7 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
* condition but we'll be fetching that cache line anyway so it's likely
* to be a low cost check.
*/
- if (!WalSndCtl->sync_standbys_defined ||+ if (!sync_standbys_defined ||
lsn <= WalSndCtl->lsn[mode])
{
LWLockRelease(SyncRepLock);
@@ -920,9 +924,10 @@ SyncRepWakeQueue(bool all, int mode)
void
SyncRepUpdateSyncStandbysDefined(void)
{
- bool sync_standbys_defined = SyncStandbysDefined();+ bool new_sync_standbys_defined = SyncStandbysDefined();+ bool old_sync_standbys_defined = (WalSndCtl->sync_standbys_defined & SYNCSTANDBYSDEFINED);- if (sync_standbys_defined != WalSndCtl->sync_standbys_defined)+ if (new_sync_standbys_defined != old_sync_standbys_defined)
{
LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
@@ -931,7 +936,7 @@ SyncRepUpdateSyncStandbysDefined(void)
* for backends to continue waiting. Since the user no longer wants
* synchronous replication, we'd better wake them up.
*/
- if (!sync_standbys_defined)+ if (!new_sync_standbys_defined)
{
int i;
@@ -946,8 +951,23 @@ 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_defined = new_sync_standbys_defined+ ? SET_SYNCSTANDBYS_DEFINED(WalSndCtl->sync_standbys_defined)+ : RESET_SYNCSTANDBYS_DEFINED(WalSndCtl->sync_standbys_defined);++ LWLockRelease(SyncRepLock);+ }+ else if (!(WalSndCtl->sync_standbys_defined & SYNCSTANDBYSINITIALIZED))+ {+ /*+ * Even if new_sync_standbys_defined == old_sync_standbys_defined, we+ * should check WalSndCtl->sync_standbys_defined is initialized and+ * set its initialize bit, if it reset. Anyway this bit should be set+ * only on start and then it will be always setted.+ */+ LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);+ WalSndCtl->sync_standbys_defined = SET_SYNCSTANDBYS_INITIALIZED(WalSndCtl->sync_standbys_defined);
LWLockRelease(SyncRepLock);
}
}
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.cindex 1028919aecb..c181c7ee977 100644--- a/src/backend/replication/walsender.c+++ b/src/backend/replication/walsender.c@@ -1676,7 +1676,7 @@ WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId
*/
if (skipped_xact &&
SyncRepRequested() &&
- ((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)+ (((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined & SYNCSTANDBYSDEFINED))
{
WalSndKeepalive(false, lsn);
diff --git a/src/include/replication/walsender_private.h b/src/include/replication/walsender_private.hindex 0fc77f1b4af..dd931b81e4f 100644--- a/src/include/replication/walsender_private.h+++ b/src/include/replication/walsender_private.h@@ -100,7 +100,7 @@ typedef struct
* config file safely, so checkpointer updates this value as needed.
* Protected by SyncRepLock.
*/
- bool sync_standbys_defined;+ uint8 sync_standbys_defined;
/* used as a registry of physical / logical walsenders to wake */
ConditionVariable wal_flush_cv;
@@ -116,6 +116,22 @@ typedef struct
WalSnd walsnds[FLEXIBLE_ARRAY_MEMBER];
} WalSndCtlData;
+/*+ * WalSndCtlData->sync_standbys_defined bit flags. 7th bit check standbys_defined or not,+ * 8th bit check WalSndCtlData->sync_standbys_defined field has been initialized.+ */+#define SYNCSTANDBYSINITIALIZED (1 << 0)+#define SYNCSTANDBYSDEFINED (1 << 1)++#define SET_SYNCSTANDBYS_INITIALIZED(sync_standbys) \+ (sync_standbys | SYNCSTANDBYSINITIALIZED)+#define SET_SYNCSTANDBYS_DEFINED(sync_standbys) \+ (sync_standbys | SYNCSTANDBYSINITIALIZED | SYNCSTANDBYSDEFINED)+#define RESET_SYNCSTANDBYS_DEFINED(sync_standbys) \+ (SET_SYNCSTANDBYS_INITIALIZED(sync_standbys) & (~SYNCSTANDBYSDEFINED))+#define IS_SYNCSTANDBYS_INITIALIZED_ONLY(sync_standbys) \+ ((sync_standbys & (SYNCSTANDBYSINITIALIZED | SYNCSTANDBYSDEFINED)) == SYNCSTANDBYSINITIALIZED)+
extern PGDLLIMPORT WalSndCtlData *WalSndCtl;
--
2.43.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: m.melnikov@postgrespro.ru, michael@paquier.xyz, reshkekirill@gmail.com
Subject: Re: sync_standbys_defined read/write race on startup
In-Reply-To: <69541672-0fa2-41ea-af52-6e6110bf7235@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox