On 2013-11-19 22:09:48 +0900, Michael Paquier wrote:
> On Tue, Nov 19, 2013 at 2:27 AM, Andres Freund <and...@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.


Andres Freund

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to