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