From fbafedf818c3985cfec12f5f3304d5a461be5901 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sun, 23 Jun 2019 14:37:32 +0200 Subject: [PATCH v2 2/2] 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, replicate the respective code to some degree. 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. Discussion: https://www.postgresql.org/message-id/flat/20190611061115.njjwkagvxp4qujhp%40alap3.anarazel.de --- src/backend/access/transam/xlog.c | 15 +++- src/backend/utils/misc/guc.c | 118 +++++++++++++++--------------- src/include/access/xlog.h | 2 +- 3 files changed, 74 insertions(+), 61 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/misc/guc.c b/src/backend/utils/misc/guc.c index 1208eb9a68..78d8abd155 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -86,7 +86,6 @@ #include "utils/float.h" #include "utils/memutils.h" #include "utils/pg_locale.h" -#include "utils/pg_lsn.h" #include "utils/plancache.h" #include "utils/portal.h" #include "utils/ps_status.h" @@ -579,7 +578,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 +11570,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 +11591,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, ×tamp) != 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 +11635,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; } @@ -11668,6 +11669,8 @@ assign_recovery_target_name(const char *newval, void *extra) recoveryTarget = RECOVERY_TARGET_UNSET; } +#define MAXPG_LSNCOMPONENT 8 + static bool check_recovery_target_lsn(char **newval, void **extra, GucSource source) { @@ -11675,33 +11678,30 @@ 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. + * Convert the LSN string given by the user to XLogRecPtr form (see + * also pg_lsn_in()). */ - 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); - return false; + char *str = *newval; + size_t len1; + size_t len2; + uint32 id; + uint32 off; + + len1 = strspn(str, "0123456789abcdefABCDEF"); + if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/') + return false; + + len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF"); + if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0') + return false; + + id = (uint32) strtoul(str, NULL, 16); + off = (uint32) strtoul(str + len1 + 1, NULL, 16); + lsn = ((uint64) id << 32) | off; } - 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; -- 2.22.0