Re: Turning recovery.conf into GUCs

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
Date: 2014-01-15 07:00:51
Message-ID: CAJKUy5jNdceVvMFSHF+zTeX-LfnO0ftmdHzLG+0nn43mPw4kQQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund <andres(at)2ndquadrant(dot)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

Attachment Content-Type Size
recovery_guc_v5.2.patch text/x-patch 111.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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