Re: Turning recovery.conf into GUCs

On Wed, Oct 16, 2013 at 1:34 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Jaime,
>> = Building =
>>
>> In pg_basebackup we have now 2 unused functions:
>> escapeConnectionParameter and escape_quotes. the only use of these
>> functions was when that tool created the recovery.conf file so they
>> aren't needed anymore.
>
> Except that we'll want 9.4's -R to do something, probably create a file
> called conf.d/replication.conf. Mind you, it won't need the same wonky
> quoting stuff.
>

Currently the patch uses -R to create the recovery trigger file

>> 1) the file to trigger recovery is now called standby.enabled. I know
>> this is because we use the file to also make the node a standby.
>> Now, reality is that the file doesn't make the node a standby but the
>> combination of this file (which starts recovery) + standby_mode=on.
>> so, i agree with the suggestion of naming the file: recovery.trigger
>>
>> 2) This patch removes the dual existence of recovery.conf: as a state
>> file and as a configuration file
>>
>> - the former is accomplished by changing the name of the file that
>> triggers recovery (from recovery.conf to standby.enabled)
>> - the latter is done by moving all parameters to postgresql.conf and
>> *not reading* recovery.conf anymore
>>
>> so, after applying this patch postgres don't use recovery.conf for
>> anything... except for complaining.
>> it's completely fair to say we are no longer using that file and
>> ignoring its existence, but why we should care if users want to use a
>> file with that name for inclusion in postgresql.conf or where they put
>> that hypotetic file?
>
> So this is a bit of a catch-22. If we allow the user to use a file
> named "recovery.conf" in $PGDATA, then the user may be unaware that the
> *meaning* of the file has changed, which can result in unplanned
> promotion of replicas after upgrade.
>

well, after upgrade you should do checks. and even if it happens,
after it happens once people will be aware of the change.
now, some suggestions were made to avoid the problem. 1) read the file
if exists last in the process of postgresql.conf, 2) add a GUC
indicating if it should read it and include it (not using it as a
trigger file). another one, 3) include in this release an
include_if_exists directive and give a warning if it sees the file
then include it, on next release remove the include_if_exists (at
least that way people will be warned before breaking compatibility)

please, keep in mind none of these suggestions include make the
recovery.conf file act as a trigger for recovery

> *on the other hand*, if we prevent creation of a configuration file
> named "recovery.conf", then we block efforts to write
> backwards-compatible management utilities.
>

and all tools and procedures that currently exists.

> AFAIK, there is no good solution for this, which is why it's taken so
> long for us to resolve the general issue. Given that, I think it's
> better to go for the breakage and all the warnings. If a user wants a
> file called recovery.conf, put it in the conf.d directory.
>

well, there should be good solutions... maybe we haven't thought them yet.
anyway, we can't postpone the decision forever. we need to make a
decision and stick with it otherwise this patch will be stalled N
releases for no good reason

> I don't remember, though: how does this patch handle PITR recovery?
>

exactly as it is now, if it sees the recovery trigger file, then it
starts ArchiveRecovery and after it finish delete the file (the only
difference) and increment the timeline

>> = Code & functionality =
>
>> + {"restore_command", PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
>> + {"archive_cleanup_command", PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
>> + {"recovery_end_command", PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
>> + {"recovery_target_xid", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
>> + {"recovery_target_name", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
>> + {"recovery_target_time", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
>> + {"trigger_file", PGC_POSTMASTER, REPLICATION_STANDBY,
>>
>> Not sure about these ones
>>
>> + {"recovery_target_timeline", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
>> + {"primary_conninfo", PGC_POSTMASTER, REPLICATION_STANDBY,
>
> It would be really nice to change these on the fly; it would help a lot
> of issues with minor changes to replication config. I can understand,
> though, that the replication code might not be prepared for that.
>

well, archive_command can be changed right now with a SIGHUP so at
least that one shouldn't change... and i don't think most of these are
too different. even if we are not sure we can do this now and change
them as SIGHUP later

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157