Re: Streaming replication and WAL archive interactions

> On 22 Jul 2026, at 11:07, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>
> Here is v7 after one more round of self-review. Also I want to add
> more context here: we used v4 in production for ~3-4 months, and
> it's mostly fine, except exceptions (see cascading replication bug,
> WAL removal on checkpoint bug in 056 tap etc).

Hi Kirill, Grigory, Jaroslav, all,

Thanks for v6/v7, and thanks to everyone who joined the thread!

I have looked int patches and ran them locally. A few things that
catched my eye:

1. The unsolicited 'a' message breaks other physical protocol
clients. WalSndArchivalReport() sends the report to any physical
walsender in CATCHUP or STREAMING state, excluding only temporary
slots. But pg_receivewal speaks the same protocol. I tried it against
a shared-mode primary (with a permanent slot): receivelog.c does not
know the message and drops the connection with

pg_receivewal: error: unrecognized streaming header: "a"

With --no-loop it just exits. With the default retry loop it
reconnects every 5 seconds, receives the next report right away (the
new walsender's report timer starts at zero), errors out again, and
makes no progress at all - I watched it restart from the same LSN
three times in a row. Existing tests do not catch this because
nothing runs pg_receivewal against a shared-mode primary.
pg_basebackup -S <slot> --wal-method=stream should hit the same path.

I think the primary recommendation here is that our own tools should
digest such messages gracefully instead of dropping the connection:
teach receivelog.c to skip messages it does not understand (or at
least this one). The physical protocol will keep growing new
informational messages, and erroring out on every unknown byte turns
each addition into a compatibility hazard.

That fixes in-core tools, but third-party tools speaking the physical
protocol are still affected. Note that the PrimaryStatusUpdate ('s')
message added in PG 18 is only ever sent in response to an explicit
request ('p') from the standby, I suppose for exactly this reason.
So making the reports opt-in - a new option to START_REPLICATION, or
a request message similar to PrimaryStatusRequest - may still be
worth considering on top of that, rather than the walsender guessing
by slot persistence.

2. The new TAP tests are not added to src/test/recovery/meson.build.
Under autotools prove picks up t/*.pl by glob, so they run, but in
meson builds (which is what most of CI runs) they are silently
skipped.

3. archive_status_report_interval is missing from
postgresql.conf.sample, and the GUC is not marked GUC_NOT_IN_SAMPLE,
so src/test/modules/test_misc/t/003_check_guc.pl should fail. There
is also a stray whitespace-only line added to guc_parameters.dat.
While at it: do we want 0 to mean "disabled"? Not proposing it as a
must.

4. protocol.sgml: the new varlistentry reuses the existing id
"protocol-replication-primary-status-update", so the id is now
duplicated and the docs build fails. It needs its own id, something
like protocol-replication-archive-status-report.

5. v7-0002 leaves make check red: pg_stat_wal_receiver gained a
column, but src/test/regress/expected/rules.out is not updated. The
commit message says "Bumps catversion", but catversion.h is not
touched. The new column is also not described in monitoring.sgml.

The assertions 0002 adds to the 055 test are also racy, and a 32-bit
CI run of mine hit it. The test captures
pg_walfile_name(pg_current_wal_lsn()), switches WAL and then
immediately expects the standby's primary_last_archived to equal the
captured name. Nothing guarantees the standby has received the
report covering that segment yet (they are sent asynchronously every
archive_status_report_interval), so on a slow machine the view still
holds the previous report. The archived_count poll on the primary
side is racy too: the counter can be bumped by archiving an older
pending segment. Both need to poll for the expected state - primary
for last_archived_wal >= the captured name, standby likewise for the
view - before checking the .done file.

6. The 055 test fails on Windows. It sets archive_command to a Unix
cp invocation with a raw tempdir path interpolated into
postgresql.conf. On Windows the path contains backslashes, and the
GUC parser processes backslash escapes inside quoted configuration
values, so the path gets mangled (\t becomes a tab and so on) and
archiving never succeeds; the test dies waiting for
archived_count > 0. The archive command needs the same treatment as
PostgreSQL::Test::Cluster::enable_archiving: copy on Windows, with
backslashes doubled. We cannot just use enable_archiving because all
nodes must share one archive directory - that is the point of the
feature. 056 has the same latent problem in its $good_command (copy,
but backslashes not doubled).

7. An operational concern from running this in production. In shared
mode a standby retains WAL without any bound while the archive
storage is down or the primary is not archiving. archive_mode is
PGC_POSTMASTER, so there is no runtime escape hatch: the standby
fills its disk and fails over, then the next standby repeats the
exercise. Downstream we ended up adding a SIGHUP GUC that disables
the retention on a standby, trading a gap in the archive for cluster
availability. For upstream, maybe something in the spirit of
max_slot_wal_keep_size would fit: a cap after which .ready segments
are recycled with a WARNING. Or, at the very least, a documentation
paragraph describing this failure mode. WDYT?

Some smaller comments on the code:

> - if (pgarch_archiveXlog(xlog))
> - {
> - /* successful */
> - pgarch_archiveDone(xlog);
> + if (pgarch_archiveXlog(xlog))
> + {
> + /* successful */
> + pgarch_archiveDone(xlog);

Looks like accidental whitespace damage left over from removing the
early return: these lines are dedented while the rest of the block
keeps the old indentation.

> - if (!XLogArchivingAlways() &&
> + if (!XLogArchivingAlways() && !(XLogArchiveMode == ARCHIVE_MODE_SHARED) &&

XLogArchiveMode != ARCHIVE_MODE_SHARED reads better; or, nicer, an
XLogArchivingShared() macro next to XLogArchivingAlways(), with the
same wal_level Assert.

> + memcpy(PgArch->primary_last_archived, primary_last_archived, sizeof(PgArch->primary_last_archived));

This copies sizeof() bytes from a local buffer of which only len+1
bytes are initialized, i.e. it reads uninitialized stack memory.
strlcpy() would do. Also, resetting primary_last_archived[0] = '\0'
right before ereport(ERROR) is dead code.

> + primary_last_archived = PgArch->primary_last_archived;

ProcessArchivalReport() reads the field through a bare pointer into
shared memory without taking the spinlock. Today the walreceiver is
the only writer, so this is safe, but it silently breaks the "lock
protects the field" invariant. The caller has just written this very
value, so maybe simply pass the local buffer as an argument?

> + pgstat_clear_snapshot();
> + archiver_stats = pgstat_fetch_stat_archiver();

On a cascading standby the fetched stats are unused (the recovery
branch reads PgArch instead), so we could fetch only in the else
branch. A related subtlety: if the most recently archived file is a
.history or .backup file, the IsXLogFileName() check skips the report
entirely, and the preceding segments get reported only when the next
regular segment is archived. Probably fine, but deserves a comment.

> + errmsg_internal("invalid archival report message with length %d, expected at most %ld",
> + (int) len, sizeof(primary_last_archived))));

%ld does not match size_t and breaks the build with gcc -Werror
(-Wformat); %zu is needed here.

Nits: 056 header says "Copyright (c) 2025"; trailing whitespace after
SpinLockInit() in PgArchShmemInit(); a stray tab-only line and a
"primary_last_archived done" double space in 055.

With the items above addressed I think 0001 is in good shape. The
handling of the new protocol message by clients (item 1) is the only
place where I think the design might need some polishing.

Thank you!

Best regards, Andrey Borodin.