I did some effort to review your patch which seems legit to me.
I think some minor things are better to be improved i.e.
1. Comment regarding
------
347 * Check for the possibility that the target is in fact a direct
348 * ancestor of the source. In that case, there is no divergent history
349 * in the target that needs rewinding.
------
are better be reformulated as overall block contents are mostly cover vice versa case of a target NOT being a direct ancestor of the source. Maybe: "We need rewind in cases when .... and don't need only if the target is a direct ancestor of the source." I think it will be more understandable if it would be a commentary with descriptions of all cases in the block or no commentary before the block at all with commentaries of these cases on each if/else inside the block instead.
2. When I do the test with no patching of pg_rewind.c I get the output:
-----
# Failed test 'pg_rewind detects rewind needed stderr /(?^:rewinding from last common checkpoint at)/'
# at t/
007_min_recovery_point.pl line 107.
# 'pg_rewind: servers diverged at WAL location 0/3000180 on timeline 2
# pg_rewind: no rewind required
# '
# doesn't match '(?^:rewinding from last common checkpoint at)'
# Looks like you failed 1 test of 2.
t/
007_min_recovery_point.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests
Test Summary Report
-------------------
t/
007_min_recovery_point.pl (Wstat: 256 Tests: 2 Failed: 1)
Failed test: 2
Non-zero exit status: 1
-------
Maybe it can just give "failed" without so many details?
Also, I think Heikki's notion could be fulfilled.
Apart from this I consider the patch clean, clear, tests are passes and I'd recommend it to commit after a minor improvement described.
Thank you!
--
