Re: Streaming replication and WAL archive interactions

Hi all!

I have revised the patches in a thread. In v6-0001 I have included
v4-0001 - v4-0003, fixes for them and new monitoring features in
v6-0002.
So, first of all, in v4 there were several bugs. One bug is spotted by
Jaroslav in [1]. The if-condition pointed by Jaroslav is indeed wrong,
it leads to WAL segments removal on standby's restartpoint, which is
unintended (they have .ready status). The new 056 TAP test now
executes this code path. I chose to create a new TAP instead of
extending 055 for sake of simplicity and readability.

Another bug is about cascading replication. In v4, only primary sends
archive status reports, so for cascading replication, there is nobody
to inform replica that WAL can be deleted (in v4 WAL file archive
status would be .ready, and files will be never cleared up). In v5,
stadby's walsender simply relays primary archive status downstream. It
was actually spotted & proposed 10 years ago [2], but never
implemented in thread.
So, for implementing this, we need shared memory for walsender &
walreceier coordination. I used Archiver Data (PgArchData) for this,
where primary_last_archived WAL is now stored. I used Archiver Data
because that was v3 choice, but I'm not opposed to using the new
shared memory region.

Note that v4 patches have cascading setup tests, which does not catch
this due to its ill-formed structure. I have reimplemented them, and
polished them a little.

v6-0001 also features changes in protocol.sgml (grabbed from Heikki's
v3 [0]). Please check the wording.

I also added archive_status_report_interval GUC to regulate archival
status report frequency. Setting this to 10ms instead of default (10s)
speeds up tests from 30-40s to 8-10s runtime.

This patch set does not address switching to archive streaming issues
from [2]. I will soon update the patch with that.

For Grigory's questions from [3]:

> 1. Should we update pg_stat_archiver on standby to support cascading
> replication or should we just resend the report, received from upstream?

Last one. We shouldn't update pg_stat_archiver standby, because there
is no actual work happening there.
While in recovery, standby's only duty is to keep archive status up to
date, archiving itself always happening on primary.
So, we need to monitor something related to standby's activity, we can
track primary_last_archived WAL for that purpose. v6-0002 implements
that.

> 2. What should we do with *.backup.ready and *.partial.ready on standby?
> Can we just XLogArchiveForceDone() them?

In v4 we only send reports for WAL segments, not backup history files
or other archived files. In v6 we check this before
ProcessArchivalReport call.

> 3. Should we keep the awkward part with switchpont calculation in
> timeline switch case? I think all segments that are not in our server
> history should just be stamped with XLogArchiveForceDone().

I didn't get this one.

> 4. Currently XLogArchiveForceDone is forced either by walreceiver (on
> receiving report from upstream) and archiver. Should we move this into
> the archiver entirely?

For what exact benefit?

I also reviewed Grigory's patch. Its description says:

> There are a number of changes, such as sending just TLI and Segno
> instead of full WAL filename, shifting some work into archiver and
> adding shared memory for walreceiver/archiver synchronization.

But I didn't find anything particularly useful. I maybe like this bit:

> + ShmemRequestStruct(.name = "Archival Report Data",
> + .size = sizeof(ArchivalReportData),
> + .ptr = (void **) &ArchReport,

But I didn't include this in v6.

Here is few random comments on v5:

> +// DIR *status_dir;
> +// struct dirent *status_de;

We don't leave commented code here.

> + elog(WARNING, "found ready file for %s", walfile);

That's DEBUG4/DEBUG5 at beast. Ideally, remove

> + // TODO: we must handle somehow partial, .history and .backup files

We use /**/ comment style.

>+ elog(WARNING, "segment %s is not yet archived by upstream", walfile);

DEBUG2

> + // some garbage in archive_status, should error here
> + if (!tliInHistory(file_tli, tli_history))
> + {
> + ereport(ERROR,
> + (errmsg("walfile %s is not in this server's history", walfile)));
> + continue;
> + }

ereport called with severity level higher that ERROR is efficiently
long jump to PG_TRY block, so there will be no 'continue'

>
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -3401,7 +3401,9 @@ LaunchMissingBackgroundProcesses(void)
> */
> if (PgArchPMChild == NULL &&
> ((XLogArchivingActive() && pmState == PM_RUN) ||
> - (XLogArchivingAlways() && (pmState == PM_RECOVERY || pmState == PM_HOT_STANDBY))) &&
> + (XLogArchivingAlways() && (pmState == PM_RECOVERY || pmState == PM_HOT_STANDBY)) ||
> + (XLogArchivingShared())
> + ) &&

Do we need this?

> +bool
> +IsSegnoArchivedByUpstream(TimeLineID tli, XLogSegNo segno)
> +{
> + elog(WARNING, "received tli: %d, segno: %ld", tli, segno);
> + elog(WARNING, "ArchReport->tli: %d, ArchReport->segno: %ld", ArchReport->tli, ArchReport->segno);
> + return tli == ArchReport->tli && segno <= ArchReport->segno;
> +}

It's DEBUG2. Also, use errdetail() for low-level variable printing.

v5 also misses use_direct_check optimisation from v4-0003

[0] https://www.postgresql.org/message-id/5550D20D.6090703%40iki.fi
[1] https://www.postgresql.org/message-id/50EF1E0E-9212-4BF2-9B2A-E38AC45D8ACD%40yandex-team.ru
[2] https://www.postgresql.org/message-id/CAHGQGwGJzp-QS7BODiv1uc291gAKtjzzCPb_nzUTxYKJhLsUCA%40mail.gmail.com
[3] https://www.postgresql.org/message-id/4516e7e8-46f0-4ded-907a-db5a7c0c75b3%40gmail.com

--
Best regards,
Kirill Reshke