Re: Corner-case bug in pg_rewind

On 11/09/2020 09:42, Ian Barwick wrote:
> Take the following cluster with:
> - node1 (initial primary)
> - node2 (standby)
> - node3 (standby)
>
> Following activity takes place (greatly simplified from a real-world situation):
>
> 1. node1 is shut down.
> 2. node3 is promoted
> 3. node2 is attached to node3.
> 4. node1 is attached to node3
> 5. node1 is then promoted (creating a split brain situation with
> node1 and node3 as primaries)
> 6. node2 and node3 are shut down (in that order).
> 7. pg_rewind is executed to reset node2 so it can reattach
> to node1 as a standby. pg_rewind claims:
>
> pg_rewind: servers diverged at WAL location X/XXXXXXX on timeline 2
> pg_rewind: no rewind required
>
> 8. based off that assurance, node2 is restarted with replication configuration
> pointing to node1 - but it is unable to attach, with node2's log reporting
> something like:
>
> new timeline 3 forked off current database system timeline 2
> before current recovery point X/XXXXXXX
>
> The cause is that pg_rewind is assuming that if the node's last
> checkpoint matches the
> divergence point, no rewind is needed:
>
> if (chkptendrec == divergerec)
> rewind_needed = false;
>
> but in this case there *are* records beyond the last checkpoint, which can be
> inferred from "minRecoveryPoint" - but this is not checked.

Yep, I think you're right.

> Attached patch addresses this. It includes a test, which doesn't make use of
> the RewindTest module, as that hard-codes a primary and a standby, while here
> three nodes are needed (I can't come up with a situation where this can be
> reproduced with only two nodes). The test sets "wal_keep_size" so would need
> modification for Pg12 and earlier.

I think we also need to change the extractPageMap() call:

> /*
> * Read the target WAL from last checkpoint before the point of fork, to
> * extract all the pages that were modified on the target cluster after
> * the fork. We can stop reading after reaching the final shutdown record.
> * XXX: If we supported rewinding a server that was not shut down cleanly,
> * we would need to replay until the end of WAL here.
> */
> if (showprogress)
> pg_log_info("reading WAL in target");
> extractPageMap(datadir_target, chkptrec, lastcommontliIndex,
> ControlFile_target.checkPoint, restore_command);
> filemap_finalize();

so that it scans all the way up to minRecoveryPoint, instead of stopping
at ControlFile_target.checkPoint. Otherwise, we will fail to rewind
changes that happened after the last checkpoint. And we need to do that
regardless of the "no rewind required" bug, any time we rewind a server
that's in DB_SHUTDOWNED_IN_RECOVERY state. I'm surprised none of the
existing tests have caught that. Am I missing something?

- Heikki