[BUG] Take a long time to reach consistent after pg_rewind

Possible fix:

The pg_rewind use pg_current_wal_insert_lsn() to set the min recovery point, which calls
GetXLogInsertRecPtr() and returns the latest wal insert pointer. Maybe we should use
GetXLogInsertEndRecPtr() which returns the latest wal record end pointer.

Thoughts?

Another solution:

If minRecoveryPoint is just after a xlog page header, we can move it to the begin of
the page. It's safe because we just skip the xlog page header. Do I miss something?

Attach a patch done like this.

--
Regards,
ChangAo Chen

Subject: Re: [BUG] Take a long time to reach consistent after pg_rewind

Hi ChangAo,

Thanks for the careful diagnosis, I reproduced the hang on macOS on the
latest postgres code (It took a lot of iterations to reproduce it)
The LSN trace matches your description and I saw the below:
minRecoveryPoint = 0/08000028
consistent recovery state reached at = 0/08000060

In my run the standby was stuck for ~9 s; consistency was eventually
declared at 0/08000060 because a small upstream record (most likely
a RUNNING_XACTS snapshot from bgwriter) landed at 0/08000028 and let
lastReplayedEndRecPtr leap past the bad finish line.
With the new primary stopped after pg_rewind, the wait was unbounded as
expected.

Regarding the fix: the underlying issue is that minRecoveryPoint is
implicitly expected to be the end-LSN of a real WAL record, because
lastReplayedEndRecPtr (the value it gets compared against)
can only ever take such values. All current writers respect this
expectation except pg_rewind: pg_basebackup uses the backup-end record's
EndRecPtr, and the in-running UpdateMinRecoveryPoint path
uses buffer LSNs, both of which are record-end LSNs by construction.
pg_rewind alone uses pg_current_wal_insert_lsn(), which can return a
position just past a page header when the source is idle.
That's why I'd lean toward fixing the producer (pg_rewind).

Concretely, your original suggestion having pg_rewind use
GetXLogInsertEndRecPtr() instead of GetXLogInsertRecPtr(), restores
the invariant globally, and doesn't require future call sites that compare
against minRecoveryPoint to know about page-header adjustments.

If we still want a defense-in-depth guard in CheckRecoveryConsistency() to
handle older pg_rewind binaries running against a newer server,
the v1 patch is on the right track, but I'd suggest:
- documenting in the helper comment why exactly SizeOfXLogShortPHD /
SizeOfXLogLongPHD past a page boundary are the only legal
"non-record-end" minRecoveryPoint values (i.e. who can produce
them and under what conditions);

- auditing the other call sites that compare against
minRecoveryPoint to confirm none of them needs the same
adjustment, with a comment recording the conclusion.

I can put together a TAP test under src/bin/pg_rewind/t/ that forces a WAL
switch on the source, runs pg_rewind against an
otherwise-idle primary, and asserts that the rewound node reaches
consistency without further upstream activity.
Happy to send a v2 with that test if useful.

This is a liveness bug with potentially unbounded wait on idle promoted
primaries, so it does seem worth back-patching.

Regards,
Surya Poondla

Hi Surya,

Thanks for the review and sorry for the late reply.

I get a new idea to fix this bug from:

/messages/by-id/05DEB465-3C99-417E-B7FA-275A28068D90@yandex-team.ru

Please see the v2 patch for details.

--
Regards,
ChangAo Chen

Hello.

At Thu, 11 Jun 2026 11:03:17 +0800, "cca5507" <cca5507@qq.com> wrote in

I get a new idea to fix this bug from:

/messages/by-id/05DEB465-3C99-417E-B7FA-275A28068D90@yandex-team.ru

Please see the v2 patch for details.

Thanks for the patch.

However, I am not sure that using the flush LSN is the right
direction.

As I understand it, the root of the problem is that
get_current_wal_insert_lsn() returns the next WAL insertion position,
while minRecoveryPoint is expected to point just past the end of the
last required record. At positions immediately following a WAL page or
segment header, those locations can be logically equivalent but
numerically different.

I think pg_rewind is probably using the insert LSN because it wants to
choose a conservative position as far ahead as possible. It might be
possible to use the flush LSN if the copying logic is carefully
arranged, but I would prefer to keep using the insert LSN if we can.

I also think it is unfortunate to require client-side tools to be
aware of the exact "end of last record + 1" representation. If both
representations are logically equivalent at those positions, I would
rather have the recovery side accept and normalize them than require
every tool to produce exactly the expected form.

So I wonder whether we should instead normalize minRecoveryPoint when
reading it from the control file, so that these equivalent
representations are treated as the same position. That would also make
the recovery side a bit more robust.

For example, something like this:

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 73b78a83fa7..660c681795d 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -955,6 +955,20 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
 	{
 		minRecoveryPoint = ControlFile->minRecoveryPoint;
 		minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
+		/*
+		 * minRecoveryPoint in the control file is expected to point to the
+		 * location immediately following the end of the target
+		 * record. However, some tools may record a location immediately after
+		 * a WAL segment or page header when the target position is at the
+		 * beginning of a segment or page. Normalize such values here before
+		 * further processing.
+		 */
+		if (XLogSegmentOffset(minRecoveryPoint, wal_segment_size)
+			== SizeOfXLogLongPHD)
+			minRecoveryPoint -= SizeOfXLogLongPHD;
+		else if (minRecoveryPoint % XLOG_BLCKSZ == SizeOfXLogShortPHD)
+			minRecoveryPoint -= SizeOfXLogShortPHD;
 	}
 	else
 	{

Thoughts?

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

As I understand it, the root of the problem is that
get_current_wal_insert_lsn() returns the next WAL insertion position,
while minRecoveryPoint is expected to point just past the end of the
last required record. At positions immediately following a WAL page or
segment header, those locations can be logically equivalent but
numerically different.

Yes.

I think pg_rewind is probably using the insert LSN because it wants to
choose a conservative position as far ahead as possible.  It might be
possible to use the flush LSN if the copying logic is carefully
arranged, but I would prefer to keep using the insert LSN if we can.

The insert LSN is not crash safe, is this really make sense to use it? For
example, the primary has insert LSN 1000, flush LSN 500, the standby
sets minRecoveryPoint to 1000, and then the primary crash and restart.
The primary now only has LSN 500, but the standby cannot reach
consistent until LSN 1000. This doesn’t make sense to me.

I also think it is unfortunate to require client-side tools to be
aware of the exact "end of last record + 1" representation. If both
representations are logically equivalent at those positions, I would
rather have the recovery side accept and normalize them than require
every tool to produce exactly the expected form.

Sounds reasonable.

So I wonder whether we should instead normalize minRecoveryPoint when
reading it from the control file, so that these equivalent
representations are treated as the same position. That would also make
the recovery side a bit more robust.

For example, something like this:

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 73b78a83fa7..660c681795d 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -955,6 +955,20 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
  {
  minRecoveryPoint = ControlFile->minRecoveryPoint;
  minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+
+ /*
+  * minRecoveryPoint in the control file is expected to point to the
+  * location immediately following the end of the target
+  * record. However, some tools may record a location immediately after
+  * a WAL segment or page header when the target position is at the
+  * beginning of a segment or page. Normalize such values here before
+  * further processing.
+  */
+ if (XLogSegmentOffset(minRecoveryPoint, wal_segment_size)
+ == SizeOfXLogLongPHD)
+ minRecoveryPoint -= SizeOfXLogLongPHD;
+ else if (minRecoveryPoint % XLOG_BLCKSZ == SizeOfXLogShortPHD)
+ minRecoveryPoint -= SizeOfXLogShortPHD;
  }
  else
  {

I had thought about this before. To update minRecoveryPoint in place, I think we
should make sure that it won't cause any side effects. That means we need to
check every places we use minRecoveryPoint. That's why the v1 patch introduces
GetEffectiveMinRecoveryPoint() rather than updates it in place.

--
Regards,
ChangAo Chen

Hello.

At Thu, 11 Jun 2026 15:50:19 +0800, "cca5507" <cca5507@qq.com> wrote in

I think pg_rewind is probably using the insert LSN because it wants to
choose a conservative position as far ahead as possible.  It might be
possible to use the flush LSN if the copying logic is carefully
arranged, but I would prefer to keep using the insert LSN if we can.

The insert LSN is not crash safe, is this really make sense to use it? For
example, the primary has insert LSN 1000, flush LSN 500, the standby
sets minRecoveryPoint to 1000, and then the primary crash and restart.
The primary now only has LSN 500, but the standby cannot reach
consistent until LSN 1000. This doesn’t make sense to me.

My understanding had been that the state produced by pg_rewind only
needed to be valid with respect to the source server at the time
pg_rewind was run.

If the expectation is that the rewound standby must also remain usable
after the source server subsequently goes through crash recovery, then
I agree that using the insert LSN becomes harder to justify.

I had thought about this before. To update minRecoveryPoint in place, I think we
should make sure that it won't cause any side effects. That means we need to
check every places we use minRecoveryPoint. That's why the v1 patch introduces
GetEffectiveMinRecoveryPoint() rather than updates it in place.

The reason I suggested that approach in my earlier email was simply
that, as far as I could tell, that was the only place that needed to
interpret a minRecoveryPoint value. That said, since minRecoveryPoint
is expected to point to the end of the last required record, I think
normalizing the value earlier would also be reasonable.

Anyway, if we decide to use the flush LSN instead, then none of this
should be necessary.

Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

Thanks ChangAo for v2, and Kyotaro for working through the alternatives. I
think v2 is on the right track.

For me the Flush LSN looks correct for the below reasons:
1. The standby can reach it. XLOG_SWITCH (driven by archive_timeout) calls
XLogFlush(EndPos) with EndPos = segment boundary. xlogreader treats SWITCH
specially and advances NextRecPtr to end-of-segment
(XLogDecodeNextRecord()), so lastReplayedEndRecPtr lands on the segment
boundary and the consistency check fires. With v2, minRecoveryPoint =
segment boundary, which is exactly reachable.

This restores the implicit producer-side invariant: pg_basebackup's
XLOG_BACKUP_END EndRecPtr, UpdateMinRecoveryPoint() via
GetCurrentReplayRecPtr(), and the standby-source path in pg_rewind all
yield a value record-driven progress can match.

Caveat: XLogBackgroundFlush rounds WriteRqst.Write down to a page boundary,
and the asyncXactLSN bump only re-targets when we've already flushed past
it. On a busy primary the flush LSN can therefore sit mid-record, but
that's fine because live insertions advance the standby past whatever
endrec we pin. Not the scenario in this bug, just worth noting that no
producer-side or recovery-side fix eliminates every theoretical liveness
corner.

2. GetXLogInsertRecPtr() reads CurrBytePos from shared memory not durable.
If the source crashes after pg_rewind, it recovers to its flush LSN,
leaving the target's minRecoveryPoint pinned to a higher
insert LSN which is unreachable forever. flush LSN sits at the segment
boundary, which is on disk and recovery cannot regress past it. So this
approach is crash-safe

3. perform_rewind() does: copy files -> fetch source control file ->
get_current_wal_flush_lsn.
Any WAL-logged page on source's disk has page-LSN <= source's flush-LSN at
the time it was written (FlushBuffer enforces XLogFlush before eviction;
hint-bit pages with checksums use XLogSaveBufferForHint + PageSetLSN).
Flush LSN is monotonic, so the value dominates every copied page-LSN.
FSM/VM pages are exempt by design (freespace.c uses MarkBufferDirtyHint
without WAL) but aren't gated by minRecoveryPoint. Unlogged tables are out
of scope. Torn pages are closed by the existing full_page_writes=on
precondition.

Review comments
1. Commit message understates the fix. It only describes the page-header
symptom. The crash-safety property is the stronger argument and the one
that resolves the back-and-forth on which LSN to use. Suggest adding:
a. "Using the flush LSN is also crash-safe with respect to the source: the
insert LSN lives only in shared memory and can be lost on a source crash,
leaving the standby's minRecoveryPoint ahead of any LSN the source can
subsequently reach."
2. Code comment should explain why flush LSN is sufficient. The current "We
must replay to the last WAL flush location" doesn't say why. Suggest:
a. "Use the source's flush LSN as the target's minRecoveryPoint: every
WAL-logged page we copied has page-LSN <= source's flush LSN at copy time
(WAL-before-data), and flush LSN is monotonic. We avoid the insert LSN
because it can sit one page-header past a record's end at segment
boundaries (where no record will end), and it is not durable, a source
crash can leave flush LSN behind an insert LSN we already pinned."
3. Worth a comment in rewind_source.h that the callback must only be
invoked against a non-standby source, pg_current_wal_flush_lsn() errors out
under recovery.
4. No regression test. We can add a regression test under
src/bin/pg_rewind/t/.

Otherwise +1 on the v2 direction.

Regards,
Surya Poondla

Hi,

Thanks for the comments!

1. Commit message understates the fix. It only describes the page-header symptom. The crash-safety property is the stronger argument and the one that resolves the > back-and-forth on which LSN to use. Suggest adding:
a. "Using the flush LSN is also crash-safe with respect to the source: the insert LSN lives only in shared memory and can be lost on a source crash, leaving the standby's minRecoveryPoint ahead of any LSN the source can subsequently reach."
2. Code comment should explain why flush LSN is sufficient. The current "We must replay to the last WAL flush location" doesn't say why. Suggest:
a. "Use the source's flush LSN as the target's minRecoveryPoint: every WAL-logged page we copied has page-LSN <= source's flush LSN at copy time (WAL-before-data), and flush LSN is monotonic. We avoid the insert LSN because it can sit one page-header past a record's end at segment boundaries (where no record will end), and it is not durable, a source crash can leave flush LSN behind an insert LSN we already pinned."
3. Worth a comment in rewind_source.h that the callback must only be invoked against a non-standby source, pg_current_wal_flush_lsn() errors out under recovery.

Fixed.

4. No regression test. We can add a regression test under src/bin/pg_rewind/t/.

Currently I don't have a good idea about the test, I will work on it later. Any help is welcome!

--
Regards,
ChangAo Chen

Hi ChangAo,

Thanks for the v3, the commit message, in-line comment, and the
rewind_source.h note all look good

On the test front: I don't think a hang-detection test can be made
reliable. The bug requires the source's insert LSN to be exactly
segment_boundary + SizeOfXLogLongPHD with no further WAL activity, but
bgwriter's periodic LogStandbySnapshot emits a RUNNING_XACTS which can
advance the insert LSN
nondeterministically between pg_switch_wal() and the rewind. In my
reproduction bgwriter ended the hang after ~9s; that's the kind of timing
we don't want in CI.

The deterministic alternative is to parse pg_controldata on the target
after pg_rewind and assert minRecoveryPoint does not land
at "boundary + SizeOfXLogLongPHD". That's a direct check on the patched
behavior independent of source idleness or replay
timing. It doesn't exercise the integration property that the rewound node
reaches consistency without further upstream WAL.
So I am not sure if this testcase is a complete one in our scenario.

Regards,
Surya Poondla

Hi all,

On Wed, Jul 15, 2026 at 5:39 PM cca5507 <cca5507@qq.com> wrote:

Hi,

Thanks for the comments!

1. Commit message understates the fix. It only describes the page-header symptom. The crash-safety property is the stronger argument and the one that resolves the > back-and-forth on which LSN to use. Suggest adding:
a. "Using the flush LSN is also crash-safe with respect to the source: the insert LSN lives only in shared memory and can be lost on a source crash, leaving the standby's minRecoveryPoint ahead of any LSN the source can subsequently reach."
2. Code comment should explain why flush LSN is sufficient. The current "We must replay to the last WAL flush location" doesn't say why. Suggest:
a. "Use the source's flush LSN as the target's minRecoveryPoint: every WAL-logged page we copied has page-LSN <= source's flush LSN at copy time (WAL-before-data), and flush LSN is monotonic. We avoid the insert LSN because it can sit one page-header past a record's end at segment boundaries (where no record will end), and it is not durable, a source crash can leave flush LSN behind an insert LSN we already pinned."
3. Worth a comment in rewind_source.h that the callback must only be invoked against a non-standby source, pg_current_wal_flush_lsn() errors out under recovery.

Fixed.

4. No regression test. We can add a regression test under src/bin/pg_rewind/t/.

Currently I don't have a good idea about the test, I will work on it later. Any help is welcome!

I reviewed and tested this patch on my PostgreSQL 20devel tree. The
patch applied cleanly and built successfully without any issues. I
first reproduced the reported issue on an unpatched tree using a
two-node streaming replication setup. After promoting the standby,
stopping the old primary, ensuring the WAL insert LSN reached the
required boundary, and running pg_rewind, the rewound server entered
standby mode and started streaming from the promoted primary. However,
it remained in recovery but the client connections failed with:

psql: error: connection to server on socket "/tmp/.s.PGSQL.55450" failed:
FATAL: the database system is not yet accepting connections
DETAIL: Consistent recovery state has not been yet reached.

Before generating new WAL:

LOG: redo starts at 0/07000028
LOG: started streaming WAL from primary at 0/0C000000 on timeline 2
FATAL: the database system is not yet accepting connections
DETAIL: Consistent recovery state has not been yet reached.

After generating WAL on the promoted primary, the server reached a
consistent recovery state only after generating additional WAL on the
promoted primary, which matches the behavior described in the
discussion thread:

LOG: completed backup recovery with redo LSN 0/07000028 and end LSN 0/0C000028
LOG: consistent recovery state reached at 0/0C000048
LOG: database system is ready to accept read-only connections

I then applied the V3 patch and repeated the same test. After
pg_rewind, the rewound standby completed backup recovery, reached a
consistent recovery state, and became ready to accept read-only
connections without requiring any additional WAL generation on the
promoted primary:

LOG: completed backup recovery with redo LSN 0/07000060 and end LSN 0/0E000060
LOG: consistent recovery state reached at 0/0E000060
LOG: invalid record length at 0/0E000060: expected at least 24, got 0
LOG: database system is ready to accept read-only connections
LOG: started streaming WAL from primary at 0/0E000000 on timeline 2

I also verified that the rewound server remained in recovery and that
the replicated data was accessible. Overall based on my testing, the
patch resolves the reported issue, and did not observe any regressions
during the tested scenario. The patch looks good to me.

Regards,
Solai