Re: Turning recovery.conf into GUCs

Hi,

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

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