Re: Turning recovery.conf into GUCs

On Tue, Nov 19, 2013 at 3:32 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
>> On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
>> > * I am not sure I like "recovery.trigger" as a name. It seems to close
>> > to what I've seen people use to trigger failover and too close to
>> > trigger_file.
>
>> This name was chosen and kept in accordance to the spec of this
>> feature. Looks fine for me...
>
> I still think "start_as_standby.trigger" or such would be much clearer
> and far less likely to be confused with the promotion trigger file.
>

the function of the file is to inform the server it's in recovery and
it needs to consider recovery parameters, not to make the server a
standby. yes, i admit that is half the way to make the server a
standby. for example, if you are doing PITR and stopping the server
before some specific point (recovery_target_*) then
"start_as_standby.trigger" will has no meaning and could confuse
people

>> > * You made recovery_target_inclusive/pause_at_recovery_target PGC_SIGHUP
>> > - did you review that actually works? Imo that should be changed in a
>> > separate commit.
>>
>> Yep, I'd rather see those parameters as PGC_POSTMASTER... As of now,
>> once recovery is started those parameter values do not change once
>> readRecoveryCommandFile is kicked. Having them SIGHUP would mean that
>> you could change them during recovery. Sounds kind of dangerous, no?
>
> I think it's desirable to make them changeable during recovery, but it's
> a separate patch.
>

uh! i read the patch and miss that. will change

>
>> > * Why did you change some of the recovery gucs to lowercase names, but
>> > left out XLogRestoreCommand?
>>
>> This was part of the former patch, perhaps you are right and keeping
>> the names as close as possible to the old ones would make sense to
>> facilitate maintenance across versions.
>
> I think lowercase is slightly more consistent with the majority of the
> other GUCs, but if you change it you should change all the new GUC variables.
>

This one was my change, in the original patch is called
"restore_command" and i changed it because archive_command's parameter
is called XLogArchiveCommand so i wanted the name to follow the same
format.

i can return it to the original name if no one likes XLogRestoreCommand

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