Re: [PATCH] Fix timeline history after recovery stops on an ancestor

Hi Stepan,

Thank you for the patch. A history file whose switchpoints go backwards is
easy to miss, and it is good to have a test that checks the content of the
file directly.

I reviewed 0001-v2-Fix-history-after-recovery-stops-on-an-ancestor.patch
(2026-09-07).

Tested against master c074737f687f604b64f6ee830fccbbe854719823 (2026-11-09),
Debian bookworm container, ./configure --enable-cassert --enable-debug
--enable-tap-tests.

Applying and building
- git apply works cleanly on current master.
- Build is clean, with no compiler warnings.

Tests
- 028_pitr_timelines.pl with only the test changes applied (no fix): fails,
1 of 4. The new check shows the bad history file:
1 0/0301FA60 no recovery target specified
2 0/0301F9F8 at restore point "rp"
So timeline 3 lists timeline 2 as its parent, with a switchpoint lower
than the point where timeline 2 itself started. The test catches the bug.
- 028 with the fix: 4 tests, PASS.
- 002_archiving.pl with v2 (fix and test changes): PASS, 6 runs out of 6.
- 002_archiving.pl with the fix but without the 002 changes: fails 10 runs
out of 10, at "00000002.history retrieved from the archives" (line 141).
More on this in comment 3.
- make check: all 243 tests passed.
- make -C src/test/recovery check: 59 files, 691 tests, PASS.

Back branches
- git apply --check of the code and test parts works on REL_15_STABLE,
REL_16_STABLE, REL_17_STABLE and REL_18_STABLE (the code there has the
same writeTimeLineHistory(newTLI, recoveryTargetTLI, ...) call).
- On REL_15_STABLE I also ran 028 and 002: 028 fails without the fix (same
check), and both pass with it.

Comments

1. The choice of lastRecTLI looks right to me. StartupXLOG() already sets
XLogCtl->PrevTimeLineID = endOfRecoveryInfo->lastRecTLI a few lines below,
and that value goes into the end-of-recovery record and the next checkpoint
as PrevTimeLineID. So on master the history file and the end-of-recovery
record can disagree about the parent timeline, and the patch makes them
consistent. endOfLogTLI would be the wrong choice, since it is the TLI in
the segment file name and can be newer than the timeline of the last record,
as the comment in xlogrecovery.h explains. It may be worth one sentence in
the commit message, or a short comment at the call site, saying that the
parent is the timeline of the last replayed record, which is not always the
target timeline.

2. Commit message. The subject "changed end of recovery timeline" does not
say much, and the body is empty. Something like "Fix parent timeline in
history file when recovery stops on an ancestor timeline", with two or three
lines describing the scenario (recovery_target_timeline = 'latest', target
found on timeline 1 before timeline 2 forked) would help the committer and
the release notes. The From line also shows "debebantur" rather than your
name, which you may want to fix with git config user.name.

3. 002_archiving.pl. To see why the 002 changes are needed, I ran the fix
with the original 002 (none of the 002 changes). It failed every time (10 of
10). As far as I can tell, the reason is this. In the original test,
standby2 never replays any WAL on timeline 2, so its last replayed record is
on timeline 1. On master, promotion still writes the new history file with
parent 2, and writeTimeLineHistory() restores 00000002.history from the
archive to copy it, which produces the log line the test checks. With the
fix the parent is 1, so 00000002.history is not restored any more, and the
check fails. So the new WAL on timeline 2 is what keeps the test doing what
it was written for (exercising RECOVERYHISTORY), and the wait that Andrey
suggested makes sure standby2 has replayed it before promotion. With v2, 002
passed in all my runs. A few small points:
- It would help to say this in the comment, for example: "standby2 must
replay some WAL on timeline 2, so that timeline 2 is the parent of the
new timeline and its history file is restored from the archive at
promotion."
- The new poll_query_until line for standby2 is indented with spaces,
while the rest of the file uses tabs. pgperltidy would fix it.

4. Practical impact. On master, the rest of 028 still passes with the bad
history (archive recovery on timeline 3 works), so I could not find a
user-visible failure beyond the wrong file content. Do you know of a case
where this breaks something, for example pg_rewind or a standby choosing
which timeline to follow? If so, it would be good to mention it in the
commit message, since it helps decide whether to back-patch.

Verdict: the fix is small and correct as far as I can see, and the tests
show the bug and the fix. I would mark it Ready for Committer once the
commit message is improved (comment 2). Comments 1, 3 and 4 are optional.

Thanks again.

Regards,
Joao Rito

Em ter., 10 de nov. de 2026 às 02:09, Филиппов Степан
<stepan(dot)filippov(at)yandex(dot)ru> escreveu:
>
> 27.08.2026, 16:17, "Andrey Borodin" <x4mmm(at)yandex-team(dot)ru>:
> > It seems to me that 002_archiving.pl has a race. Generating and
> > archiving WAL on timeline 2 does not guarantee that standby2 replays it
> > before promotion. It can still be promoted while lastRecTLI is 1, so
> > the test would not prove that the restored timeline 2 history was used.
> > I think it needs to wait for the recorded timeline_2_lsn before calling
> > promote(), for example:
> >
> > $caughtup_query =
> > "SELECT '$timeline_2_lsn'::pg_lsn <= pg_last_wal_replay_lsn()";
> > $node_standby2->poll_query_until('postgres', $caughtup_query)
> > or die "Timed out while waiting for standby2 to reach timeline 2";
>
> Hi, Andrey!
>
> Thank you for the review. You are right, there is a race.
>
> In v2 i added suggested query to wait timeline_2_lsn. This will help
> to avoid promotion before timeline2 is applied.
>
> Best regards, Stepan Filippov.