From 21f428ebde39339487c271a830fed135d6032d73 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sun, 30 Jun 2019 10:15:25 +0200 Subject: [PATCH] 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 Reviewed-by: Michael Paquier Discussion: https://www.postgresql.org/message-id/flat/20190611061115.njjwkagvxp4qujhp%40alap3.anarazel.de --- src/backend/access/transam/xlog.c | 15 ++++- src/backend/utils/adt/pg_lsn.c | 38 +++++++---- src/backend/utils/misc/guc.c | 102 ++++++++++++------------------ src/include/access/xlog.h | 2 +- src/include/utils/pg_lsn.h | 2 + 5 files changed, 85 insertions(+), 74 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e08320e829..13e0d2366f 100644 --- 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 7242d3cfed..eb58685152 100644 --- 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 1208eb9a68..92c4fee8f8 100644 --- 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(); + /* + * parse timestamp value (see also timestamptz_in()) + */ { - time = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in, - CStringGetDatum(*newval), - ObjectIdGetDatum(InvalidOid), - Int32GetDatum(-1))); + 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, ×tamp) != 0) + { + GUC_check_errdetail("timestamp out of range: \"%s\"", str); + return false; + } } - 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(); - - 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; + bool have_error = false; - /* - * 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(); - - /* 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 237f4e0350..d519252aad 100644 --- 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 def5b25aa3..70d8640ef3 100644 --- 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 */