Re: Turning recovery.conf into GUCs

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Jaime Casanova <jaime(at)2ndquadrant(dot)com>
Cc: Josh Berkus <josh(at)agliodbs(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Turning recovery.conf into GUCs
Date: 2013-11-18 17:27:48
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2013-11-15 22:38:05 -0500, Jaime Casanova wrote:
> sorry, i clearly misunderstood you. attached a rebased patch with
> those functions removed.

* --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.
* CheckRecoveryReadyFile() doesn't seem to be a very descriptive
function name.
* 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
* You check for a trigger file in a way that's not compatible with it
being NULL. Why did you change that?
- if (TriggerFile == NULL)
+ if (!trigger_file[0])
* 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.
* 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...
* 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?
* 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.
* You access recovery_target_name[0] unconditionally, although it's
intialized to NULL.
* Generally, ISTM there's too much logic in the assign hooks.
* Why do you include xlog_internal.h in guc.c and not xlog.h?

Ok, I think that's enough for now ;)


Andres Freund

Andres Freund
PostgreSQL Development, 24x7 Support, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Josh Berkus 2013-11-18 17:49:23 Re: additional json functionality
Previous Message Andrew Dunstan 2013-11-18 17:25:53 Re: Review: pre-commit triggers