check_recovery_target_lsn() does a PG_CATCH without a throw

Started by Andres Freundover 6 years ago16 messages

andres@anarazel.de

over 6 years ago

Hi,

While working on fixing [1]CALDaNm1KXK9gbZfY-p_peRFm_XrBh1OwQO1Kk6Gig0c0fVZ2uw@mail.gmail.com I noticed that 2dedf4d9a899 "Integrate
recovery.conf into postgresql.conf" added two non-rethrowing PG_CATCH
uses. That's not OK. See

/messages/by-id/1676.1548726280@sss.pgh.pa.us
/messages/by-id/20190206160958.GA22304@alvherre.pgsql
etc.

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 ||
strcmp(*newval, "today") == 0 ||
strcmp(*newval, "tomorrow") == 0 ||
strcmp(*newval, "yesterday") == 0)
{
return false;
}

PG_TRY();
{
time = 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();

same in check_recovery_target_lsn.

I'll add an open item.

Greetings,

Andres Freund

[1]: CALDaNm1KXK9gbZfY-p_peRFm_XrBh1OwQO1Kk6Gig0c0fVZ2uw@mail.gmail.com

Sergei Kornilov

sk@zsrv.org

over 6 years ago

In reply to: Andres Freund (#1)

1 attachment(s)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Hello

That's not OK.

hmm. Did you mean catching only needed errors by errcode? Something like attached?

regards, Sergei

Attachments:

check_recovery_target_re_throw_error.patchtext/x-diff; name=check_recovery_target_re_throw_error.patchDownload

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a68..2993b8aa08 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11603,16 +11603,25 @@ check_recovery_target_time(char **newval, void **extra, GucSource source)
 		PG_CATCH();
 		{
 			ErrorData  *edata;
+			MemoryContext ecxt;
 
 			/* Save error info */
-			MemoryContextSwitchTo(oldcontext);
+			ecxt = MemoryContextSwitchTo(oldcontext);
 			edata = CopyErrorData();
-			FlushErrorState();
-
-			/* Pass the error message */
-			GUC_check_errdetail("%s", edata->message);
-			FreeErrorData(edata);
-			return false;
+			switch (edata->sqlerrcode)
+			{
+				case ERRCODE_INVALID_DATETIME_FORMAT:
+				case ERRCODE_DATETIME_FIELD_OVERFLOW:
+				case ERRCODE_INVALID_TIME_ZONE_DISPLACEMENT_VALUE:
+					FlushErrorState();
+					/* Pass the error message */
+					GUC_check_errdetail("%s", edata->message);
+					FreeErrorData(edata);
+					return false;
+				default:
+					MemoryContextSwitchTo(ecxt);
+					PG_RE_THROW();
+			}
 		}
 		PG_END_TRY();
 
@@ -11690,16 +11699,23 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
 		PG_CATCH();
 		{
 			ErrorData  *edata;
+			MemoryContext ecxt;
 
 			/* Save error info */
-			MemoryContextSwitchTo(oldcontext);
+			ecxt = MemoryContextSwitchTo(oldcontext);
 			edata = CopyErrorData();
-			FlushErrorState();
-
-			/* Pass the error message */
-			GUC_check_errdetail("%s", edata->message);
-			FreeErrorData(edata);
-			return false;
+			if (edata->sqlerrcode == ERRCODE_INVALID_TEXT_REPRESENTATION)
+			{
+				FlushErrorState();
+				GUC_check_errdetail("%s", edata->message);
+				FreeErrorData(edata);
+				return false;
+			}
+			else
+			{
+				MemoryContextSwitchTo(ecxt);
+				PG_RE_THROW();
+			}
 		}
 		PG_END_TRY();

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Sergei Kornilov (#2)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Sergei Kornilov <sk@zsrv.org> writes:

That's not OK.

hmm. Did you mean catching only needed errors by errcode? Something like attached?

No, he means you can't EVER catch an error and not re-throw it, unless
you do a full (sub)transaction abort and cleanup instead of re-throwing.
We've been around on this repeatedly because people want to believe they
can take shortcuts. (See e.g. discussions for the jsonpath stuff.)
It doesn't reliably work to do so, and we have a project policy against
trying, and this code should never have been committed in this state.

regards, tom lane

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Tom Lane (#3)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Hi,

On 2019-06-11 10:49:28 -0400, Tom Lane wrote:

It doesn't reliably work to do so, and we have a project policy against
trying, and this code should never have been committed in this state.

I'll also note that I complained about this specific instance being
introduced all the way back in 2013 and then again 2016:

/messages/by-id/20131118172748.GG20305@awork2.anarazel.de

On 2013-11-18 18:27:48 +0100, Andres Freund wrote:

* Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
really strangely formatted (multiline :? inside a function?) it
doesn't a) seem to be correct to ignore potential memory allocation
errors by just switching back into the context that just errored out,
and continue to work there b) forgo cleanup by just continuing as if
nothing happened. That's unlikely to be acceptable.
* You access recovery_target_name[0] unconditionally, although it's

/messages/by-id/20140123133424.GD29782@awork2.anarazel.de

On 2016-11-12 08:09:49 -0800, Andres Freund wrote:

+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...

I don't have particularly polite words about this.

Greetings,

Andres Freund

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Andres Freund (#1)

2 attachment(s)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On 2019-06-11 08:11, Andres Freund wrote:

While working on fixing [1] I noticed that 2dedf4d9a899 "Integrate
recovery.conf into postgresql.conf" added two non-rethrowing PG_CATCH
uses. That's not OK.

Right. Here is a patch that addresses this by copying the relevant code
from pg_lsn_in() and timestamptz_in() directly into the check hooks.
It's obviously a bit unfortunate not to be able to share that code, but
it's not actually that much.

I haven't figured out the time zone issue yet, but I guess the solution
might involve moving some of the code from check_recovery_target_time()
to assign_recovery_target_time().

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Remove-explicit-error-handling-for-obsolete-date-tim.patchtext/plain; charset=UTF-8; name=0001-Remove-explicit-error-handling-for-obsolete-date-tim.patch; x-mac-creator=0; x-mac-type=0Download

From 01f26abca6442b80a03359b9a6057610fd2068a4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Jun 2019 11:29:53 +0200
Subject: [PATCH 1/2] Remove explicit error handling for obsolete date/time
 values

The date/time values 'current', 'invalid', and 'undefined' were
removed a long time ago, but the code still contains explicit error
handling for the transition.  To simplify the code and avoid having to
handle these values everywhere, just remove the recognition of these
tokens altogether now.
---
 src/backend/utils/adt/date.c               |  8 --------
 src/backend/utils/adt/datetime.c           | 20 --------------------
 src/backend/utils/adt/timestamp.c          | 22 ----------------------
 src/include/utils/datetime.h               |  2 --
 src/interfaces/ecpg/pgtypeslib/dt.h        |  2 --
 src/interfaces/ecpg/pgtypeslib/dt_common.c |  5 -----
 src/interfaces/ecpg/pgtypeslib/timestamp.c |  4 ----
 src/test/regress/expected/date.out         |  3 ---
 src/test/regress/expected/timestamp.out    | 13 -------------
 src/test/regress/expected/timestamptz.out  | 13 -------------
 src/test/regress/sql/date.sql              |  1 -
 src/test/regress/sql/timestamp.sql         |  4 ----
 src/test/regress/sql/timestamptz.sql       |  4 ----
 13 files changed, 101 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1ff3cfea8b..e440a4fedd 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -138,14 +138,6 @@ date_in(PG_FUNCTION_ARGS)
 		case DTK_DATE:
 			break;
 
-		case DTK_CURRENT:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"current\" is no longer supported")));
-
-			GetCurrentDateTime(tm);
-			break;
-
 		case DTK_EPOCH:
 			GetEpochTime(tm);
 			break;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 9def318c57..e9add385ba 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -99,7 +99,6 @@ static const datetkn datetktbl[] = {
 	{"aug", MONTH, 8},
 	{"august", MONTH, 8},
 	{DB_C, ADBC, BC},			/* "bc" for years <= 0 */
-	{DCURRENT, RESERV, DTK_CURRENT},	/* "current" is always now */
 	{"d", UNITS, DTK_DAY},		/* "day of month" for ISO input */
 	{"dec", MONTH, 12},
 	{"december", MONTH, 12},
@@ -113,7 +112,6 @@ static const datetkn datetktbl[] = {
 	{"friday", DOW, 5},
 	{"h", UNITS, DTK_HOUR},		/* "hour" */
 	{LATE, RESERV, DTK_LATE},	/* "infinity" reserved for "late time" */
-	{INVALID, RESERV, DTK_INVALID}, /* "invalid" reserved for bad time */
 	{"isodow", UNITS, DTK_ISODOW},	/* ISO day of week, Sunday == 7 */
 	{"isoyear", UNITS, DTK_ISOYEAR},	/* year in terms of the ISO week date */
 	{"j", UNITS, DTK_JULIAN},
@@ -157,7 +155,6 @@ static const datetkn datetktbl[] = {
 	{"tue", DOW, 2},
 	{"tues", DOW, 2},
 	{"tuesday", DOW, 2},
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"wed", DOW, 3},
 	{"wednesday", DOW, 3},
 	{"weds", DOW, 3},
@@ -191,7 +188,6 @@ static const datetkn deltatktbl[] = {
 	{"hours", UNITS, DTK_HOUR}, /* "hours" relative */
 	{"hr", UNITS, DTK_HOUR},	/* "hour" relative */
 	{"hrs", UNITS, DTK_HOUR},	/* "hours" relative */
-	{INVALID, RESERV, DTK_INVALID}, /* reserved for invalid time */
 	{"m", UNITS, DTK_MINUTE},	/* "minute" relative */
 	{"microsecon", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"mil", UNITS, DTK_MILLENNIUM}, /* "millennium" relative */
@@ -222,7 +218,6 @@ static const datetkn deltatktbl[] = {
 	{DTIMEZONE, UNITS, DTK_TZ}, /* "timezone" time offset */
 	{"timezone_h", UNITS, DTK_TZ_HOUR}, /* timezone hour units */
 	{"timezone_m", UNITS, DTK_TZ_MINUTE},	/* timezone minutes units */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"us", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"usec", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{DMICROSEC, UNITS, DTK_MICROSEC},	/* "microsecond" relative */
@@ -1186,14 +1181,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
 					case RESERV:
 						switch (val)
 						{
-							case DTK_CURRENT:
-								ereport(ERROR,
-										(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-										 errmsg("date/time value \"current\" is no longer supported")));
-
-								return DTERR_BAD_FORMAT;
-								break;
-
 							case DTK_NOW:
 								tmask = (DTK_DATE_M | DTK_TIME_M | DTK_M(TZ));
 								*dtype = DTK_DATE;
@@ -2097,13 +2084,6 @@ DecodeTimeOnly(char **field, int *ftype, int nf,
 					case RESERV:
 						switch (val)
 						{
-							case DTK_CURRENT:
-								ereport(ERROR,
-										(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-										 errmsg("date/time value \"current\" is no longer supported")));
-								return DTERR_BAD_FORMAT;
-								break;
-
 							case DTK_NOW:
 								tmask = DTK_TIME_M;
 								*dtype = DTK_TIME;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index e5ac371fa0..8d129731a6 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -188,14 +188,6 @@ timestamp_in(PG_FUNCTION_ARGS)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-
-			TIMESTAMP_NOEND(result);
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing timestamp \"%s\"",
 				 dtype, str);
@@ -439,14 +431,6 @@ timestamptz_in(PG_FUNCTION_ARGS)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-
-			TIMESTAMP_NOEND(result);
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing timestamptz \"%s\"",
 				 dtype, str);
@@ -946,12 +930,6 @@ interval_in(PG_FUNCTION_ARGS)
 						 errmsg("interval out of range")));
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
 				 dtype, str);
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index 4de78ebe36..b8a199cdde 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -148,8 +148,6 @@ struct tzEntry;
 #define DTK_AGO			5
 
 #define DTK_SPECIAL		6
-#define DTK_INVALID		7
-#define DTK_CURRENT		8
 #define DTK_EARLY		9
 #define DTK_LATE		10
 #define DTK_EPOCH		11
diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index a9884f278c..c5fd6bdaed 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -143,8 +143,6 @@ typedef int32 fsec_t;
 #define DTK_AGO			5
 
 #define DTK_SPECIAL		6
-#define DTK_INVALID		7
-#define DTK_CURRENT		8
 #define DTK_EARLY		9
 #define DTK_LATE		10
 #define DTK_EPOCH		11
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index f5aed794fd..24fff28f0a 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -111,7 +111,6 @@ static const datetkn datetktbl[] = {
 #endif
 	{"cot", TZ, -18000},		/* Columbia Time */
 	{"cst", TZ, -21600},		/* Central Standard Time */
-	{DCURRENT, RESERV, DTK_CURRENT},	/* "current" is always now */
 #if 0
 	cvst
 #endif
@@ -201,7 +200,6 @@ static const datetkn datetktbl[] = {
 	idt							/* Israeli, Iran, Indian Daylight Time */
 #endif
 	{LATE, RESERV, DTK_LATE},	/* "infinity" reserved for "late time" */
-	{INVALID, RESERV, DTK_INVALID}, /* "invalid" reserved for bad time */
 	{"iot", TZ, 18000},			/* Indian Chagos Time */
 	{"irkst", DTZ, 32400},		/* Irkutsk Summer Time */
 	{"irkt", TZ, 28800},		/* Irkutsk Time */
@@ -372,7 +370,6 @@ static const datetkn datetktbl[] = {
 #endif
 	{"ulast", DTZ, 32400},		/* Ulan Bator Summer Time */
 	{"ulat", TZ, 28800},		/* Ulan Bator Time */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"ut", TZ, 0},
 	{"utc", TZ, 0},
 	{"uyst", DTZ, -7200},		/* Uruguay Summer Time */
@@ -440,7 +437,6 @@ static const datetkn deltatktbl[] = {
 	{"hours", UNITS, DTK_HOUR}, /* "hours" relative */
 	{"hr", UNITS, DTK_HOUR},	/* "hour" relative */
 	{"hrs", UNITS, DTK_HOUR},	/* "hours" relative */
-	{INVALID, RESERV, DTK_INVALID}, /* reserved for invalid time */
 	{"m", UNITS, DTK_MINUTE},	/* "minute" relative */
 	{"microsecon", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"mil", UNITS, DTK_MILLENNIUM}, /* "millennium" relative */
@@ -471,7 +467,6 @@ static const datetkn deltatktbl[] = {
 	{DTIMEZONE, UNITS, DTK_TZ}, /* "timezone" time offset */
 	{"timezone_h", UNITS, DTK_TZ_HOUR}, /* timezone hour units */
 	{"timezone_m", UNITS, DTK_TZ_MINUTE},	/* timezone minutes units */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"us", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"usec", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{DMICROSEC, UNITS, DTK_MICROSEC},	/* "microsecond" relative */
diff --git a/src/interfaces/ecpg/pgtypeslib/timestamp.c b/src/interfaces/ecpg/pgtypeslib/timestamp.c
index a7e0fe66f0..efd9f3aa71 100644
--- a/src/interfaces/ecpg/pgtypeslib/timestamp.c
+++ b/src/interfaces/ecpg/pgtypeslib/timestamp.c
@@ -253,10 +253,6 @@ PGTYPEStimestamp_from_asc(char *str, char **endptr)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			errno = PGTYPES_TS_BAD_TIMESTAMP;
-			return noresult;
-
 		default:
 			errno = PGTYPES_TS_BAD_TIMESTAMP;
 			return noresult;
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index 1bcc9465a9..4686d0d8ca 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1444,9 +1444,6 @@ SELECT EXTRACT(EPOCH      FROM DATE 'infinity');    --  Infinity
 SELECT EXTRACT(MICROSEC  FROM DATE 'infinity');     -- ERROR:  timestamp units "microsec" not recognized
 ERROR:  timestamp units "microsec" not recognized
 CONTEXT:  SQL function "date_part" statement 1
-SELECT EXTRACT(UNDEFINED FROM DATE 'infinity');     -- ERROR:  timestamp units "undefined" not supported
-ERROR:  timestamp units "undefined" not supported
-CONTEXT:  SQL function "date_part" statement 1
 -- test constructors
 select make_date(2013, 7, 15);
  make_date  
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index b2b171f560..715680e330 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -79,19 +79,6 @@ TRUNCATE TIMESTAMP_TBL;
 INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-ERROR:  date/time value "invalid" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-                                          ^
-INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-ERROR:  date/time value "undefined" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-                                          ^
-INSERT INTO TIMESTAMP_TBL VALUES ('current');
-ERROR:  date/time value "current" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('current');
-                                          ^
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 -- Variations on Postgres v6.1 standard output format
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 8a4c719993..5551fa6610 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -78,19 +78,6 @@ DELETE FROM TIMESTAMPTZ_TBL;
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-ERROR:  date/time value "invalid" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-                                            ^
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-ERROR:  date/time value "undefined" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-                                            ^
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
-ERROR:  date/time value "current" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
-                                            ^
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 -- Variations on Postgres v6.1 standard output format
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 22f80f2ee2..4c5b94a14a 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -334,7 +334,6 @@ CREATE TABLE DATE_TBL (f1 date);
 -- wrong fields from non-finite date:
 --
 SELECT EXTRACT(MICROSEC  FROM DATE 'infinity');     -- ERROR:  timestamp units "microsec" not recognized
-SELECT EXTRACT(UNDEFINED FROM DATE 'infinity');     -- ERROR:  timestamp units "undefined" not supported
 
 -- test constructors
 select make_date(2013, 7, 15);
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 150eb54c87..031b22bc3c 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -50,10 +50,6 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
 INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-INSERT INTO TIMESTAMP_TBL VALUES ('current');
 
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index c3bd46c233..28c76d6b72 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -49,10 +49,6 @@ CREATE TABLE TIMESTAMPTZ_TBL (d1 timestamp(2) with time zone);
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
 
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
-- 
2.22.0

0002-Don-t-call-data-type-input-functions-in-GUC-check-ho.patchtext/plain; charset=UTF-8; name=0002-Don-t-call-data-type-input-functions-in-GUC-check-ho.patch; x-mac-creator=0; x-mac-type=0Download

From dc5011a7a798b1dfc5208bd41e83f002e708c7d1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Jun 2019 11:12:13 +0200
Subject: [PATCH 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.
---
 src/backend/utils/misc/guc.c | 103 ++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a68..3a52b4090b 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"
@@ -11577,15 +11576,11 @@ check_recovery_target_time(char **newval, void **extra, GucSource source)
 {
 	if (strcmp(*newval, "") != 0)
 	{
-		TimestampTz time;
+		TimestampTz timestamp;
 		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,31 +11588,40 @@ 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];
+
+			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;
+		*myextra = timestamp;
 		*extra = (void *) myextra;
 	}
 	return true;
@@ -11668,6 +11672,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 +11681,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;
-- 
2.22.0

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#5)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On Wed, Jun 12, 2019 at 01:16:54PM +0200, Peter Eisentraut wrote:

Right. Here is a patch that addresses this by copying the relevant code
from pg_lsn_in() and timestamptz_in() directly into the check hooks.
It's obviously a bit unfortunate not to be able to share that code,
but it's not actually that much.

+    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;
Speaking about pg_lsn.  We have introduced it to reduce the amount of
duplication when mapping an LSN to text, so I am not much a fan of
this patch which adds again a duplication.  You also lose some error
context as you get the same type of error when parsing the first or
the second part of the LSN.  Couldn't you refactor the whole so as an
error string is present as in GUC_check_errdetail()?  I would just put
a wrapper in pg_lsn.c, like pg_lsn_parse() which returns uint64.

The same remark applies to the timestamp_in portion..
--
Michael

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Michael Paquier (#6)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On 2019-06-13 08:55, Michael Paquier wrote:

Speaking about pg_lsn. We have introduced it to reduce the amount of
duplication when mapping an LSN to text, so I am not much a fan of
this patch which adds again a duplication. You also lose some error
context as you get the same type of error when parsing the first or
the second part of the LSN. Couldn't you refactor the whole so as an
error string is present as in GUC_check_errdetail()?

There isn't really much more detail to be had. pg_lsn_in() just reports
"invalid input syntax for type pg_lsn", and with the current patch the
GUC system would report something like 'invalid value for parameter
"recovery_target_time"'.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Peter Eisentraut (#5)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On 2019-06-12 13:16, Peter Eisentraut wrote:

I haven't figured out the time zone issue yet, but I guess the solution
might involve moving some of the code from check_recovery_target_time()
to assign_recovery_target_time().

I think that won't work either. What we need to do is postpone the
interpretation of the timestamp string until after all the GUC
processing is done. So check_recovery_target_time() would just do some
basic parsing checks, but stores the string. Then when we need the
recovery_target_time_value we do the final parsing. Then we can be sure
that the time zone is all set.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Peter Eisentraut (#8)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

Hi,

On 2019-06-20 15:42:14 +0200, Peter Eisentraut wrote:

On 2019-06-12 13:16, Peter Eisentraut wrote:

I haven't figured out the time zone issue yet, but I guess the solution
might involve moving some of the code from check_recovery_target_time()
to assign_recovery_target_time().

I think that won't work either. What we need to do is postpone the
interpretation of the timestamp string until after all the GUC
processing is done. So check_recovery_target_time() would just do some
basic parsing checks, but stores the string. Then when we need the
recovery_target_time_value we do the final parsing. Then we can be sure
that the time zone is all set.

That sounds right to me.

Greetings,

Andres Freund

#10

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Andres Freund (#9)

2 attachment(s)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On 2019-06-20 18:05, Andres Freund wrote:

Hi,

On 2019-06-20 15:42:14 +0200, Peter Eisentraut wrote:

On 2019-06-12 13:16, Peter Eisentraut wrote:

I haven't figured out the time zone issue yet, but I guess the solution
might involve moving some of the code from check_recovery_target_time()
to assign_recovery_target_time().

I think that won't work either. What we need to do is postpone the
interpretation of the timestamp string until after all the GUC
processing is done. So check_recovery_target_time() would just do some
basic parsing checks, but stores the string. Then when we need the
recovery_target_time_value we do the final parsing. Then we can be sure
that the time zone is all set.

That sounds right to me.

Updated patch for that.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v2-0001-Remove-explicit-error-handling-for-obsolete-date-.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-explicit-error-handling-for-obsolete-date-.patch; x-mac-creator=0; x-mac-type=0Download

From 67099f9487354efd771fd4211b0737e1d7d40dec Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Jun 2019 11:29:53 +0200
Subject: [PATCH v2 1/2] Remove explicit error handling for obsolete date/time
 values

The date/time values 'current', 'invalid', and 'undefined' were
removed a long time ago, but the code still contains explicit error
handling for the transition.  To simplify the code and avoid having to
handle these values everywhere, just remove the recognition of these
tokens altogether now.
---
 src/backend/utils/adt/date.c               |  8 --------
 src/backend/utils/adt/datetime.c           | 20 --------------------
 src/backend/utils/adt/timestamp.c          | 22 ----------------------
 src/include/utils/datetime.h               |  2 --
 src/interfaces/ecpg/pgtypeslib/dt.h        |  2 --
 src/interfaces/ecpg/pgtypeslib/dt_common.c |  5 -----
 src/interfaces/ecpg/pgtypeslib/timestamp.c |  4 ----
 src/test/regress/expected/date.out         |  3 ---
 src/test/regress/expected/timestamp.out    | 13 -------------
 src/test/regress/expected/timestamptz.out  | 13 -------------
 src/test/regress/sql/date.sql              |  1 -
 src/test/regress/sql/timestamp.sql         |  4 ----
 src/test/regress/sql/timestamptz.sql       |  4 ----
 13 files changed, 101 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1ff3cfea8b..e440a4fedd 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -138,14 +138,6 @@ date_in(PG_FUNCTION_ARGS)
 		case DTK_DATE:
 			break;
 
-		case DTK_CURRENT:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"current\" is no longer supported")));
-
-			GetCurrentDateTime(tm);
-			break;
-
 		case DTK_EPOCH:
 			GetEpochTime(tm);
 			break;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 9def318c57..e9add385ba 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -99,7 +99,6 @@ static const datetkn datetktbl[] = {
 	{"aug", MONTH, 8},
 	{"august", MONTH, 8},
 	{DB_C, ADBC, BC},			/* "bc" for years <= 0 */
-	{DCURRENT, RESERV, DTK_CURRENT},	/* "current" is always now */
 	{"d", UNITS, DTK_DAY},		/* "day of month" for ISO input */
 	{"dec", MONTH, 12},
 	{"december", MONTH, 12},
@@ -113,7 +112,6 @@ static const datetkn datetktbl[] = {
 	{"friday", DOW, 5},
 	{"h", UNITS, DTK_HOUR},		/* "hour" */
 	{LATE, RESERV, DTK_LATE},	/* "infinity" reserved for "late time" */
-	{INVALID, RESERV, DTK_INVALID}, /* "invalid" reserved for bad time */
 	{"isodow", UNITS, DTK_ISODOW},	/* ISO day of week, Sunday == 7 */
 	{"isoyear", UNITS, DTK_ISOYEAR},	/* year in terms of the ISO week date */
 	{"j", UNITS, DTK_JULIAN},
@@ -157,7 +155,6 @@ static const datetkn datetktbl[] = {
 	{"tue", DOW, 2},
 	{"tues", DOW, 2},
 	{"tuesday", DOW, 2},
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"wed", DOW, 3},
 	{"wednesday", DOW, 3},
 	{"weds", DOW, 3},
@@ -191,7 +188,6 @@ static const datetkn deltatktbl[] = {
 	{"hours", UNITS, DTK_HOUR}, /* "hours" relative */
 	{"hr", UNITS, DTK_HOUR},	/* "hour" relative */
 	{"hrs", UNITS, DTK_HOUR},	/* "hours" relative */
-	{INVALID, RESERV, DTK_INVALID}, /* reserved for invalid time */
 	{"m", UNITS, DTK_MINUTE},	/* "minute" relative */
 	{"microsecon", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"mil", UNITS, DTK_MILLENNIUM}, /* "millennium" relative */
@@ -222,7 +218,6 @@ static const datetkn deltatktbl[] = {
 	{DTIMEZONE, UNITS, DTK_TZ}, /* "timezone" time offset */
 	{"timezone_h", UNITS, DTK_TZ_HOUR}, /* timezone hour units */
 	{"timezone_m", UNITS, DTK_TZ_MINUTE},	/* timezone minutes units */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"us", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"usec", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{DMICROSEC, UNITS, DTK_MICROSEC},	/* "microsecond" relative */
@@ -1186,14 +1181,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
 					case RESERV:
 						switch (val)
 						{
-							case DTK_CURRENT:
-								ereport(ERROR,
-										(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-										 errmsg("date/time value \"current\" is no longer supported")));
-
-								return DTERR_BAD_FORMAT;
-								break;
-
 							case DTK_NOW:
 								tmask = (DTK_DATE_M | DTK_TIME_M | DTK_M(TZ));
 								*dtype = DTK_DATE;
@@ -2097,13 +2084,6 @@ DecodeTimeOnly(char **field, int *ftype, int nf,
 					case RESERV:
 						switch (val)
 						{
-							case DTK_CURRENT:
-								ereport(ERROR,
-										(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-										 errmsg("date/time value \"current\" is no longer supported")));
-								return DTERR_BAD_FORMAT;
-								break;
-
 							case DTK_NOW:
 								tmask = DTK_TIME_M;
 								*dtype = DTK_TIME;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index e5ac371fa0..8d129731a6 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -188,14 +188,6 @@ timestamp_in(PG_FUNCTION_ARGS)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-
-			TIMESTAMP_NOEND(result);
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing timestamp \"%s\"",
 				 dtype, str);
@@ -439,14 +431,6 @@ timestamptz_in(PG_FUNCTION_ARGS)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-
-			TIMESTAMP_NOEND(result);
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing timestamptz \"%s\"",
 				 dtype, str);
@@ -946,12 +930,6 @@ interval_in(PG_FUNCTION_ARGS)
 						 errmsg("interval out of range")));
 			break;
 
-		case DTK_INVALID:
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("date/time value \"%s\" is no longer supported", str)));
-			break;
-
 		default:
 			elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
 				 dtype, str);
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index 4de78ebe36..b8a199cdde 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -148,8 +148,6 @@ struct tzEntry;
 #define DTK_AGO			5
 
 #define DTK_SPECIAL		6
-#define DTK_INVALID		7
-#define DTK_CURRENT		8
 #define DTK_EARLY		9
 #define DTK_LATE		10
 #define DTK_EPOCH		11
diff --git a/src/interfaces/ecpg/pgtypeslib/dt.h b/src/interfaces/ecpg/pgtypeslib/dt.h
index a9884f278c..c5fd6bdaed 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt.h
+++ b/src/interfaces/ecpg/pgtypeslib/dt.h
@@ -143,8 +143,6 @@ typedef int32 fsec_t;
 #define DTK_AGO			5
 
 #define DTK_SPECIAL		6
-#define DTK_INVALID		7
-#define DTK_CURRENT		8
 #define DTK_EARLY		9
 #define DTK_LATE		10
 #define DTK_EPOCH		11
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index f5aed794fd..24fff28f0a 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -111,7 +111,6 @@ static const datetkn datetktbl[] = {
 #endif
 	{"cot", TZ, -18000},		/* Columbia Time */
 	{"cst", TZ, -21600},		/* Central Standard Time */
-	{DCURRENT, RESERV, DTK_CURRENT},	/* "current" is always now */
 #if 0
 	cvst
 #endif
@@ -201,7 +200,6 @@ static const datetkn datetktbl[] = {
 	idt							/* Israeli, Iran, Indian Daylight Time */
 #endif
 	{LATE, RESERV, DTK_LATE},	/* "infinity" reserved for "late time" */
-	{INVALID, RESERV, DTK_INVALID}, /* "invalid" reserved for bad time */
 	{"iot", TZ, 18000},			/* Indian Chagos Time */
 	{"irkst", DTZ, 32400},		/* Irkutsk Summer Time */
 	{"irkt", TZ, 28800},		/* Irkutsk Time */
@@ -372,7 +370,6 @@ static const datetkn datetktbl[] = {
 #endif
 	{"ulast", DTZ, 32400},		/* Ulan Bator Summer Time */
 	{"ulat", TZ, 28800},		/* Ulan Bator Time */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"ut", TZ, 0},
 	{"utc", TZ, 0},
 	{"uyst", DTZ, -7200},		/* Uruguay Summer Time */
@@ -440,7 +437,6 @@ static const datetkn deltatktbl[] = {
 	{"hours", UNITS, DTK_HOUR}, /* "hours" relative */
 	{"hr", UNITS, DTK_HOUR},	/* "hour" relative */
 	{"hrs", UNITS, DTK_HOUR},	/* "hours" relative */
-	{INVALID, RESERV, DTK_INVALID}, /* reserved for invalid time */
 	{"m", UNITS, DTK_MINUTE},	/* "minute" relative */
 	{"microsecon", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"mil", UNITS, DTK_MILLENNIUM}, /* "millennium" relative */
@@ -471,7 +467,6 @@ static const datetkn deltatktbl[] = {
 	{DTIMEZONE, UNITS, DTK_TZ}, /* "timezone" time offset */
 	{"timezone_h", UNITS, DTK_TZ_HOUR}, /* timezone hour units */
 	{"timezone_m", UNITS, DTK_TZ_MINUTE},	/* timezone minutes units */
-	{"undefined", RESERV, DTK_INVALID}, /* pre-v6.1 invalid time */
 	{"us", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{"usec", UNITS, DTK_MICROSEC},	/* "microsecond" relative */
 	{DMICROSEC, UNITS, DTK_MICROSEC},	/* "microsecond" relative */
diff --git a/src/interfaces/ecpg/pgtypeslib/timestamp.c b/src/interfaces/ecpg/pgtypeslib/timestamp.c
index a7e0fe66f0..efd9f3aa71 100644
--- a/src/interfaces/ecpg/pgtypeslib/timestamp.c
+++ b/src/interfaces/ecpg/pgtypeslib/timestamp.c
@@ -253,10 +253,6 @@ PGTYPEStimestamp_from_asc(char *str, char **endptr)
 			TIMESTAMP_NOBEGIN(result);
 			break;
 
-		case DTK_INVALID:
-			errno = PGTYPES_TS_BAD_TIMESTAMP;
-			return noresult;
-
 		default:
 			errno = PGTYPES_TS_BAD_TIMESTAMP;
 			return noresult;
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index 1bcc9465a9..4686d0d8ca 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1444,9 +1444,6 @@ SELECT EXTRACT(EPOCH      FROM DATE 'infinity');    --  Infinity
 SELECT EXTRACT(MICROSEC  FROM DATE 'infinity');     -- ERROR:  timestamp units "microsec" not recognized
 ERROR:  timestamp units "microsec" not recognized
 CONTEXT:  SQL function "date_part" statement 1
-SELECT EXTRACT(UNDEFINED FROM DATE 'infinity');     -- ERROR:  timestamp units "undefined" not supported
-ERROR:  timestamp units "undefined" not supported
-CONTEXT:  SQL function "date_part" statement 1
 -- test constructors
 select make_date(2013, 7, 15);
  make_date  
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index b2b171f560..715680e330 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -79,19 +79,6 @@ TRUNCATE TIMESTAMP_TBL;
 INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-ERROR:  date/time value "invalid" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-                                          ^
-INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-ERROR:  date/time value "undefined" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-                                          ^
-INSERT INTO TIMESTAMP_TBL VALUES ('current');
-ERROR:  date/time value "current" is no longer supported
-LINE 1: INSERT INTO TIMESTAMP_TBL VALUES ('current');
-                                          ^
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 -- Variations on Postgres v6.1 standard output format
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 8a4c719993..5551fa6610 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -78,19 +78,6 @@ DELETE FROM TIMESTAMPTZ_TBL;
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-ERROR:  date/time value "invalid" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-                                            ^
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-ERROR:  date/time value "undefined" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-                                            ^
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
-ERROR:  date/time value "current" is no longer supported
-LINE 1: INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
-                                            ^
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 -- Variations on Postgres v6.1 standard output format
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 22f80f2ee2..4c5b94a14a 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -334,7 +334,6 @@ CREATE TABLE DATE_TBL (f1 date);
 -- wrong fields from non-finite date:
 --
 SELECT EXTRACT(MICROSEC  FROM DATE 'infinity');     -- ERROR:  timestamp units "microsec" not recognized
-SELECT EXTRACT(UNDEFINED FROM DATE 'infinity');     -- ERROR:  timestamp units "undefined" not supported
 
 -- test constructors
 select make_date(2013, 7, 15);
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 150eb54c87..031b22bc3c 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -50,10 +50,6 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
 INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMP_TBL VALUES ('invalid');
-INSERT INTO TIMESTAMP_TBL VALUES ('undefined');
-INSERT INTO TIMESTAMP_TBL VALUES ('current');
 
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index c3bd46c233..28c76d6b72 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -49,10 +49,6 @@ CREATE TABLE TIMESTAMPTZ_TBL (d1 timestamp(2) with time zone);
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('epoch');
--- Obsolete special values
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('invalid');
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('undefined');
-INSERT INTO TIMESTAMPTZ_TBL VALUES ('current');
 
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMPTZ_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');

base-commit: 1323bfce55109dd54ee164828aab7983d3020a25
-- 
2.22.0

v2-0002-Don-t-call-data-type-input-functions-in-GUC-check.patchtext/plain; charset=UTF-8; name=v2-0002-Don-t-call-data-type-input-functions-in-GUC-check.patch; x-mac-creator=0; x-mac-type=0Download

From fbafedf818c3985cfec12f5f3304d5a461be5901 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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, &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 +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

#11

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#10)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On Sun, Jun 23, 2019 at 07:21:02PM +0200, Peter Eisentraut wrote:

Updated patch for that.

I have been looking at this patch set. Patch 0001 looks good to me.
You are removing all traces of a set of timestamp keywords not
supported anymore, and no objections from my side for this cleanup.

+#define MAXPG_LSNCOMPONENT 8
+
static bool
check_recovery_target_lsn(char **newval, void **extra, GucSource source)
Let's avoid the duplication for the declarations. I would suggest to
move the definitions of MAXPG_LSNLEN and MAXPG_LSNCOMPONENT to
pg_lsn.h. Funny part, I was actually in need of this definition a
couple of days ago for a LSN string in a frontend tool. I would
suggest renames at the same time:
- PG_LSN_LEN
- PG_LSN_COMPONENT

I think that should have a third definition for
"0123456789abcdefABCDEF", say PG_LSN_CHARACTERS, and we could have one
more for the separator '/'.

Avoiding the duplication between pg_lsn.c and guc.c is proving to be
rather ugly and reduces the readability within pg_lsn.c, so please let
me withdraw my previous objection. (Looked at that part.)

- if (strcmp(*newval, "epoch") == 0 ||
- strcmp(*newval, "infinity") == 0 ||
- strcmp(*newval, "-infinity") == 0 ||
Why do you remove these? They should still be rejected because they
make no sense as recovery targets, no?

It may be worth mentioning that AdjustTimestampForTypmod() is not
duplicated because we don't care about the typmod in this case.
--
Michael

#12

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Michael Paquier (#11)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On 2019-06-24 06:06, Michael Paquier wrote:

- if (strcmp(*newval, "epoch") == 0 ||
- strcmp(*newval, "infinity") == 0 ||
- strcmp(*newval, "-infinity") == 0 ||
Why do you remove these? They should still be rejected because they
make no sense as recovery targets, no?

Yeah but the new code already rejects those anyway. Note how
timestamptz_in() has explicit switch cases to accept those, and we
didn't carry those over into check_recovery_time().

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#13

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#12)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On Mon, Jun 24, 2019 at 11:27:26PM +0200, Peter Eisentraut wrote:

Yeah but the new code already rejects those anyway. Note how
timestamptz_in() has explicit switch cases to accept those, and we
didn't carry those over into check_recovery_time().

Ditto. I was not paying much attention to the code. Your patch
indeed rejects anything else than DTK_DATE. So we are good here,
sorry for the noise.
--
Michael

#14

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Michael Paquier (#11)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

This has been committed.

On 2019-06-24 06:06, Michael Paquier wrote:

I have been looking at this patch set. Patch 0001 looks good to me.
You are removing all traces of a set of timestamp keywords not
supported anymore, and no objections from my side for this cleanup.

+#define MAXPG_LSNCOMPONENT 8
+
static bool
check_recovery_target_lsn(char **newval, void **extra, GucSource source)
Let's avoid the duplication for the declarations. I would suggest to
move the definitions of MAXPG_LSNLEN and MAXPG_LSNCOMPONENT to
pg_lsn.h. Funny part, I was actually in need of this definition a
couple of days ago for a LSN string in a frontend tool. I would
suggest renames at the same time:
- PG_LSN_LEN
- PG_LSN_COMPONENT

I ended up rewriting this by extracting the parsing code into
pg_lsn_in_internal() and having both pg_lsn_in() and
check_recovery_target_lsn() calling it. This mirrors similar
arrangements in float.c

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#15

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Peter Eisentraut (#14)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On Sun, Jun 30, 2019 at 11:06:58AM +0200, Peter Eisentraut wrote:

I ended up rewriting this by extracting the parsing code into
pg_lsn_in_internal() and having both pg_lsn_in() and
check_recovery_target_lsn() calling it. This mirrors similar
arrangements in float.c

The refactoring looks good to me (including what you have just fixed
with PG_RETURN_LSN). Thanks for considering it.
--
Michael

#16

Michael Paquier

michael@paquier.xyz

over 6 years ago

In reply to: Michael Paquier (#15)

Re: check_recovery_target_lsn() does a PG_CATCH without a throw

On Sun, Jun 30, 2019 at 09:35:52PM +0900, Michael Paquier wrote:

The refactoring looks good to me (including what you have just fixed
with PG_RETURN_LSN). Thanks for considering it.

This issue was still listed as an open item for v12, so I have removed
it.
--
Michael