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>
