agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feed[PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
10+ messages / 5 participants
[nested] [flat]
* [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-01 08:53 Adam Lee <adam8157@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Adam Lee @ 2026-04-01 08:53 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Michael Paquier <michael@paquier.xyz>
Hi hackers,
I ran into this while working on recovery pre-check logic that relies on
pg_controldata to verify whether replay has reached a specific restore point.
Reproducer:
```
-- on primary:
CHECKPOINT;
SELECT pg_create_restore_point('test_rp');
-- recover with:
-- recovery_target_name = 'test_rp'
-- recovery_target_action = 'shutdown'
-- after recovery shuts down:
pg_controldata shows minRecoveryPoint 104 bytes behind
pg_create_restore_point's return value (104 bytes = one
RESTORE_POINT WAL record).
```
My RCA:
When recovery_target_action=shutdown triggers, the checkpointer performs a
shutdown restartpoint via CreateRestartPoint(). If a CHECKPOINT record was
replayed shortly before the recovery target, CreateRestartPoint advances
minRecoveryPoint to the end of that CHECKPOINT record.
However, any no-op records replayed after the CHECKPOINT (such as
RESTORE_POINT) do not dirty pages, so the lazy minRecoveryPoint update that
normally happens during page flushes never fires for them. As a result,
minRecoveryPoint in pg_control ends up behind the actual replay position.
My Fix:
The attached patch fixes this by reading the current replay position from
shared memory after advancing minRecoveryPoint to the checkpoint end, and
advancing further if replay has progressed past it. This is safe because
CheckPointGuts() has already flushed all dirty buffers and the startup process
has exited, so replayEndRecPtr is stable and all pages are on disk.
--
Adam
From 8a6b070d860b8241a41057f021a621b7daa55f22 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory after
advancing minRecoveryPoint to the checkpoint, and advancing it further
if replay has progressed past the checkpoint.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2c1c6f88b74..ec639054620 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7868,11 +7868,34 @@ CreateRestartPoint(int flags)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ }
+
+ /*
+ * Also advance minRecoveryPoint past any WAL replayed after
+ * the checkpoint. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
+ {
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
--
2.47.3
Attachments:
[text/plain] 0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch (3.2K, ../../aczc9cxkr7SEHXV5@MAC-CVW1VHW5R6/2-0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch)
download | inline diff:
From 8a6b070d860b8241a41057f021a621b7daa55f22 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory after
advancing minRecoveryPoint to the checkpoint, and advancing it further
if replay has progressed past the checkpoint.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2c1c6f88b74..ec639054620 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7868,11 +7868,34 @@ CreateRestartPoint(int flags)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ }
+
+ /*
+ * Also advance minRecoveryPoint past any WAL replayed after
+ * the checkpoint. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
+ {
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
--
2.47.3
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-01 09:38 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Adam Lee <adam8157@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Heikki Linnakangas @ 2026-04-01 09:38 UTC (permalink / raw)
To: Adam Lee <adam8157@gmail.com>; pgsql-hackers@lists.postgresql.org; +Cc: Michael Paquier <michael@paquier.xyz>
On 01/04/2026 11:53, Adam Lee wrote:
> Hi hackers,
>
> I ran into this while working on recovery pre-check logic that relies on
> pg_controldata to verify whether replay has reached a specific restore point.
>
> Reproducer:
>
> ```
> -- on primary:
> CHECKPOINT;
> SELECT pg_create_restore_point('test_rp');
>
> -- recover with:
> -- recovery_target_name = 'test_rp'
> -- recovery_target_action = 'shutdown'
>
> -- after recovery shuts down:
> pg_controldata shows minRecoveryPoint 104 bytes behind
> pg_create_restore_point's return value (104 bytes = one
> RESTORE_POINT WAL record).
> ```
>
> My RCA:
>
> When recovery_target_action=shutdown triggers, the checkpointer performs a
> shutdown restartpoint via CreateRestartPoint(). If a CHECKPOINT record was
> replayed shortly before the recovery target, CreateRestartPoint advances
> minRecoveryPoint to the end of that CHECKPOINT record.
>
> However, any no-op records replayed after the CHECKPOINT (such as
> RESTORE_POINT) do not dirty pages, so the lazy minRecoveryPoint update that
> normally happens during page flushes never fires for them. As a result,
> minRecoveryPoint in pg_control ends up behind the actual replay position.
Hmm, what exactly does minRecoveryPoint mean? The current behavior is
correct in the sense that if you restarted recovery, you could still
stop the recovery at the earlier LSN that's the minRecoveryPoint in the
control file, and the system would be consistent. I agree it feels
pretty weird though, it would seem natural to advance minRecoveryPoint
to the last replayed record on a restartpoint.
> My Fix:
>
> The attached patch fixes this by reading the current replay position from
> shared memory after advancing minRecoveryPoint to the checkpoint end, and
> advancing further if replay has progressed past it. This is safe because
> CheckPointGuts() has already flushed all dirty buffers and the startup process
> has exited, so replayEndRecPtr is stable and all pages are on disk.
We have this comment earlier in CreateRestartPoint():
> * We don't explicitly advance minRecoveryPoint when we do create a
> * restartpoint. It's assumed that flushing the buffers will do that as a
> * side-effect.
That assumption is not quite right, then.
Perhaps we should simply call UpdateMinRecoveryPoint()? That would cause
the control file to be flushed twice though, so it's a little
inefficient, but maybe that's fine.
If we go with your patch, does it make this existing logic below obsolete?
> if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
> {
> if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
> {
> ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
> ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
>
> /* update local copy */
> LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
> LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
> }
> if (flags & CHECKPOINT_IS_SHUTDOWN)
> ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
> }
- Heikki
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-01 11:19 Adam Lee <adam8157@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 10+ messages in thread
From: Adam Lee @ 2026-04-01 11:19 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
On Wed, Apr 01, 2026 at 12:38:15PM +0300, Heikki Linnakangas wrote:
> > My RCA:
> >
> > When recovery_target_action=shutdown triggers, the checkpointer performs a
> > shutdown restartpoint via CreateRestartPoint(). If a CHECKPOINT record was
> > replayed shortly before the recovery target, CreateRestartPoint advances
> > minRecoveryPoint to the end of that CHECKPOINT record.
> >
> > However, any no-op records replayed after the CHECKPOINT (such as
> > RESTORE_POINT) do not dirty pages, so the lazy minRecoveryPoint update that
> > normally happens during page flushes never fires for them. As a result,
> > minRecoveryPoint in pg_control ends up behind the actual replay position.
>
> Hmm, what exactly does minRecoveryPoint mean? The current behavior is
> correct in the sense that if you restarted recovery, you could still stop
> the recovery at the earlier LSN that's the minRecoveryPoint in the control
> file, and the system would be consistent. I agree it feels pretty weird
> though, it would seem natural to advance minRecoveryPoint to the last
> replayed record on a restartpoint.
Yes, the system is consistent either way. But for the shutdown action,
it would be natural to advance minRecoveryPoint to the last replayed
record, same as the pause and promote actions do, whose startup process
don't exit and have the chance calling UpdateMinRecoveryPoint().
> Perhaps we should simply call UpdateMinRecoveryPoint()? That would cause the
> control file to be flushed twice though, so it's a little inefficient, but
> maybe that's fine.
And calling UpdateMinRecoveryPoint() still needs to explain why later
the codes need to ensure minRecoveryPoint is past the checkpoint record,
to me it doesn't make things simpler, but I'm OK either way.
> If we go with your patch, does it make this existing logic below obsolete?
>
> > if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
> > {
> > if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
> > {
> > ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
> > ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
> >
> > /* update local copy */
> > LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
> > LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
> > }
> > if (flags & CHECKPOINT_IS_SHUTDOWN)
> > ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
> > }
Indeed, no need to check lastCheckPointEndPtr, replayEndRecPtr is always >= lastCheckPointEndPtr
--
Adam
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-03 05:10 Adam Lee <adam8157@gmail.com>
parent: Adam Lee <adam8157@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Adam Lee @ 2026-04-03 05:10 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
PATCH v2 removed the variable lastCheckPointEndPtr and refined the comments.
Thanks for reviewing.
--
Adam
From f639b7c0bc76ab06ff5a05c26aceb1764955a849 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH v2] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory and
advancing minRecoveryPoint to match it. Since the replay position is
always at least as far as the checkpoint end, this also subsumes the
previous lastCheckPointEndPtr update.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2c1c6f88b74..ff9e373c4fa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7721,7 +7721,6 @@ bool
CreateRestartPoint(int flags)
{
XLogRecPtr lastCheckPointRecPtr;
- XLogRecPtr lastCheckPointEndPtr;
CheckPoint lastCheckPoint;
XLogRecPtr PriorRedoPtr;
XLogRecPtr receivePtr;
@@ -7737,7 +7736,6 @@ CreateRestartPoint(int flags)
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
- lastCheckPointEndPtr = XLogCtl->lastCheckPointEndPtr;
lastCheckPoint = XLogCtl->lastCheckPoint;
SpinLockRelease(&XLogCtl->info_lck);
@@ -7864,15 +7862,32 @@ CreateRestartPoint(int flags)
*/
if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ /*
+ * Advance minRecoveryPoint to at least the current replay
+ * position. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
--
2.47.3
Attachments:
[text/plain] 0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch (3.9K, ../../ac9LsF7bL6UW75VM@MAC-CVW1VHW5R6/2-0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch)
download | inline diff:
From f639b7c0bc76ab06ff5a05c26aceb1764955a849 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH v2] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory and
advancing minRecoveryPoint to match it. Since the replay position is
always at least as far as the checkpoint end, this also subsumes the
previous lastCheckPointEndPtr update.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2c1c6f88b74..ff9e373c4fa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7721,7 +7721,6 @@ bool
CreateRestartPoint(int flags)
{
XLogRecPtr lastCheckPointRecPtr;
- XLogRecPtr lastCheckPointEndPtr;
CheckPoint lastCheckPoint;
XLogRecPtr PriorRedoPtr;
XLogRecPtr receivePtr;
@@ -7737,7 +7736,6 @@ CreateRestartPoint(int flags)
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
- lastCheckPointEndPtr = XLogCtl->lastCheckPointEndPtr;
lastCheckPoint = XLogCtl->lastCheckPoint;
SpinLockRelease(&XLogCtl->info_lck);
@@ -7864,15 +7862,32 @@ CreateRestartPoint(int flags)
*/
if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ /*
+ * Advance minRecoveryPoint to at least the current replay
+ * position. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
--
2.47.3
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-08 02:41 Michael Paquier <michael@paquier.xyz>
parent: Adam Lee <adam8157@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-04-08 02:41 UTC (permalink / raw)
To: Adam Lee <adam8157@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers@lists.postgresql.org
On Fri, Apr 03, 2026 at 01:10:08PM +0800, Adam Lee wrote:
> PATCH v2 removed the variable lastCheckPointEndPtr and refined the comments.
>
> Thanks for reviewing.
TBH, I am not convinced that this optimization in the control file is
worth it. minRecoveryPoint refers to a state where the on-disk pages
are all consistent based on their stored LSNs, see also the
cross-check that we do at the end of recovery in the event of
inconsistent pages. In most cases (say in the 99%-ish range), we will
have page flushes, making it non-relevant.
With time, I have also learnt the hard way that the less code paths
that update minRecoveryPoint in the control file, as well as the local
copies, the better. Simplifying this code is something we should try
to work on. Complicating it more has less value.
If we decide that this optimization is worth having, I am going to
request a TAP test to validate the behavior you'd expect out of it.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../adXAXO2qASwOHaQ0@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-04-08 07:05 Adam Lee <adam8157@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Adam Lee @ 2026-04-08 07:05 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers@lists.postgresql.org
Thanks for reviewing!
On Wed, Apr 08, 2026 at 11:41:32AM +0900, Michael Paquier wrote:
> TBH, I am not convinced that this optimization in the control file is
> worth it. minRecoveryPoint refers to a state where the on-disk pages
> are all consistent based on their stored LSNs, see also the
> cross-check that we do at the end of recovery in the event of
> inconsistent pages. In most cases (say in the 99%-ish range), we will
> have page flushes, making it non-relevant.
I understand your point about minRecoveryPoint being primarily about on-disk
page consistency.
However, this value is also exposed via pg_controldata and used by external
tools - for example, pg_rewind reads it to determine where to start replaying
WAL. Third-party backup/recovery tools (like the project I'm working on) may
rely on it to verify that recovery has actually reached a specific restore
point. When the value is behind the actual replay position, these tools can
draw incorrect conclusions.
> With time, I have also learnt the hard way that the less code paths
> that update minRecoveryPoint in the control file, as well as the local
> copies, the better. Simplifying this code is something we should try
> to work on. Complicating it more has less value.
Agree. Note that this patch actually removes a code path rather than adding one
- the previous lastCheckPointEndPtr update is subsumed by the GetCurrentReplayRecPtr()
call, the complexity is roughly the same in terms of code paths and logic (I think).
> If we decide that this optimization is worth having, I am going to
> request a TAP test to validate the behavior you'd expect out of it.
PATCH v3 attached, which includes a TAP test covering multiple scenarios:
shutdown with and without a preceding CHECKPOINT, as well as promote and
pause actions for completeness. The test verifies that minRecoveryPoint
reaches at least the restore point LSN in each case.
--
Adam
From 5c9fcd5a5c0f54773c3eb9fc85c7b8dd59dea690 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH v3] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory and
advancing minRecoveryPoint to match it. Since the replay position is
always at least as far as the checkpoint end, this also subsumes the
previous lastCheckPointEndPtr update.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 31 ++-
.../t/053_min_recovery_point_target.pl | 215 ++++++++++++++++++
2 files changed, 238 insertions(+), 8 deletions(-)
create mode 100644 src/test/recovery/t/053_min_recovery_point_target.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 260fc801ce2..1864ed148cb 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8129,7 +8129,6 @@ bool
CreateRestartPoint(int flags)
{
XLogRecPtr lastCheckPointRecPtr;
- XLogRecPtr lastCheckPointEndPtr;
CheckPoint lastCheckPoint;
XLogRecPtr PriorRedoPtr;
XLogRecPtr receivePtr;
@@ -8145,7 +8144,6 @@ CreateRestartPoint(int flags)
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
- lastCheckPointEndPtr = XLogCtl->lastCheckPointEndPtr;
lastCheckPoint = XLogCtl->lastCheckPoint;
SpinLockRelease(&XLogCtl->info_lck);
@@ -8272,15 +8270,32 @@ CreateRestartPoint(int flags)
*/
if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ /*
+ * Advance minRecoveryPoint to at least the current replay
+ * position. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
diff --git a/src/test/recovery/t/053_min_recovery_point_target.pl b/src/test/recovery/t/053_min_recovery_point_target.pl
new file mode 100644
index 00000000000..08ff2bd4d92
--- /dev/null
+++ b/src/test/recovery/t/053_min_recovery_point_target.pl
@@ -0,0 +1,215 @@
+
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Test that minRecoveryPoint in pg_control is correctly advanced
+# when recovering to a restore point with various recovery_target_action
+# settings. The bug being tested: when a CHECKPOINT record is replayed
+# immediately before a no-op record like RESTORE_POINT, the shutdown
+# restartpoint would only advance minRecoveryPoint to the CHECKPOINT end,
+# not to the actual replay position.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+# Helper: parse minRecoveryPoint from pg_controldata output
+sub get_min_recovery_point
+{
+ my $datadir = shift;
+ my ($stdout, $stderr) = run_command([ 'pg_controldata', $datadir ]);
+ my @control_data = split("\n", $stdout);
+ foreach (@control_data)
+ {
+ if ($_ =~ /^Minimum recovery ending location:\s*(.*)$/mg)
+ {
+ return $1;
+ }
+ }
+ die "No minRecoveryPoint in control file found\n";
+}
+
+# Initialize primary node with archiving
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Take a backup before generating the WAL we want to recover
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# Create two restore points:
+# 1) "rp_no_ckpt" before any new CHECKPOINT
+# 2) "rp_after_ckpt" immediately after a CHECKPOINT (triggers the bug)
+#
+# Capture the LSN returned by pg_create_restore_point() directly to
+# avoid a race with background WAL activity (e.g. RUNNING_XACTS).
+my $lsn_no_ckpt = $node_primary->safe_psql('postgres',
+ "SELECT pg_create_restore_point('rp_no_ckpt');");
+
+my $lsn_after_ckpt = $node_primary->safe_psql('postgres',
+ "CHECKPOINT; SELECT pg_create_restore_point('rp_after_ckpt');");
+
+# Force WAL switch to ensure the segment is archived
+$node_primary->safe_psql('postgres', "SELECT pg_switch_wal()");
+my $walfile = $node_primary->safe_psql('postgres',
+ "SELECT pg_walfile_name('$lsn_no_ckpt')");
+$node_primary->poll_query_until('postgres',
+ "SELECT last_archived_wal >= '$walfile' FROM pg_stat_archiver")
+ or die "Timed out while waiting for WAL archiving";
+
+$node_primary->stop;
+
+##
+## Test 1: recovery_target_action = shutdown, with preceding CHECKPOINT
+##
+## This is the scenario that triggers the bug. CreateRestartPoint
+## processes the new CHECKPOINT and only advances minRecoveryPoint to
+## the end of that CHECKPOINT record, missing the RESTORE_POINT that
+## follows.
+##
+my $node_shutdown_ckpt = PostgreSQL::Test::Cluster->new('shutdown_ckpt');
+$node_shutdown_ckpt->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_shutdown_ckpt->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'shutdown'
+});
+
+# Use run_log + pg_ctl because the server shuts itself down after
+# reaching the recovery target.
+run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_shutdown_ckpt->data_dir,
+ '--log' => $node_shutdown_ckpt->logfile,
+ 'start',
+ ]);
+
+foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
+{
+ last if !-f $node_shutdown_ckpt->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+
+my $logfile = slurp_file($node_shutdown_ckpt->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'shutdown with checkpoint: recovery reached the restore point');
+
+my $min_lsn = get_min_recovery_point($node_shutdown_ckpt->data_dir);
+ok($min_lsn ge $lsn_after_ckpt,
+ "shutdown with checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_after_ckpt)");
+
+##
+## Test 2: recovery_target_action = shutdown, without preceding CHECKPOINT
+##
+## The restore point is created before the CHECKPOINT, so there is no
+## unprocessed checkpoint when recovery reaches the target.
+## CreateRestartPoint calls UpdateMinRecoveryPoint() which reads the
+## replay position from shared memory. This path was already correct,
+## but we test it for completeness.
+##
+my $node_shutdown_no_ckpt = PostgreSQL::Test::Cluster->new('shutdown_no_ckpt');
+$node_shutdown_no_ckpt->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_shutdown_no_ckpt->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_no_ckpt'
+recovery_target_action = 'shutdown'
+});
+
+run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_shutdown_no_ckpt->data_dir,
+ '--log' => $node_shutdown_no_ckpt->logfile,
+ 'start',
+ ]);
+
+foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
+{
+ last if !-f $node_shutdown_no_ckpt->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+
+$logfile = slurp_file($node_shutdown_no_ckpt->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_no_ckpt"/,
+ 'shutdown without checkpoint: recovery reached the restore point');
+
+$min_lsn = get_min_recovery_point($node_shutdown_no_ckpt->data_dir);
+ok($min_lsn ge $lsn_no_ckpt,
+ "shutdown without checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_no_ckpt)");
+
+##
+## Test 3: recovery_target_action = promote, with preceding CHECKPOINT
+##
+## After promotion, the server writes an end-of-recovery checkpoint and
+## minRecoveryPoint is cleared, so we can only verify that recovery
+## reached the target.
+##
+my $node_promote = PostgreSQL::Test::Cluster->new('promote_ckpt');
+$node_promote->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_promote->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'promote'
+});
+
+$node_promote->start;
+$node_promote->poll_query_until('postgres',
+ "SELECT pg_is_in_recovery() = 'f';")
+ or die "Timed out while waiting for promotion";
+
+$logfile = slurp_file($node_promote->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'promote with checkpoint: recovery reached the restore point');
+
+$node_promote->stop;
+
+##
+## Test 4: recovery_target_action = pause, with preceding CHECKPOINT
+##
+## While paused, the server is still in recovery. After a clean
+## shutdown, the checkpointer writes a shutdown restartpoint, so we
+## can verify minRecoveryPoint offline.
+##
+my $node_pause = PostgreSQL::Test::Cluster->new('pause_ckpt');
+$node_pause->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_pause->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'pause'
+});
+
+$node_pause->start;
+
+# Wait until recovery reaches the pause point
+$node_pause->poll_query_until('postgres',
+ "SELECT pg_get_wal_replay_pause_state() = 'paused';")
+ or die "Timed out while waiting for recovery to pause";
+
+$logfile = slurp_file($node_pause->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'pause with checkpoint: recovery reached the restore point');
+
+# Shut down cleanly, which triggers a shutdown restartpoint
+$node_pause->stop('fast');
+
+$min_lsn = get_min_recovery_point($node_pause->data_dir);
+ok($min_lsn ge $lsn_after_ckpt,
+ "pause with checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_after_ckpt)");
+
+done_testing();
--
2.47.3
Attachments:
[text/plain] 0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch (11.4K, ../../adX97VrD4pJKF8zt@MAC-CVW1VHW5R6/2-0001-Fix-minRecoveryPoint-not-advanced-past-checkpoint-in.patch)
download | inline diff:
From 5c9fcd5a5c0f54773c3eb9fc85c7b8dd59dea690 Mon Sep 17 00:00:00 2001
From: Adam Lee <adam8157@gmail.com>
Date: Tue, 31 Mar 2026 18:43:53 +0800
Subject: [PATCH v3] Fix minRecoveryPoint not advanced past checkpoint in
CreateRestartPoint
When recovery_target_action=shutdown triggers, the checkpointer performs
a shutdown restartpoint via CreateRestartPoint. If a new CHECKPOINT
record was replayed shortly before the recovery target, the restartpoint
advances minRecoveryPoint to the end of that CHECKPOINT record. And the
following replay doesn't advance minRecoveryPoint, it's assumed that
flushing the buffers will do that as a side-effect.
But no-op records replayed after the CHECKPOINT (such as RESTORE_POINT) do
not dirty any pages, so the minRecoveryPoint is not updated as expected.
As a result, minRecoveryPoint in pg_control ends up behind the actual
replay position. This does not cause a recovery correctness issue,
however the inaccurate pg_controldata "Minimum recovery ending location"
prevents users or tools from using this value to verify that recovery
has reached a specific restore point.
Fix by reading the current replay position from shared memory and
advancing minRecoveryPoint to match it. Since the replay position is
always at least as far as the checkpoint end, this also subsumes the
previous lastCheckPointEndPtr update.
Reproducer:
CHECKPOINT; SELECT pg_create_restore_point('test_rp');
-- recover with recovery_target_name + recovery_target_action=shutdown
-- pg_controldata shows minRecoveryPoint 104 bytes behind
---
src/backend/access/transam/xlog.c | 31 ++-
.../t/053_min_recovery_point_target.pl | 215 ++++++++++++++++++
2 files changed, 238 insertions(+), 8 deletions(-)
create mode 100644 src/test/recovery/t/053_min_recovery_point_target.pl
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 260fc801ce2..1864ed148cb 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8129,7 +8129,6 @@ bool
CreateRestartPoint(int flags)
{
XLogRecPtr lastCheckPointRecPtr;
- XLogRecPtr lastCheckPointEndPtr;
CheckPoint lastCheckPoint;
XLogRecPtr PriorRedoPtr;
XLogRecPtr receivePtr;
@@ -8145,7 +8144,6 @@ CreateRestartPoint(int flags)
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr;
- lastCheckPointEndPtr = XLogCtl->lastCheckPointEndPtr;
lastCheckPoint = XLogCtl->lastCheckPoint;
SpinLockRelease(&XLogCtl->info_lck);
@@ -8272,15 +8270,32 @@ CreateRestartPoint(int flags)
*/
if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ /*
+ * Advance minRecoveryPoint to at least the current replay
+ * position. Normally this happens as a side effect of
+ * flushing dirty buffers, but during a shutdown restartpoint
+ * there may be records between the checkpoint and the
+ * recovery target that didn't dirty any buffers (e.g. a
+ * RESTORE_POINT record). Without this, a shutdown triggered
+ * by recovery_target_action leaves minRecoveryPoint behind
+ * the actual replay position.
+ */
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ XLogRecPtr replayPtr;
+ TimeLineID replayTLI;
- /* update local copy */
- LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
- LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ replayPtr = GetCurrentReplayRecPtr(&replayTLI);
+ if (ControlFile->minRecoveryPoint < replayPtr)
+ {
+ ControlFile->minRecoveryPoint = replayPtr;
+ ControlFile->minRecoveryPointTLI = replayTLI;
+ }
}
+
+ /* update local copy */
+ LocalMinRecoveryPoint = ControlFile->minRecoveryPoint;
+ LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
}
diff --git a/src/test/recovery/t/053_min_recovery_point_target.pl b/src/test/recovery/t/053_min_recovery_point_target.pl
new file mode 100644
index 00000000000..08ff2bd4d92
--- /dev/null
+++ b/src/test/recovery/t/053_min_recovery_point_target.pl
@@ -0,0 +1,215 @@
+
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Test that minRecoveryPoint in pg_control is correctly advanced
+# when recovering to a restore point with various recovery_target_action
+# settings. The bug being tested: when a CHECKPOINT record is replayed
+# immediately before a no-op record like RESTORE_POINT, the shutdown
+# restartpoint would only advance minRecoveryPoint to the CHECKPOINT end,
+# not to the actual replay position.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+
+# Helper: parse minRecoveryPoint from pg_controldata output
+sub get_min_recovery_point
+{
+ my $datadir = shift;
+ my ($stdout, $stderr) = run_command([ 'pg_controldata', $datadir ]);
+ my @control_data = split("\n", $stdout);
+ foreach (@control_data)
+ {
+ if ($_ =~ /^Minimum recovery ending location:\s*(.*)$/mg)
+ {
+ return $1;
+ }
+ }
+ die "No minRecoveryPoint in control file found\n";
+}
+
+# Initialize primary node with archiving
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(has_archiving => 1, allows_streaming => 1);
+$node_primary->start;
+
+# Take a backup before generating the WAL we want to recover
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# Create two restore points:
+# 1) "rp_no_ckpt" before any new CHECKPOINT
+# 2) "rp_after_ckpt" immediately after a CHECKPOINT (triggers the bug)
+#
+# Capture the LSN returned by pg_create_restore_point() directly to
+# avoid a race with background WAL activity (e.g. RUNNING_XACTS).
+my $lsn_no_ckpt = $node_primary->safe_psql('postgres',
+ "SELECT pg_create_restore_point('rp_no_ckpt');");
+
+my $lsn_after_ckpt = $node_primary->safe_psql('postgres',
+ "CHECKPOINT; SELECT pg_create_restore_point('rp_after_ckpt');");
+
+# Force WAL switch to ensure the segment is archived
+$node_primary->safe_psql('postgres', "SELECT pg_switch_wal()");
+my $walfile = $node_primary->safe_psql('postgres',
+ "SELECT pg_walfile_name('$lsn_no_ckpt')");
+$node_primary->poll_query_until('postgres',
+ "SELECT last_archived_wal >= '$walfile' FROM pg_stat_archiver")
+ or die "Timed out while waiting for WAL archiving";
+
+$node_primary->stop;
+
+##
+## Test 1: recovery_target_action = shutdown, with preceding CHECKPOINT
+##
+## This is the scenario that triggers the bug. CreateRestartPoint
+## processes the new CHECKPOINT and only advances minRecoveryPoint to
+## the end of that CHECKPOINT record, missing the RESTORE_POINT that
+## follows.
+##
+my $node_shutdown_ckpt = PostgreSQL::Test::Cluster->new('shutdown_ckpt');
+$node_shutdown_ckpt->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_shutdown_ckpt->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'shutdown'
+});
+
+# Use run_log + pg_ctl because the server shuts itself down after
+# reaching the recovery target.
+run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_shutdown_ckpt->data_dir,
+ '--log' => $node_shutdown_ckpt->logfile,
+ 'start',
+ ]);
+
+foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
+{
+ last if !-f $node_shutdown_ckpt->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+
+my $logfile = slurp_file($node_shutdown_ckpt->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'shutdown with checkpoint: recovery reached the restore point');
+
+my $min_lsn = get_min_recovery_point($node_shutdown_ckpt->data_dir);
+ok($min_lsn ge $lsn_after_ckpt,
+ "shutdown with checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_after_ckpt)");
+
+##
+## Test 2: recovery_target_action = shutdown, without preceding CHECKPOINT
+##
+## The restore point is created before the CHECKPOINT, so there is no
+## unprocessed checkpoint when recovery reaches the target.
+## CreateRestartPoint calls UpdateMinRecoveryPoint() which reads the
+## replay position from shared memory. This path was already correct,
+## but we test it for completeness.
+##
+my $node_shutdown_no_ckpt = PostgreSQL::Test::Cluster->new('shutdown_no_ckpt');
+$node_shutdown_no_ckpt->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_shutdown_no_ckpt->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_no_ckpt'
+recovery_target_action = 'shutdown'
+});
+
+run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_shutdown_no_ckpt->data_dir,
+ '--log' => $node_shutdown_no_ckpt->logfile,
+ 'start',
+ ]);
+
+foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default)
+{
+ last if !-f $node_shutdown_no_ckpt->data_dir . '/postmaster.pid';
+ usleep(100_000);
+}
+
+$logfile = slurp_file($node_shutdown_no_ckpt->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_no_ckpt"/,
+ 'shutdown without checkpoint: recovery reached the restore point');
+
+$min_lsn = get_min_recovery_point($node_shutdown_no_ckpt->data_dir);
+ok($min_lsn ge $lsn_no_ckpt,
+ "shutdown without checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_no_ckpt)");
+
+##
+## Test 3: recovery_target_action = promote, with preceding CHECKPOINT
+##
+## After promotion, the server writes an end-of-recovery checkpoint and
+## minRecoveryPoint is cleared, so we can only verify that recovery
+## reached the target.
+##
+my $node_promote = PostgreSQL::Test::Cluster->new('promote_ckpt');
+$node_promote->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_promote->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'promote'
+});
+
+$node_promote->start;
+$node_promote->poll_query_until('postgres',
+ "SELECT pg_is_in_recovery() = 'f';")
+ or die "Timed out while waiting for promotion";
+
+$logfile = slurp_file($node_promote->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'promote with checkpoint: recovery reached the restore point');
+
+$node_promote->stop;
+
+##
+## Test 4: recovery_target_action = pause, with preceding CHECKPOINT
+##
+## While paused, the server is still in recovery. After a clean
+## shutdown, the checkpointer writes a shutdown restartpoint, so we
+## can verify minRecoveryPoint offline.
+##
+my $node_pause = PostgreSQL::Test::Cluster->new('pause_ckpt');
+$node_pause->init_from_backup($node_primary, $backup_name,
+ standby => 0,
+ has_restoring => 1);
+$node_pause->append_conf('postgresql.conf', qq{
+recovery_target_name = 'rp_after_ckpt'
+recovery_target_action = 'pause'
+});
+
+$node_pause->start;
+
+# Wait until recovery reaches the pause point
+$node_pause->poll_query_until('postgres',
+ "SELECT pg_get_wal_replay_pause_state() = 'paused';")
+ or die "Timed out while waiting for recovery to pause";
+
+$logfile = slurp_file($node_pause->logfile());
+like(
+ $logfile,
+ qr/recovery stopping at restore point "rp_after_ckpt"/,
+ 'pause with checkpoint: recovery reached the restore point');
+
+# Shut down cleanly, which triggers a shutdown restartpoint
+$node_pause->stop('fast');
+
+$min_lsn = get_min_recovery_point($node_pause->data_dir);
+ok($min_lsn ge $lsn_after_ckpt,
+ "pause with checkpoint: minRecoveryPoint ($min_lsn) >= restore point LSN ($lsn_after_ckpt)");
+
+done_testing();
--
2.47.3
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-06-09 12:38 Nitin Jadhav <nitinjadhavpostgres@gmail.com>
parent: Adam Lee <adam8157@gmail.com>
0 siblings, 2 replies; 10+ messages in thread
From: Nitin Jadhav @ 2026-06-09 12:38 UTC (permalink / raw)
To: Adam Lee <adam8157@gmail.com>; Michael Paquier <michael@paquier.xyz>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers@lists.postgresql.org
Hi Adam, Michael,
I went through the discussion and wanted to share my thoughts.
I agree with Michael's point that semantically, minRecoveryPoint
represents the minimum LSN needed for on-disk page consistency, and
from that strict perspective, the current behavior is technically
correct. However, I also acknowledge Adam's concern about the
practical impact on tooling and automation that relies on
pg_controldata to accurately reflect recovery progress.
Given that the current behavior creates inconsistency across
recovery_target_action settings (pause and promote behave differently
than shutdown), external tools such as pg_rewind, backup solutions,
and monitoring systems depend on this value for operational decisions,
I support moving forward with this patch.
Regarding Michael's point about simplifying the minRecoveryPoint
update logic—I completely agree that reducing code paths and
complexity in this area would be valuable. If there are specific areas
that could benefit from simplification or refactoring, I would be
interested in helping with that work.
I haven't reviewed the patch in detail yet, but I will do so and share
any feedback or comments.
Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-06-10 01:16 Michael Paquier <michael@paquier.xyz>
parent: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2026-06-10 01:16 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Adam Lee <adam8157@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers@lists.postgresql.org
On Tue, Jun 09, 2026 at 06:08:36PM +0530, Nitin Jadhav wrote:
> Given that the current behavior creates inconsistency across
> recovery_target_action settings (pause and promote behave differently
> than shutdown), external tools such as pg_rewind, backup solutions,
> and monitoring systems depend on this value for operational decisions,
> I support moving forward with this patch.
FWIW, I am still unconvinced even after a second read of the thread.
In basically all workloads (hand-waving a number but let's say 99%),
we are going to have page flushes anyway between a CHECKPOINT record
replayed and the recovery target, where each one is going to naturally
update the minRecoveryPoint. I'd still want less paths that update
minRecoveryPoint at the end of the day, not more of them.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aii66Z4j3W2gm8K1@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-06-10 04:53 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 1 reply; 10+ messages in thread
From: Kyotaro Horiguchi @ 2026-06-10 04:53 UTC (permalink / raw)
To: nitinjadhavpostgres@gmail.com; +Cc: adam8157@gmail.com; michael@paquier.xyz; hlinnaka@iki.fi; pgsql-hackers@lists.postgresql.org
I tend to agree with Michael on the meaning of minRecoveryPoint.
If a tool is using minRecoveryPoint to determine how far recovery has
progressed, then I would say it is looking at the wrong value. I also
do not quite understand what such a tool is trying to verify by
comparing minRecoveryPoint with the recovery target LSN.
If the goal is to verify that recovery reached the configured target,
PostgreSQL already performs that check itself and exits with a FATAL
error otherwise:
> ereport(FATAL,
> (errcode(ERRCODE_CONFIG_FILE_ERROR),
> errmsg("recovery ended before configured recovery target was reached")));
If the underlying use case were explained in more detail, we could
probably have a more concrete discussion about how to address
it. However, I do not think changing the semantics of minRecoveryPoint
is the right solution based on the information available so far.
Regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint
@ 2026-06-25 13:48 Nitin Jadhav <nitinjadhavpostgres@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Nitin Jadhav @ 2026-06-25 13:48 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: adam8157@gmail.com; michael@paquier.xyz; hlinnaka@iki.fi; pgsql-hackers@lists.postgresql.org
Hi Michael, Kyotaro,
Thanks for the clarifications and feedback.
I spent some time looking at how core tools like pg_rewind use this
value. After going through the code paths and the existing pg_rewind
tests, I understand the concern that minRecoveryPoint should primarily
be treated as a consistency-floor value. I also see that pg_rewind
currently depends on minRecoveryPoint in some standby-related
decisions, but I agree that this does not necessarily mean it should
be treated as a general indicator of how far recovery has progressed.
Given that, I’m okay to treat the current behavior as expected for now
and avoid pushing further on this until we have Adam’s view on whether
he still wants to pursue the change with stronger justification and
tests, or whether he prefers to drop or rework it based on this
semantic direction.
Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-06-25 13:48 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-01 08:53 [PATCH] Fix minRecoveryPoint not advanced past checkpoint in CreateRestartPoint Adam Lee <adam8157@gmail.com>
2026-04-01 09:38 ` Heikki Linnakangas <hlinnaka@iki.fi>
2026-04-01 11:19 ` Adam Lee <adam8157@gmail.com>
2026-04-03 05:10 ` Adam Lee <adam8157@gmail.com>
2026-04-08 02:41 ` Michael Paquier <michael@paquier.xyz>
2026-04-08 07:05 ` Adam Lee <adam8157@gmail.com>
2026-06-09 12:38 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2026-06-10 01:16 ` Michael Paquier <michael@paquier.xyz>
2026-06-10 04:53 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2026-06-25 13:48 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox