Re: Turning recovery.conf into GUCs - Mailing list pgsql-hackers

On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> 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
or pitr.
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)
forced_recovery.trigger
user_forced_recovery.trigger

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

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

look above

> * 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])

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
change

> * 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[0] 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

thank you

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566         Cell: +593 987171157