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

On 27.09.2019 17:28, Alvaro Herrera wrote:
>
>> +        # Now, when pg_rewind apparently succeeded with minimal permissions,
>> +        # add REPLICATION privilege.  So we could test that new standby
>> +        # is able to connect to the new master with generated config.
>> +        $node_standby->psql(
>> +            'postgres', "ALTER ROLE rewind_user WITH REPLICATION;");
> I think this better use safe_psql.
>

Yes, indeed.

On 30.09.2019 10:07, Paul Guo wrote:
>
>     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();
> +   }
>
>

Yeah, it makes sense. It is excessive for remote if we add '-R' there. 
I've updated and attached my test adding patch.



-- 
Alexey Kondratov

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