On Fri, Oct 27, 2023 at 09:31:10AM -0400, David Steele wrote: > That sounds like the right plan to me. Nice and simple.
I'll tackle that in a separate thread with a patch registered for the upcoming CF of November. > I'm still +1 for the patch as it stands. I have been reviewing the patch, and applied portions of it as of dc5bd388 and 1ffdc03c and they're quite independent pieces. After that, the remaining bits of the patch to change the behavior is now straight-forward. I have written a commit message for it, while on it, as per the attached. -- Michael
From 26a8432fe3ab8426e7797d85d19b0fe69d3384c9 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 30 Oct 2023 16:02:52 +0900 Subject: [PATCH v4] Require recovery.signal or standby.signal when reading a backup_file Historically, the startup process uses two static variables to control if archive recovery should happen, when either recovery.signal or standby.signal are defined in the data folder at the beginning of recovery: - ArchiveRecoveryRequested, set to "true" if either .signal file exists. This controls if archive recovery has been requested. - InArchiveRecovery, that controls if recovery should switch to archive mode during recovery. It is initially "false" but switched to "true" during recovery for two cases: -- Just after finding and reading a valid backup_label file, close to the beginning of recovery. -- When no backup_label file is found, it would be set to "false" to force crash recovery, where all the WAL located in pg_wal/ is read first. When all the local WAL records have been consumed, it is changed to "true" to switch to archive recovery mode, equivalent to a base backup. Now comes the fact that recovery has an old logical problem: when a backup_label is found but no .signal file is defined, the startup process finishes in a state where ArchiveRecoveryRequested is "false" but InArchiveRecovery is "true". This is proving to cause various issues, that got worse since restart points can run during crash recovery for the benefit of being able to start and stop instances without doing crash recovery from its initial point (7ff23c6d277d): - Some standby states needed by archive recovery would not be set, causing correctness issues, like assertion failures during recovery. - recoveryWakeupLatch would not be set. - Some GUCs are messed up, hot_standby that depends on ArchiveRecoveryRequested. This configuration was possible when recovering from a base backup taken by pg_basebackup without -R. Note that the documentation requires at least to set recovery.signal to restore from a backup, but the startup process was not making this policy explicit. In most cases, one would have been able to complete recovery, but that's a matter of luck, really, as it depends on the workload of the origin server. This has the advantage of simplifying the logic for the archive recovery case, as InArchiveRecovery now requires ArchiveRecoveryRequested to be set. Recovering a self-contained backup now requires a recovery.signal, with a restore_command set in postgresql.conf (or related conf file). One test of pg_basebackup and one test of pg_rewind need to be tweaked to avoid the FATAL introduced by this patch when a base_backup is found without a .signal file, even if the intention of both is to have a recovery.signal set. Reviewed-by: David Steele, Bowen Shi Discussion: https://postgr.es/m/zarvomifjze7f...@paquier.xyz Discussion: https://postgr.es/m/Y/Q/17rpys7yg...@paquier.xyz Discussion: https://postgr.es/m/Y/v0c+3W89NBT/i...@paquier.xyz --- src/backend/access/transam/xlogrecovery.c | 22 ++++++++++++++++--- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 3 ++- src/bin/pg_rewind/t/008_min_recovery_point.pl | 1 + 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c61566666a..0088dfec2e 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -125,14 +125,18 @@ static TimeLineID curFileTLI; /* * When ArchiveRecoveryRequested is set, archive recovery was requested, - * ie. signal files were present. When InArchiveRecovery is set, we are - * currently recovering using offline XLOG archives. These variables are only - * valid in the startup process. + * ie. signal files or backup_label were present. When InArchiveRecovery is + * set, we are currently recovering using offline XLOG archives. These + * variables are only valid in the startup process. Note that the presence of + * a backup_label file requires the presence of one of the two .signal files + * to enforce archive recovery. * * When ArchiveRecoveryRequested is true, but InArchiveRecovery is false, we're * currently performing crash recovery using only XLOG files in pg_wal, but * will switch to using offline XLOG archives as soon as we reach the end of * WAL in pg_wal. + * + * InArchiveRecovery should never be set without ArchiveRecoveryRequested. */ bool ArchiveRecoveryRequested = false; bool InArchiveRecovery = false; @@ -594,6 +598,12 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr, { List *tablespaces = NIL; + if (!ArchiveRecoveryRequested) + ereport(FATAL, + (errmsg("could not find recovery.signal or standby.signal when recovering with backup_label"), + errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.", + DataDir, DataDir))); + /* * Archive recovery was requested, and thanks to the backup label * file, we know how far we need to replay to reach consistency. Enter @@ -1591,6 +1601,12 @@ ShutdownWalRecovery(void) */ if (ArchiveRecoveryRequested) DisownLatch(&XLogRecoveryCtl->recoveryWakeupLatch); + + /* + * InArchiveRecovery should never have been set without + * ArchiveRecoveryRequested. + */ + Assert(ArchiveRecoveryRequested || !InArchiveRecovery); } /* diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index b9f5e1266b..b9e54f4562 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -392,7 +392,8 @@ SKIP: my $node2 = PostgreSQL::Test::Cluster->new('replica'); # Recover main data directory - $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar); + $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar, + has_restoring => 1); # Recover tablespace into a new directory (not where it was!) my $repTsDir = "$tempdir/tblspc1replica"; diff --git a/src/bin/pg_rewind/t/008_min_recovery_point.pl b/src/bin/pg_rewind/t/008_min_recovery_point.pl index d4c89451e6..1cff5b7019 100644 --- a/src/bin/pg_rewind/t/008_min_recovery_point.pl +++ b/src/bin/pg_rewind/t/008_min_recovery_point.pl @@ -152,6 +152,7 @@ move( "$tmp_folder/node_2-postgresql.conf.tmp", "$node_2_pgdata/postgresql.conf"); +$node_2->append_conf('standby.signal', ''); $node_2->start; # Check contents of the test tables after rewind. The rows inserted in node 3 -- 2.42.0
signature.asc
Description: PGP signature
