Re: [HACKERS] timestamptz parsing bug?
On Wed, Aug 15, 2012 at 05:29:26PM -0400, Bruce Momjian wrote:
>
> I assume we want to apply this patch based on discussion that we should
> allow a wider range of date/time formats.
Applied, thanks.
---
>
> On Mon, Aug 29, 2011 at 06:40:07PM +0100, Dean Rasheed wrote:
> > On 29 August 2011 15:40, Andrew Dunstan wrote:
> > >
> > > Why do we parse this as a correct timestamptz literal:
> > >
> > > 2011-08-29T09:11:14.123 CDT
> > >
> > > but not this:
> > >
> > > 2011-08-29T09:11:14.123 America/Chicago
> > >
> > > Replace the ISO-8601 style T between the date and time parts of the latter
> > > with a space and the parser is happy again.
> > >
> > >
> > > cheers
> > >
> > > andrew
> > >
> >
> > Funny, I've just recently been looking at this code.
> >
> > I think that the issue is in the DTK_TIME handling code in DecodeDateTime().
> >
> > For this input string the "T" is recognised as the start of an ISO
> > time, and the ptype variable is set to DTK_TIME. The next field is a
> > DTK_TIME, however, when it is handled it doesn't reset the ptype
> > variable.
> >
> > When it gets to the timezone "America/Chicago" at the end, this is
> > handled in the DTK_DATE case, because of the "/". But because ptype is
> > still set, it is expecting this to be an ISO time, so it errors out.
> >
> > The attached patch seems to fix it. Could probably use a new
> > regression test though.
> >
> > Regards,
> > Dean
>
> > diff --git a/src/backend/utils/adt/datetime.c
> > b/src/backend/utils/adt/datetime.c
> > new file mode 100644
> > index 3d320cc..a935d98
> > *** a/src/backend/utils/adt/datetime.c
> > --- b/src/backend/utils/adt/datetime.c
> > *** DecodeDateTime(char **field, int *ftype,
> > *** 942,947
> > --- 942,957
> > break;
> >
> > case DTK_TIME:
> > + /*
> > +* This might be an ISO time following a "t"
> > field.
> > +*/
> > + if (ptype != 0)
> > + {
> > + /* Sanity check; should not fail this
> > test */
> > + if (ptype != DTK_TIME)
> > + return DTERR_BAD_FORMAT;
> > + ptype = 0;
> > + }
> > dterr = DecodeTime(field[i], fmask,
> > INTERVAL_FULL_RANGE,
> >&tmask, tm,
> > fsec);
> > if (dterr)
>
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
>
>
> --
> Bruce Momjian http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
I assume we want to apply this patch based on discussion that we should
allow a wider range of date/time formats.
---
On Mon, Aug 29, 2011 at 06:40:07PM +0100, Dean Rasheed wrote:
> On 29 August 2011 15:40, Andrew Dunstan wrote:
> >
> > Why do we parse this as a correct timestamptz literal:
> >
> > 2011-08-29T09:11:14.123 CDT
> >
> > but not this:
> >
> > 2011-08-29T09:11:14.123 America/Chicago
> >
> > Replace the ISO-8601 style T between the date and time parts of the latter
> > with a space and the parser is happy again.
> >
> >
> > cheers
> >
> > andrew
> >
>
> Funny, I've just recently been looking at this code.
>
> I think that the issue is in the DTK_TIME handling code in DecodeDateTime().
>
> For this input string the "T" is recognised as the start of an ISO
> time, and the ptype variable is set to DTK_TIME. The next field is a
> DTK_TIME, however, when it is handled it doesn't reset the ptype
> variable.
>
> When it gets to the timezone "America/Chicago" at the end, this is
> handled in the DTK_DATE case, because of the "/". But because ptype is
> still set, it is expecting this to be an ISO time, so it errors out.
>
> The attached patch seems to fix it. Could probably use a new
> regression test though.
>
> Regards,
> Dean
> diff --git a/src/backend/utils/adt/datetime.c
> b/src/backend/utils/adt/datetime.c
> new file mode 100644
> index 3d320cc..a935d98
> *** a/src/backend/utils/adt/datetime.c
> --- b/src/backend/utils/adt/datetime.c
> *** DecodeDateTime(char **field, int *ftype,
> *** 942,947
> --- 942,957
> break;
>
> case DTK_TIME:
> + /*
> + * This might be an ISO time following a "t"
> field.
> + */
> + if (ptype != 0)
> + {
> + /* Sanity check; should not fail this
> test */
> + if (ptype != DTK_TIME)
> + return DTERR_BAD_FORMAT;
> + ptype = 0;
> + }
> dterr = DecodeTime(field[i], fmask,
> INTERVAL_FULL_RANGE,
> &tmask, tm,
> fsec);
> if (dterr)
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
On 29 August 2011 20:43, Andrew Dunstan wrote: > > > On 08/29/2011 03:35 PM, David E. Wheeler wrote: >> >> On Aug 29, 2011, at 12:30 PM, Tom Lane wrote: >> When it gets to the timezone "America/Chicago" at the end, this is handled in the DTK_DATE case, because of the "/". But because ptype is still set, it is expecting this to be an ISO time, so it errors out. >>> >>> Do we actually *want* to support this? The "T" is supposed to mean that >>> the string is strictly ISO-conformant, no? >> >> I didn't realize that appending a time zone was not conformant, but >> apparently it's not. >> >> http://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators >> >> Only appending a "Z" or an offset seems to be legal. Interesting. >> >> > > In that case we shouldn't be accepting an abbreviation either. > The remit of the function is to support inputs in "almost any reasonable format", not just ISO format. I'd say that supporting both extensions of the ISO format is reasonable. Supporting one and not the other is inconsistent, and removing support for one will likely break someone's code. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
Andrew Dunstan writes: > On 08/29/2011 03:35 PM, David E. Wheeler wrote: >> On Aug 29, 2011, at 12:30 PM, Tom Lane wrote: >>> Do we actually *want* to support this? The "T" is supposed to mean that >>> the string is strictly ISO-conformant, no? > In that case we shouldn't be accepting an abbreviation either. Yeah, that would be the logical conclusion. OTOH you could argue that we don't want to remove the abbreviation case for backward-compatibility reasons, in which case allowing full names as well is a reasonable thing. I don't know the answer, I'm just asking the question. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
On 08/29/2011 03:35 PM, David E. Wheeler wrote: On Aug 29, 2011, at 12:30 PM, Tom Lane wrote: When it gets to the timezone "America/Chicago" at the end, this is handled in the DTK_DATE case, because of the "/". But because ptype is still set, it is expecting this to be an ISO time, so it errors out. Do we actually *want* to support this? The "T" is supposed to mean that the string is strictly ISO-conformant, no? I didn't realize that appending a time zone was not conformant, but apparently it's not. http://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators Only appending a "Z" or an offset seems to be legal. Interesting. In that case we shouldn't be accepting an abbreviation either. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
On Aug 29, 2011, at 12:30 PM, Tom Lane wrote: >> When it gets to the timezone "America/Chicago" at the end, this is >> handled in the DTK_DATE case, because of the "/". But because ptype is >> still set, it is expecting this to be an ISO time, so it errors out. > > Do we actually *want* to support this? The "T" is supposed to mean that > the string is strictly ISO-conformant, no? I didn't realize that appending a time zone was not conformant, but apparently it's not. http://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators Only appending a "Z" or an offset seems to be legal. Interesting. David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
Dean Rasheed writes: > On 29 August 2011 15:40, Andrew Dunstan wrote: >> Why do we parse this as a correct timestamptz literal: >>2011-08-29T09:11:14.123 CDT >> but not this: >>2011-08-29T09:11:14.123 America/Chicago > For this input string the "T" is recognised as the start of an ISO > time, and the ptype variable is set to DTK_TIME. The next field is a > DTK_TIME, however, when it is handled it doesn't reset the ptype > variable. > When it gets to the timezone "America/Chicago" at the end, this is > handled in the DTK_DATE case, because of the "/". But because ptype is > still set, it is expecting this to be an ISO time, so it errors out. Do we actually *want* to support this? The "T" is supposed to mean that the string is strictly ISO-conformant, no? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] timestamptz parsing bug?
On 29 August 2011 15:40, Andrew Dunstan wrote: > > Why do we parse this as a correct timestamptz literal: > > 2011-08-29T09:11:14.123 CDT > > but not this: > > 2011-08-29T09:11:14.123 America/Chicago > > Replace the ISO-8601 style T between the date and time parts of the latter > with a space and the parser is happy again. > > > cheers > > andrew > Funny, I've just recently been looking at this code. I think that the issue is in the DTK_TIME handling code in DecodeDateTime(). For this input string the "T" is recognised as the start of an ISO time, and the ptype variable is set to DTK_TIME. The next field is a DTK_TIME, however, when it is handled it doesn't reset the ptype variable. When it gets to the timezone "America/Chicago" at the end, this is handled in the DTK_DATE case, because of the "/". But because ptype is still set, it is expecting this to be an ISO time, so it errors out. The attached patch seems to fix it. Could probably use a new regression test though. Regards, Dean datetime.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] timestamptz parsing bug?
Why do we parse this as a correct timestamptz literal: 2011-08-29T09:11:14.123 CDT but not this: 2011-08-29T09:11:14.123 America/Chicago Replace the ISO-8601 style T between the date and time parts of the latter with a space and the parser is happy again. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
