Horton

Re: Corner-case bug in pg_rewind

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Ian Barwick <ian.barwick@2ndquadrant.com>, Pgsql Hackers <pgsql-hackers@postgresql.org>
Date: 2020-11-10T09:06:46Z
Lists: pgsql-hackers 
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>

Commits

  1. Fix more race conditions in the newly-added pg_rewind test.

  2. Fix race conditions in newly-added test.

  3. Fix pg_rewind bugs when rewinding a standby server.

  4. Allow pg_rewind to use a standby server as the source system.

  5. Support clean switchover.