Horton

Re: BUG #19093: Behavioral change in walreceiver termination between PostgreSQL 14.17 and 14.18

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Xuneng Zhou <xunengzhou@gmail.com>
Cc: Noah Misch <noah@leadboat.com>, ryanzxg@gmail.com, pgsql-bugs@lists.postgresql.org, Euler Taveira <euler@eulerto.com>
Date: 2025-10-29T23:36:43Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add assertions checking for the startup process in WAL replay routines

  2. Add assertion check for WAL receiver state during stream-archive transition

  3. Add WalRcvGetState() to retrieve the state of a WAL receiver

  4. Fix unconditional WAL receiver shutdown during stream-archive transition

  5. Add test tracking WAL receiver shutdown for primary_conninfo updates

  6. Reset InstallXLogFileSegmentActive after walreceiver self-initiated exit.

  7. Replace durable_rename_excl() by durable_rename(), take two

  8. Split xlog.c into xlog.c and xlogrecovery.c.

On Wed, Oct 29, 2025 at 02:36:10PM +0800, Xuneng Zhou wrote:
> Should we wrap this assertion within #ifdef USE_ASSERT_CHECKING?
> 
> #ifdef USE_ASSERT_CHECKING
> {
> WalRcvData *walrcv = WalRcv;
> WalRcvState state;
> SpinLockAcquire(&walrcv->mutex);
> state = walrcv->walRcvState;
> SpinLockRelease(&walrcv->mutex);
> Assert(state != WALRCV_STOPPING);
> }
> #endif

Yep.  Or provide a wrapper routine that's able to retrieve the state
of a WAL receiver, then add it in xlogrecovery.c within an Assert().
It may be better to only do that on HEAD, the back-branches stress me
a bit when we enforce such new assumptions..  Perhaps Noah has a
different opinion.

> However, I think Michael’s approach is better — it’s simpler, more
> straightforward, and captures the buggy behavior well. I plan to
> incorporate it into the next version of the patch.

Thanks!

One weakness of this approach is that this test would not be able to
work if the error message is reworded for some reason.  On stable
branches, I doubt that this error message will ever change, but it
could be updated to something else on HEAD.

Hence, I would suggest one extra improvement for HEAD: let's also add
a check for a valid case where a WAL receiver is stopped.  We have one
in 040_standby_failover_slots_sync.pl, where primary_conninfo is
updated in standby1 and its configuration reloaded, so I'd be tempted
to add a valid check in the server logs of $standby1.  One difference
is that we should grab the offset of the logs before the reload, then
make sure that the log entry gets created with a wait_for_log(), as
there may be a delay between the reload and the moment the log entry
is generated.
--
Michael