On Thu, Jan 22, 2026 at 06:02:54PM +0800, Xuneng Zhou wrote: >> Before this patch, this is no state change here, thus rest logic >> can handle STOPPING. After this patch, if the race occurs, in dev >> mode, the Assert is fired; in production mode, STOPPING is overwritten >> by STREAMING, which is wrong. >> >> So, instead of Assert(), I think we should check if current state >> is CONNECTING, if not, it should not proceed. > > It makes sense to me. Please check the updated patch.
+ if (state != WALRCV_CONNECTING)
+ {
+ if (state == WALRCV_STOPPING)
+ proc_exit(0);
+ elog(FATAL, "unexpected walreceiver state");
+ }
Sorry, but this addition does not make sense to me. Before this
patch, if the startup process decides to mark a WAL receiver as
stopping after it has been started in WalReceiverMain(), then we would
run at least one loop until we reach WalRcvWaitForStartPosition(),
which is the path where the WAL receiver exits. Aka, I don't think we
should increase the number of paths where we decide if a WAL receiver
should fast-exit or not (I'd rather try to reduce them, but I don't
think we can do that currently based on the ping-pong game between the
startup process and WAL receiver process). Saying that, you are right
about the fact that the assertion is wrong, and that we should not
upgrade the status to STREAMING if the WAL receiver is not CONNECTING.
So it seems to me that this code should remain as simple as followed,
documenting that we switch to STREAMING when the first connection has
been established after the WAL receiver has started, or when the WAL
receiver is switched after WalRcvWaitForStartPosition() once
startstreaming() has acknowledged that streaming is happening (I would
add a comment saying that):
if (state == WALRCV_CONNECTING)
walrcv->walRcvState = WALRCV_STREAMING;
--
Michael
signature.asc
Description: PGP signature
