Re: BUG #18575: Sometimes pg_rewind mistakenly assumes that nothing needs to be done.

On 08/08/2024 16:07, Heikki Linnakangas wrote:
> So it seems true that rewind is not required in those cases. However, if
> the WAL is already written on the standby's disk, just not replayed yet,
> then when you restart the server, it will replay the WAL from timeline
> 1. That does seem surprising. Perhaps pg_rewind should just update the
> minRecoveryPoint and minRecoveryTLI in control file in that case, to
> force WAL recovery to follow the timeline switch to TLI 2.
>
> I will try to write a TAP test for the "Bad" and the assertion failure
> case, fix the assertion failure, and test how updating the
> minRecoveryPoint would behave.

I've divided this into three separate issues:

1. I think the assertion in pg_rewind is simply bogus, and we should
remove it. Attached 0002-Remove-bogus-assertion-in-pg_rewind.patch does
that, and adds a test case to cover it.

2. Independently of pg_rewind: When you start PostgreSQL, it will first
try to recover all the WAL it has locally in pg_wal. That goes wrong if
you have set a recovery target TLI. For example, imagine this situation:

- Recovery target TLI is 2, set explicitly in postgresql.conf
- The switchpoint from TLI 1 to 2 happened at WAL position 0/1510198
(the switchpoint is found in 00000002.history)
- There is a WAL file 000000010000000000000001 under pg_wal, which
contains valid WAL up to 0/1590000

When you start the server, it will first recover all the WAL from
000000010000000000000001, up to 0/1590000. Then it will connect to the
primary to fetch mor WAL, but it will fail to make any progress because
it already blew past the switch point.

It's obviously wrong to replay the WAL from timeline 1 beyond the 1->2
switchpoint, when the recovery target is TLI 2. The attached
0003-Don-t-read-past-current-TLI-during-archive-recovery.patch fixes
that. However, the logic to find the right WAL segment file and read the
WAL is extremely complicated, and I don't feel comfortable that I got
all the cases right. Review would be highly appreciated.

The patch includes a test case to demonstrate the case, with no
pg_rewind. It does include one "manual" step to copy a timeline history
file into pg_wal, marked with XXX, however. So I'm not sure how possible
this scenario is in production setups .

3. When pg_rewind has nothing to do, the target server is left
unmodified, in a state such that when you restart it, it will replay all
the WAL it has locally in pg_wal first, before connecting to the
primary. Even though the target is a direct ancestor of the source and
hence it *can* follow the WAL to the source's position without
rewinding, it doesn't mean that it *will* actually do so.

The attached changes it so that it updates the control file in that
case, setting minRecoveryPoint and minRecoveryPointTLI to point to the
source's current WAL position. That way, when you start it up, it will
follow the timeline history to reach that point. (This requires fixing
issue 2, because otherwise it still won't follow the history correctly
to reach the minRecoveryPointTLI)

To make the test pass, it actually would be sufficient to copy the
timeline history file into pg_wal (which the patch also does). But
updating the minRecoveryPoint seems good to ensure that it follows the
right timeline. Otherwise it relies on the logic at startup to find the
latest timeline, and that the latest timeline is the one you tried to
rewind to. I think it would go wrong if there was another
higher-numbered history file present in pg_wal for some reason.

All in all, I don't feel very confident about all this. The assertion
seems straightforward, so barring objections I'll commit and backpatch
that. The timeline-following at startup (issue 2) seems pretty clearly
wrong, but I'm not sure it's worth the risk to backpatch. Similarly
issue 3 might not be worth the risk to backpatch, especially if we don't
also backpatch 2. I would love to hear comments on those.

Georgy, if you have the possibility to test this patches with your repro
script, that would be highly appreciated.

--
Heikki Linnakangas
Neon (https://neon.tech)