On Tue, Sep 13, 2022 at 8:52 AM Noah Misch <n...@leadboat.com> wrote: > > > > > [1] - > > > > https://www.postgresql.org/message-id/flat/20220909.172949.2223165886970819060.horikyota.ntt%40gmail.com > > I plan to use that message's patch, because it guarantees WALRCV_STOPPED at > the code location being changed. Today, in the unlikely event of > !WalRcvStreaming() due to WALRCV_WAITING or WALRCV_STOPPING, that code > proceeds without waiting for WALRCV_STOPPED.
Hm. That was the original fix [2] proposed and it works. The concern is that XLogShutdownWalRcv() does a bunch of work via ShutdownWalRcv() - it calls ConditionVariablePrepareToSleep(), ConditionVariableCancelSleep() (has lock 2 acquisitions and requisitions) and 1 function call WalRcvRunning()) even for WALRCV_STOPPED case, all this is unnecessary IMO when we determine the walreceiver is state is already WALRCV_STOPPED. I think we can do something like [3] to allay this concern and go with the fix proposed at [1] unconditionally calling XLogShutdownWalRcv(). > If WALRCV_WAITING or WALRCV_STOPPING can happen at that patch's code site, I > perhaps should back-patch the change to released versions. Does anyone know > whether one or both can happen? IMO, we must back-patch to the version where cc2c7d65fc27e877c9f407587b0b92d46cd6dd16 got introduced irrespective of any of the above happening. Thoughts? [1] - https://www.postgresql.org/message-id/flat/20220909.172949.2223165886970819060.horikyota.ntt%40gmail.com [2] - https://www.postgresql.org/message-id/flat/CALj2ACUoBWbaFo_t0gew%2Bx6n0V%2BmpvB_23HLvsVD9abgCShV5A%40mail.gmail.com#e762ee94cbbe32a0b8c72c60793626b3 [3] -diff --git a/src/backend/replication/walreceiverfuncs.c b/src/backend/replication/walreceiverfuncs.c index 90798b9d53..3487793c7a 100644 --- a/src/backend/replication/walreceiverfuncs.c +++ b/src/backend/replication/walreceiverfuncs.c @@ -181,6 +181,7 @@ ShutdownWalRcv(void) WalRcvData *walrcv = WalRcv; pid_t walrcvpid = 0; bool stopped = false; + bool was_stopped = false; /* * Request walreceiver to stop. Walreceiver will switch to WALRCV_STOPPED @@ -191,6 +192,7 @@ ShutdownWalRcv(void) switch (walrcv->walRcvState) { case WALRCV_STOPPED: + was_stopped = true; break; case WALRCV_STARTING: walrcv->walRcvState = WALRCV_STOPPED; @@ -208,6 +210,10 @@ ShutdownWalRcv(void) } SpinLockRelease(&walrcv->mutex); + /* Quick exit if walreceiver was already stopped. */ + if (was_stopped) + return; + /* Unnecessary but consistent. */ if (stopped) ConditionVariableBroadcast(&walrcv->walRcvStoppedCV); -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
