Horton

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

Xuneng Zhou <xunengzhou@gmail.com>

View thread
From: Xuneng Zhou <xunengzhou@gmail.com>
To: Michael Paquier <michael@paquier.xyz>, Noah Misch <noah@leadboat.com>
Cc: ryanzxg@gmail.com, pgsql-bugs@lists.postgresql.org, Euler Taveira <euler@eulerto.com>
Date: 2025-10-30T16:07:01Z
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.

Attachments

Hi,

On Thu, Oct 30, 2025 at 7:36 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Using a wrapper function seems better here.

> > 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.

Thanks for your suggestion!

I’ve incorporated both tests and the assertion into the patch applied to master.
For earlier stable branches, the test in
040_standby_failover_slots_sync.pl and the assertion for
WAL receiver state checking are not included (if Noah agrees too). Please check.

Best,
Xuneng