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(at)2ndquadrant(dot)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