Horton

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

Paul Guo <pguo@pivotal.io>

View thread View on postgr.es
From: Paul Guo <pguo@pivotal.io>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, 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-09-30T07:07:48Z
Lists: pgsql-hackers

Attachments

>
>
> I went through the remaining two patches and they seem to be very clear
> and concise. However, there are two points I could complain about:
>
> 1) Maybe I've missed it somewhere in the thread above, but currently
> pg_rewind allows to run itself with -R and --source-pgdata. In that case
> -R option is just swallowed and neither standby.signal, nor
> postgresql.auto.conf is written, which is reasonable though. Should it
> be stated somehow in the docs that -R option always has to go altogether
> with --source-server? Or should pg_rewind notify user that options are
> incompatible and no recovery configuration will be written?
>

I modified code & doc to address this. In code, pg_rewind will error out
for the local case.


> 2) Are you going to leave -R option completely without tap-tests?
> Attached is a small patch, which tests -R option along with the existing
> 'remote' case. If needed it may be split into two separate cases. First,
> it tests that pg_rewind is able to succeed with minimal permissions
> according to the Michael's patch d9f543e [1]. Next, it checks presence
> of standby.signal and adds REPLICATION permission to rewind_user to test
> that new standby is able to start with generated recovery configuration.
>
> [1]
>
> https://github.com/postgres/postgres/commit/d9f543e9e9be15f92abdeaf870e57ef289020191
>
>
It seems that we could further disabling recovery info setting code for the
'remote' test case?

-   my $port_standby = $node_standby->port;
-   $node_master->append_conf(
-       'postgresql.conf', qq(
-primary_conninfo='port=$port_standby'
-));
+   if ($test_mode ne "remote")
+   {
+       my $port_standby = $node_standby->port;
+       $node_master->append_conf(
+           'postgresql.conf',
+           qq(primary_conninfo='port=$port_standby'));

-   $node_master->set_standby_mode();
+       $node_master->set_standby_mode();
+   }

Thanks.

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