Don't call data type input functions in GUC check hooks

Instead of calling pg_lsn_in() in check_recovery_target_lsn and
timestamptz_in() in check_recovery_target_time, reorganize the
respective code so that we don't raise any errors in the check hooks.
The previous code tried to use PG_TRY/PG_CATCH to handle errors in a
way that is not safe, so now the code contains no ereport() calls and
can operate safely within the GUC error handling system.

Moreover, since the interpretation of the recovery_target_time string
may depend on the time zone, we cannot do the final processing of that
string until all the GUC processing is done.  Instead,
check_recovery_target_time() now does some parsing for syntax
checking, but the actual conversion to a timestamptz value is done
later in the recovery code that uses it.

Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/20190611061115.njjwkagvxp4qujhp%40alap3.anarazel.de

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e08320e8290893e120563b22bc971483690c0987..13e0d2366f55480f25f805cf73dacaf5912dd457 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -272,7 +272,8 @@ RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
 bool           recoveryTargetInclusive = true;
 int                    recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
 TransactionId recoveryTargetXid;
-TimestampTz recoveryTargetTime;
+char      *recovery_target_time_string;
+static TimestampTz recoveryTargetTime;
 const char *recoveryTargetName;
 XLogRecPtr     recoveryTargetLSN;
 int                    recovery_min_apply_delay = 0;
@@ -5409,6 +5410,18 @@ validateRecoveryParameters(void)
                !EnableHotStandby)
                recoveryTargetAction = RECOVERY_TARGET_ACTION_SHUTDOWN;
 
+       /*
+        * Final parsing of recovery_target_time string; see also
+        * check_recovery_target_time().
+        */
+       if (recoveryTarget == RECOVERY_TARGET_TIME)
+       {
+               recoveryTargetTime = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
+                                                                                                                                        CStringGetDatum(recovery_target_time_string),
+                                                                                                                                        ObjectIdGetDatum(InvalidOid),
+                                                                                                                                        Int32GetDatum(-1)));
+       }
+
        /*
         * If user specified recovery_target_timeline, validate it or compute the
         * "latest" value.  We can't do this until after we've gotten the restore

diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index 7242d3cfed485148edfcc73fdb3830d5e617688f..eb586851529e1a9a143ca51e8292f8efc50ec1eb 100644 (file)
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -25,10 +25,9 @@
  * Formatting and conversion routines.
  *---------------------------------------------------------*/
 
-Datum
-pg_lsn_in(PG_FUNCTION_ARGS)
+XLogRecPtr
+pg_lsn_in_internal(const char *str, bool *have_error)
 {
-       char       *str = PG_GETARG_CSTRING(0);
        int                     len1,
                                len2;
        uint32          id,
@@ -38,16 +37,16 @@ pg_lsn_in(PG_FUNCTION_ARGS)
        /* Sanity check input format. */
        len1 = strspn(str, "0123456789abcdefABCDEF");
        if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
-               ereport(ERROR,
-                               (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
-                                errmsg("invalid input syntax for type %s: \"%s\"",
-                                               "pg_lsn", str)));
+       {
+               *have_error = true;
+               return InvalidXLogRecPtr;
+       }
        len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF");
        if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0')
-               ereport(ERROR,
-                               (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
-                                errmsg("invalid input syntax for type %s: \"%s\"",
-                                               "pg_lsn", str)));
+       {
+               *have_error = true;
+               return InvalidXLogRecPtr;
+       }
 
        /* Decode result. */
        id = (uint32) strtoul(str, NULL, 16);
@@ -57,6 +56,23 @@ pg_lsn_in(PG_FUNCTION_ARGS)
        PG_RETURN_LSN(result);
 }
 
+Datum
+pg_lsn_in(PG_FUNCTION_ARGS)
+{
+       char       *str = PG_GETARG_CSTRING(0);
+       XLogRecPtr      result;
+       bool have_error = false;
+
+       result = pg_lsn_in_internal(str, &have_error);
+       if (have_error)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+                                errmsg("invalid input syntax for type %s: \"%s\"",
+                                               "pg_lsn", str)));
+
+       PG_RETURN_LSN(result);
+}
+
 Datum
 pg_lsn_out(PG_FUNCTION_ARGS)
 {

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a6836033a8dd8ee991799d00a204c6f0f..92c4fee8f8b404d613220ae61fb6da5dcd4c7de6 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -579,7 +579,6 @@ static bool assert_enabled;
 static char *recovery_target_timeline_string;
 static char *recovery_target_string;
 static char *recovery_target_xid_string;
-static char *recovery_target_time_string;
 static char *recovery_target_name_string;
 static char *recovery_target_lsn_string;
 
@@ -11572,20 +11571,20 @@ assign_recovery_target_xid(const char *newval, void *extra)
                recoveryTarget = RECOVERY_TARGET_UNSET;
 }
 
+/*
+ * The interpretation of the recovery_target_time string can depend on the
+ * time zone setting, so we need to wait until after all GUC processing is
+ * done before we can do the final parsing of the string.  This check function
+ * only does a parsing pass to catch syntax errors, but we store the string
+ * and parse it again when we need to use it.
+ */
 static bool
 check_recovery_target_time(char **newval, void **extra, GucSource source)
 {
        if (strcmp(*newval, "") != 0)
        {
-               TimestampTz time;
-               TimestampTz *myextra;
-               MemoryContext oldcontext = CurrentMemoryContext;
-
                /* reject some special values */
-               if (strcmp(*newval, "epoch") == 0 ||
-                       strcmp(*newval, "infinity") == 0 ||
-                       strcmp(*newval, "-infinity") == 0 ||
-                       strcmp(*newval, "now") == 0 ||
+               if (strcmp(*newval, "now") == 0 ||
                        strcmp(*newval, "today") == 0 ||
                        strcmp(*newval, "tomorrow") == 0 ||
                        strcmp(*newval, "yesterday") == 0)
@@ -11593,32 +11592,38 @@ check_recovery_target_time(char **newval, void **extra, GucSource source)
                        return false;
                }
 
-               PG_TRY();
-               {
-                       time = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
-                                                                                                                  CStringGetDatum(*newval),
-                                                                                                                  ObjectIdGetDatum(InvalidOid),
-                                                                                                                  Int32GetDatum(-1)));
-               }
-               PG_CATCH();
+               /*
+                * parse timestamp value (see also timestamptz_in())
+                */
                {
-                       ErrorData  *edata;
-
-                       /* Save error info */
-                       MemoryContextSwitchTo(oldcontext);
-                       edata = CopyErrorData();
-                       FlushErrorState();
-
-                       /* Pass the error message */
-                       GUC_check_errdetail("%s", edata->message);
-                       FreeErrorData(edata);
-                       return false;
+                       char       *str = *newval;
+                       fsec_t          fsec;
+                       struct pg_tm tt,
+                                          *tm = &tt;
+                       int                     tz;
+                       int                     dtype;
+                       int                     nf;
+                       int                     dterr;
+                       char       *field[MAXDATEFIELDS];
+                       int                     ftype[MAXDATEFIELDS];
+                       char            workbuf[MAXDATELEN + MAXDATEFIELDS];
+                       TimestampTz timestamp;
+
+                       dterr = ParseDateTime(str, workbuf, sizeof(workbuf),
+                                                                 field, ftype, MAXDATEFIELDS, &nf);
+                       if (dterr == 0)
+                               dterr = DecodeDateTime(field, ftype, nf, &dtype, tm, &fsec, &tz);
+                       if (dterr != 0)
+                               return false;
+                       if (dtype !=  DTK_DATE)
+                               return false;
+
+                       if (tm2timestamp(tm, fsec, &tz, &timestamp) != 0)
+                       {
+                               GUC_check_errdetail("timestamp out of range: \"%s\"", str);
+                               return false;
+                       }
                }
-               PG_END_TRY();
-
-               myextra = (TimestampTz *) guc_malloc(ERROR, sizeof(TimestampTz));
-               *myextra = time;
-               *extra = (void *) myextra;
        }
        return true;
 }
@@ -11631,10 +11636,7 @@ assign_recovery_target_time(const char *newval, void *extra)
                error_multiple_recovery_targets();
 
        if (newval && strcmp(newval, "") != 0)
-       {
                recoveryTarget = RECOVERY_TARGET_TIME;
-               recoveryTargetTime = *((TimestampTz *) extra);
-       }
        else
                recoveryTarget = RECOVERY_TARGET_UNSET;
 }
@@ -11675,33 +11677,11 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
        {
                XLogRecPtr      lsn;
                XLogRecPtr *myextra;
-               MemoryContext oldcontext = CurrentMemoryContext;
-
-               /*
-                * Convert the LSN string given by the user to XLogRecPtr form.
-                */
-               PG_TRY();
-               {
-                       lsn = DatumGetLSN(DirectFunctionCall3(pg_lsn_in,
-                                                                                                 CStringGetDatum(*newval),
-                                                                                                 ObjectIdGetDatum(InvalidOid),
-                                                                                                 Int32GetDatum(-1)));
-               }
-               PG_CATCH();
-               {
-                       ErrorData  *edata;
-
-                       /* Save error info */
-                       MemoryContextSwitchTo(oldcontext);
-                       edata = CopyErrorData();
-                       FlushErrorState();
+               bool            have_error = false;
 
-                       /* Pass the error message */
-                       GUC_check_errdetail("%s", edata->message);
-                       FreeErrorData(edata);
+               lsn = pg_lsn_in_internal(*newval, &have_error);
+               if (have_error)
                        return false;
-               }
-               PG_END_TRY();
 
                myextra = (XLogRecPtr *) guc_malloc(ERROR, sizeof(XLogRecPtr));
                *myextra = lsn;

diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 237f4e0350c89ec1ad0ea28fdb6cfc916e36e623..d519252aad77a37c1a2d7da98f4b93d7afdbb5a2 100644 (file)
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -132,7 +132,7 @@ extern char *PrimarySlotName;
 
 /* indirectly set via GUC system */
 extern TransactionId recoveryTargetXid;
-extern TimestampTz recoveryTargetTime;
+extern char *recovery_target_time_string;
 extern const char *recoveryTargetName;
 extern XLogRecPtr recoveryTargetLSN;
 extern RecoveryTargetType recoveryTarget;

diff --git a/src/include/utils/pg_lsn.h b/src/include/utils/pg_lsn.h
index def5b25aa3710ac60e6dbdca14e883e212c21239..70d8640ef3eabb46bb84863ede58ebdfebd5501e 100644 (file)
--- a/src/include/utils/pg_lsn.h
+++ b/src/include/utils/pg_lsn.h
@@ -24,4 +24,6 @@
 #define PG_GETARG_LSN(n)        DatumGetLSN(PG_GETARG_DATUM(n))
 #define PG_RETURN_LSN(x)        return LSNGetDatum(x)
 
+extern XLogRecPtr pg_lsn_in_internal(const char *str, bool *have_error);
+
 #endif                                                 /* PG_LSN_H */