Обсуждение: 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
Вложения
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
On Sat, Mar 4, 2023 at 11:23 AM Keisuke Kuroda <kuroda.keisuke@nttcom.co.jp> 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
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
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
Вложения
Joseph Koshakow <koshy44@gmail.com> 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
On Sat, Mar 4, 2023 at 1:56 PM Tom Lane <tgl@sss.pgh.pa.us> 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.
>
> 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.
Joseph Koshakow <koshy44@gmail.com> writes:
> On Sat, Mar 4, 2023 at 1:56 PM Tom Lane <tgl@sss.pgh.pa.us> 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
On Sat, Mar 4, 2023 at 2:48 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Вложения
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
Joseph Koshakow <koshy44@gmail.com> writes:
> Please see the attached patch with these changes.
Pushed with a couple of cosmetic adjustments.
regards, tom lane