Re: Proposal for changes to recovery.conf API

Hi,

On 2016-11-01 05:14:58 +0530, Abhijit Menon-Sen wrote:
> Subject: Convert recovery.conf settings to GUCs

I really want to get this in, we've been back and forth about this for
far too long.

>     <para>
>-     Create a recovery command file <filename>recovery.conf</> in the cluster
>-     data directory (see <xref linkend="recovery-config">). You might
>+     Set up recovery parameters in <filename>postgresql.conf</> (see
>+     <xref linkend="runtime-config-wal-archive-recovery"> and
>+     <xref linkend="runtime-config-wal-recovery-target">).
>+    </para>
>+   </listitem>
>+   <listitem>
>+    <para>
>+     Create a recovery trigger file <filename>recovery.trigger</>
>+     in the cluster data directory. You might
>      also want to temporarily modify <filename>pg_hba.conf</> to prevent
>      ordinary users from connecting until you are sure the recovery was successful.
>     </para>

I think this means we need to rephrase a number of docs and code
references to trigger files, because they'll currently often refer to
the promotion trigger, no?


> diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access/transam/recovery.conf.sample
> index 7a16751..15ce784 100644
> --- a/src/backend/access/transam/recovery.conf.sample
> +++ b/src/backend/access/transam/recovery.conf.sample
> @@ -2,23 +2,14 @@
>  # PostgreSQL recovery config file
>  # -------------------------------
>  #
> -# Edit this file to provide the parameters that PostgreSQL needs to
> -# perform an archive recovery of a database, or to act as a replication
> -# standby.
> +# PostgreSQL 10.0 or later, recovery.conf is no longer used. Instead,
> +# specify all recovery parameters in postgresql.conf and create
> +# recovery.trigger to enter archive recovery or standby mode.
>  #
> -# If "recovery.conf" is present in the PostgreSQL data directory, it is
> -# read on postmaster startup.  After successful recovery, it is renamed
> -# to "recovery.done" to ensure that we do not accidentally re-enter
> -# archive recovery or standby mode.
> +# If you must use recovery.conf, specify "include directives" in
> +# postgresql.conf like this:
>  #
> -# This file consists of lines of the form:
> -#
> -#   name = value
> -#
> -# Comments are introduced with '#'.
> -#
> -# The complete list of option names and allowed values can be found
> -# in the PostgreSQL documentation.
> +#   include 'recovery.conf'

This should probably warn about the differences in "trigger" behaviour
of recovery.conf being present.

>  #---------------------------------------------------------------------------
>  # ARCHIVE RECOVERY PARAMETERS
> @@ -131,14 +122,6 @@
>  #
>  #primary_slot_name = ''

I don't think it makes much sense to still list all this?


> +bool        standby_mode = false;

I'm very tempted to rename this during the move to GUCs.


> +char       *primary_conninfo = NULL;
> +char       *primary_slot_name = NULL;

Slightly less so, but still tempted to also rename these. They're not
actually necessarily pointing towards a primary, and namespace-wise
they're not grouped with recovery_*, which has become more important now
that recovery.conf isn't a separate namespace anymore.

> -static int    recovery_min_apply_delay = 0;
>  static TimestampTz recoveryDelayUntilTime;
>
> +static TimestampTz recoveryDelayUntilTime;
> +

Isn't this a repeated definition?

>  /*
>   * During normal operation, the only timeline we care about is ThisTimeLineID.
>   * During recovery, however, things are more complicated.  To simplify life
> @@ -577,10 +578,10 @@ typedef struct XLogCtlData
>      TimeLineID    PrevTimeLineID;
>
>      /*
> -     * archiveCleanupCommand is read from recovery.conf but needs to be in
> -     * shared memory so that the checkpointer process can access it.
> +     * archive_cleanup_command is read from recovery.conf but needs to
> +     * be in shared memory so that the checkpointer process can access it.
>       */

References recovery.conf. It'd be a good idea to grep for all remaining
references to recovery.conf.


> +static bool
> +check_recovery_target(char **newval, void **extra, GucSource source)
> +{
> +    RecoveryTargetType *myextra;
> +    RecoveryTargetType rt = RECOVERY_TARGET_UNSET;
> +
> +    if (strcmp(*newval, "xid") == 0)
> +        rt = RECOVERY_TARGET_XID;
> +    else if (strcmp(*newval, "time") == 0)
> +        rt = RECOVERY_TARGET_TIME;
> +    else if (strcmp(*newval, "name") == 0)
> +        rt = RECOVERY_TARGET_NAME;
> +    else if (strcmp(*newval, "lsn") == 0)
> +        rt = RECOVERY_TARGET_LSN;
> +    else if (strcmp(*newval, "immediate") == 0)
> +        rt = RECOVERY_TARGET_IMMEDIATE;
> +    else if (strcmp(*newval, "") != 0)
> +    {
> +        GUC_check_errdetail("recovery_target is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    myextra = (RecoveryTargetType *) guc_malloc(ERROR, sizeof(RecoveryTargetType));
> +    *myextra = rt;
> +    *extra = (void *) myextra;
> +
> +    return true;
> +}

Shouldn't this instead be an enum type GUC?

> +static bool
> +check_recovery_target_time(char **newval, void **extra, GucSource source)
> +{
> +    TimestampTz     time;
> +    TimestampTz     *myextra;
> +    MemoryContext oldcontext = CurrentMemoryContext;
> +
> +    PG_TRY();
> +    {
> +        time = (strcmp(*newval, "") == 0) ?
> +            0 :
> +            DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
> +                                                    CStringGetDatum(*newval),
> +                                                    ObjectIdGetDatum(InvalidOid),
> +                                                    Int32GetDatum(-1)));
> +    }
> +    PG_CATCH();
> +    {
> +        ErrorData  *edata;
> +
> +        /* Save error info */
> +        MemoryContextSwitchTo(oldcontext);
> +        edata = CopyErrorData();
> +        FlushErrorState();
> +
> +        /* Pass the error message */
> +        GUC_check_errdetail("%s", edata->message);
> +        FreeErrorData(edata);
> +        return false;
> +    }
> +    PG_END_TRY();

Hm, I'm not happy to do that kind of thing. What if there's ever any
database interaction added to timestamptz_in?

It's also problematic because the parsing of timestamps depends on the
timezone GUC - which might not yet have taken effect...


> +static bool
> +check_recovery_target_lsn(char **newval, void **extra, GucSource source)
> +{
> +    XLogRecPtr        lsn;
> +    XLogRecPtr       *myextra;
> +    MemoryContext oldcontext = CurrentMemoryContext;
> +
> +    PG_TRY();
> +    {
> +        lsn = (strcmp(*newval, "") == 0) ?
> +            InvalidXLogRecPtr :
> +            DatumGetLSN(DirectFunctionCall3(pg_lsn_in,
> +                                            CStringGetDatum(*newval),
> +                                            ObjectIdGetDatum(InvalidOid),
> +                                            Int32GetDatum(-1)));
> +    }
> +    PG_CATCH();
> +    {
> +        ErrorData  *edata;
> +
> +        /* Save error info */
> +        MemoryContextSwitchTo(oldcontext);
> +        edata = CopyErrorData();
> +        FlushErrorState();
> +
> +        /* Pass the error message */
> +        GUC_check_errdetail("%s", edata->message);
> +        FreeErrorData(edata);
> +        return false;
> +    }
> +    PG_END_TRY();

That seems like a pretty pointless use of pg_lsn_in, given the problems
that PG_CATCHing an error without rethrowing brings.


> +static bool
> +check_recovery_target_action(char **newval, void **extra, GucSource source)
> +{
> +    RecoveryTargetAction rta = RECOVERY_TARGET_ACTION_PAUSE;
> +    RecoveryTargetAction *myextra;
> +
> +    if (strcmp(*newval, "pause") == 0)
> +        rta = RECOVERY_TARGET_ACTION_PAUSE;
> +    else if (strcmp(*newval, "promote") == 0)
> +        rta = RECOVERY_TARGET_ACTION_PROMOTE;
> +    else if (strcmp(*newval, "shutdown") == 0)
> +        rta = RECOVERY_TARGET_ACTION_SHUTDOWN;
> +    else if (strcmp(*newval, "") != 0)
> +    {
> +        GUC_check_errdetail("recovery_target_action is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    myextra = (RecoveryTargetAction *) guc_malloc(ERROR, sizeof(RecoveryTargetAction));
> +    *myextra = rta;
> +    *extra = (void *) myextra;
> +
> +    return true;
> +}

Should be an enum.


> +static bool
> +check_primary_slot_name(char **newval, void **extra, GucSource source)
> +{
> +    if (strcmp(*newval,"") != 0 &&
> +        !ReplicationSlotValidateName(*newval, WARNING))
> +    {
> +        GUC_check_errdetail("primary_slot_name is not valid: \"%s\"", *newval);
> +        return false;
> +    }
> +
> +    return true;
> +}

ReplicationSlotValidateName will emit WARNINGs this way - hm. That'll
often loose detail about what the problem actually is, e.g. at server
startup.

> +#standby_mode = off            # "on" starts the server as a standby
> +                    # (change requires restart)
> +#primary_conninfo = ''            # connection string to connect to the master
> +#trigger_file = ''            # trigger file to promote the standby

Too general a name imo.


Greetings,

Andres Freund