On 2020/08/20 11:29, Kyotaro Horiguchi wrote:
At Wed, 19 Aug 2020 13:20:46 +0000, Asim Praveen <pa...@vmware.com> wrote in
On 12-Aug-2020, at 12:02 PM, Masahiko Sawada <masahiko.saw...@2ndquadrant.com>
wrote:
On Wed, 12 Aug 2020 at 14:06, Asim Praveen <pa...@vmware.com> wrote:
On 11-Aug-2020, at 8:57 PM, Robert Haas <robertmh...@gmail.com> wrote:
I think this gets to the root of the issue. If we check the flag
without a lock, we might see a slightly stale value. But, considering
that there's no particular amount of time within which configuration
changes are guaranteed to take effect, maybe that's OK. However, there
is one potential gotcha here: if the walsender declares the standby to
be synchronous, a user can see that, right? So maybe there's this
problem: a user sees that the standby is synchronous and expects a
transaction committing afterward to provoke a wait, but really it
doesn't. Now the user is unhappy, feeling that the system didn't
perform according to expectations.
Yes, pg_stat_replication reports a standby in sync as soon as walsender updates
priority of the standby to something other than 0.
The potential gotcha referred above doesn’t seem too severe. What is the
likelihood of someone setting synchronous_standby_names GUC with either “*” or
a standby name and then immediately promoting that standby? If the standby is
promoted before the checkpointer on master gets a chance to update
sync_standbys_defined in shared memory, commits made during this interval on
master may not make it to standby. Upon promotion, those commits may be lost.
I think that if the standby is quite behind the primary and in case of
the primary crashes, the likelihood of losing commits might get
higher. The user can see the standby became synchronous standby via
pg_stat_replication but commit completes without a wait because the
checkpointer doesn't update sync_standbys_defined yet. If the primary
crashes before standby catching up and the user does failover, the
committed transaction will be lost, even though the user expects that
transaction commit has been replicated to the standby synchronously.
And this can happen even without the patch, right?
It is correct that the issue is orthogonal to the patch upthread and I’ve marked
the commitfest entry as ready-for-committer.
Yes, thanks for the review!
I find the name of SyncStandbysDefined macro is very confusing with
the struct member sync_standbys_defined, but that might be another
issue..
- * Fast exit if user has not requested sync replication.
+ * Fast exit if user has not requested sync replication, or there are no
+ * sync replication standby names defined.
This comment sounds like we just do that twice. The reason for the
check is to avoid wasteful exclusive locks on SyncRepLock, or to form
double-checked locking on the variable. I think we should explain that
here.
I added the following comments based on the suggestion by Sawada-san upthread.
Thought?
+ * 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_define without the lock and exit
+ * immediately if it's false. If it's true, we check it again later
+ * while holding the lock, to avoid the race condition described
+ * in SyncRepUpdateSyncStandbysDefined().
Attached is the updated version of the patch. I didn't change how to
fix the issue. But I changed the check for fast exit so that it's called
before setting the "mode", to avoid a few cycle.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index df1e341c76..c30ebaa0a6 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -158,18 +158,27 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
*/
Assert(InterruptHoldoffCount > 0);
+ /*
+ * Fast exit if user has not requested sync replication, or there are no
+ * 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_define without the lock and exit
+ * immediately if it's false. If it's true, we check it again later
+ * while holding the lock, to avoid the race condition described
+ * in SyncRepUpdateSyncStandbysDefined().
+ */
+ if (!SyncRepRequested() ||
+ !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+ return;
+
/* Cap the level for anything other than commit to remote flush only. */
if (commit)
mode = SyncRepWaitMode;
else
mode = Min(SyncRepWaitMode, SYNC_REP_WAIT_FLUSH);
- /*
- * Fast exit if user has not requested sync replication.
- */
- if (!SyncRepRequested())
- return;
-
Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
Assert(WalSndCtl != NULL);
