Horton

Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

Alexey Kondratov <a.kondratov@postgrespro.ru>

View thread View on postgr.es
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>
Cc: Michael Paquier <michael@paquier.xyz>, Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Jimmy Yih <jyih@pivotal.io>, Ashwin Agrawal <aagrawal@pivotal.io>
Date: 2019-10-02T17:28:09Z
Lists: pgsql-hackers

Attachments

Hi Alvaro,

On 30.09.2019 20:13, Alvaro Herrera wrote:
> OK, I pushed this patch as well as Alexey's test patch.  It all works
> for me, and the coverage report shows that we're doing the new thing ...
> though only in the case that rewind *is* required.  There is no test to
> verify the case where rewind is *not* required.  I guess it'd also be
> good to test the case when we throw the new error, if only for
> completeness ...

I've directly followed your guess and tried to elaborate pg_rewind test 
cases and... It seems I've caught a few bugs:

1) --dry-run actually wasn't completely 'dry'. It did update target 
controlfile, which could cause repetitive pg_rewind calls to fail after 
dry-run ones.

2) --no-ensure-shutdown flag was broken, it simply didn't turn off this 
new feature.

3) --write-recovery-conf didn't obey the --dry-run flag.

Thus, it was definitely a good idea to add new tests. Two patches are 
attached:

1) First one fixes all the issues above;

2) Second one slightly increases pg_rewind overall code coverage from 
74% to 78.6%.

Should I put this fix on the next commitfest?


Regards

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company


P.S. My apologies that I've missed two of these bugs during review.

Commits

  1. Improve test coverage of pg_rewind

  2. Improve handling and coverage of --no-ensure-shutdown in pg_rewind

  3. Fix issues in pg_rewind with --no-ensure-shutdown/--write-recovery-conf

  4. Fix --dry-run mode of pg_rewind

  5. pg_rewind: test new --write-recovery-conf functionality

  6. pg_rewind: Allow writing recovery configuration

  7. Have pg_rewind run crash recovery before rewinding

  8. Split out recovery confing-writing code from pg_basebackup

  9. Switch TAP tests of pg_rewind to use non-superuser role, take two