Re: Corner-case bug in pg_rewind

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!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com>