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