On 2020/08/27 15:59, Asim Praveen wrote:
On 26-Aug-2020, at 11:10 PM, Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
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().
+1. May I suggest the following addition to the above comment (feel free to
rephrase / reject)?
"If sync_standbys_defined was being set from false to true and we observe it as
false, it ok to skip the wait. Replication was async and it is in the process
of being changed to sync, due to user request. Subsequent commits will observe
the change and start waiting.”
Thanks for the suggestion! I'm not sure if it's worth adding this because
it seems obvious thing. But maybe you imply that we need to comment
why the lock is not necessary when sync_standbys_defined is false. Right?
If so, what about updating the comments as follows?
+ * 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.
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.
Looks good to me. There is a typo in the comment:
s/sync_standbys_define/sync_standbys_defined/
Fixed. Thanks!
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..6e8c76537a 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -158,18 +158,30 @@ 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_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.
+ */
+ 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);
