Обсуждение: Add WALRCV_CONNECTING state to walreceiver

Поиск
Список
Период
Сортировка

Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Hackers,

Bug #19093 [1] reported that pg_stat_wal_receiver.status = 'streaming'
does not accurately reflect streaming health.  In that discussion,
Noah noted that even before the reported regression, status =
'streaming' was unreliable because walreceiver sets it during early
startup, before attempting a connection. He suggested:

"Long-term, in master only, perhaps we should introduce another status
like 'connecting'. Perhaps enact the connecting->streaming status
transition just before tendering the first byte of streamed WAL to the
startup process. Alternatively, enact that transition when the startup
process accepts the
first streamed byte."

Michael and I also thought this could be a useful addition. This patch
implements that suggestion by adding a new WALRCV_CONNECTING state.

== Background ==
Currently, walreceiver transitions directly from STARTING to STREAMING
early in WalReceiverMain(), before any WAL data has been received.
This means status = 'streaming' can be observed even when:

- The connection to the primary has not been established
- No WAL data has actually been received or flushed

This makes it difficult for monitoring tools to distinguish between a
healthy streaming replica and one that is merely attempting to stream.

== Proposal ==

Introduce WALRCV_CONNECTING as an intermediate state between STARTING
and STREAMING:

- When walreceiver starts, it enters CONNECTING (instead of going
directly to STREAMING).
- The transition to STREAMING occurs in XLogWalRcvFlush(), inside the
existing spinlock-protected block that updates flushedUpto.

Feedbacks welcome.

[1] https://www.postgresql.org/message-id/flat/19093-c4fff49a608f82a0%40postgresql.org

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Noah Misch
Дата:
On Fri, Dec 12, 2025 at 12:51:00PM +0800, Xuneng Zhou wrote:
> Bug #19093 [1] reported that pg_stat_wal_receiver.status = 'streaming'
> does not accurately reflect streaming health.  In that discussion,
> Noah noted that even before the reported regression, status =
> 'streaming' was unreliable because walreceiver sets it during early
> startup, before attempting a connection. He suggested:
> 
> "Long-term, in master only, perhaps we should introduce another status
> like 'connecting'. Perhaps enact the connecting->streaming status
> transition just before tendering the first byte of streamed WAL to the
> startup process. Alternatively, enact that transition when the startup
> process accepts the
> first streamed byte."

> == Proposal ==
> 
> Introduce WALRCV_CONNECTING as an intermediate state between STARTING
> and STREAMING:
> 
> - When walreceiver starts, it enters CONNECTING (instead of going
> directly to STREAMING).
> - The transition to STREAMING occurs in XLogWalRcvFlush(), inside the
> existing spinlock-protected block that updates flushedUpto.

I think this has the drawback that if the primary's WAL is incompatible,
e.g. unacceptable timeline, the walreceiver will still briefly enter
STREAMING.  That could trick monitoring.  Waiting for applyPtr to advance
would avoid the short-lived STREAMING.  What's the feasibility of that?



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Noah,

On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Fri, Dec 12, 2025 at 12:51:00PM +0800, Xuneng Zhou wrote:
> > Bug #19093 [1] reported that pg_stat_wal_receiver.status = 'streaming'
> > does not accurately reflect streaming health.  In that discussion,
> > Noah noted that even before the reported regression, status =
> > 'streaming' was unreliable because walreceiver sets it during early
> > startup, before attempting a connection. He suggested:
> >
> > "Long-term, in master only, perhaps we should introduce another status
> > like 'connecting'. Perhaps enact the connecting->streaming status
> > transition just before tendering the first byte of streamed WAL to the
> > startup process. Alternatively, enact that transition when the startup
> > process accepts the
> > first streamed byte."
>
> > == Proposal ==
> >
> > Introduce WALRCV_CONNECTING as an intermediate state between STARTING
> > and STREAMING:
> >
> > - When walreceiver starts, it enters CONNECTING (instead of going
> > directly to STREAMING).
> > - The transition to STREAMING occurs in XLogWalRcvFlush(), inside the
> > existing spinlock-protected block that updates flushedUpto.
>
> I think this has the drawback that if the primary's WAL is incompatible,
> e.g. unacceptable timeline, the walreceiver will still briefly enter
> STREAMING.  That could trick monitoring.

Thanks for pointing this out.

 Waiting for applyPtr to advance
> would avoid the short-lived STREAMING.  What's the feasibility of that?

I think this could work, but with complications. If replay latency is
high or replay is paused with pg_wal_replay_pause, the WalReceiver
would stay in the CONNECTING state longer than expected. Whether this
is ok depends on the definition of the 'connecting' state. For the
implementation, deciding where and when to check applyPtr against LSNs
like receiveStart is more difficult—the WalReceiver doesn't know when
applyPtr advances. While the WalReceiver can read applyPtr from shared
memory, it isn't automatically notified when that pointer advances.
This leads to latency between checking and replay if this is done in
the WalReceiver part unless we let the startup process set the state,
which would couple the two components. Am I missing something here?

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Fri, Dec 12, 2025 at 4:45 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Noah,
>
> On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Fri, Dec 12, 2025 at 12:51:00PM +0800, Xuneng Zhou wrote:
> > > Bug #19093 [1] reported that pg_stat_wal_receiver.status = 'streaming'
> > > does not accurately reflect streaming health.  In that discussion,
> > > Noah noted that even before the reported regression, status =
> > > 'streaming' was unreliable because walreceiver sets it during early
> > > startup, before attempting a connection. He suggested:
> > >
> > > "Long-term, in master only, perhaps we should introduce another status
> > > like 'connecting'. Perhaps enact the connecting->streaming status
> > > transition just before tendering the first byte of streamed WAL to the
> > > startup process. Alternatively, enact that transition when the startup
> > > process accepts the
> > > first streamed byte."
> >
> > > == Proposal ==
> > >
> > > Introduce WALRCV_CONNECTING as an intermediate state between STARTING
> > > and STREAMING:
> > >
> > > - When walreceiver starts, it enters CONNECTING (instead of going
> > > directly to STREAMING).
> > > - The transition to STREAMING occurs in XLogWalRcvFlush(), inside the
> > > existing spinlock-protected block that updates flushedUpto.
> >
> > I think this has the drawback that if the primary's WAL is incompatible,
> > e.g. unacceptable timeline, the walreceiver will still briefly enter
> > STREAMING.  That could trick monitoring.
>
> Thanks for pointing this out.
>
>  Waiting for applyPtr to advance
> > would avoid the short-lived STREAMING.  What's the feasibility of that?
>
> I think this could work, but with complications. If replay latency is
> high or replay is paused with pg_wal_replay_pause, the WalReceiver
> would stay in the CONNECTING state longer than expected. Whether this
> is ok depends on the definition of the 'connecting' state. For the
> implementation, deciding where and when to check applyPtr against LSNs
> like receiveStart is more difficult—the WalReceiver doesn't know when
> applyPtr advances. While the WalReceiver can read applyPtr from shared
> memory, it isn't automatically notified when that pointer advances.
> This leads to latency between checking and replay if this is done in
> the WalReceiver part unless we let the startup process set the state,
> which would couple the two components. Am I missing something here?
>

After some thoughts, a potential approach could be to expose a new
function in the WAL receiver that transitions the state from
CONNECTING to STREAMING. This function can then be invoked directly
from WaitForWALToBecomeAvailable in the startup process, ensuring the
state change aligns with the actual acceptance of the WAL stream.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Fri, Dec 12, 2025 at 9:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Fri, Dec 12, 2025 at 4:45 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >
> > Hi Noah,
> >
> > On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
> > >
> > > On Fri, Dec 12, 2025 at 12:51:00PM +0800, Xuneng Zhou wrote:
> > > > Bug #19093 [1] reported that pg_stat_wal_receiver.status = 'streaming'
> > > > does not accurately reflect streaming health.  In that discussion,
> > > > Noah noted that even before the reported regression, status =
> > > > 'streaming' was unreliable because walreceiver sets it during early
> > > > startup, before attempting a connection. He suggested:
> > > >
> > > > "Long-term, in master only, perhaps we should introduce another status
> > > > like 'connecting'. Perhaps enact the connecting->streaming status
> > > > transition just before tendering the first byte of streamed WAL to the
> > > > startup process. Alternatively, enact that transition when the startup
> > > > process accepts the
> > > > first streamed byte."
> > >
> > > > == Proposal ==
> > > >
> > > > Introduce WALRCV_CONNECTING as an intermediate state between STARTING
> > > > and STREAMING:
> > > >
> > > > - When walreceiver starts, it enters CONNECTING (instead of going
> > > > directly to STREAMING).
> > > > - The transition to STREAMING occurs in XLogWalRcvFlush(), inside the
> > > > existing spinlock-protected block that updates flushedUpto.
> > >
> > > I think this has the drawback that if the primary's WAL is incompatible,
> > > e.g. unacceptable timeline, the walreceiver will still briefly enter
> > > STREAMING.  That could trick monitoring.
> >
> > Thanks for pointing this out.
> >
> >  Waiting for applyPtr to advance
> > > would avoid the short-lived STREAMING.  What's the feasibility of that?
> >
> > I think this could work, but with complications. If replay latency is
> > high or replay is paused with pg_wal_replay_pause, the WalReceiver
> > would stay in the CONNECTING state longer than expected. Whether this
> > is ok depends on the definition of the 'connecting' state. For the
> > implementation, deciding where and when to check applyPtr against LSNs
> > like receiveStart is more difficult—the WalReceiver doesn't know when
> > applyPtr advances. While the WalReceiver can read applyPtr from shared
> > memory, it isn't automatically notified when that pointer advances.
> > This leads to latency between checking and replay if this is done in
> > the WalReceiver part unless we let the startup process set the state,
> > which would couple the two components. Am I missing something here?
> >
>
> After some thoughts, a potential approach could be to expose a new
> function in the WAL receiver that transitions the state from
> CONNECTING to STREAMING. This function can then be invoked directly
> from WaitForWALToBecomeAvailable in the startup process, ensuring the
> state change aligns with the actual acceptance of the WAL stream.
>

V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
the first valid WAL record is processed by the startup process. A new
function WalRcvSetStreaming is introduced to enable the transition.

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Noah Misch
Дата:
On Sun, Dec 14, 2025 at 12:45:46PM +0800, Xuneng Zhou wrote:
> On Fri, Dec 12, 2025 at 9:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Fri, Dec 12, 2025 at 4:45 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
> > > > Waiting for applyPtr to advance
> > > > would avoid the short-lived STREAMING.  What's the feasibility of that?
> > >
> > > I think this could work, but with complications. If replay latency is
> > > high or replay is paused with pg_wal_replay_pause, the WalReceiver
> > > would stay in the CONNECTING state longer than expected. Whether this
> > > is ok depends on the definition of the 'connecting' state. For the
> > > implementation, deciding where and when to check applyPtr against LSNs
> > > like receiveStart is more difficult—the WalReceiver doesn't know when
> > > applyPtr advances. While the WalReceiver can read applyPtr from shared
> > > memory, it isn't automatically notified when that pointer advances.
> > > This leads to latency between checking and replay if this is done in
> > > the WalReceiver part unless we let the startup process set the state,
> > > which would couple the two components. Am I missing something here?
> >
> > After some thoughts, a potential approach could be to expose a new
> > function in the WAL receiver that transitions the state from
> > CONNECTING to STREAMING. This function can then be invoked directly
> > from WaitForWALToBecomeAvailable in the startup process, ensuring the
> > state change aligns with the actual acceptance of the WAL stream.
> 
> V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> the first valid WAL record is processed by the startup process. A new
> function WalRcvSetStreaming is introduced to enable the transition.

The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
callee XLogWalRcvSendReply() already fetches applyPtr to send a status
message.  So I would try the following before involving the startup process
like v2 does:

1. store the applyPtr when we enter CONNECTING
2. force a status message as long as we remain in CONNECTING
3. become STREAMING when applyPtr differs from the one stored at (1)

A possible issue with all patch versions: when the primary is writing no WAL
and the standby was caught up before this walreceiver started, CONNECTING
could persist for an unbounded amount of time.  Only actual primary WAL
generation would move the walreceiver to STREAMING.  This relates to your
above point about high latency.  If that's a concern, perhaps this change
deserves a total of two new states, CONNECTING and a state that represents
"connection exists, no WAL yet applied"?



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Sun, Dec 14, 2025 at 12:45:46PM +0800, Xuneng Zhou wrote:
> > On Fri, Dec 12, 2025 at 9:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Fri, Dec 12, 2025 at 4:45 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
> > > > > Waiting for applyPtr to advance
> > > > > would avoid the short-lived STREAMING.  What's the feasibility of that?
> > > >
> > > > I think this could work, but with complications. If replay latency is
> > > > high or replay is paused with pg_wal_replay_pause, the WalReceiver
> > > > would stay in the CONNECTING state longer than expected. Whether this
> > > > is ok depends on the definition of the 'connecting' state. For the
> > > > implementation, deciding where and when to check applyPtr against LSNs
> > > > like receiveStart is more difficult—the WalReceiver doesn't know when
> > > > applyPtr advances. While the WalReceiver can read applyPtr from shared
> > > > memory, it isn't automatically notified when that pointer advances.
> > > > This leads to latency between checking and replay if this is done in
> > > > the WalReceiver part unless we let the startup process set the state,
> > > > which would couple the two components. Am I missing something here?
> > >
> > > After some thoughts, a potential approach could be to expose a new
> > > function in the WAL receiver that transitions the state from
> > > CONNECTING to STREAMING. This function can then be invoked directly
> > > from WaitForWALToBecomeAvailable in the startup process, ensuring the
> > > state change aligns with the actual acceptance of the WAL stream.
> >
> > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> > the first valid WAL record is processed by the startup process. A new
> > function WalRcvSetStreaming is introduced to enable the transition.
>
> The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> message.  So I would try the following before involving the startup process
> like v2 does:
>
> 1. store the applyPtr when we enter CONNECTING
> 2. force a status message as long as we remain in CONNECTING
> 3. become STREAMING when applyPtr differs from the one stored at (1)

Thanks for the suggestion. Using XLogWalRcvSendReply() for the
transition could make sense. My concern before is about latency in a
rare case: if the first flush completes but applyPtr hasn't advanced
yet at the time of check and then the flush stalls after that, we
might wait up to wal_receiver_status_interval (default 10s) before the
next check or indefinitely if (wal_receiver_status_interval <= 0).
This could be mitigated by shortening the wakeup interval while in
CONNECTING (step 2), which reduces worst-case latency to ~1 second.
Given that monitoring typically doesn't require sub-second precision,
this approach could be feasible.

case WALRCV_WAKEUP_REPLY:
if (WalRcv->walRcvState == WALRCV_CONNECTING)
{
/* Poll frequently while CONNECTING to avoid long latency */
wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
}

> A possible issue with all patch versions: when the primary is writing no WAL
> and the standby was caught up before this walreceiver started, CONNECTING
> could persist for an unbounded amount of time.  Only actual primary WAL
> generation would move the walreceiver to STREAMING.  This relates to your
> above point about high latency.  If that's a concern, perhaps this change
> deserves a total of two new states, CONNECTING and a state that represents
> "connection exists, no WAL yet applied"?

Yes, this could be an issue. Using two states would help address it.
That said, when the primary is idle in this case, we might end up
repeatedly polling the apply status in the state before streaming if
we implement the 1s short-interval checking like above, which could be
costful. However, If we do not implement it &&
wal_receiver_status_interval is set to < 0 && flush stalls, the
walreceiver could stay in the pre-streaming state indefinitely even if
streaming did occur, which violates the semantics. Do you think this
is a valid concern or just an artificial edge case?

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,


On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Sun, Dec 14, 2025 at 12:45:46PM +0800, Xuneng Zhou wrote:
> > > On Fri, Dec 12, 2025 at 9:52 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > On Fri, Dec 12, 2025 at 4:45 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > > > On Fri, Dec 12, 2025 at 1:05 PM Noah Misch <noah@leadboat.com> wrote:
> > > > > > Waiting for applyPtr to advance
> > > > > > would avoid the short-lived STREAMING.  What's the feasibility of that?
> > > > >
> > > > > I think this could work, but with complications. If replay latency is
> > > > > high or replay is paused with pg_wal_replay_pause, the WalReceiver
> > > > > would stay in the CONNECTING state longer than expected. Whether this
> > > > > is ok depends on the definition of the 'connecting' state. For the
> > > > > implementation, deciding where and when to check applyPtr against LSNs
> > > > > like receiveStart is more difficult—the WalReceiver doesn't know when
> > > > > applyPtr advances. While the WalReceiver can read applyPtr from shared
> > > > > memory, it isn't automatically notified when that pointer advances.
> > > > > This leads to latency between checking and replay if this is done in
> > > > > the WalReceiver part unless we let the startup process set the state,
> > > > > which would couple the two components. Am I missing something here?
> > > >
> > > > After some thoughts, a potential approach could be to expose a new
> > > > function in the WAL receiver that transitions the state from
> > > > CONNECTING to STREAMING. This function can then be invoked directly
> > > > from WaitForWALToBecomeAvailable in the startup process, ensuring the
> > > > state change aligns with the actual acceptance of the WAL stream.
> > >
> > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> > > the first valid WAL record is processed by the startup process. A new
> > > function WalRcvSetStreaming is introduced to enable the transition.
> >
> > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> > message.  So I would try the following before involving the startup process
> > like v2 does:
> >
> > 1. store the applyPtr when we enter CONNECTING
> > 2. force a status message as long as we remain in CONNECTING
> > 3. become STREAMING when applyPtr differs from the one stored at (1)
>
> Thanks for the suggestion. Using XLogWalRcvSendReply() for the
> transition could make sense. My concern before is about latency in a
> rare case: if the first flush completes but applyPtr hasn't advanced
> yet at the time of check and then the flush stalls after that, we
> might wait up to wal_receiver_status_interval (default 10s) before the
> next check or indefinitely if (wal_receiver_status_interval <= 0).
> This could be mitigated by shortening the wakeup interval while in
> CONNECTING (step 2), which reduces worst-case latency to ~1 second.
> Given that monitoring typically doesn't require sub-second precision,
> this approach could be feasible.
>
> case WALRCV_WAKEUP_REPLY:
> if (WalRcv->walRcvState == WALRCV_CONNECTING)
> {
> /* Poll frequently while CONNECTING to avoid long latency */
> wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
> }
>
> > A possible issue with all patch versions: when the primary is writing no WAL
> > and the standby was caught up before this walreceiver started, CONNECTING
> > could persist for an unbounded amount of time.  Only actual primary WAL
> > generation would move the walreceiver to STREAMING.  This relates to your
> > above point about high latency.  If that's a concern, perhaps this change
> > deserves a total of two new states, CONNECTING and a state that represents
> > "connection exists, no WAL yet applied"?
>
> Yes, this could be an issue. Using two states would help address it.
> That said, when the primary is idle in this case, we might end up
> repeatedly polling the apply status in the state before streaming if
> we implement the 1s short-interval checking like above, which could be
> costful. However, If we do not implement it &&
> wal_receiver_status_interval is set to < 0 && flush stalls, the
> walreceiver could stay in the pre-streaming state indefinitely even if
> streaming did occur, which violates the semantics. Do you think this
> is a valid concern or just an artificial edge case?

After looking more closely, I found that true indefinite waiting
requires ALL of:

wal_receiver_status_interval <= 0 (disables status updates)
wal_receiver_timeout <= 0
Primary sends no keepalives
No more WAL arrives after the first failed-check flush
Startup never sets force_reply

which is quite impossible and artificial, sorry for the noise here.
The worst-case latency of state-transition in the scenario described
above would be max(Primary keepalive, REPLY timeout, PING timeout),
which might be ok without the short-interval mitigation, given this
case is pretty rare. I plan to implement the following approach with
two new states like you suggested as v3.

1. enter CONNECTING
2. transite the state to CONNECTED/IDLE when START_REPLICATION
succeeds, store the applyPtr
2. force a status message in XLogWalRcvFlush  as long as we remain in
CONNECTED/IDLE
3. become STREAMING when applyPtr differs from the one stored at (2)

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Noah Misch
Дата:
On Sun, Dec 14, 2025 at 06:17:34PM +0800, Xuneng Zhou wrote:
> On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
> > > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> > > > the first valid WAL record is processed by the startup process. A new
> > > > function WalRcvSetStreaming is introduced to enable the transition.
> > >
> > > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> > > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> > > message.  So I would try the following before involving the startup process
> > > like v2 does:
> > >
> > > 1. store the applyPtr when we enter CONNECTING
> > > 2. force a status message as long as we remain in CONNECTING
> > > 3. become STREAMING when applyPtr differs from the one stored at (1)
> >
> > Thanks for the suggestion. Using XLogWalRcvSendReply() for the
> > transition could make sense. My concern before is about latency in a
> > rare case: if the first flush completes but applyPtr hasn't advanced
> > yet at the time of check and then the flush stalls after that, we
> > might wait up to wal_receiver_status_interval (default 10s) before the
> > next check or indefinitely if (wal_receiver_status_interval <= 0).
> > This could be mitigated by shortening the wakeup interval while in
> > CONNECTING (step 2), which reduces worst-case latency to ~1 second.
> > Given that monitoring typically doesn't require sub-second precision,
> > this approach could be feasible.
> >
> > case WALRCV_WAKEUP_REPLY:
> > if (WalRcv->walRcvState == WALRCV_CONNECTING)
> > {
> > /* Poll frequently while CONNECTING to avoid long latency */
> > wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
> > }
> >
> > > A possible issue with all patch versions: when the primary is writing no WAL
> > > and the standby was caught up before this walreceiver started, CONNECTING
> > > could persist for an unbounded amount of time.  Only actual primary WAL
> > > generation would move the walreceiver to STREAMING.  This relates to your
> > > above point about high latency.  If that's a concern, perhaps this change
> > > deserves a total of two new states, CONNECTING and a state that represents
> > > "connection exists, no WAL yet applied"?
> >
> > Yes, this could be an issue. Using two states would help address it.
> > That said, when the primary is idle in this case, we might end up
> > repeatedly polling the apply status in the state before streaming if
> > we implement the 1s short-interval checking like above, which could be
> > costful. However, If we do not implement it &&
> > wal_receiver_status_interval is set to < 0 && flush stalls, the
> > walreceiver could stay in the pre-streaming state indefinitely even if
> > streaming did occur, which violates the semantics. Do you think this
> > is a valid concern or just an artificial edge case?
> 
> After looking more closely, I found that true indefinite waiting
> requires ALL of:
> 
> wal_receiver_status_interval <= 0 (disables status updates)
> wal_receiver_timeout <= 0
> Primary sends no keepalives
> No more WAL arrives after the first failed-check flush
> Startup never sets force_reply
> 
> which is quite impossible and artificial, sorry for the noise here.

Even if indefinite wait is a negligible concern, you identified a lot of
intricacy that I hadn't pictured.  That makes your startup-process-driven
version potentially more attractive.  Forcing status messages like I was
thinking may also yield an unwanted flurry of them if the startup process is
slow.  Let's see what the patch reviewer thinks.



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Mon, Dec 15, 2025 at 12:14 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Sun, Dec 14, 2025 at 06:17:34PM +0800, Xuneng Zhou wrote:
> > On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
> > > > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> > > > > the first valid WAL record is processed by the startup process. A new
> > > > > function WalRcvSetStreaming is introduced to enable the transition.
> > > >
> > > > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> > > > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> > > > message.  So I would try the following before involving the startup process
> > > > like v2 does:
> > > >
> > > > 1. store the applyPtr when we enter CONNECTING
> > > > 2. force a status message as long as we remain in CONNECTING
> > > > 3. become STREAMING when applyPtr differs from the one stored at (1)
> > >
> > > Thanks for the suggestion. Using XLogWalRcvSendReply() for the
> > > transition could make sense. My concern before is about latency in a
> > > rare case: if the first flush completes but applyPtr hasn't advanced
> > > yet at the time of check and then the flush stalls after that, we
> > > might wait up to wal_receiver_status_interval (default 10s) before the
> > > next check or indefinitely if (wal_receiver_status_interval <= 0).
> > > This could be mitigated by shortening the wakeup interval while in
> > > CONNECTING (step 2), which reduces worst-case latency to ~1 second.
> > > Given that monitoring typically doesn't require sub-second precision,
> > > this approach could be feasible.
> > >
> > > case WALRCV_WAKEUP_REPLY:
> > > if (WalRcv->walRcvState == WALRCV_CONNECTING)
> > > {
> > > /* Poll frequently while CONNECTING to avoid long latency */
> > > wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
> > > }
> > >
> > > > A possible issue with all patch versions: when the primary is writing no WAL
> > > > and the standby was caught up before this walreceiver started, CONNECTING
> > > > could persist for an unbounded amount of time.  Only actual primary WAL
> > > > generation would move the walreceiver to STREAMING.  This relates to your
> > > > above point about high latency.  If that's a concern, perhaps this change
> > > > deserves a total of two new states, CONNECTING and a state that represents
> > > > "connection exists, no WAL yet applied"?
> > >
> > > Yes, this could be an issue. Using two states would help address it.
> > > That said, when the primary is idle in this case, we might end up
> > > repeatedly polling the apply status in the state before streaming if
> > > we implement the 1s short-interval checking like above, which could be
> > > costful. However, If we do not implement it &&
> > > wal_receiver_status_interval is set to < 0 && flush stalls, the
> > > walreceiver could stay in the pre-streaming state indefinitely even if
> > > streaming did occur, which violates the semantics. Do you think this
> > > is a valid concern or just an artificial edge case?
> >
> > After looking more closely, I found that true indefinite waiting
> > requires ALL of:
> >
> > wal_receiver_status_interval <= 0 (disables status updates)
> > wal_receiver_timeout <= 0
> > Primary sends no keepalives
> > No more WAL arrives after the first failed-check flush
> > Startup never sets force_reply
> >
> > which is quite impossible and artificial, sorry for the noise here.
>
> Even if indefinite wait is a negligible concern, you identified a lot of
> intricacy that I hadn't pictured.  That makes your startup-process-driven
> version potentially more attractive.  Forcing status messages like I was
> thinking may also yield an unwanted flurry of them if the startup process is
> slow.  Let's see what the patch reviewer thinks.

OK, both approaches are presented for review.  Adding two states to
avoid the confusion of the status caused by the stall you depicted
earlier seems reasonable to me. So, I adapted it in v3.


--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Rahila Syed
Дата:

Hi,

On Mon, Dec 15, 2025 at 9:44 AM Noah Misch <noah@leadboat.com> wrote:
On Sun, Dec 14, 2025 at 06:17:34PM +0800, Xuneng Zhou wrote:
> On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
> > > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> > > > the first valid WAL record is processed by the startup process. A new
> > > > function WalRcvSetStreaming is introduced to enable the transition.
> > >
> > > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> > > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> > > message.  So I would try the following before involving the startup process
> > > like v2 does:
> > >
> > > 1. store the applyPtr when we enter CONNECTING
> > > 2. force a status message as long as we remain in CONNECTING
> > > 3. become STREAMING when applyPtr differs from the one stored at (1)
> >
> > Thanks for the suggestion. Using XLogWalRcvSendReply() for the
> > transition could make sense. My concern before is about latency in a
> > rare case: if the first flush completes but applyPtr hasn't advanced
> > yet at the time of check and then the flush stalls after that, we
> > might wait up to wal_receiver_status_interval (default 10s) before the
> > next check or indefinitely if (wal_receiver_status_interval <= 0).
> > This could be mitigated by shortening the wakeup interval while in
> > CONNECTING (step 2), which reduces worst-case latency to ~1 second.
> > Given that monitoring typically doesn't require sub-second precision,
> > this approach could be feasible.
> >
> > case WALRCV_WAKEUP_REPLY:
> > if (WalRcv->walRcvState == WALRCV_CONNECTING)
> > {
> > /* Poll frequently while CONNECTING to avoid long latency */
> > wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
> > }
> >
> > > A possible issue with all patch versions: when the primary is writing no WAL
> > > and the standby was caught up before this walreceiver started, CONNECTING
> > > could persist for an unbounded amount of time.  Only actual primary WAL
> > > generation would move the walreceiver to STREAMING.  This relates to your
> > > above point about high latency.  If that's a concern, perhaps this change
> > > deserves a total of two new states, CONNECTING and a state that represents
> > > "connection exists, no WAL yet applied"?
> >
> > Yes, this could be an issue. Using two states would help address it.
> > That said, when the primary is idle in this case, we might end up
> > repeatedly polling the apply status in the state before streaming if
> > we implement the 1s short-interval checking like above, which could be
> > costful. However, If we do not implement it &&
> > wal_receiver_status_interval is set to < 0 && flush stalls, the
> > walreceiver could stay in the pre-streaming state indefinitely even if
> > streaming did occur, which violates the semantics. Do you think this
> > is a valid concern or just an artificial edge case?
>
> After looking more closely, I found that true indefinite waiting
> requires ALL of:
>
> wal_receiver_status_interval <= 0 (disables status updates)
> wal_receiver_timeout <= 0
> Primary sends no keepalives
> No more WAL arrives after the first failed-check flush
> Startup never sets force_reply
>
> which is quite impossible and artificial, sorry for the noise here.

Even if indefinite wait is a negligible concern, you identified a lot of
intricacy that I hadn't pictured.  That makes your startup-process-driven
version potentially more attractive.  Forcing status messages like I was
thinking may also yield an unwanted flurry of them if the startup process is
slow.  Let's see what the patch reviewer thinks.

FWIW, I think doing it in startup might be slightly better.
It seems more logical to make the state change near the point where the status
is updated, as this helps prevent reading the status from shared memory and
reduces related delays.

The current proposal is to advance the state to STREAMING after applyPtr has
been updated.
IIUC, the rationale is to avoid having a short-lived streaming state if applying WAL fails.
However, this approach can be confusing because the receiver may already be receiving
WAL from the primary, yet its state remains CONNECTING until the WAL is flushed.

Would it be better to advance the state to streaming after the connection
is successfully established and the following LOG message is emitted?
    
        if (walrcv_startstreaming(wrconn, &options))
        {
            if (first_stream)
                ereport(LOG,
                        errmsg("started streaming WAL from primary at %X/%08X on timeline %u",
                               LSN_FORMAT_ARGS(startpoint), startpointTLI));

Thank you,
Rahila Syed
            

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Rahila,

Thanks for looking into this.

On Mon, Dec 15, 2025 at 9:48 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
>
>
> Hi,
>
> On Mon, Dec 15, 2025 at 9:44 AM Noah Misch <noah@leadboat.com> wrote:
>>
>> On Sun, Dec 14, 2025 at 06:17:34PM +0800, Xuneng Zhou wrote:
>> > On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>> > > On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
>> > > > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
>> > > > > the first valid WAL record is processed by the startup process. A new
>> > > > > function WalRcvSetStreaming is introduced to enable the transition.
>> > > >
>> > > > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
>> > > > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
>> > > > message.  So I would try the following before involving the startup process
>> > > > like v2 does:
>> > > >
>> > > > 1. store the applyPtr when we enter CONNECTING
>> > > > 2. force a status message as long as we remain in CONNECTING
>> > > > 3. become STREAMING when applyPtr differs from the one stored at (1)
>> > >
>> > > Thanks for the suggestion. Using XLogWalRcvSendReply() for the
>> > > transition could make sense. My concern before is about latency in a
>> > > rare case: if the first flush completes but applyPtr hasn't advanced
>> > > yet at the time of check and then the flush stalls after that, we
>> > > might wait up to wal_receiver_status_interval (default 10s) before the
>> > > next check or indefinitely if (wal_receiver_status_interval <= 0).
>> > > This could be mitigated by shortening the wakeup interval while in
>> > > CONNECTING (step 2), which reduces worst-case latency to ~1 second.
>> > > Given that monitoring typically doesn't require sub-second precision,
>> > > this approach could be feasible.
>> > >
>> > > case WALRCV_WAKEUP_REPLY:
>> > > if (WalRcv->walRcvState == WALRCV_CONNECTING)
>> > > {
>> > > /* Poll frequently while CONNECTING to avoid long latency */
>> > > wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
>> > > }
>> > >
>> > > > A possible issue with all patch versions: when the primary is writing no WAL
>> > > > and the standby was caught up before this walreceiver started, CONNECTING
>> > > > could persist for an unbounded amount of time.  Only actual primary WAL
>> > > > generation would move the walreceiver to STREAMING.  This relates to your
>> > > > above point about high latency.  If that's a concern, perhaps this change
>> > > > deserves a total of two new states, CONNECTING and a state that represents
>> > > > "connection exists, no WAL yet applied"?
>> > >
>> > > Yes, this could be an issue. Using two states would help address it.
>> > > That said, when the primary is idle in this case, we might end up
>> > > repeatedly polling the apply status in the state before streaming if
>> > > we implement the 1s short-interval checking like above, which could be
>> > > costful. However, If we do not implement it &&
>> > > wal_receiver_status_interval is set to < 0 && flush stalls, the
>> > > walreceiver could stay in the pre-streaming state indefinitely even if
>> > > streaming did occur, which violates the semantics. Do you think this
>> > > is a valid concern or just an artificial edge case?
>> >
>> > After looking more closely, I found that true indefinite waiting
>> > requires ALL of:
>> >
>> > wal_receiver_status_interval <= 0 (disables status updates)
>> > wal_receiver_timeout <= 0
>> > Primary sends no keepalives
>> > No more WAL arrives after the first failed-check flush
>> > Startup never sets force_reply
>> >
>> > which is quite impossible and artificial, sorry for the noise here.
>>
>> Even if indefinite wait is a negligible concern, you identified a lot of
>> intricacy that I hadn't pictured.  That makes your startup-process-driven
>> version potentially more attractive.  Forcing status messages like I was
>> thinking may also yield an unwanted flurry of them if the startup process is
>> slow.  Let's see what the patch reviewer thinks.
>
>
> FWIW, I think doing it in startup might be slightly better.
> It seems more logical to make the state change near the point where the status
> is updated, as this helps prevent reading the status from shared memory and
> reduces related delays.
>
> The current proposal is to advance the state to STREAMING after applyPtr has
> been updated.
> IIUC, the rationale is to avoid having a short-lived streaming state if applying WAL fails.
> However, this approach can be confusing because the receiver may already be receiving
> WAL from the primary, yet its state remains CONNECTING until the WAL is flushed.
>
> Would it be better to advance the state to streaming after the connection
> is successfully established and the following LOG message is emitted?
>
>         if (walrcv_startstreaming(wrconn, &options))
>         {
>             if (first_stream)
>                 ereport(LOG,
>                         errmsg("started streaming WAL from primary at %X/%08X on timeline %u",
>                                LSN_FORMAT_ARGS(startpoint), startpointTLI));

AFAICS, this may depend on how we define the streaming status. If
streaming is defined simply as “the connection has been established
and walreceiver is ready to operate,” then this approach fits well and
keeps the model simple. However, if streaming is meant to indicate
that WAL has actually started flowing and replay is in progress, then
this approach could fall short, particularly for the short-lived
streaming cases you mentioned. Introducing finer-grained states can
handle these edge cases more accurately, but it also makes the state
transitions more complex. That said, I’m not well positioned to fully
evaluate the trade-offs here, as I’m not a day-to-day end user.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Mon, Dec 15, 2025 at 11:38 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Rahila,
>
> Thanks for looking into this.
>
> On Mon, Dec 15, 2025 at 9:48 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
> >
> >
> > Hi,
> >
> > On Mon, Dec 15, 2025 at 9:44 AM Noah Misch <noah@leadboat.com> wrote:
> >>
> >> On Sun, Dec 14, 2025 at 06:17:34PM +0800, Xuneng Zhou wrote:
> >> > On Sun, Dec 14, 2025 at 4:55 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >> > > On Sun, Dec 14, 2025 at 1:14 PM Noah Misch <noah@leadboat.com> wrote:
> >> > > > > V2 makes the transition from WALRCV_CONNECTING to STREAMING only when
> >> > > > > the first valid WAL record is processed by the startup process. A new
> >> > > > > function WalRcvSetStreaming is introduced to enable the transition.
> >> > > >
> >> > > > The original patch set STREAMING in XLogWalRcvFlush().  XLogWalRcvFlush()
> >> > > > callee XLogWalRcvSendReply() already fetches applyPtr to send a status
> >> > > > message.  So I would try the following before involving the startup process
> >> > > > like v2 does:
> >> > > >
> >> > > > 1. store the applyPtr when we enter CONNECTING
> >> > > > 2. force a status message as long as we remain in CONNECTING
> >> > > > 3. become STREAMING when applyPtr differs from the one stored at (1)
> >> > >
> >> > > Thanks for the suggestion. Using XLogWalRcvSendReply() for the
> >> > > transition could make sense. My concern before is about latency in a
> >> > > rare case: if the first flush completes but applyPtr hasn't advanced
> >> > > yet at the time of check and then the flush stalls after that, we
> >> > > might wait up to wal_receiver_status_interval (default 10s) before the
> >> > > next check or indefinitely if (wal_receiver_status_interval <= 0).
> >> > > This could be mitigated by shortening the wakeup interval while in
> >> > > CONNECTING (step 2), which reduces worst-case latency to ~1 second.
> >> > > Given that monitoring typically doesn't require sub-second precision,
> >> > > this approach could be feasible.
> >> > >
> >> > > case WALRCV_WAKEUP_REPLY:
> >> > > if (WalRcv->walRcvState == WALRCV_CONNECTING)
> >> > > {
> >> > > /* Poll frequently while CONNECTING to avoid long latency */
> >> > > wakeup[reason] = TimestampTzPlusMilliseconds(now, 1000);
> >> > > }
> >> > >
> >> > > > A possible issue with all patch versions: when the primary is writing no WAL
> >> > > > and the standby was caught up before this walreceiver started, CONNECTING
> >> > > > could persist for an unbounded amount of time.  Only actual primary WAL
> >> > > > generation would move the walreceiver to STREAMING.  This relates to your
> >> > > > above point about high latency.  If that's a concern, perhaps this change
> >> > > > deserves a total of two new states, CONNECTING and a state that represents
> >> > > > "connection exists, no WAL yet applied"?
> >> > >
> >> > > Yes, this could be an issue. Using two states would help address it.
> >> > > That said, when the primary is idle in this case, we might end up
> >> > > repeatedly polling the apply status in the state before streaming if
> >> > > we implement the 1s short-interval checking like above, which could be
> >> > > costful. However, If we do not implement it &&
> >> > > wal_receiver_status_interval is set to < 0 && flush stalls, the
> >> > > walreceiver could stay in the pre-streaming state indefinitely even if
> >> > > streaming did occur, which violates the semantics. Do you think this
> >> > > is a valid concern or just an artificial edge case?
> >> >
> >> > After looking more closely, I found that true indefinite waiting
> >> > requires ALL of:
> >> >
> >> > wal_receiver_status_interval <= 0 (disables status updates)
> >> > wal_receiver_timeout <= 0
> >> > Primary sends no keepalives
> >> > No more WAL arrives after the first failed-check flush
> >> > Startup never sets force_reply
> >> >
> >> > which is quite impossible and artificial, sorry for the noise here.
> >>
> >> Even if indefinite wait is a negligible concern, you identified a lot of
> >> intricacy that I hadn't pictured.  That makes your startup-process-driven
> >> version potentially more attractive.  Forcing status messages like I was
> >> thinking may also yield an unwanted flurry of them if the startup process is
> >> slow.  Let's see what the patch reviewer thinks.
> >
> >
> > FWIW, I think doing it in startup might be slightly better.
> > It seems more logical to make the state change near the point where the status
> > is updated, as this helps prevent reading the status from shared memory and
> > reduces related delays.
> >
> > The current proposal is to advance the state to STREAMING after applyPtr has
> > been updated.
> > IIUC, the rationale is to avoid having a short-lived streaming state if applying WAL fails.
> > However, this approach can be confusing because the receiver may already be receiving
> > WAL from the primary, yet its state remains CONNECTING until the WAL is flushed.
> >
> > Would it be better to advance the state to streaming after the connection
> > is successfully established and the following LOG message is emitted?
> >
> >         if (walrcv_startstreaming(wrconn, &options))
> >         {
> >             if (first_stream)
> >                 ereport(LOG,
> >                         errmsg("started streaming WAL from primary at %X/%08X on timeline %u",
> >                                LSN_FORMAT_ARGS(startpoint), startpointTLI));
>
> AFAICS, this may depend on how we define the streaming status. If
> streaming is defined simply as “the connection has been established
> and walreceiver is ready to operate,” then this approach fits well and
> keeps the model simple. However, if streaming is meant to indicate
> that WAL has actually started flowing and replay is in progress, then
> this approach could fall short, particularly for the short-lived
> streaming cases you mentioned. Introducing finer-grained states can
> handle these edge cases more accurately, but it also makes the state
> transitions more complex. That said, I’m not well positioned to fully
> evaluate the trade-offs here, as I’m not a day-to-day end user.
>

After some thoughts, I’m more inclined toward a startup-process–driven
approach. Marking the status as streaming immediately after the
connection is established seems not provide sufficient accuracy for
monitoring purposes. Introducing an intermediate state, such as
connected, would help reduce confusion when the startup process is
stalled and would make it easier for users to detect and diagnose
anomalies.

V4 whitelisted CONNECTED and CONNECTING in WalRcvWaitForStartPosition
to handle valid stream termination scenarios without triggering a
FATAL error.

Specifically, the walreceiver may need to transition to WAITING (idle) if:
   1. 'CONNECTED': The handshake succeeded (COPY_BOTH started), but
the stream ended before any WAL was applied (e.g., timeline divergence
detected mid-stream).
   2. 'CONNECTING': The handshake completed (START_REPLICATION
acknowledged), but the primary declined to stream (e.g., no WAL
available on the requested timeline).

  In both cases, the receiver should pause and await a new timeline or
restart position from the startup process.

Both approaches have been updated.

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Sun, Jan 11, 2026 at 08:56:57PM +0800, Xuneng Zhou wrote:
> After some thoughts, I’m more inclined toward a startup-process–driven
> approach. Marking the status as streaming immediately after the
> connection is established seems not provide sufficient accuracy for
> monitoring purposes. Introducing an intermediate state, such as
> connected, would help reduce confusion when the startup process is
> stalled and would make it easier for users to detect and diagnose
> anomalies.
>
> V4 whitelisted CONNECTED and CONNECTING in WalRcvWaitForStartPosition
> to handle valid stream termination scenarios without triggering a
> FATAL error.
>
> Specifically, the walreceiver may need to transition to WAITING (idle) if:
>    1. 'CONNECTED': The handshake succeeded (COPY_BOTH started), but
> the stream ended before any WAL was applied (e.g., timeline divergence
> detected mid-stream).
>    2. 'CONNECTING': The handshake completed (START_REPLICATION
> acknowledged), but the primary declined to stream (e.g., no WAL
> available on the requested timeline).
>
>   In both cases, the receiver should pause and await a new timeline or
> restart position from the startup process.

This stuff depends on the philosophical difference you want to put
behind "connecting" and "streaming".  My own opinion is that it is a
non-starter to introduce more states that can be set by the startup
process, and that a new state should reflect what we do in the code.
We already have some of that for in the start and stop phases because
we want some ordering when the WAL receiver process is spawned and at
shutdown.  That's just a simple way to say that we should not rely on
more static variables to control how to set one or more states, and I
don't see why that's actually required here?  initialApplyPtr and
force_reply are what I see as potential recipes for more bugs in the
long term, as showed in the first approach.  The second patch,
introducing a similar new complexity with walrcv_streaming_set, is no
better in terms of complexity added.

The main take that I can retrieve from this thread is that it may take
time between the moment we begin a WAL receiver in WalReceiverMain(),
where walRcvState is switched to WALRCV_STREAMING, and the moment we
actually have established a connection, location where "first_stream =
false" (which is just to track if a WAL receiver is restarting,
actually) after walrcv_startstreaming() has returned true, so as far
as I can see you would be happy enough with the addition of a single
state called CONNECTING, set at the beginning of WalReceiverMain()
instead of where STREAMING is set now.  The same would sound kind of
true for WalRcvWaitForStartPosition(), because we are not actively
streaming yet, still we are marking the WAL receiver as streaming, so
the current code feels like we are cheating as if we define
"streaming" as a WAL receiver that has already done an active
connection.  We also want the WAL receiver to be killable by the
startup process while in "connecting" or "streaming" more.

Hence I would suggest something like the following guidelines:
- Add only a CONNECTING state.  Set this state where we switch the
state to "streaming" now, aka the two locations in the tree now.
- Switch to STREAMING once the connection has been established, as
returned by walrcv_startstreaming(), because we are acknowledging *in
the code* that we have started streaming successfully.
- Update the docs to reflect the new state, because this state can
show up in the system view pg_stat_wal_receiver.
- I am not convinved by what we gain with a CONNECTED state, either.
Drop it.
- The fact that we'd want to switch the state once the startup process
has acknowleged the reception of the first byte from the stream is
already something we track in the WAL receiver, AFAIK.  I think that
there would be a point in expanding the SQL functions to report more
states of the startup process, including the data received by the
startup process, but we should not link that to the state of the WAL
receiver.  An extra reason to not do that: WAL receivers are not the
only source feeding data to the startup process, we could have data
pushed to pg_wal/, or archive commands/modules doing this job.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Michael,

On Mon, Jan 19, 2026 at 8:13 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Jan 11, 2026 at 08:56:57PM +0800, Xuneng Zhou wrote:
> > After some thoughts, I’m more inclined toward a startup-process–driven
> > approach. Marking the status as streaming immediately after the
> > connection is established seems not provide sufficient accuracy for
> > monitoring purposes. Introducing an intermediate state, such as
> > connected, would help reduce confusion when the startup process is
> > stalled and would make it easier for users to detect and diagnose
> > anomalies.
> >
> > V4 whitelisted CONNECTED and CONNECTING in WalRcvWaitForStartPosition
> > to handle valid stream termination scenarios without triggering a
> > FATAL error.
> >
> > Specifically, the walreceiver may need to transition to WAITING (idle) if:
> >    1. 'CONNECTED': The handshake succeeded (COPY_BOTH started), but
> > the stream ended before any WAL was applied (e.g., timeline divergence
> > detected mid-stream).
> >    2. 'CONNECTING': The handshake completed (START_REPLICATION
> > acknowledged), but the primary declined to stream (e.g., no WAL
> > available on the requested timeline).
> >
> >   In both cases, the receiver should pause and await a new timeline or
> > restart position from the startup process.
>
> This stuff depends on the philosophical difference you want to put
> behind "connecting" and "streaming".  My own opinion is that it is a
> non-starter to introduce more states that can be set by the startup
> process, and that a new state should reflect what we do in the code.
> We already have some of that for in the start and stop phases because
> we want some ordering when the WAL receiver process is spawned and at
> shutdown.  That's just a simple way to say that we should not rely on
> more static variables to control how to set one or more states, and I
> don't see why that's actually required here?  initialApplyPtr and
> force_reply are what I see as potential recipes for more bugs in the
> long term, as showed in the first approach.  The second patch,
> introducing a similar new complexity with walrcv_streaming_set, is no
> better in terms of complexity added.
> The main take that I can retrieve from this thread is that it may take
> time between the moment we begin a WAL receiver in WalReceiverMain(),
> where walRcvState is switched to WALRCV_STREAMING, and the moment we
> actually have established a connection, location where "first_stream =
> false" (which is just to track if a WAL receiver is restarting,
> actually) after walrcv_startstreaming() has returned true, so as far
> as I can see you would be happy enough with the addition of a single
> state called CONNECTING, set at the beginning of WalReceiverMain()
> instead of where STREAMING is set now.  The same would sound kind of
> true for WalRcvWaitForStartPosition(), because we are not actively
> streaming yet, still we are marking the WAL receiver as streaming, so
> the current code feels like we are cheating as if we define
> "streaming" as a WAL receiver that has already done an active
> connection.  We also want the WAL receiver to be killable by the
> startup process while in "connecting" or "streaming" more.
>
> Hence I would suggest something like the following guidelines:
> - Add only a CONNECTING state.  Set this state where we switch the
> state to "streaming" now, aka the two locations in the tree now.
> - Switch to STREAMING once the connection has been established, as
> returned by walrcv_startstreaming(), because we are acknowledging *in
> the code* that we have started streaming successfully.
> - Update the docs to reflect the new state, because this state can
> show up in the system view pg_stat_wal_receiver.
> - I am not convinved by what we gain with a CONNECTED state, either.
> Drop it.
> - The fact that we'd want to switch the state once the startup process
> has acknowleged the reception of the first byte from the stream is
> already something we track in the WAL receiver, AFAIK.

Thank you for the detailed feedback. I agree with your analysis — the
simpler approach seems preferable and should be sufficient in most
cases. Tightly coupling the startup process with the WAL receiver to
set state is not very ideal. I'll post v5 with the simplified
walreceiver changes as you suggested shortly.

> I think that
> there would be a point in expanding the SQL functions to report more
> states of the startup process, including the data received by the
> startup process, but we should not link that to the state of the WAL
> receiver.  An extra reason to not do that: WAL receivers are not the
> only source feeding data to the startup process, we could have data
> pushed to pg_wal/, or archive commands/modules doing this job.

+1. I'll prepare a separate patch to expose startup process metrics
like pg_stat_get_wal_receiver does. This would complement
pg_stat_wal_receiver without coupling the two subsystems.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Mon, Jan 19, 2026 at 11:35:28PM +0800, Xuneng Zhou wrote:
> On Mon, Jan 19, 2026 at 8:13 AM Michael Paquier <michael@paquier.xyz> wrote:
>> I think that
>> there would be a point in expanding the SQL functions to report more
>> states of the startup process, including the data received by the
>> startup process, but we should not link that to the state of the WAL
>> receiver.  An extra reason to not do that: WAL receivers are not the
>> only source feeding data to the startup process, we could have data
>> pushed to pg_wal/, or archive commands/modules doing this job.
>
> +1. I'll prepare a separate patch to expose startup process metrics
> like pg_stat_get_wal_receiver does. This would complement
> pg_stat_wal_receiver without coupling the two subsystems.

In this area, I mean to expose the contents of XLogRecoveryCtlData at
SQL level.  It may be better to move this structure to a header, and
have the new SQL function in xlogfuncs.c.  That's at least how I would
shape such a change.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Tue, Jan 20, 2026 at 7:21 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jan 19, 2026 at 11:35:28PM +0800, Xuneng Zhou wrote:
> > On Mon, Jan 19, 2026 at 8:13 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> I think that
> >> there would be a point in expanding the SQL functions to report more
> >> states of the startup process, including the data received by the
> >> startup process, but we should not link that to the state of the WAL
> >> receiver.  An extra reason to not do that: WAL receivers are not the
> >> only source feeding data to the startup process, we could have data
> >> pushed to pg_wal/, or archive commands/modules doing this job.
> >
> > +1. I'll prepare a separate patch to expose startup process metrics
> > like pg_stat_get_wal_receiver does. This would complement
> > pg_stat_wal_receiver without coupling the two subsystems.
>
> In this area, I mean to expose the contents of XLogRecoveryCtlData at
> SQL level.  It may be better to move this structure to a header, and
> have the new SQL function in xlogfuncs.c.  That's at least how I would
> shape such a change.
> --
> Michael

Thanks for the suggestion! It makes sense to me.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Mon, Jan 19, 2026 at 11:35 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi Michael,
>
> On Mon, Jan 19, 2026 at 8:13 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Sun, Jan 11, 2026 at 08:56:57PM +0800, Xuneng Zhou wrote:
> > > After some thoughts, I’m more inclined toward a startup-process–driven
> > > approach. Marking the status as streaming immediately after the
> > > connection is established seems not provide sufficient accuracy for
> > > monitoring purposes. Introducing an intermediate state, such as
> > > connected, would help reduce confusion when the startup process is
> > > stalled and would make it easier for users to detect and diagnose
> > > anomalies.
> > >
> > > V4 whitelisted CONNECTED and CONNECTING in WalRcvWaitForStartPosition
> > > to handle valid stream termination scenarios without triggering a
> > > FATAL error.
> > >
> > > Specifically, the walreceiver may need to transition to WAITING (idle) if:
> > >    1. 'CONNECTED': The handshake succeeded (COPY_BOTH started), but
> > > the stream ended before any WAL was applied (e.g., timeline divergence
> > > detected mid-stream).
> > >    2. 'CONNECTING': The handshake completed (START_REPLICATION
> > > acknowledged), but the primary declined to stream (e.g., no WAL
> > > available on the requested timeline).
> > >
> > >   In both cases, the receiver should pause and await a new timeline or
> > > restart position from the startup process.
> >
> > This stuff depends on the philosophical difference you want to put
> > behind "connecting" and "streaming".  My own opinion is that it is a
> > non-starter to introduce more states that can be set by the startup
> > process, and that a new state should reflect what we do in the code.
> > We already have some of that for in the start and stop phases because
> > we want some ordering when the WAL receiver process is spawned and at
> > shutdown.  That's just a simple way to say that we should not rely on
> > more static variables to control how to set one or more states, and I
> > don't see why that's actually required here?  initialApplyPtr and
> > force_reply are what I see as potential recipes for more bugs in the
> > long term, as showed in the first approach.  The second patch,
> > introducing a similar new complexity with walrcv_streaming_set, is no
> > better in terms of complexity added.
> > The main take that I can retrieve from this thread is that it may take
> > time between the moment we begin a WAL receiver in WalReceiverMain(),
> > where walRcvState is switched to WALRCV_STREAMING, and the moment we
> > actually have established a connection, location where "first_stream =
> > false" (which is just to track if a WAL receiver is restarting,
> > actually) after walrcv_startstreaming() has returned true, so as far
> > as I can see you would be happy enough with the addition of a single
> > state called CONNECTING, set at the beginning of WalReceiverMain()
> > instead of where STREAMING is set now.  The same would sound kind of
> > true for WalRcvWaitForStartPosition(), because we are not actively
> > streaming yet, still we are marking the WAL receiver as streaming, so
> > the current code feels like we are cheating as if we define
> > "streaming" as a WAL receiver that has already done an active
> > connection.  We also want the WAL receiver to be killable by the
> > startup process while in "connecting" or "streaming" more.
> >
> > Hence I would suggest something like the following guidelines:
> > - Add only a CONNECTING state.  Set this state where we switch the
> > state to "streaming" now, aka the two locations in the tree now.
> > - Switch to STREAMING once the connection has been established, as
> > returned by walrcv_startstreaming(), because we are acknowledging *in
> > the code* that we have started streaming successfully.
> > - Update the docs to reflect the new state, because this state can
> > show up in the system view pg_stat_wal_receiver.
> > - I am not convinved by what we gain with a CONNECTED state, either.
> > Drop it.
> > - The fact that we'd want to switch the state once the startup process
> > has acknowleged the reception of the first byte from the stream is
> > already something we track in the WAL receiver, AFAIK.
>
> Thank you for the detailed feedback. I agree with your analysis — the
> simpler approach seems preferable and should be sufficient in most
> cases. Tightly coupling the startup process with the WAL receiver to
> set state is not very ideal. I'll post v5 with the simplified
> walreceiver changes as you suggested shortly.

Please see v5 of the updated patch.


--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Wed, Jan 21, 2026 at 08:40:16PM +0800, Xuneng Zhou wrote:
> Please see v5 of the updated patch.

This seems acceptable here, cool.

Something worth a note: I thought that the states of the WAL sender
were documented already, and I was wrong in thinking that it was the
case.  Sorry.  :)

By the way, the list of state values you are specifying in the patch
is not complete.  In theory, we allow all state values like "stopped"
to show up in a single function call of pg_stat_get_wal_receiver(),
but you are not documenting all of them.  Using a list (like with the
<itemizedlist> markup) would be better.

The documentation improvement can be treated as a separate change,
worth its own before adding the new "connecting" state.  Could you
split that into two patches please?  That would make one patch for
the documentation changes with the list of state values that can be
reported, and a second patch for the new "connecting" state.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

Thanks for the feedbacks.

On Thu, Jan 22, 2026 at 8:20 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 21, 2026 at 08:40:16PM +0800, Xuneng Zhou wrote:
> > Please see v5 of the updated patch.
>
> This seems acceptable here, cool.
>
> Something worth a note: I thought that the states of the WAL sender
> were documented already, and I was wrong in thinking that it was the
> case.  Sorry.  :)
>
> By the way, the list of state values you are specifying in the patch
> is not complete.  In theory, we allow all state values like "stopped"
> to show up in a single function call of pg_stat_get_wal_receiver(),
> but you are not documenting all of them.

My concern for not adding it before is that this status could be
invisible to users.
Looking at pg_stat_get_wal_receiver():

if (pid == 0 || !ready_to_display)
    PG_RETURN_NULL();

The function returns NULL (no row) when pid == 0.

When the WAL receiver is in WALRCV_STOPPED state (WalRcvDie), pid is set to 0:
walrcv->walRcvState = WALRCV_STOPPED;
walrcv->pid = 0;

So the view returns no row rather than a row with status = 'stopped'.
But for completeness, maybe we should add it.

> Using a list (like with the
> <itemizedlist> markup) would be better.

The states are now wrapped in an itemizedlist.

> The documentation improvement can be treated as a separate change,
> worth its own before adding the new "connecting" state.  Could you
> split that into two patches please?  That would make one patch for
> the documentation changes with the list of state values that can be
> reported, and a second patch for the new "connecting" state.

The changes have been splitted into two patches as suggested.

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Chao Li
Дата:

> On Jan 22, 2026, at 12:37, Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> Thanks for the feedbacks.
>
> On Thu, Jan 22, 2026 at 8:20 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Wed, Jan 21, 2026 at 08:40:16PM +0800, Xuneng Zhou wrote:
>>> Please see v5 of the updated patch.
>>
>> This seems acceptable here, cool.
>>
>> Something worth a note: I thought that the states of the WAL sender
>> were documented already, and I was wrong in thinking that it was the
>> case.  Sorry.  :)
>>
>> By the way, the list of state values you are specifying in the patch
>> is not complete.  In theory, we allow all state values like "stopped"
>> to show up in a single function call of pg_stat_get_wal_receiver(),
>> but you are not documenting all of them.
>
> My concern for not adding it before is that this status could be
> invisible to users.
> Looking at pg_stat_get_wal_receiver():
>
> if (pid == 0 || !ready_to_display)
>    PG_RETURN_NULL();
>
> The function returns NULL (no row) when pid == 0.
>
> When the WAL receiver is in WALRCV_STOPPED state (WalRcvDie), pid is set to 0:
> walrcv->walRcvState = WALRCV_STOPPED;
> walrcv->pid = 0;
>
> So the view returns no row rather than a row with status = 'stopped'.
> But for completeness, maybe we should add it.
>
>> Using a list (like with the
>> <itemizedlist> markup) would be better.
>
> The states are now wrapped in an itemizedlist.
>
>> The documentation improvement can be treated as a separate change,
>> worth its own before adding the new "connecting" state.  Could you
>> split that into two patches please?  That would make one patch for
>> the documentation changes with the list of state values that can be
>> reported, and a second patch for the new "connecting" state.
>
> The changes have been splitted into two patches as suggested.
>
> --
> Best,
> Xuneng
>
<v6-0002-Add-WALRCV_CONNECTING-state-to-walreceiver.patch><v6-0001-Doc-document-all-pg_stat_wal_receiver-status-valu.patch>

Hi Xuneng,

I just reviewed the patch. It splits the current STREAMING state into CONNECTING and STREAMING, where CONNECTING
representsthe connection establishing phase, which makes sense. The patch is solid, I have just a few small comments: 

1 - 0001
```
+         <para>
+          <literal>stopped</literal>: WAL receiver process is not running.
+          This state is normally not visible because the view returns no row
+          when the WAL receiver is not running.
+         </para>
```

“Is not running” appears twice in this short paragraph, looks redundant. Suggestion:
```
<literal>stopped</literal>: WAL receiver process is not running.
This state is normally not visible because the view returns no row in this case.
```

2 - 0002
```
+    WALRCV_CONNECTING,            /* connecting to primary, replication not yet
+                                 * started */
```

I think “primary” is inaccurate. A connection destination could be a primary, or the other standby, so maybe change
“primary”to “upstream server”. 


3 - 0002
```
     * Mark walreceiver as running in shared memory.
```

I think we should update this comment, changing “running” to “connecting”. There is not a “running” state, before this
patch,“streaming” can roughly equal to “running”, after this patch, “running” is kinda unclear. 

4 - 0002
```
+            /* Connection established, switch to streaming state */
+            SpinLockAcquire(&walrcv->mutex);
+            Assert(walrcv->walRcvState == WALRCV_CONNECTING);
+            walrcv->walRcvState = WALRCV_STREAMING;
+            SpinLockRelease(&walrcv->mutex);
```

I don’t feel good with this Assert(). Looking at ShutdownWalRcv(), the startup process might set the state to STOPPING,
sothere is a race. 

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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/







Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Thu, Jan 22, 2026 at 12:37:42PM +0800, Xuneng Zhou wrote:
> So the view returns no row rather than a row with status = 'stopped'.
> But for completeness, maybe we should add it.

Yeah, you are making me doubt here, "stopped" is the only state that
would never show up.  After pondering a bit on this one, I have
removed this part, and applied the rest of 0001 on HEAD after some
reordering of the items and fixing a <para> markup which was at an
incorrect location.  That's one.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Thu, Jan 22, 2026 at 03:34:32PM +0800, Chao Li wrote:
> “Is not running” appears twice in this short paragraph, looks redundant. Suggestion:
> ```
> <literal>stopped</literal>: WAL receiver process is not running.
> This state is normally not visible because the view returns no row in this case.
> ```

I have removed this part at the end, see 1a1e733b6231.  Let's see
later about the second episode of this saga.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Chao,

Thanks for looking into this.

>
> Hi Xuneng,
>
> I just reviewed the patch. It splits the current STREAMING state into CONNECTING and STREAMING, where CONNECTING
representsthe connection establishing phase, which makes sense. The patch is solid, I have just a few small comments: 
>
> 1 - 0001
> ```
> +         <para>
> +          <literal>stopped</literal>: WAL receiver process is not running.
> +          This state is normally not visible because the view returns no row
> +          when the WAL receiver is not running.
> +         </para>
> ```
>
> “Is not running” appears twice in this short paragraph, looks redundant. Suggestion:
> ```
> <literal>stopped</literal>: WAL receiver process is not running.
> This state is normally not visible because the view returns no row in this case.
> ```
>
> 2 - 0002
> ```
> +       WALRCV_CONNECTING,                      /* connecting to primary, replication not yet
> +                                                                * started */
> ```
>
> I think “primary” is inaccurate. A connection destination could be a primary, or the other standby, so maybe change
“primary”to “upstream server”. 

Yeah, "upstream server" is more precise. There are multiple
user-facing log messages in this file that also use “primary”. I’m
wondering whether we should update those as well.

>
>
> 3 - 0002
> ```
>      * Mark walreceiver as running in shared memory.
> ```
>
> I think we should update this comment, changing “running” to “connecting”. There is not a “running” state, before
thispatch, “streaming” can roughly equal to “running”, after this patch, “running” is kinda unclear. 
>

It has been changed.

> 4 - 0002
> ```
> +                       /* Connection established, switch to streaming state */
> +                       SpinLockAcquire(&walrcv->mutex);
> +                       Assert(walrcv->walRcvState == WALRCV_CONNECTING);
> +                       walrcv->walRcvState = WALRCV_STREAMING;
> +                       SpinLockRelease(&walrcv->mutex);
> ```
>
> I don’t feel good with this Assert(). Looking at ShutdownWalRcv(), the startup process might set the state to
STOPPING,so there is a race. 
>
> 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.

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Thu, Jan 22, 2026 at 4:08 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jan 22, 2026 at 12:37:42PM +0800, Xuneng Zhou wrote:
> > So the view returns no row rather than a row with status = 'stopped'.
> > But for completeness, maybe we should add it.
>
> Yeah, you are making me doubt here, "stopped" is the only state that
> would never show up.  After pondering a bit on this one, I have
> removed this part, and applied the rest of 0001 on HEAD after some
> reordering of the items and fixing a <para> markup which was at an
> incorrect location.  That's one.
> --
> Michael

Thanks for updating and pushing it.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
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

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Chao Li
Дата:

> On Jan 23, 2026, at 06:54, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

+1.

If current state is not CONNECTING, then leave it as is, which will then follow the original logic. Unless we find some
issuein future, I think we can take this approach for now. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/







Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Fri, Jan 23, 2026 at 6:54 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

Thanks Michael — agreed. Patch v7 dropped the extra fast‑exit path and
kept the change minimal: only switch to STREAMING if the state is
still CONNECTING, otherwise leave it unchanged.

--
Best,
Xuneng

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Fri, Jan 23, 2026 at 12:07:36PM +0800, Xuneng Zhou wrote:
> Thanks Michael — agreed. Patch v7 dropped the extra fast‑exit path and
> kept the change minimal: only switch to STREAMING if the state is
> still CONNECTING, otherwise leave it unchanged.

Thanks for the newer version of the patch.  I have been playing with
it today, even before you have sent this v7, with a set of injection
points to make the WAL receiver wait at some of its steps, then
double-checked that the startup process was able to control the WAL
receiver as it should, flipping primary_conninfo with reloads.  AFAIK
as I can see, that felt OK, so applied as a36164e7465f.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi,

On Fri, Jan 23, 2026 at 1:31 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 23, 2026 at 12:07:36PM +0800, Xuneng Zhou wrote:
> > Thanks Michael — agreed. Patch v7 dropped the extra fast‑exit path and
> > kept the change minimal: only switch to STREAMING if the state is
> > still CONNECTING, otherwise leave it unchanged.
>
> Thanks for the newer version of the patch.  I have been playing with
> it today, even before you have sent this v7, with a set of injection
> points to make the WAL receiver wait at some of its steps, then
> double-checked that the startup process was able to control the WAL
> receiver as it should, flipping primary_conninfo with reloads.  AFAIK
> as I can see, that felt OK, so applied as a36164e7465f.
> --
> Michael

Thanks for checking and applying it. I’m playing with exposing
XLogRecoveryCtlData metrics at the SQL level, following your input.
I’ll post the patches and possibly start a new thread for discussion.

--
Best,
Xuneng



Re: Add WALRCV_CONNECTING state to walreceiver

От
Michael Paquier
Дата:
On Fri, Jan 23, 2026 at 04:47:43PM +0800, Xuneng Zhou wrote:
> I’ll post the patches and possibly start a new thread for discussion.

Thanks for all that.  I'd suggest posting the startup proposal into an
entirely new thread for clarity.
--
Michael

Вложения

Re: Add WALRCV_CONNECTING state to walreceiver

От
Xuneng Zhou
Дата:
Hi Michael,

On Sun, Jan 25, 2026 at 9:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 23, 2026 at 04:47:43PM +0800, Xuneng Zhou wrote:
> > I’ll post the patches and possibly start a new thread for discussion.
>
> Thanks for all that.  I'd suggest posting the startup proposal into an
> entirely new thread for clarity.
> --
> Michael

I have started a new thread [1] and posted the patches there, as you suggested.

[1] https://www.postgresql.org/message-id/CABPTF7W+Nody-+P9y4PNk37-QWuLpfUrEonHuEhrX+Vx9Kq+Kw@mail.gmail.com

--
Best,
Xuneng