Re: Date-time extraneous fields with reserved keywords
Joseph Koshakow writes: > Please see the attached patch with these changes. Pushed with a couple of cosmetic adjustments. regards, tom lane
Re: Date-time extraneous fields with reserved keywords
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Thank you for the response and new patch. The scope of impact is limited to 'epoch' and 'infinity'. Also, it is unlikely that these reserved words will be used in combination with time/date, so this patch is appropriate. The new status of this patch is: Ready for Committer
Re: Date-time extraneous fields with reserved keywords
On Sat, Mar 4, 2023 at 2:48 PM Tom Lane wrote:
>
>Right. So really we ought to move the ValidateDate call as
>well as the next half-dozen lines about "mer" down into
>the subsequent "do additional checking" stanza. It's all
>only relevant to normal date specs.
>
>BTW, looking at the set of RESERV tokens in datetktbl[],
>it looks to me like this change renders the final "default:"
>case unreachable, so probably we could just make that an error.
Please see the attached patch with these changes.
- Joe Koshakow
From 64a71ed287aa9611c22eaa6e2cbb7e080d93be79 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sun, 11 Dec 2022 16:08:43 -0500
Subject: [PATCH] Handle extraneous fields in date-time input
DecodeDateTime sometimest allowed extraneous fields to be included with
reserved keywords. For example `date '1995-08-06 epoch'` would be
parsed successfully, but the date was ignored. This commit fixes the
issue so an error is returned instead.
---
src/backend/utils/adt/datetime.c | 35 ++-
src/test/regress/expected/date.out| 33 +
src/test/regress/expected/timestamp.out | 33 +
src/test/regress/expected/timestamptz.out | 33 +
src/test/regress/sql/date.sql | 10 +++
src/test/regress/sql/timestamp.sql| 10 +++
src/test/regress/sql/timestamptz.sql | 10 +++
7 files changed, 150 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 01660637a2..0c1207223c 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -1431,8 +1431,15 @@ DecodeDateTime(char **field, int *ftype, int nf,
*tzp = 0;
break;
- default:
+ case DTK_EPOCH:
+ case DTK_LATE:
+ case DTK_EARLY:
+tmask = (DTK_DATE_M | DTK_TIME_M | DTK_M(TZ));
*dtype = val;
+break;
+
+ default:
+return DTERR_BAD_FORMAT;
}
break;
@@ -1567,22 +1574,22 @@ DecodeDateTime(char **field, int *ftype, int nf,
fmask |= tmask;
} /* end loop over fields */
- /* do final checking/adjustment of Y/M/D fields */
- dterr = ValidateDate(fmask, isjulian, is2digits, bc, tm);
- if (dterr)
- return dterr;
-
- /* handle AM/PM */
- if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
- return DTERR_FIELD_OVERFLOW;
- if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
- tm->tm_hour = 0;
- else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
- tm->tm_hour += HOURS_PER_DAY / 2;
-
/* do additional checking for full date specs... */
if (*dtype == DTK_DATE)
{
+ /* do final checking/adjustment of Y/M/D fields */
+ dterr = ValidateDate(fmask, isjulian, is2digits, bc, tm);
+ if (dterr)
+ return dterr;
+
+ /* handle AM/PM */
+ if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
+ return DTERR_FIELD_OVERFLOW;
+ if (mer == AM && tm->tm_hour == HOURS_PER_DAY / 2)
+ tm->tm_hour = 0;
+ else if (mer == PM && tm->tm_hour != HOURS_PER_DAY / 2)
+ tm->tm_hour += HOURS_PER_DAY / 2;
+
if ((fmask & DTK_DATE_M) != DTK_DATE_M)
{
if ((fmask & DTK_TIME_M) == DTK_TIME_M)
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index f5949f3d17..c874f06546 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1532,3 +1532,36 @@ select make_time(10, 55, 100.1);
ERROR: time field value out of range: 10:55:100.1
select make_time(24, 0, 2.1);
ERROR: time field value out of range: 24:00:2.1
+-- test errors with reserved keywords
+SELECT date '1995-08-06 epoch';
+ERROR: invalid input syntax for type date: "1995-08-06 epoch"
+LINE 1: SELECT date '1995-08-06 epoch';
+^
+SELECT date '1995-08-06 infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 infinity"
+LINE 1: SELECT date '1995-08-06 infinity';
+^
+SELECT date '1995-08-06 -infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 -infinity"
+LINE 1: SELECT date '1995-08-06 -infinity';
+^
+SELECT date 'epoch 1995-08-06';
+ERROR: invalid input syntax for type date: "epoch 1995-08-06"
+LINE 1: SELECT date 'epoch 1995-08-06';
+^
+SELECT date 'infinity 1995-08-06';
+ERROR: invalid input syntax for type date: "infinity 1995-08-06"
+LINE 1: SELECT date 'infinity 1995-08-06';
+^
+SELECT date '-infinity 1995-08-06';
+ERROR: invalid input syntax for type date: "-infinity 1995-08-06"
+LINE 1: SELECT date '-infinity 1995-08-06';
+^
+SELECT date 'now infinity';
+ERROR: invalid input syntax for type date: "now infinity"
+LINE 1: SELECT date 'now infinity';
+^
+SELECT date '-infinity infinity';
+ERROR: invalid input syntax for type date: "-infinity infinity"
+LINE 1: SELECT date '-infinity infinity';
+^
diff --git
Re: Date-time extraneous fields with reserved keywords
Joseph Koshakow writes: > On Sat, Mar 4, 2023 at 1:56 PM Tom Lane wrote: >> Why do you want to skip ValidateDate in some cases? If we've not >> had to do that before, I don't see why it's a good idea now. > This goes back to the abstraction break of > setting tmask without updating tm. Certain > validations will check that if a field is set in > fmask (which is an accumulation of tmask from > every iteration) then it's value in tm is valid. Ah. Another way could be to fill tm with something that would satisfy ValidateDate, but that seems pretty silly. > As far as I can tell dtype always equals DTK_DATE > except when the timestamp/date is 'epoch', > 'infinity', '-infinity', and none of the > validations apply to those date/timestamps. Right. So really we ought to move the ValidateDate call as well as the next half-dozen lines about "mer" down into the subsequent "do additional checking" stanza. It's all only relevant to normal date specs. BTW, looking at the set of RESERV tokens in datetktbl[], it looks to me like this change renders the final "default:" case unreachable, so probably we could just make that an error. regards, tom lane
Re: Date-time extraneous fields with reserved keywords
On Sat, Mar 4, 2023 at 1:56 PM Tom Lane wrote:
>
>I think we should tread very carefully about disallowing inputs that
>have been considered acceptable for 25 years. I agree with disallowing
>numeric fields along with 'epoch' and 'infinity', but for example
>this seems perfectly useful and sensible:
>
># select timestamptz 'today 12:34';
> timestamptz
>
> 2023-03-04 12:34:00-05
>(1 row)
Yeah, that makes sense. I'll leave it as is with
the explicit case for 'epoch', 'infinity', and
'-infinity'.
>Why do you want to skip ValidateDate in some cases? If we've not
>had to do that before, I don't see why it's a good idea now.
This goes back to the abstraction break of
setting tmask without updating tm. Certain
validations will check that if a field is set in
fmask (which is an accumulation of tmask from
every iteration) then it's value in tm is valid.
For example:
if (fmask & DTK_M(YEAR))
{
// ...
else
{
/* there is no year zero in AD/BC notation */
if (tm->tm_year <= 0)
return DTERR_FIELD_OVERFLOW;
}
}
As far as I can tell dtype always equals DTK_DATE
except when the timestamp/date is 'epoch',
'infinity', '-infinity', and none of the
validations apply to those date/timestamps.
Though, I think you're right this is probably
not a good idea. I'll try and brainstorm a
different approach, unless you have some ideas.
Re: Date-time extraneous fields with reserved keywords
Joseph Koshakow writes: > - I'm not sure if we should hard code in those > three specific reserved keywords or set tmask > in the default case. I think we should tread very carefully about disallowing inputs that have been considered acceptable for 25 years. I agree with disallowing numeric fields along with 'epoch' and 'infinity', but for example this seems perfectly useful and sensible: # select timestamptz 'today 12:34'; timestamptz 2023-03-04 12:34:00-05 (1 row) > Any thoughts? Why do you want to skip ValidateDate in some cases? If we've not had to do that before, I don't see why it's a good idea now. regards, tom lane
Re: Date-time extraneous fields with reserved keywords
Attached is the described patch. I have two notes
after implementing it:
- It feels like a bit of an abstraction break to
set tmask without actually setting any fields in
tm.
- I'm not sure if we should hard code in those
three specific reserved keywords or set tmask
in the default case.
Any thoughts?
- Joe Koshakow
From 78d8f39db8df68502369ffd9edd6f6e38f4dadb8 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sun, 11 Dec 2022 16:08:43 -0500
Subject: [PATCH] Handle extraneous fields in date-time input
DecodeDateTime sometimest allowed extraneous fields to be included with
reserved keywords. For example `date '1995-08-06 epoch'` would be
parsed successfully, but the date was ignored. This commit fixes the
issue so an error is returned instead.
---
src/backend/utils/adt/datetime.c | 18 ++---
src/test/regress/expected/date.out| 33 +++
src/test/regress/expected/timestamp.out | 33 +++
src/test/regress/expected/timestamptz.out | 33 +++
src/test/regress/sql/date.sql | 10 +++
src/test/regress/sql/timestamp.sql| 10 +++
src/test/regress/sql/timestamptz.sql | 10 +++
7 files changed, 143 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 01660637a2..6f82465fd1 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -1431,6 +1431,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
*tzp = 0;
break;
+ case DTK_EPOCH:
+ case DTK_LATE:
+ case DTK_EARLY:
+tmask = (DTK_DATE_M | DTK_TIME_M | DTK_M(TZ));
+*dtype = val;
+break;
+
default:
*dtype = val;
}
@@ -1567,10 +1574,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
fmask |= tmask;
} /* end loop over fields */
- /* do final checking/adjustment of Y/M/D fields */
- dterr = ValidateDate(fmask, isjulian, is2digits, bc, tm);
- if (dterr)
- return dterr;
+ if (*dtype == DTK_DATE)
+ {
+ /* do final checking/adjustment of Y/M/D fields */
+ dterr = ValidateDate(fmask, isjulian, is2digits, bc, tm);
+ if (dterr)
+ return dterr;
+ }
/* handle AM/PM */
if (mer != HR24 && tm->tm_hour > HOURS_PER_DAY / 2)
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index f5949f3d17..c874f06546 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1532,3 +1532,36 @@ select make_time(10, 55, 100.1);
ERROR: time field value out of range: 10:55:100.1
select make_time(24, 0, 2.1);
ERROR: time field value out of range: 24:00:2.1
+-- test errors with reserved keywords
+SELECT date '1995-08-06 epoch';
+ERROR: invalid input syntax for type date: "1995-08-06 epoch"
+LINE 1: SELECT date '1995-08-06 epoch';
+^
+SELECT date '1995-08-06 infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 infinity"
+LINE 1: SELECT date '1995-08-06 infinity';
+^
+SELECT date '1995-08-06 -infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 -infinity"
+LINE 1: SELECT date '1995-08-06 -infinity';
+^
+SELECT date 'epoch 1995-08-06';
+ERROR: invalid input syntax for type date: "epoch 1995-08-06"
+LINE 1: SELECT date 'epoch 1995-08-06';
+^
+SELECT date 'infinity 1995-08-06';
+ERROR: invalid input syntax for type date: "infinity 1995-08-06"
+LINE 1: SELECT date 'infinity 1995-08-06';
+^
+SELECT date '-infinity 1995-08-06';
+ERROR: invalid input syntax for type date: "-infinity 1995-08-06"
+LINE 1: SELECT date '-infinity 1995-08-06';
+^
+SELECT date 'now infinity';
+ERROR: invalid input syntax for type date: "now infinity"
+LINE 1: SELECT date 'now infinity';
+^
+SELECT date '-infinity infinity';
+ERROR: invalid input syntax for type date: "-infinity infinity"
+LINE 1: SELECT date '-infinity infinity';
+^
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index c64bcb7c12..c2159c2cec 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2125,3 +2125,36 @@ select * from generate_series('2020-01-01 00:00'::timestamp,
'2020-01-02 03:00'::timestamp,
'0 hour'::interval);
ERROR: step size cannot equal zero
+-- test errors with reserved keywords
+SELECT timestamp '1995-08-06 01:01:01 epoch';
+ERROR: invalid input syntax for type timestamp: "1995-08-06 01:01:01 epoch"
+LINE 1: SELECT timestamp '1995-08-06 01:01:01 epoch';
+ ^
+SELECT timestamp '1995-08-06 01:01:01 infinity';
+ERROR: invalid input syntax for type timestamp: "1995-08-06 01:01:01 infinity"
+LINE 1: SELECT timestamp '1995-08-06 01:01:01 infinity';
+
Re: Date-time extraneous fields with reserved keywords
On Sat, Mar 4, 2023 at 11:23 AM Keisuke Kuroda wrote: > >Good catch. >Of the reserved words that are special values of type Date/Time, >'now', 'today', 'tomorrow', 'yesterday', and 'allballs', >I get an error even before applying the patch. Thanks for pointing this out. After taking a look at the code, 'now', 'today', 'tomorrow', 'yesterday', and 'allballs' all set the appropriate tmask field which is what causes them to error. case DTK_NOW: tmask = (DTK_DATE_M | DTK_TIME_M | DTK_M(TZ)); case DTK_YESTERDAY: tmask = DTK_DATE_M; case DTK_TODAY: tmask = DTK_DATE_M; case DTK_TOMORROW: tmask = DTK_DATE_M; case DTK_ZULU: tmask = (DTK_TIME_M | DTK_M(TZ)); while 'epoch', 'infinity', and '-infinity' do not set tmask (note the default below handles all of these fields) default: *dtype = val; So I think a better fix here would be to also set tmask for those three reserved keywords. >One thing I noticed is that the following SQL >returns normal results even after applying the patch. > >postgres=# select timestamp 'epoch 01:01:01'; > timestamp >- > 1970-01-01 00:00:00 >(1 row) > >When 'epoch','infinity','-infinity' and time are specified together, >the time specified in the SQL is not included in result. >I think it might be better to assume that this pattern is also an error. >What do you think? I agree this pattern should also be an error. I think that the tmask approach will cause an error for this pattern as well. Thanks, Joe Koshakow
Re: Date-time extraneous fields with reserved keywords
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hi Joseph, Good catch. Of the reserved words that are special values of type Date/Time, 'now', 'today', 'tomorrow', 'yesterday', and 'allballs', I get an error even before applying the patch. One thing I noticed is that the following SQL returns normal results even after applying the patch. postgres=# select timestamp 'epoch 01:01:01'; timestamp - 1970-01-01 00:00:00 (1 row) When 'epoch','infinity','-infinity' and time are specified together, the time specified in the SQL is not included in result. I think it might be better to assume that this pattern is also an error. What do you think? As a side note, reserved words such as 'today', 'tomorrow', and 'yesterday' can be used to specify a time. postgres=# select timestamp 'today 01:01:01'; timestamp - 2023-03-03 01:01:01 (1 row) Best Regards, Keisuke Kuroda NTT Comware The new status of this patch is: Waiting on Author
Date-time extraneous fields with reserved keywords
Hi all,
Attached is a patch to fix another parsing error for date-time types
that allow extraneous fields with certain reserved keywords. For
example both `date '1995-08-06 epoch'` and `date 'today epoch'` were
considered valid dates that both resolve to 1970-01-01.
- Joe Koshakow
From fb4c161afff08b926eea12d8689a148e99cbdb5c Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sun, 11 Dec 2022 16:08:43 -0500
Subject: [PATCH] Handle extraneous fields in date-time input
DecodeDateTime sometimest allowed extraneous fields to be included with
reserved keywords. For example `date '1995-08-06 epoch'` would be
parsed successfully, but the date was ignored. This commit fixes the
issue so an error is returned instead.
---
src/backend/utils/adt/datetime.c| 3 +++
src/test/regress/expected/date.out | 17 +
src/test/regress/expected/timestamp.out | 17 +
src/test/regress/sql/date.sql | 6 ++
src/test/regress/sql/timestamp.sql | 6 ++
5 files changed, 49 insertions(+)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e141a06f4 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -1431,6 +1431,9 @@ DecodeDateTime(char **field, int *ftype, int nf,
break;
default:
+/* only allowed if we haven't already parsed some fields */
+if (fmask)
+ return DTERR_BAD_FORMAT;
*dtype = val;
}
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index f8f83e40e9..50a4a52d8c 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1526,3 +1526,20 @@ select make_time(10, 55, 100.1);
ERROR: time field value out of range: 10:55:100.1
select make_time(24, 0, 2.1);
ERROR: time field value out of range: 24:00:2.1
+-- test errors with reserved keywords
+SELECT date '1995-08-06 epoch';
+ERROR: invalid input syntax for type date: "1995-08-06 epoch"
+LINE 1: SELECT date '1995-08-06 epoch';
+^
+SELECT date '1995-08-06 infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 infinity"
+LINE 1: SELECT date '1995-08-06 infinity';
+^
+SELECT date '1995-08-06 -infinity';
+ERROR: invalid input syntax for type date: "1995-08-06 -infinity"
+LINE 1: SELECT date '1995-08-06 -infinity';
+^
+SELECT date 'now infinity';
+ERROR: invalid input syntax for type date: "now infinity"
+LINE 1: SELECT date 'now infinity';
+^
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index be66274738..f68ecd19ea 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2110,3 +2110,20 @@ select * from generate_series('2020-01-01 00:00'::timestamp,
'2020-01-02 03:00'::timestamp,
'0 hour'::interval);
ERROR: step size cannot equal zero
+-- test errors with reserved keywords
+SELECT timestamp '1995-08-06 01:01:01 epoch';
+ERROR: invalid input syntax for type timestamp: "1995-08-06 01:01:01 epoch"
+LINE 1: SELECT timestamp '1995-08-06 01:01:01 epoch';
+ ^
+SELECT timestamp '1995-08-06 01:01:01 infinity';
+ERROR: invalid input syntax for type timestamp: "1995-08-06 01:01:01 infinity"
+LINE 1: SELECT timestamp '1995-08-06 01:01:01 infinity';
+ ^
+SELECT timestamp '1995-08-06 01:01:01 -infinity';
+ERROR: invalid input syntax for type timestamp: "1995-08-06 01:01:01 -infinity"
+LINE 1: SELECT timestamp '1995-08-06 01:01:01 -infinity';
+ ^
+SELECT timestamp 'today epoch';
+ERROR: invalid input syntax for type timestamp: "today epoch"
+LINE 1: SELECT timestamp 'today epoch';
+ ^
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 9fd15be5f9..82da992e3a 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -371,3 +371,9 @@ select make_date(2013, 13, 1);
select make_date(2013, 11, -1);
select make_time(10, 55, 100.1);
select make_time(24, 0, 2.1);
+
+-- test errors with reserved keywords
+SELECT date '1995-08-06 epoch';
+SELECT date '1995-08-06 infinity';
+SELECT date '1995-08-06 -infinity';
+SELECT date 'now infinity';
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index e1175b12ce..f7e3fe1270 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -391,3 +391,9 @@ select generate_series('2022-01-01 00:00'::timestamp,
select * from generate_series('2020-01-01 00:00'::timestamp,
'2020-01-02 03:00'::timestamp,
'0 hour'::interval);
+
+-- test errors with reserved keywords
+SELECT timestamp '1995-08-06 01:01:01 epoch';
+SELECT timestamp '1995-08-06 01:01:01 infinity';
