|From:||Jaime Casanova <jaime(at)2ndquadrant(dot)com>|
|To:||Andres Freund <andres(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|
|Views:||Raw Message | Whole Thread | Download mbox|
On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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.
we can add code that looks for postgresql.conf in $PGDATA but if
postgresql.conf is not there (like the case in debian, there is not
much we can do about it) or we can write a file ready to be included
in postgresql.conf, any sugestion?
> * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
> function name.
I left it as CheckStartingAsStandby() but i still have a problem of
this not being completely clear. this function is useful for standby
which leads me to the other problem i have: the recovery trigger file,
i have left it as standby.enabled but i still don't like it.
other options for the recovery trigger file:
recovery.trigger (Andres objected on this name)
or just allow standby.enabled and pitr.enabled. One advantage of this
is that if you put pitr.enabled you can check that standby_mode is
the bad part on this approach is that it can end being
any_number_of_features.enabled, but i don't think that will happen
> * Why does StartupXLOG() now use ArchiveRecoveryRequested &&
> StandbyModeRequested instead of just the former?
good question. anyway, this patch shouldn't change that. if that
should be changed that is probably a bug and deserves to be in its own
> * 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)
returned to the old way of checking it
> * 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.
agreed. i left all parameters that were in recovery.conf as
PGC_POSTMASTER and will start a new thread about which ones we should
> * 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 don't have a problem with this, anyone else? if no one speaks i will
do what Andres suggests
> * the description of archive_cleanup_command seems wrong to me.
why? it seems to be the same that was in recovery.conf. where did you
see the description you're complaining at?
> * Why did you change some of the recovery gucs to lowercase names, but
> left out XLogRestoreCommand?
my bad, left it as it was in the original patch: restore_command
> * 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.
the code that read recovery.conf didn't has that, so i just removed it
> * You access recovery_target_name unconditionally, although it's
> intialized to NULL.
> * Generally, ISTM there's too much logic in the assign hooks.
probably that is mood now, because the comment that Heikki put in
commit 815d71deed5df2a91b06da76edbe5bc64965bfea says "If multiple
recovery_targets are specified, use the latest one." so now we should
just use the last one setted. Heikki? Any opinions?
> * Why do you include xlog_internal.h in guc.c and not xlog.h?
we actually need both but including xlog_internal.h also includes xlog.h
i added xlog.h and if someone things is enough only putting
xlog_internal.h let me know
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157
|Next Message||Oleg Bartunov||2014-01-15 07:01:52||Re: nested hstore patch - FailedAssertion("!(value->array.nelems == 1)|
|Previous Message||KONDO Mitsumasa||2014-01-15 06:53:07||drop duplicate buffers in OS|