At Fri, 10 Mar 2023 15:59:04 +0900, Michael Paquier <mich...@paquier.xyz> wrote
in
> My apologies for the long message, but this deserves some attention,
> IMHO.
>
> So, any thoughts?
Sorry for being late. However, I agree with David's concern regarding
the unnecessary inconvenience it introduces. I'd like to maintain the
functionality.
While I agree that InArchiveRecovery should be activated only if
ArchiveReArchiveRecoveryRequested is true, I oppose to the notion that
the mere presence of backup_label should be interpreted as a request
for archive recovery (even if it is implied in a comment in
InitWalRecovery()). Instead, I propose that we separate backup_label
and archive recovery, in other words, we should not turn on
InArchiveRecovery if !ArchiveRecoveryRequested, regardless of the
presence of backup_label. We can know the minimum required recovery
LSN by the XLOG_BACKUP_END record.
The attached is a quick mock-up, but providing an approximation of my
thoughts. (For example, end_of_backup_reached could potentially be
extended to the ArchiveRecoveryRequested case and we could simplify
the condition..)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index fcbde10529..e4af945319 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5489,17 +5489,15 @@ StartupXLOG(void)
set_ps_display("");
/*
- * When recovering from a backup (we are in recovery, and archive
recovery
- * was requested), complain if we did not roll forward far enough to
reach
- * the point where the database is consistent. For regular online
- * backup-from-primary, that means reaching the end-of-backup WAL record
- * (at which point we reset backupStartPoint to be Invalid), for
- * backup-from-replica (which can't inject records into the WAL stream),
- * that point is when we reach the minRecoveryPoint in pg_control (which
- * we purposefully copy last when backing up from a replica). For
- * pg_rewind (which creates a backup_label with a method of "pg_rewind")
- * or snapshot-style backups (which don't), backupEndRequired will be
set
- * to false.
+ * When recovering from a backup, complain if we did not roll forward
far
+ * enough to reach the point where the database is consistent. For
regular
+ * online backup-from-primary, that means reaching the end-of-backup WAL
+ * record, for backup-from-replica (which can't inject records into the
WAL
+ * stream), that point is when we reach the minRecoveryPoint in
pg_control
+ * (which we purposefully copy last when backing up from a replica).
For
+ * pg_rewind (which creates a backup_label with a method of
"pg_rewind") or
+ * snapshot-style backups (which don't), backupEndRequired will be set
to
+ * false.
*
* Note: it is indeed okay to look at the local variable
* LocalMinRecoveryPoint here, even though ControlFile->minRecoveryPoint
@@ -5508,7 +5506,7 @@ StartupXLOG(void)
*/
if (InRecovery &&
(EndOfLog < LocalMinRecoveryPoint ||
- !XLogRecPtrIsInvalid(ControlFile->backupStartPoint)))
+ (haveBackupLabel &&
!endOfRecoveryInfo->end_of_backup_reached)))
{
/*
* Ran off end of WAL before reaching end-of-backup WAL record,
or
@@ -5516,16 +5514,13 @@ StartupXLOG(void)
* recover from an online backup but never called
pg_backup_stop(), or
* you didn't archive all the WAL needed.
*/
- if (ArchiveRecoveryRequested || ControlFile->backupEndRequired)
- {
- if (!XLogRecPtrIsInvalid(ControlFile->backupStartPoint)
|| ControlFile->backupEndRequired)
- ereport(FATAL,
- (errmsg("WAL ends before end of
online backup"),
- errhint("All WAL generated
while online backup was taken must be available at recovery.")));
- else
- ereport(FATAL,
- (errmsg("WAL ends before
consistent recovery point")));
- }
+ if (haveBackupLabel &&
!endOfRecoveryInfo->end_of_backup_reached)
+ ereport(FATAL,
+ (errmsg("WAL ends before end of online
backup"),
+ errhint("All WAL generated while
online backup was taken must be available at recovery.")));
+ else
+ ereport(FATAL,
+ (errmsg("WAL ends before consistent
recovery point")));
}
/*
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index becc2bda62..d3cbf0703b 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -281,6 +281,7 @@ static TimeLineID minRecoveryPointTLI;
static XLogRecPtr backupStartPoint;
static XLogRecPtr backupEndPoint;
static bool backupEndRequired = false;
+static bool backupEndReached = false;
/*
* Have we reached a consistent database state? In crash recovery, we have
@@ -615,11 +616,12 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
List *tablespaces = NIL;
/*
- * Archive recovery was requested, and thanks to the backup
label
+ * When archive recovery was requested, thanks to the backup
label
* file, we know how far we need to replay to reach
consistency. Enter
* archive recovery directly.
*/
- InArchiveRecovery = true;
+ if (ArchiveRecoveryRequested)
+ InArchiveRecovery = true;
if (StandbyModeRequested)
EnableStandbyMode();
@@ -1531,6 +1533,19 @@ FinishWalRecovery(void)
result->standby_signal_file_found = standby_signal_file_found;
result->recovery_signal_file_found = recovery_signal_file_found;
+ /*
+ * If archive recovery was performed, backupEndReached indicates passing
+ * the end of backup. If not, a valid backupEndPoint value suggests the
+ * recovery began from a base backup; verify if recovery surpasses that
+ * point.
+ */
+ if (backupEndReached ||
+ (!XLogRecPtrIsInvalid(backupEndPoint) &&
+ backupEndPoint <= XLogRecoveryCtl->lastReplayedEndRecPtr))
+ result->end_of_backup_reached = true;
+ else
+ result->end_of_backup_reached = false;
+
return result;
}
@@ -2133,6 +2148,7 @@ CheckRecoveryConsistency(void)
backupStartPoint = InvalidXLogRecPtr;
backupEndPoint = InvalidXLogRecPtr;
backupEndRequired = false;
+ backupEndReached = true;
}
/*
diff --git a/src/include/access/xlogrecovery.h
b/src/include/access/xlogrecovery.h
index 47c29350f5..b8c1a97224 100644
--- a/src/include/access/xlogrecovery.h
+++ b/src/include/access/xlogrecovery.h
@@ -129,6 +129,8 @@ typedef struct
*/
bool standby_signal_file_found;
bool recovery_signal_file_found;
+
+ bool end_of_backup_reached;
} EndOfWalRecoveryInfo;
extern EndOfWalRecoveryInfo *FinishWalRecovery(void);
