Re: Turning recovery.conf into GUCs

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

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

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

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

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

>
> * the description of archive_cleanup_command seems wrong to me.
> * 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.

>
> * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
> really strangely formatted (multiline :? inside a function?) it
> doesn't a) seem to be correct to ignore potential memory allocation
> errors by just switching back into the context that just errored out,
> and continue to work there b) forgo cleanup by just continuing as if
> nothing happened. That's unlikely to be acceptable.

Interestingly, that was part of the first versions of the patch as
well. I don't recall modifying anything in this area when I hacked
that... But yes it should be modified to something like what is in
check_recovery_target_xid.

Regards,
--
Michael