($INBOX_DIR/description missing)
help / color / mirror / Atom feed[PATCH v2] Correctly update contfol file at the end of archive recovery
16+ messages / 3 participants
[nested] [flat]
* [PATCH v2] Correctly update contfol file at the end of archive recovery
@ 2022-02-14 04:04 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-14 04:04 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 ++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6208e123e5..ff4a90eacc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9587,6 +9587,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9653,7 +9656,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9672,7 +9675,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9680,31 +9686,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9716,8 +9720,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9804,7 +9825,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Wed_Mar_16_10_24_44_2022_775)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0001-Correctly-update-contfol-file-at-the-end-of_v13.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v2] Correctly update contfol file at the end of archive recovery
@ 2022-02-14 04:04 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-14 04:04 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Since all buffers have flushed out, we may safely regard that restart
point established regardless of the recovery state. Thus we may and
should update checkpoint LSNs of checkpoint file in that case. Still
we update minRecoveryPoint only during archive recovery but explicitly
clear if we have exited recovery.
Addition to that fix, this commit makes some cosmetic changes that
consist with the changes we are going to make on the master branch.
---
src/backend/access/transam/xlog.c | 81 ++++++++++++++++++++-----------
1 file changed, 53 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6208e123e5..28c3c4b7cf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9653,7 +9653,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9672,7 +9672,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9681,52 +9684,74 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Update pg_control, using current time if no later checkpoints have been
+ * performed.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (PriorRedoPtr < RedoRecPtr)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
ControlFile->time = (pg_time_t) time(NULL);
/*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened
+ * already while writing out dirty buffers, but not necessarily -
+ * e.g. because no buffers were dirtied. We do this because a
+ * non-exclusive base backup uses minRecoveryPoint to determine which
+ * WAL files must be included in the backup, and the file (or files)
+ * containing the checkpoint record must be included, at a
+ * minimum. Note that for an ordinary restart of recovery there's no
+ * value in having the minimum recovery point any earlier than this
* anyway, because redo will begin just after the checkpoint record.
+ * This is a quick hack to make sure nothing really bad happens if
+ * somehow we get here after the end-of-recovery checkpoint.
*/
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ {
+ ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
+ ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
- /* update local copy */
- minRecoveryPoint = ControlFile->minRecoveryPoint;
- minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ /* update local copy */
+ minRecoveryPoint = ControlFile->minRecoveryPoint;
+ minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ }
+ if (flags & CHECKPOINT_IS_SHUTDOWN)
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
- if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
+ else
+ {
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+ }
+
UpdateControlFile();
}
LWLockRelease(ControlFileLock);
/*
* Update the average distance between checkpoints/restartpoints if the
- * prior checkpoint exists.
+ * prior checkpoint exists and we have advanced REDO LSN.
*/
- if (PriorRedoPtr != InvalidXLogRecPtr)
+ if (PriorRedoPtr != InvalidXLogRecPtr && RedoRecPtr > PriorRedoPtr)
UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
+ /*
+ * Now we can safely regard this restart point as established.
+ *
+ * Since all buffers have been flushed so we regard this restart point as
+ * established. We could omit some of the following steps in the case
+ * where we have omitted control file updates, but we don't bother avoid
+ * them from performing since that case rarely happens and they don't harm
+ * even if they take place.
+ */
+
/*
* Delete old log files, those no longer needed for last restartpoint to
* prevent the disk holding the xlog from growing full.
@@ -9804,7 +9829,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Mon_Feb_14_14_40_22_2022_983)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v2] Correctly update contfol file at the end of archive recovery
@ 2022-02-14 04:04 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-14 04:04 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 ++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6208e123e5..ff4a90eacc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9587,6 +9587,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9653,7 +9656,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9672,7 +9675,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9680,31 +9686,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9716,8 +9720,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9804,7 +9825,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Fri_Feb_25_15_31_12_2022_240)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v3-0001-Correctly-update-contfol-file-at-the-end-of_v13.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v2] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 05:46 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 05:46 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 ++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3d76fad128..3670ff81e7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9376,6 +9376,9 @@ CreateRestartPoint(int flags)
*/
LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9445,7 +9448,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9461,7 +9464,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9469,31 +9475,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9505,8 +9509,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9590,7 +9611,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Wed_Mar_16_10_24_44_2022_775)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0001-Correctly-update-contfol-file-at-the-end-of_v12-11.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v2] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 05:46 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 05:46 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 ++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3d76fad128..3670ff81e7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9376,6 +9376,9 @@ CreateRestartPoint(int flags)
*/
LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -9445,7 +9448,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9461,7 +9464,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9469,31 +9475,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9505,8 +9509,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9590,7 +9611,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Fri_Feb_25_15_31_12_2022_240)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v3] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 06:04 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 06:04 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 71 +++++++++++++++++++------------
1 file changed, 44 insertions(+), 27 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 885558f291..2b2568c475 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9334,7 +9334,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9350,7 +9350,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9358,31 +9361,28 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9393,9 +9393,26 @@ CreateRestartPoint(int flags)
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9470,7 +9487,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Wed_Mar_16_10_24_44_2022_775)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0001-Correctly-update-contfol-file-at-the-end-of_v10.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v3] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 06:04 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 06:04 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 71 +++++++++++++++++++------------
1 file changed, 44 insertions(+), 27 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 885558f291..2b2568c475 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9334,7 +9334,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9350,7 +9350,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9358,31 +9361,28 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9393,9 +9393,26 @@ CreateRestartPoint(int flags)
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9470,7 +9487,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Fri_Feb_25_16_47_01_2022_087)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v3-0001-Correctly-update-contfol-file-at-the-end-of_v10.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v3] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 07:35 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 07:35 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 +++++++++++++++++++------------
1 file changed, 45 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c64febdb53..9fb66ad7d5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9434,7 +9434,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9450,7 +9450,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo pointer, used later to determine
@@ -9458,32 +9461,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->prevCheckPoint = ControlFile->checkPoint;
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still running. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->prevCheckPoint = ControlFile->checkPoint;
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9494,9 +9494,26 @@ CreateRestartPoint(int flags)
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9579,7 +9596,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("last completed transaction was at log time %s",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Fri_Feb_25_16_47_01_2022_087)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v3] Correctly update contfol file at the end of archive recovery
@ 2022-02-25 07:35 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-25 07:35 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 73 +++++++++++++++++++------------
1 file changed, 45 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c64febdb53..9fb66ad7d5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9434,7 +9434,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9450,7 +9450,10 @@ CreateRestartPoint(int flags)
if (log_checkpoints)
LogCheckpointStart(flags, true);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo pointer, used later to determine
@@ -9458,32 +9461,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->prevCheckPoint = ControlFile->checkPoint;
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still running. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->prevCheckPoint = ControlFile->checkPoint;
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
- ControlFile->time = (pg_time_t) time(NULL);
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9494,9 +9494,26 @@ CreateRestartPoint(int flags)
minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ minRecoveryPoint = InvalidXLogRecPtr;
+ minRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -9579,7 +9596,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- (uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
+ (uint32) (RedoRecPtr >> 32), (uint32) RedoRecPtr),
xtime ? errdetail("last completed transaction was at log time %s",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Wed_Mar_16_10_24_44_2022_775)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v4] Correctly update contfol file at the end of archive recovery
@ 2022-03-04 04:18 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-04 04:18 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 69 ++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ed16f279b1..dd9c564988 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6900,6 +6900,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -6965,7 +6968,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -6984,7 +6987,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -6992,30 +6998,26 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -7027,8 +7029,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ LocalMinRecoveryPoint = InvalidXLogRecPtr;
+ LocalMinRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -7105,7 +7124,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Wed_Mar_16_10_24_44_2022_775)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0001-Correctly-update-contfol-file-at-the-end-of_v14.txt"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v11 1/5] Correctly update contfol file at the end of archive recovery
@ 2022-03-04 04:18 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-04 04:18 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 72 ++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0d2bd7a357..3987aa81de 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6899,6 +6899,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -6964,7 +6967,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -6983,7 +6986,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -6991,30 +6997,29 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /* Update control file using current time */
+ ControlFile->time = (pg_time_t) time(NULL);
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -7026,8 +7031,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ LocalMinRecoveryPoint = InvalidXLogRecPtr;
+ LocalMinRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -7104,7 +7126,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Fri_Mar__4_14_10_38_2022_481)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v11-0002-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v12 1/5] Correctly update contfol file at the end of archive recovery
@ 2022-03-04 04:18 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-04 04:18 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 69 ++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0d2bd7a357..bd962763cc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6899,6 +6899,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -6964,7 +6967,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -6983,7 +6986,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -6991,30 +6997,26 @@ CreateRestartPoint(int flags)
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record. This is a quick hack to make sure nothing really bad
+ * happens if somehow we get here after the end-of-recovery checkpoint.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -7026,8 +7028,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ LocalMinRecoveryPoint = InvalidXLogRecPtr;
+ LocalMinRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
@@ -7104,7 +7123,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
----Next_Part(Tue_Mar_15_17_23_40_2022_661)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v12-0002-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-.patch"
^ permalink raw reply [nested|flat] 16+ messages in thread
* [PATCH v5] Correctly update contfol file at the end of archive recovery
@ 2022-04-27 01:41 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-27 01:41 UTC (permalink / raw)
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Along with that fix, we remove a dead code path for the case some
other process ran a simultaneous checkpoint. It seems like just a
preventive measure but it's no longer useful because we are sure that
checkpoint is performed only by checkpointer except single process
mode.
---
src/backend/access/transam/xlog.c | 62 ++++++++++++++++++++-----------
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 61cda56c6f..a03b8de593 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6921,6 +6921,9 @@ CreateRestartPoint(int flags)
XLogSegNo _logSegNo;
TimestampTz xtime;
+ /* we don't assume concurrent checkpoint/restartpoint to run */
+ Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER);
+
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
@@ -7007,36 +7010,34 @@ CreateRestartPoint(int flags)
CheckPointGuts(lastCheckPoint.redo, flags);
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+
/*
* Remember the prior checkpoint's redo ptr for
* UpdateCheckPointDistanceEstimate()
*/
PriorRedoPtr = ControlFile->checkPointCopy.redo;
+ Assert (PriorRedoPtr < RedoRecPtr);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
*/
- LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a backup performed in recovery uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -7048,8 +7049,25 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /* recovery mode is not supposed to end during shutdown restartpoint */
+ Assert((flags & CHECKPOINT_IS_SHUTDOWN) == 0);
+
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+
+ /* also update local copy */
+ LocalMinRecoveryPoint = InvalidXLogRecPtr;
+ LocalMinRecoveryPointTLI = 0;
+ }
+
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
--
2.27.0
----Next_Part(Wed_Apr_27_10_43_53_2022_048)----
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: DSA_ALLOC_NO_OOM doesn't work
@ 2024-02-21 21:30 Thomas Munro <[email protected]>
2024-02-21 23:03 ` Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Munro @ 2024-02-21 21:30 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Thu, Feb 22, 2024 at 8:19 AM Heikki Linnakangas <[email protected]> wrote:
> - Separate dsm_handle, used by backend code to interact with the high
> level interface in dsm.c, from dsm_impl_handle, which is used to
> interact with the low-level functions in dsm_impl.c. This gets rid of
> the convention in dsm.c of reserving odd numbers for DSM segments stored
> in the main shmem area. There is now an explicit flag for that the
> control slot. For generating dsm_handles, we now use the same scheme we
> used to use for main-area shm segments for all DSM segments, which
> includes the slot number in the dsm_handle. The implementations use
> their own mechanisms for generating the low-level dsm_impl_handles (all
> but the SysV implementation generate a random handle and retry on
> collision).
Could we use slot number and generation number, instead of slot number
and random number? I have never liked the random number thing, which
IIUC was needed because of SysV key space management problems 'leaking'
up to the handle level (yuck). With generations, you could keep
collisions arbitrarily far apart (just decide how many bits to use).
Collisions aren't exactly likely, but if there is no need for that
approach, I'm not sure why we'd keep it. (I remember dealing with
actual collisions in the wild due to lack of PRNG seeding in workers,
which admittedly should be vanishingly rare now).
If the slot number is encoded into the handle, why do we still need a
linear search for the slot?
> - create() no longer returns the mapped_size. The old Windows
> implementation had some code to read the actual mapped size after
> creating the mapping, and returned that in *mapped_size. Others just
> returned the requested size. In principle reading the actual size might
> be useful; the caller might be able to make use of the whole mapped size
> when it's larger than requested. In practice, the callers didn't do
> that. Also, POSIX shmem on FreeBSD has similar round-up-to-page-size
> behavior but the implementation did not query the actual mapped size
> after creating the segment, so you could not rely on it.
I think that is an interesting issue with the main shmem area. There,
we can set huge_page_size to fantastically large sizes up to 16GB on
some architectures, but we have nothing to make sure we don't waste
some or most of the final page. But I agree that there's not much
point in worrying about this for DSM.
> - Added a test that exercises basic create, detach, attach functionality
> using all the different implementations supported on the current platform.
I wonder how we could test the cleanup-after-crash behaviour.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: DSA_ALLOC_NO_OOM doesn't work
2024-02-21 21:30 Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
@ 2024-02-21 23:03 ` Thomas Munro <[email protected]>
2024-02-22 00:05 ` Re: DSA_ALLOC_NO_OOM doesn't work Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Munro @ 2024-02-21 23:03 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Thu, Feb 22, 2024 at 10:30 AM Thomas Munro <[email protected]> wrote:
> collisions arbitrarily far apart (just decide how many bits to use).
. o O ( Perhaps if you also allocated slots using a FIFO freelist,
instead of the current linear search for the first free slot, you
could maximise the time before a slot is reused, improving the
collision-avoiding power of a generation scheme? )
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: DSA_ALLOC_NO_OOM doesn't work
2024-02-21 21:30 Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
2024-02-21 23:03 ` Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
@ 2024-02-22 00:05 ` Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Heikki Linnakangas @ 2024-02-22 00:05 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On 22/02/2024 01:03, Thomas Munro wrote:
> On Thu, Feb 22, 2024 at 10:30 AM Thomas Munro <[email protected]> wrote:
>> collisions arbitrarily far apart (just decide how many bits to use).
>
> . o O ( Perhaps if you also allocated slots using a FIFO freelist,
> instead of the current linear search for the first free slot, you
> could maximise the time before a slot is reused, improving the
> collision-avoiding power of a generation scheme? )
We could also enlarge dsm_handle from 32-bits to 64-bits, if we're
worried about collisions.
I actually experimented with something like that too: I encoded the "is
this in main region" in one of the high bits and let the implementation
use the low bits. One small issue with that is that we have a few places
that pass a DSM handle as the 'bgw_main' argument when launching a
worker process, and on 32-bit platforms that would not be wide enough.
Those could be changed to use the wider 'bgw_extra' field instead, though.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2024-02-22 00:05 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-14 04:04 [PATCH v2] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-14 04:04 [PATCH v2] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-14 04:04 [PATCH v2] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 05:46 [PATCH v2] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 05:46 [PATCH v2] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 06:04 [PATCH v3] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 06:04 [PATCH v3] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 07:35 [PATCH v3] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-02-25 07:35 [PATCH v3] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-03-04 04:18 [PATCH v4] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-03-04 04:18 [PATCH v11 1/5] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-03-04 04:18 [PATCH v12 1/5] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2022-04-27 01:41 [PATCH v5] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2024-02-21 21:30 Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
2024-02-21 23:03 ` Re: DSA_ALLOC_NO_OOM doesn't work Thomas Munro <[email protected]>
2024-02-22 00:05 ` Re: DSA_ALLOC_NO_OOM doesn't work Heikki Linnakangas <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox