On 2020/11/05 6:02, Fujii Masao wrote:
On 2020/11/05 5:36, Heikki Linnakangas wrote:
On 04/11/2020 15:17, Heikki Linnakangas wrote:
On 04/11/2020 14:03, Fujii Masao wrote:
Or ISTM that WakeupRecovery() should set the latch only when the latch
has not been reset to NULL yet.
Got to be careful with race conditions, if the latch is set to NULL at
the same time that WakeupRecovery() is called.
I don't think commit 113d3591b8 got this quite right:
void
WakeupRecovery(void)
{
if (XLogCtl->recoveryWakeupLatch)
SetLatch(XLogCtl->recoveryWakeupLatch);
}
If XLogCtl->recoveryWakeupLatch is set to NULL between the if and the SetLatch,
you'll still get a segfault. That's highly unlikely to happen in practice because
the compiler will optimize that into a single load instruction, but could happen
with -O0. I think you'd need to do the access only once, using a volatile pointer,
to make that safe.
On second thought, since fetching the latch pointer might not be atomic,
maybe we need to use spinlock like WalRcvForceReply() does. But using
spinlock in every calls of WakeupRecovery() might cause performance
overhead. So I'm thinking to use spinlock only when it's necessary, i.e.,
when the latch may be reset to NULL concurrently. Attached is the POC
patch implementing that. Thought?
Maybe it's simpler to just not reset it to NULL, after all.
Yes, you're right. I agree it's simpler to remove the code resetting
the latch to NULL. Also as the comment for that code explains,
basically it's not necessary to reset it to NULL.
On second thought, your comment in upthread "it seems a bit sloppy to
leave it pointing to a latch that belongs to a random process though."
seems right. So I'm inclined to avoid this approach, for now.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index a1078a7cfc..fc8ae318fd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7488,11 +7488,15 @@ StartupXLOG(void)
ResetUnloggedRelations(UNLOGGED_RELATION_INIT);
/*
- * We don't need the latch anymore. It's not strictly necessary to reset
- * it to NULL, but let's do it for the sake of tidiness.
+ * Reset recoveryWakeupLatch to NULL to prevent us from leaving it
+ * pointing to the latch that might belong to other process later.
*/
if (ArchiveRecoveryRequested)
+ {
+ SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->recoveryWakeupLatch = NULL;
+ SpinLockRelease(&XLogCtl->info_lck);
+ }
/*
* We are now done reading the xlog from stream. Turn off streaming
@@ -12669,12 +12673,24 @@ CheckPromoteSignal(void)
/*
* Wake up startup process to replay newly arrived WAL, or to notice that
* failover has been requested.
+ *
+ * use_lock: if true, acquire spinlock to fetch the latch pointer. This
+ * should be true if walreceiver has already been marked as inactive.
+ * Because the startup process might reset the latch to NULL concurrently
+ * in that case.
*/
void
-WakeupRecovery(void)
+WakeupRecovery(bool use_lock)
{
- if (XLogCtl->recoveryWakeupLatch)
- SetLatch(XLogCtl->recoveryWakeupLatch);
+ Latch *latch;
+
+ if (use_lock)
+ SpinLockAcquire(&XLogCtl->info_lck);
+ latch = XLogCtl->recoveryWakeupLatch;
+ if (use_lock)
+ SpinLockRelease(&XLogCtl->info_lck);
+ if (latch)
+ SetLatch(latch);
}
/*
diff --git a/src/backend/replication/walreceiver.c
b/src/backend/replication/walreceiver.c
index bb1d44ccb7..c89757ec69 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -666,7 +666,7 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint,
TimeLineID *startpointTLI)
* nudge startup process to notice that we've stopped streaming and are
* now waiting for instructions.
*/
- WakeupRecovery();
+ WakeupRecovery(false);
for (;;)
{
ResetLatch(walrcv->latch);
@@ -803,7 +803,7 @@ WalRcvDie(int code, Datum arg)
walrcv_disconnect(wrconn);
/* Wake up the startup process to notice promptly that we're gone */
- WakeupRecovery();
+ WakeupRecovery(true);
}
/* SIGHUP: set flag to re-read config file at next convenient time */
@@ -1025,7 +1025,7 @@ XLogWalRcvFlush(bool dying)
SpinLockRelease(&walrcv->mutex);
/* Signal the startup process and walsender that new WAL has
arrived */
- WakeupRecovery();
+ WakeupRecovery(false);
if (AllowCascadeReplication())
WalSndWakeup();
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 221af87e71..43cd047250 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -343,7 +343,7 @@ extern void RemovePromoteSignalFiles(void);
extern bool PromoteIsTriggered(void);
extern bool CheckPromoteSignal(void);
-extern void WakeupRecovery(void);
+extern void WakeupRecovery(bool use_lock);
extern void SetWalWriterSleeping(bool sleeping);
extern void StartupRequestWalReceiverRestart(void);
