Re: Corner-case bug in pg_rewind

2020年11月10日(火) 18:07 Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>:
>
> I did some effort to review your patch which seems legit to me.

Thanks for the review and feedback.

> 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.

The comment you cite is not part of this patch. I suspect there might be some
scope for improving the comments in general, but that would need to be done
seperately.

> 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?

That's just the way the tests work - an individual test has no influence
over that output.

> Also, I think Heikki's notion could be fulfilled.

I spent a bit of time looking at that suggestion but couldn't actually
verify it was an issue which needed fixing.

Note that the patch may require reworking for HEAD due to changes in
commit 9c4f5192f6. I'll try to take another look this week.

Regards

Ian Barwick

--
EnterpriseDB: https://www.enterprisedb.com