Re: Turning recovery.conf into GUCs

On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
> On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > * --write-standby-enable seems to loose quite some functionality in
> >   comparison to --write-recovery-conf since it doesn't seem to set
> >   primary_conninfo, standby anymore.

> Yes... The idea here might be to generate a new file that is then
> included in postgresql.conf or to add parameters at the bottom of
> postgresql.conf itself. The code for plain base backup is
> straight-forward, but it could get ugly for the tar part...

Well, just removing most of the feature - which the current patch seems
to be doing - looks like a regression to me, so it has to be either
fixed or explicitly discussed.

> > * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
> >   function name.

> CheckRecoveryTriggerPresence?

CheckStartingAsStandby()? Recovery alone doesn't say very much.

> > * Why does StartupXLOG() now use ArchiveRecoveryRequested &&
> >   StandbyModeRequested instead of just the former?

?

> > * 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.

> > * 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.

> > * Maybe we should rename names like pause_at_recovery_target to
> >   recovery_pause_at_target? Since we already force everyone to bother
> >   changing their setup...

> I disagree here. It is true that this patch introduces many changes
> with a new configuration file layer, but this idea with this patch was
> to allow people to reuse their old recovery.conf as it is. And what is
> actually wrong with pause_at_recovery_target?

That nearly all the other variables start with recovery_. But I don't
feel very strongly abou thtis.

> > * 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.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services