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: 2014-01-23 13:34:24
Message-ID: 20140123133424.GD29782@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2014-01-15 02:00:51 -0500, Jaime Casanova wrote:
> 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?

People might not like me for the suggestion, but I think we should
simply always include a 'recovery.conf' in $PGDATA
unconditionally. That'd make this easier.
Alternatively we could pass a filename to --write-recovery-conf.

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

> recovery.trigger (Andres objected on this name)
> forced_recovery.trigger
> user_forced_recovery.trigger

stay_in_recovery.trigger? That'd be pretty clear for anybody involved in
pg, but otherwise...

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

I dislike the description in guc.c

> + {"archive_cleanup_command", PGC_POSTMASTER, WAL_ARCHIVE_RECOVERY,
> + gettext_noop("Sets the shell command that will be executed at every restartpoint."),
> + NULL
> + },
> + &archive_cleanup_command,

previously it was:

> -# specifies an optional shell command to execute at every restartpoint.
> -# This can be useful for cleaning up the archive of a standby server.

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

Well, that's not necessarily correct. recovery.conf was only read during
startup, while this is read on SIGHUP.

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

What's required from xlog_internal.h?

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index b53ae87..54f6a0d 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -64,11 +64,12 @@
> extern uint32 bootstrap_data_checksum_version;
>
> /* File path names (all relative to $PGDATA) */
> -#define RECOVERY_COMMAND_FILE "recovery.conf"
> -#define RECOVERY_COMMAND_DONE "recovery.done"
> +#define RECOVERY_ENABLE_FILE "standby.enabled"

Imo the file and variable names should stay coherent.

> +/* recovery.conf is not supported anymore */
> +#define RECOVERY_COMMAND_FILE "recovery.conf"

> +bool StandbyModeRequested = false;
> +static TimestampTz recoveryDelayUntilTime;

This imo should be lowercase now, the majority of GUC variables are.

> +/* are we currently in standby mode? */
> +bool StandbyMode = false;

Why did you move this?

> - if (rtliGiven)
> + if (strcmp(recovery_target_timeline_string, "") != 0)
> {

Why not have the convention that NULL indicates a unset target_timeline
like you use for other GUCs? Mixing things like this is confusing.

Why is recovery_target_timeline stored as a string? Because it's a
unsigned int? If so, you should have an assign hook setting up a)
rtliGiven, b) properly typed variable.

> - if (rtli)
> + if (recovery_target_timeline)
> {
> /* Timeline 1 does not have a history file, all else should */
> - if (rtli != 1 && !existsTimeLineHistory(rtli))
> + if (recovery_target_timeline != 1 &&
> + !existsTimeLineHistory(recovery_target_timeline))
> ereport(FATAL,
> (errmsg("recovery target timeline %u does not exist",
> - rtli)));
> - recoveryTargetTLI = rtli;
> + recovery_target_timeline)));
> + recoveryTargetTLI = recovery_target_timeline;
> recoveryTargetIsLatest = false;

So, now we have a recoveryTargetTLI and recovery_target_timeline
variable? Really? Why do we need recoveryTargetTLI at all now?

> +static void
> +assign_recovery_target_xid(const char *newval, void *extra)
> +{
> + recovery_target_xid = *((TransactionId *) extra);
> +
> + if (recovery_target_xid != InvalidTransactionId)
> + recovery_target = RECOVERY_TARGET_XID;
> + else if (recovery_target_name[0])
> + recovery_target = RECOVERY_TARGET_NAME;
> + else if (recovery_target_time != 0)
> + recovery_target = RECOVERY_TARGET_TIME;
> + else
> + recovery_target = RECOVERY_TARGET_UNSET;
> +}

> +static void
> +assign_recovery_target_time(const char *newval, void *extra)
> +{
> + recovery_target_time = *((TimestampTz *) extra);
> +
> + if (recovery_target_xid != InvalidTransactionId)
> + recovery_target = RECOVERY_TARGET_XID;
> + else if (recovery_target_name[0])
> + recovery_target = RECOVERY_TARGET_NAME;
> + else if (recovery_target_time != 0)
> + recovery_target = RECOVERY_TARGET_TIME;
> + else
> + recovery_target = RECOVERY_TARGET_UNSET;
> +}
> +

I don't think it's correct to do such hangups in the assign hook - you
have no ideas in which order they will be called and such. Imo that
should happen at startup, like we also do it for other interdependent
variables like wal_buffers.

> +static bool
> +check_recovery_target_time(char **newval, void **extra, GucSource source)
> +{
> + TimestampTz time;
> + TimestampTz *myextra;
> +
> + if (strcmp(*newval, "") == 0)
> + time = 0;
> + else
> + time = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
> + CStringGetDatum(*newval),
> + ObjectIdGetDatum(InvalidOid),
> + Int32GetDatum(-1)));
> +
> + myextra = (TimestampTz *) guc_malloc(ERROR, sizeof(TimestampTz));
> + *myextra = time;
> + *extra = (void *) myextra;
> +
> + return true;
> +}

Trailing space behind the strcmp().

I don't think that's correct. Afaics it will cause the postmaster to
crash on a SIGHUP with invalid data. I think you're unfortunately going
to have to copy a fair bit of timestamptz_in() and even
DateTimeParseError(), replacing the ereport()s by the guc error
reporting.

Greetings,

Andres Freund

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kohei KaiGai 2014-01-23 14:03:40 Re: dynamic shared memory and locks
Previous Message Peter Eisentraut 2014-01-23 13:11:09 commit fest 2014-01 week 1 report