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

On 27.09.2019 6:27, Paul Guo wrote:
>
>
>     Secondarily, I see no reason to test connstr_source rather than just
>     "conn" in the other patch; doing it the other way is more natural,
>     since
>     it's that thing that's tested as an argument.
>
>     pg_rewind.c: Please put the new #include line keeping the alphabetical
>     order.
>
>
> Agreed to the above suggestions. I attached the v9.
>

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?

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


Regards

-- 
Alexey Kondratov

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