($INBOX_DIR/description missing)
help / color / mirror / Atom feedSmall run-time pruning doc fix
15+ messages / 3 participants
[nested] [flat]
* Small run-time pruning doc fix
@ 2018-10-08 08:22 David Rowley <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: David Rowley @ 2018-10-08 08:22 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Before 5220bb7533f a note in ddl.sgml used to mention that run-time
pruning was only implemented for Append. When we got MergeAppend
support the commit updated this to mention MergeAppend is supported
too. This is slightly weird as it's not all that obvious what exactly
isn't supported when we mention:
<para>
Both of these behaviors are likely to be changed in a future release
of <productname>PostgreSQL</productname>.
</para>
The attached patch updates this to mention that ModifyTable is
unsupported which I think makes the above fragment make sense again.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
[application/octet-stream] small_runtime_pruning_doc_fix.patch (608B, ../../CAKJS1f-kkMLSfjxwJF0JiT0UzKkGFfzzRzifhKJiCfzXbZAWLg@mail.gmail.com/2-small_runtime_pruning_doc_fix.patch)
download | inline diff:
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index b5ed1b7939..cab6cf24ee 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3944,8 +3944,10 @@ EXPLAIN SELECT count(*) FROM measurement WHERE logdate >= DATE '2008-01-01';
</para>
<para>
- Execution-time partition pruning currently occurs for the
+ Execution-time partition pruning currently only occurs for the
<literal>Append</literal> and <literal>MergeAppend</literal> node types.
+ It is not yet implemented for the <literal>ModifyTable</literal> node
+ type.
</para>
<para>
^ permalink raw reply [nested|flat] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread
* RE: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL
@ 2023-07-07 12:54 Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2023-07-07 12:54 UTC (permalink / raw)
To: 'Peter Smith' <[email protected]>; +Cc: Postgres hackers <[email protected]>; Önder Kalacı <[email protected]>
Dear Peter,
Thanks for reviewing. PSA new version.
I planned to post new version after supporting more indexes, but it may take more time.
So I want to address comments from you once.
> ======
> src/backend/executor/execReplication.c
>
> 1. get_equal_strategy_number
>
> +/*
> + * Return the appropriate strategy number which corresponds to the equality
> + * comparisons.
> + *
> + * TODO: support other indexes: GiST, GIN, SP-GiST, BRIN
> + */
> +static int
> +get_equal_strategy_number(Oid opclass)
> +{
> + Oid am_method = get_opclass_method(opclass);
> + int ret;
> +
> + switch (am_method)
> + {
> + case BTREE_AM_OID:
> + ret = BTEqualStrategyNumber;
> + break;
> + case HASH_AM_OID:
> + ret = HTEqualStrategyNumber;
> + break;
> + case GIST_AM_OID:
> + case GIN_AM_OID:
> + case SPGIST_AM_OID:
> + case BRIN_AM_OID:
> + /* TODO */
> + default:
> + /* XXX: Do we have to support extended indexes? */
> + ret = InvalidStrategy;
> + break;
> + }
> +
> + return ret;
> +}
>
> 1a.
> In the file syscache.c there are already some other functions like
> get_op_opfamily_strategy so I am wondering if this new function also
> really belongs in that file.
According to atop comment in the syscache.c, it contains routines which access
system catalog cache. get_equal_strategy_number() does not check it, so I don't
think it should be at the file.
> 1b.
> Should that say "operator" instead of "comparisons"?
Changed.
> 1c.
> "am" stands for "access method" so "am_method" is like "access method
> method" – is it correct?
Changed to "am".
> 2. build_replindex_scan_key
>
> int table_attno = indkey->values[index_attoff];
> + int strategy_number;
>
>
> Ought to say this is a strategy for "equality", so a better varname
> might be "equality_strategy_number" or "eq_strategy" or similar.
Changed to "eq_strategy".
> src/backend/replication/logical/relation.c
>
> 3. IsIndexUsableForReplicaIdentityFull
>
> It seems there is some overlap between this code which hardwired 2
> valid AMs and the switch statement in your other
> get_equal_strategy_number function which returns a strategy number for
> those 2 AMs.
>
> Would it be better to make another common function like
> get_equality_strategy_for_am(), and then you don’t have to hardwire
> anything? Instead, you can say:
>
> is_usable_type = get_equality_strategy_for_am(indexInfo->ii_Am) !=
> InvalidStrategy;
Added. How do you think?
> src/backend/utils/cache/lsyscache.c
>
> 4. get_opclass_method
>
> +/*
> + * get_opclass_method
> + *
> + * Returns the OID of the index access method operator family is for.
> + */
> +Oid
> +get_opclass_method(Oid opclass)
> +{
> + HeapTuple tp;
> + Form_pg_opclass cla_tup;
> + Oid result;
> +
> + tp = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
> + if (!HeapTupleIsValid(tp))
> + elog(ERROR, "cache lookup failed for opclass %u", opclass);
> + cla_tup = (Form_pg_opclass) GETSTRUCT(tp);
> +
> + result = cla_tup->opcmethod;
> + ReleaseSysCache(tp);
> + return result;
> +}
>
> Is the comment correct? IIUC, this function is not doing anything for
> "families".
Modified to "class".
> src/test/subscription/t/034_hash.pl
>
> 5.
> +# insert some initial data within the range 0-9 for y
> +$node_publisher->safe_psql('postgres',
> + "INSERT INTO test_replica_id_full SELECT i, (i%10)::text FROM
> generate_series(0,10) i"
> +);
>
> Why does the comment only say "for y"?
After considering more, I thought we do not have to mention data.
So removed the part " within the range 0-9 for y".
> 6.
> +# wait until the index is used on the subscriber
> +# XXX: the test will be suspended here
> +$node_publisher->wait_for_catchup($appname);
> +$node_subscriber->poll_query_until('postgres',
> + q{select (idx_scan = 4) from pg_stat_all_indexes where indexrelname
> = 'test_replica_id_full_idx';}
> + )
> + or die
> + "Timed out while waiting for check subscriber tap_sub_rep_full
> updates 4 rows via index";
> +
>
> AFAIK this is OK but it was slightly misleading because it says
> "updates 4 rows" whereas the previous UPDATE was only for 2 rows. So
> here I think the 4 also counts the 2 DELETED rows. The comment can be
> expanded slightly to clarify this.
Clarified two rows were deleted.
> 7.
> FYI, when I ran the TAP test the result was this:
>
> t/034_hash.pl ...................... 1/? # Tests were run but no plan
> was declared and done_testing() was not seen.
> t/034_hash.pl ...................... All 2 subtests passed
> t/100_bugs.pl ...................... ok
>
> Test Summary Report
> -------------------
> t/034_hash.pl (Wstat: 0 Tests: 2 Failed: 0)
> Parse errors: No plan found in TAP output
> Files=36, Tests=457, 338 wallclock secs ( 0.29 usr 0.07 sys + 206.73
> cusr 47.94 csys = 255.03 CPU)
> Result: FAIL
>
> ~
>
> Just adding the missing done_testing() seemed to fix this.
Right. I used meson build system and it said OK. Added.
Furthermore, I added a check to reject indexes which do not implement "amgettuple" API.
More detail, please see [1].
[1]: https://www.postgresql.org/message-id/TYAPR01MB5866E02638D40C4D198334B4F52DA%40TYAPR01MB5866.jpnprd0...
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v2-0001-Allow-to-use-Hash-index-on-subscriber.patch (12.3K, ../../TYAPR01MB58666F301B3D284B77DE7633F52DA@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-v2-0001-Allow-to-use-Hash-index-on-subscriber.patch)
download | inline diff:
From 9b102ac120313b4443e15a7d3d46a59a8b263ff3 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Mon, 19 Jun 2023 08:28:00 +0000
Subject: [PATCH v2] Allow to use Hash index on subscriber
---
src/backend/executor/execReplication.c | 69 ++++++++++++++--
src/backend/replication/logical/relation.c | 27 +++++--
src/backend/utils/cache/lsyscache.c | 22 +++++
src/include/executor/executor.h | 3 +
src/include/utils/lsyscache.h | 1 +
src/test/subscription/meson.build | 1 +
src/test/subscription/t/034_hash.pl | 93 ++++++++++++++++++++++
7 files changed, 204 insertions(+), 12 deletions(-)
create mode 100644 src/test/subscription/t/034_hash.pl
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 9dd7168461..023585b619 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -19,6 +19,8 @@
#include "access/tableam.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "catalog/pg_am_d.h"
+#include "commands/defrem.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/nodeModifyTable.h"
@@ -41,15 +43,67 @@
static bool tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
TypeCacheEntry **eq);
+/*
+ * Return the strategy number which corresponds to the equality operator for
+ * given index access method.
+ *
+ * TODO: support other indexes: GiST, SP-GiST, other user-defined idexes.
+ */
+int
+get_equal_strategy_number_for_am(Oid am)
+{
+ int ret;
+
+ switch (am)
+ {
+ case BTREE_AM_OID:
+ ret = BTEqualStrategyNumber;
+ break;
+ case HASH_AM_OID:
+ ret = HTEqualStrategyNumber;
+ break;
+ case GIST_AM_OID:
+ case SPGIST_AM_OID:
+ /* TODO */
+ case GIN_AM_OID:
+ case BRIN_AM_OID:
+ /*
+ * XXX: GIN and BRIN do not support for the moment because they do not
+ * implement amgettuple API.
+ */
+ default:
+ /* XXX: Do we have to support extended indexes? */
+ ret = InvalidStrategy;
+ break;
+ }
+
+ return ret;
+}
+
+/*
+ * Return the appropriate strategy number which corresponds to the equality
+ * operator.
+ *
+ * TODO: support other indexes: GiST, SP-GiST, other user-defined idexes.
+ */
+int
+get_equal_strategy_number(Oid opclass)
+{
+ Oid am = get_opclass_method(opclass);
+
+ return get_equal_strategy_number_for_am(am);
+}
+
+
/*
* Setup a ScanKey for a search in the relation 'rel' for a tuple 'key' that
* is setup to match 'rel' (*NOT* idxrel!).
*
* Returns how many columns to use for the index scan.
*
- * This is not generic routine, it expects the idxrel to be a btree, non-partial
- * and have at least one column reference (i.e. cannot consist of only
- * expressions).
+ * This is not generic routine, it expects the idxrel to be a btree or a hash,
+ * non-partial and have at least one column reference (i.e. cannot consist of
+ * only expressions).
*
* By definition, replication identity of a rel meets all limitations associated
* with that. Note that any other index could also meet these limitations.
@@ -77,6 +131,7 @@ build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel,
Oid opfamily;
RegProcedure regop;
int table_attno = indkey->values[index_attoff];
+ int eq_strategy;
if (!AttributeNumberIsValid(table_attno))
{
@@ -93,20 +148,22 @@ build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel,
*/
optype = get_opclass_input_type(opclass->values[index_attoff]);
opfamily = get_opclass_family(opclass->values[index_attoff]);
+ eq_strategy = get_equal_strategy_number(opclass->values[index_attoff]);
operator = get_opfamily_member(opfamily, optype,
optype,
- BTEqualStrategyNumber);
+ eq_strategy);
+
if (!OidIsValid(operator))
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- BTEqualStrategyNumber, optype, optype, opfamily);
+ eq_strategy, optype, optype, opfamily);
regop = get_opcode(operator);
/* Initialize the scankey. */
ScanKeyInit(&skey[skey_attoff],
index_attoff + 1,
- BTEqualStrategyNumber,
+ eq_strategy,
regop,
searchslot->tts_values[table_attno - 1]);
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 57ad22b48a..5f5da4c9d8 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -17,6 +17,7 @@
#include "postgres.h"
+#include "access/amapi.h"
#include "access/genam.h"
#include "access/table.h"
#include "catalog/namespace.h"
@@ -779,8 +780,8 @@ RemoteRelContainsLeftMostColumnOnIdx(IndexInfo *indexInfo, AttrMap *attrmap)
/*
* Returns the oid of an index that can be used by the apply worker to scan
- * the relation. The index must be btree, non-partial, and have at least
- * one column reference (i.e. cannot consist of only expressions). These
+ * the relation. The index must be btree or hash, non-partial, and have at
+ * least one column reference (i.e. cannot consist of only expressions). These
* limitations help to keep the index scan similar to PK/RI index scans.
*
* Note that the limitations of index scans for replica identity full only
@@ -841,11 +842,25 @@ FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap)
bool
IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
{
- bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
- bool is_partial = (indexInfo->ii_Predicate != NIL);
- bool is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+ IndexAmRoutine *routine;
+ bool is_suitable_type;
+ bool is_partial;
+ bool is_only_on_expression;
- return is_btree && !is_partial && !is_only_on_expression;
+ /*
+ * XXX: Support only indexes which implement amgettuple API. This is
+ * because RelationFindReplTupleByIndex() assumes to be able to fetch
+ * tuples one by one by the API.
+ */
+ routine = GetIndexAmRoutineByAmId(indexInfo->ii_Am, false);
+ is_suitable_type = ((routine->amgettuple != NULL) &&
+ (get_equal_strategy_number_for_am(indexInfo->ii_Am)
+ != InvalidStrategy));
+
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ return is_suitable_type && !is_partial && !is_only_on_expression;
}
/*
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 60978f9415..05813d98ae 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -1255,6 +1255,28 @@ get_opclass_opfamily_and_input_type(Oid opclass, Oid *opfamily, Oid *opcintype)
return true;
}
+/*
+ * get_opclass_method
+ *
+ * Returns the OID of the index access method operator class is for.
+ */
+Oid
+get_opclass_method(Oid opclass)
+{
+ HeapTuple tp;
+ Form_pg_opclass cla_tup;
+ Oid result;
+
+ tp = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for opclass %u", opclass);
+ cla_tup = (Form_pg_opclass) GETSTRUCT(tp);
+
+ result = cla_tup->opcmethod;
+ ReleaseSysCache(tp);
+ return result;
+}
+
/* ---------- OPERATOR CACHE ---------- */
/*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index ac02247947..3a23510478 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -666,6 +666,9 @@ extern void CheckCmdReplicaIdentity(Relation rel, CmdType cmd);
extern void CheckSubscriptionRelkind(char relkind, const char *nspname,
const char *relname);
+extern int get_equal_strategy_number(Oid opclass);
+extern int get_equal_strategy_number_for_am(Oid am);
+
/*
* prototypes from functions in nodeModifyTable.c
*/
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 4f5418b972..ea8fbe42cd 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -106,6 +106,7 @@ extern Oid get_opclass_family(Oid opclass);
extern Oid get_opclass_input_type(Oid opclass);
extern bool get_opclass_opfamily_and_input_type(Oid opclass,
Oid *opfamily, Oid *opcintype);
+extern Oid get_opclass_method(Oid opclass);
extern RegProcedure get_opcode(Oid opno);
extern char *get_opname(Oid opno);
extern Oid get_op_rettype(Oid opno);
diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build
index bd673a9d68..5fa5f0a790 100644
--- a/src/test/subscription/meson.build
+++ b/src/test/subscription/meson.build
@@ -40,6 +40,7 @@ tests += {
't/031_column_list.pl',
't/032_subscribe_use_index.pl',
't/033_run_as_table_owner.pl',
+ 't/034_hash.pl',
't/100_bugs.pl',
],
},
diff --git a/src/test/subscription/t/034_hash.pl b/src/test/subscription/t/034_hash.pl
new file mode 100644
index 0000000000..3aead6958b
--- /dev/null
+++ b/src/test/subscription/t/034_hash.pl
@@ -0,0 +1,93 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+# Test logical replication behavior with subscriber using available index
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# create publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# create subscriber node
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+my $appname = 'tap_sub';
+my $result = '';
+
+# =============================================================================
+# Testcase start: Subscription can use hash index
+#
+
+# create tables pub and sub
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int, y text)");
+$node_publisher->safe_psql('postgres',
+ "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL");
+$node_subscriber->safe_psql('postgres',
+ "CREATE TABLE test_replica_id_full (x int, y text)");
+$node_subscriber->safe_psql('postgres',
+ "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full USING HASH (x)");
+
+# insert some initial data
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO test_replica_id_full SELECT i, (i%10)::text FROM generate_series(0,10) i"
+);
+
+# create pub/sub
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full");
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tap_sub_rep_full CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub_rep_full"
+);
+
+# wait for initial table synchronization to finish
+$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
+
+# delete 2 rows
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM test_replica_id_full WHERE x IN (5, 6)");
+
+# update 2 rows
+$node_publisher->safe_psql('postgres',
+ "UPDATE test_replica_id_full SET x = 100, y = '200' WHERE x IN (1, 2)");
+
+# wait until the index is used on the subscriber
+# XXX: the test will be suspended here
+$node_publisher->wait_for_catchup($appname);
+$node_subscriber->poll_query_until('postgres',
+ q{select (idx_scan = 4) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
+ )
+ or die
+ "Timed out while waiting for check subscriber tap_sub_rep_full deletes 2 rows and updates 2 rows via index";
+
+# make sure that the subscriber has the correct data after the UPDATE
+$result = $node_subscriber->safe_psql('postgres',
+ "select count(*) from test_replica_id_full WHERE (x = 100 and y = '200')"
+);
+is($result, qq(2),
+ 'ensure subscriber has the correct data at the end of the test');
+
+# make sure that the subscriber has the correct data after the first DELETE
+$result = $node_subscriber->safe_psql('postgres',
+ "select count(*) from test_replica_id_full where x in (5, 6)");
+is($result, qq(0),
+ 'ensure subscriber has the correct data at the end of the test');
+
+# cleanup pub
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_rep_full");
+$node_publisher->safe_psql('postgres', "DROP TABLE test_replica_id_full");
+# cleanup sub
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_rep_full");
+$node_subscriber->safe_psql('postgres', "DROP TABLE test_replica_id_full");
+
+# Testcase end: Subscription can use index
+# =============================================================================
+
+done_testing();
--
2.27.0
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2023-07-07 12:54 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-10-08 08:22 Small run-time pruning doc fix David Rowley <[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-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 v12 1/5] 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-04-27 01:41 [PATCH v5] Correctly update contfol file at the end of archive recovery Kyotaro Horiguchi <[email protected]>
2023-07-07 12:54 RE: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL Hayato Kuroda (Fujitsu) <[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