Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit3339847

Browse files
committed
Fix race with synchronous_standby_names at startup
synchronous_standby_names cannot be reloaded safely by backends, and thecheckpointer is in charge of updating a state in shared memory if theGUC is enabled in WalSndCtl, to let the backends know if they shouldwait or not for a given LSN. This provides a strict control on thetiming of the waiting queues if the GUC is enabled or disabled, thenreloaded. The checkpointer is also in charge of waking up the backendsthat could be waiting for a LSN when the GUC is disabled.This logic had a race condition at startup, where it would be possiblefor backends to not wait for a LSN even if synchronous_standby_names isenabled. This would cause visibility issues with transactions that weshould be waiting for but they were not. The problem lasts until thecheckpointer does its initial update of the shared memory state when itloads synchronous_standby_names.In order to take care of this problem, the shared memory state inWalSndCtl is extended to detect if it has been initialized by thecheckpointer, and not only check if synchronous_standby_names isdefined. In WalSndCtlData, sync_standbys_defined is renamed tosync_standbys_status, a bits8 able to know about two states:- If the shared memory state has been initialized. This flag is set bythe checkpointer at startup once, and never removed.- If synchronous_standby_names is known as defined in the shared memorystate. This is the same as the previous sync_standbys_defined inWalSndCtl.This method gives a way for backends to decide what they should do untilthe shared memory area is initialized, and they now ultimately fall backto a check on the GUC value in this case, which is the best thing thatcan be done.Fortunately, SyncRepUpdateSyncStandbysDefined() is called immediately bythe checkpointer when this process starts, so the window is very narrow.It is possible to enlarge the problematic window by making thecheckpointer wait at the beginning of SyncRepUpdateSyncStandbysDefined()with a hardcoded sleep for example, and doing so has showed that a 2PCvisibility test is indeed failing. On machines slow enough, this bugwould cause spurious failures.In 17~, we have looked at the possibility of adding an injection pointto have a reproducible test, but as the problematic window happens atearly startup, we would need to invent a way to make an injection pointoptionally persistent across restarts when attached, something thatwould be fine for this case as it would involve the checkpointer. Thisissue is quite old, and can be reproduced on all the stable branches.Author: Melnikov Maksim <m.melnikov@postgrespro.ru>Co-authored-by: Michael Paquier <michael@paquier.xyz>Discussion:https://postgr.es/m/163fcbec-900b-4b07-beaa-d2ead8634bec@postgrespro.ruBackpatch-through: 13
1 parent03faf38 commit3339847

File tree

3 files changed

+104
-22
lines changed

3 files changed

+104
-22
lines changed

‎src/backend/replication/syncrep.c

Lines changed: 82 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -161,16 +161,23 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
161161
* sync replication standby names defined.
162162
*
163163
* Since this routine gets called every commit time, it's important to
164-
* exit quickly if sync replication is not requested. So we check
165-
* WalSndCtl->sync_standbys_defined flag without the lock and exit
166-
* immediately if it's false. If it's true, we need to check it again
167-
* later while holding the lock, to check the flag and operate the sync
168-
* rep queue atomically. This is necessary to avoid the race condition
169-
* described in SyncRepUpdateSyncStandbysDefined(). On the other hand, if
170-
* it's false, the lock is not necessary because we don't touch the queue.
164+
* exit quickly if sync replication is not requested.
165+
*
166+
* We check WalSndCtl->sync_standbys_status flag without the lock and exit
167+
* immediately if SYNC_STANDBY_INIT is set (the checkpointer has
168+
* initialized this data) but SYNC_STANDBY_DEFINED is missing (no sync
169+
* replication requested).
170+
*
171+
* If SYNC_STANDBY_DEFINED is set, we need to check the status again later
172+
* while holding the lock, to check the flag and operate the sync rep
173+
* queue atomically. This is necessary to avoid the race condition
174+
* described in SyncRepUpdateSyncStandbysDefined(). On the other hand, if
175+
* SYNC_STANDBY_DEFINED is not set, the lock is not necessary because we
176+
* don't touch the queue.
171177
*/
172178
if (!SyncRepRequested()||
173-
!((volatileWalSndCtlData*)WalSndCtl)->sync_standbys_defined)
179+
((((volatileWalSndCtlData*)WalSndCtl)->sync_standbys_status)&
180+
(SYNC_STANDBY_INIT |SYNC_STANDBY_DEFINED))==SYNC_STANDBY_INIT)
174181
return;
175182

176183
/* Cap the level for anything other than commit to remote flush only. */
@@ -186,16 +193,52 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
186193
Assert(MyProc->syncRepState==SYNC_REP_NOT_WAITING);
187194

188195
/*
189-
* We don't wait for sync rep ifWalSndCtl->sync_standbys_defined is not
190-
*set. SeeSyncRepUpdateSyncStandbysDefined.
196+
* We don't wait for sync rep ifSYNC_STANDBY_DEFINED is not set. See
197+
* SyncRepUpdateSyncStandbysDefined().
191198
*
192199
* Also check that the standby hasn't already replied. Unlikely race
193200
* condition but we'll be fetching that cache line anyway so it's likely
194201
* to be a low cost check.
202+
*
203+
* If the sync standby data has not been initialized yet
204+
* (SYNC_STANDBY_INIT is not set), fall back to a check based on the LSN,
205+
* then do a direct GUC check.
195206
*/
196-
if (!WalSndCtl->sync_standbys_defined||
197-
lsn <=WalSndCtl->lsn[mode])
207+
if (WalSndCtl->sync_standbys_status&SYNC_STANDBY_INIT)
208+
{
209+
if ((WalSndCtl->sync_standbys_status&SYNC_STANDBY_DEFINED)==0||
210+
lsn <=WalSndCtl->lsn[mode])
211+
{
212+
LWLockRelease(SyncRepLock);
213+
return;
214+
}
215+
}
216+
elseif (lsn <=WalSndCtl->lsn[mode])
198217
{
218+
/*
219+
* The LSN is older than what we need to wait for. The sync standby
220+
* data has not been initialized yet, but we are OK to not wait
221+
* because we know that there is no point in doing so based on the
222+
* LSN.
223+
*/
224+
LWLockRelease(SyncRepLock);
225+
return;
226+
}
227+
elseif (!SyncStandbysDefined())
228+
{
229+
/*
230+
* If we are here, the sync standby data has not been initialized yet,
231+
* and the LSN is newer than what need to wait for, so we have fallen
232+
* back to the best thing we could do in this case: a check on
233+
* SyncStandbysDefined() to see if the GUC is set or not.
234+
*
235+
* When the GUC has a value, we wait until the checkpointer updates
236+
* the status data because we cannot be sure yet if we should wait or
237+
* not. Here, the GUC has *no* value, we are sure that there is no
238+
* point to wait; this matters for example when initializing a
239+
* cluster, where we should never wait, and no sync standbys is the
240+
* default behavior.
241+
*/
199242
LWLockRelease(SyncRepLock);
200243
return;
201244
}
@@ -912,7 +955,7 @@ SyncRepWakeQueue(bool all, int mode)
912955

913956
/*
914957
* The checkpointer calls this as needed to update the shared
915-
*sync_standbys_defined flag, so that backends don't remain permanently wedged
958+
*sync_standbys_status flag, so that backends don't remain permanently wedged
916959
* if synchronous_standby_names is unset. It's safe to check the current value
917960
* without the lock, because it's only ever updated by one process. But we
918961
* must take the lock to change it.
@@ -922,7 +965,8 @@ SyncRepUpdateSyncStandbysDefined(void)
922965
{
923966
boolsync_standbys_defined=SyncStandbysDefined();
924967

925-
if (sync_standbys_defined!=WalSndCtl->sync_standbys_defined)
968+
if (sync_standbys_defined!=
969+
((WalSndCtl->sync_standbys_status&SYNC_STANDBY_DEFINED)!=0))
926970
{
927971
LWLockAcquire(SyncRepLock,LW_EXCLUSIVE);
928972

@@ -946,7 +990,30 @@ SyncRepUpdateSyncStandbysDefined(void)
946990
* backend that hasn't yet reloaded its config might go to sleep on
947991
* the queue (and never wake up). This prevents that.
948992
*/
949-
WalSndCtl->sync_standbys_defined=sync_standbys_defined;
993+
WalSndCtl->sync_standbys_status=SYNC_STANDBY_INIT |
994+
(sync_standbys_defined ?SYNC_STANDBY_DEFINED :0);
995+
996+
LWLockRelease(SyncRepLock);
997+
}
998+
elseif ((WalSndCtl->sync_standbys_status&SYNC_STANDBY_INIT)==0)
999+
{
1000+
LWLockAcquire(SyncRepLock,LW_EXCLUSIVE);
1001+
1002+
/*
1003+
* Note that there is no need to wake up the queues here. We would
1004+
* reach this path only if SyncStandbysDefined() returns false, or it
1005+
* would mean that some backends are waiting with the GUC set. See
1006+
* SyncRepWaitForLSN().
1007+
*/
1008+
Assert(!SyncStandbysDefined());
1009+
1010+
/*
1011+
* Even if there is no sync standby defined, let the readers of this
1012+
* information know that the sync standby data has been initialized.
1013+
* This can just be done once, hence the previous check on
1014+
* SYNC_STANDBY_INIT to avoid useless work.
1015+
*/
1016+
WalSndCtl->sync_standbys_status |=SYNC_STANDBY_INIT;
9501017

9511018
LWLockRelease(SyncRepLock);
9521019
}

‎src/backend/replication/walsender.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1697,13 +1697,13 @@ WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId
16971697
* When skipping empty transactions in synchronous replication, we send a
16981698
* keepalive message to avoid delaying such transactions.
16991699
*
1700-
* It is okay to checksync_standbys_defined flagwithout lock here as in
1701-
*theworst case we will just send an extra keepalive message when it is
1700+
* It is okay to checksync_standbys_statuswithout lock here as in the
1701+
* worst case we will just send an extra keepalive message when it is
17021702
* really not required.
17031703
*/
17041704
if (skipped_xact&&
17051705
SyncRepRequested()&&
1706-
((volatileWalSndCtlData*)WalSndCtl)->sync_standbys_defined)
1706+
(((volatileWalSndCtlData*)WalSndCtl)->sync_standbys_status&SYNC_STANDBY_DEFINED))
17071707
{
17081708
WalSndKeepalive(false,lsn);
17091709

‎src/include/replication/walsender_private.h

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,11 @@ typedef struct
103103
XLogRecPtrlsn[NUM_SYNC_REP_WAIT_MODE];
104104

105105
/*
106-
*Are any sync standbys defined? Waiting backends can't reload the
107-
* config file safely, so checkpointer updates this value as needed.
108-
* Protected by SyncRepLock.
106+
*Status of data related to the synchronous standbys. Waiting backends
107+
*can't reload theconfig file safely, so checkpointer updates this value
108+
*as needed.Protected by SyncRepLock.
109109
*/
110-
boolsync_standbys_defined;
110+
bits8sync_standbys_status;
111111

112112
/* used as a registry of physical / logical walsenders to wake */
113113
ConditionVariablewal_flush_cv;
@@ -123,6 +123,21 @@ typedef struct
123123
WalSndwalsnds[FLEXIBLE_ARRAY_MEMBER];
124124
}WalSndCtlData;
125125

126+
/* Flags for WalSndCtlData->sync_standbys_status */
127+
128+
/*
129+
* Is the synchronous standby data initialized from the GUC? This is set the
130+
* first time synchronous_standby_names is processed by the checkpointer.
131+
*/
132+
#defineSYNC_STANDBY_INIT(1 << 0)
133+
134+
/*
135+
* Is the synchronous standby data defined? This is set when
136+
* synchronous_standby_names has some data, after being processed by the
137+
* checkpointer.
138+
*/
139+
#defineSYNC_STANDBY_DEFINED(1 << 1)
140+
126141
externPGDLLIMPORTWalSndCtlData*WalSndCtl;
127142

128143

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp