Re: Turning recovery.conf into GUCs

Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>
>> Well, the latest version of this patch fails to start when it sees
>> 'recovery.conf' in PGDATA:
>>
>> FATAL: "recovery.conf" is not supported anymore as a recovery method
>> DETAIL: Refer to appropriate documentation about migration methods
>>
>> I've missed all the discussion behind this decision and after reading
>> the ALTER SYSTEM/conf.d thread I'm even more confused so I'd like
>> someone more knowledgeable to speak up on the status of this.
>
> The argument was that people wanted to be able to have an empty
> recovery.conf file as a "we're in standby" trigger so that they could
> preserve backwards compatibility with external tools. I don't agree
> with this argument, but several people championed it.

It doesn't look like we can provide for 100% backwards compatibility
with existing tools, here's why:

a) The only sensible way to use recovery.conf as GUC source is via
include/include_dir from postgresql.conf.

b) The server used to rename $PGDATA/recovery.conf to
$PGDATA/recovery.done so when it is re-started it doesn't
accidentally start in recovery mode.

We can't keep doing (b) because that will make postgres fail to start on
a missing include. Well, it won't error out if we place the file under
include_dir, as only files with the ".conf" suffix are included, but
then again the server wouldn't know which file to rename unless we tell
it or hard-code the the filename. Either way this is not 100% backwards
compatible.

Now the question is if we are going to break compatibility anyway:
should we try to minimize the difference or will it disserve the user by
providing a false sense of compatibility?

For one, if we're breaking things, the trigger_file GUC should be
renamed to end_recovery_trigger_file or something like that, IMO.
That's if we decide to keep it at all.

>> The docs use the term "continuous recovery".
>>
>> Either way, from the code it is clear that we only stay in recovery if
>> standby_mode is directly turned on. This makes the whole check for a
>> specially named file unnecessary, IMO: we should just check the value of
>> standby_mode (which is off by default).
>
> So, what happens when someone does "pg_ctl promote"? Somehow
> standby_mode needs to get set to "off". Maybe we write "standby_mode =
> off" to postgresql.auto.conf?

Well, standby_mode is only consulted at startup after crash recovery.
But suddenly, a tool that *writes* recovery.conf needs to also
consult/edit postgresql.auto.conf... Maybe that's inevitable, but still
a point to consider.

>> By the way, is there any use in setting standby_mode=on and any of the
>> recovery_target* GUCs at the same time?
>
> See my thread on this topic from last week. Short answer: No.
>
>> I think it can only play together if you set the target farther than the
>> latest point you've got in the archive locally. So that's sort of
>> "Point-in-Future-Recovery". Does that make any sense at all?
>
> Again, see the thread. This doesn't work in a useful way, so there's no
> reason to write code to enable it.

Makes sense. Should we incorporate the actual tech and doc fix in this
patch?

>>>> /* File path names (all relative to $PGDATA) */
>>>> -#define RECOVERY_COMMAND_FILE "recovery.conf"
>>>> -#define RECOVERY_COMMAND_DONE "recovery.done"
>>>> +#define RECOVERY_ENABLE_FILE "standby.enabled"
>>>
>>> Imo the file and variable names should stay coherent.
>>
>> Yes, once we settle on the name (and if we really need that extra
>> trigger file.)
>
> Personally, if we have three methods of promotion:
>
> 1) pg_ctl promote
> 2) edit postgresql.conf and reload
> 3) ALTER SYSTEM SET and reload
>
> ... I don't honestly think we need a 4th method for promotion.

Me neither. It is tempting to make everything spin around GUCs without
the need for any external trigger files.

> The main reason to want a "we're in recovery file" is for PITR rather
> than for replication, where it has a number of advantages as a method,
> the main one being that recovery.conf is unlikely to be overwritten by
> the contents of the backup.
>
> HOWEVER, there's a clear out for this with conf.d. If we enable conf.d
> by default, then we can simply put recovery settings in conf.d, and
> since that specific file (which can have whatever name the user chooses)
> will not be part of backups, it would have the same advantage as
> recovery.conf, without the drawbacks.
>
> Discuss?

Well, I don't readily see how conf.d is special with regard to base
backup, wouldn't you need to exclude it explicitly still?

--
Alex