Re: Proposal for changes to recovery.conf API

On 1 September 2016 at 06:34, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Thu, Sep 1, 2016 at 1:15 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> This is a summary of proposed changes to the recovery.conf API for
>> v10. These are based in part on earlier discussions, and represent a
>> minimal modification to current usage.
>>
>> Proposed changes (with reference to patches from Abhijit Menon-Sen and myself)
>>
>> * pg_ctl start -M (normal|recover|continue)
...
> That looks like a sound design.

Thanks

>> * Recovery parameters would now be part of the main postgresql.conf
>> infrastructure
>> Any parameters set in $DATADIR/recovery.conf will be read after the
>> main parameter file, similar to the way that postgresql.conf.auto is
>> read.
>> (Abhijit)
>> * pg_basebackup -R will continue to generate a parameter file called
>> recovery.conf as it does now, but will also create a file named
>> recovery.trigger.
>> (This part is WIP; patch doesn't include implementation for tar format yet)
>
> Or we could just throw away this dependency with recovery.conf,
> simply. I see no need to keep it if recovery is triggered depending on
> recovery.trigger, nor recommend its use. We could instead add
> include_if_exists 'recovery.conf' at the bottom of postgresql.conf and
> let the infrastructure do the rest to simplify the patch.

It works exactly the same as ALTER SYSTEM and adds only one small hunk of code.

>> * Parameters
>> All of the parameters formerly set in recovery.conf can be set in
>> postgresql.conf using RELOAD
>> These parameters will have no defaults in postgresql.conf.sample
>> Setting them has no effect during normal running, or once recovery ends.
>>  https://www.postgresql.org/docs/devel/static/archive-recovery-settings.html
>>  https://www.postgresql.org/docs/devel/static/recovery-target-settings.html
>>  https://www.postgresql.org/docs/devel/static/standby-settings.html
>> (Abhijit)
>
> Hm. I think that what would make sense here is a new GUC category,
> meaning that once recovery is launched the new parameters are not
> taken into account once again. Even updating primary_conninfo would
> need a restart of the WAL receiver, so we could just make them
> GUC_POSTMASTER and be done with it.

We definitely want most of them set at RELOAD, especially recovery targets.

Almost all of them will have no effect when normal mode starts, so I
don't see any other special handling needed.

>> Related cleanup
>> * Promotion signal file is now called "promote.trigger" rather than
>> just "promote"
>
> If that's not strictly necessary this renaming is not mandatory.

I think it makes sense to keep both files with same suffix, for clarity.

>> * Remove user configurable "trigger_file" mechanism - use
>> "promote.trigger" for all cases
>
> Ugh. I am -1 on that. There are likely backup tools and users that
> rely on this option, particularly to be able to trigger promotion
> using a file that is on a different partition than PGDATA.

OK, I wasn't thinking of that. Perhaps we should have a trigger
directory parameter?

>> * Remove Fallback promote mechanism, so all promotion is now "fast" in xlog.c
>
> No problem with that. Now others have surely other opinions. That
> could be addressed as a separate patch.

I'll post separate patch.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services