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

Commite2f42f8

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 parentdeb99dc commite2f42f8

File tree

2 files changed

+86
-11
lines changed

2 files changed

+86
-11
lines changed

‎src/backend/replication/syncrep.c

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,16 +177,52 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
177177
Assert(MyProc->syncRepState==SYNC_REP_NOT_WAITING);
178178

179179
/*
180-
* We don't wait for sync rep ifWalSndCtl->sync_standbys_defined is not
181-
*set. SeeSyncRepUpdateSyncStandbysDefined.
180+
* We don't wait for sync rep ifSYNC_STANDBY_DEFINED is not set. See
181+
* SyncRepUpdateSyncStandbysDefined().
182182
*
183183
* Also check that the standby hasn't already replied. Unlikely race
184184
* condition but we'll be fetching that cache line anyway so it's likely
185185
* to be a low cost check.
186+
*
187+
* If the sync standby data has not been initialized yet
188+
* (SYNC_STANDBY_INIT is not set), fall back to a check based on the LSN,
189+
* then do a direct GUC check.
186190
*/
187-
if (!WalSndCtl->sync_standbys_defined||
188-
lsn <=WalSndCtl->lsn[mode])
191+
if (WalSndCtl->sync_standbys_status&SYNC_STANDBY_INIT)
192+
{
193+
if ((WalSndCtl->sync_standbys_status&SYNC_STANDBY_DEFINED)==0||
194+
lsn <=WalSndCtl->lsn[mode])
195+
{
196+
LWLockRelease(SyncRepLock);
197+
return;
198+
}
199+
}
200+
elseif (lsn <=WalSndCtl->lsn[mode])
201+
{
202+
/*
203+
* The LSN is older than what we need to wait for. The sync standby
204+
* data has not been initialized yet, but we are OK to not wait
205+
* because we know that there is no point in doing so based on the
206+
* LSN.
207+
*/
208+
LWLockRelease(SyncRepLock);
209+
return;
210+
}
211+
elseif (!SyncStandbysDefined())
189212
{
213+
/*
214+
* If we are here, the sync standby data has not been initialized yet,
215+
* and the LSN is newer than what need to wait for, so we have fallen
216+
* back to the best thing we could do in this case: a check on
217+
* SyncStandbysDefined() to see if the GUC is set or not.
218+
*
219+
* When the GUC has a value, we wait until the checkpointer updates
220+
* the status data because we cannot be sure yet if we should wait or
221+
* not. Here, the GUC has *no* value, we are sure that there is no
222+
* point to wait; this matters for example when initializing a
223+
* cluster, where we should never wait, and no sync standbys is the
224+
* default behavior.
225+
*/
190226
LWLockRelease(SyncRepLock);
191227
return;
192228
}
@@ -927,7 +963,7 @@ SyncRepWakeQueue(bool all, int mode)
927963

928964
/*
929965
* The checkpointer calls this as needed to update the shared
930-
*sync_standbys_defined flag, so that backends don't remain permanently wedged
966+
*sync_standbys_status flag, so that backends don't remain permanently wedged
931967
* if synchronous_standby_names is unset. It's safe to check the current value
932968
* without the lock, because it's only ever updated by one process. But we
933969
* must take the lock to change it.
@@ -937,7 +973,8 @@ SyncRepUpdateSyncStandbysDefined(void)
937973
{
938974
boolsync_standbys_defined=SyncStandbysDefined();
939975

940-
if (sync_standbys_defined!=WalSndCtl->sync_standbys_defined)
976+
if (sync_standbys_defined!=
977+
((WalSndCtl->sync_standbys_status&SYNC_STANDBY_DEFINED)!=0))
941978
{
942979
LWLockAcquire(SyncRepLock,LW_EXCLUSIVE);
943980

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

9661026
LWLockRelease(SyncRepLock);
9671027
}

‎src/include/replication/walsender_private.h

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,30 @@ typedef struct
9898
XLogRecPtrlsn[NUM_SYNC_REP_WAIT_MODE];
9999

100100
/*
101-
*Are any sync standbys defined? Waiting backends can't reload the
102-
* config file safely, so checkpointer updates this value as needed.
103-
* Protected by SyncRepLock.
101+
*Status of data related to the synchronous standbys. Waiting backends
102+
*can't reload theconfig file safely, so checkpointer updates this value
103+
*as needed.Protected by SyncRepLock.
104104
*/
105-
boolsync_standbys_defined;
105+
bits8sync_standbys_status;
106106

107107
WalSndwalsnds[FLEXIBLE_ARRAY_MEMBER];
108108
}WalSndCtlData;
109109

110+
/* Flags for WalSndCtlData->sync_standbys_status */
111+
112+
/*
113+
* Is the synchronous standby data initialized from the GUC? This is set the
114+
* first time synchronous_standby_names is processed by the checkpointer.
115+
*/
116+
#defineSYNC_STANDBY_INIT(1 << 0)
117+
118+
/*
119+
* Is the synchronous standby data defined? This is set when
120+
* synchronous_standby_names has some data, after being processed by the
121+
* checkpointer.
122+
*/
123+
#defineSYNC_STANDBY_DEFINED(1 << 1)
124+
110125
externWalSndCtlData*WalSndCtl;
111126

112127

0 commit comments

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