Re: [HACKERS] timestamptz parsing bug?

2012-08-25 Thread Bruce Momjian
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 and...@dunslane.net 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  br...@momjian.ushttp://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  br...@momjian.ushttp://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?

2012-08-15 Thread Bruce Momjian

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 and...@dunslane.net 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  br...@momjian.ushttp://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?

2011-08-29 Thread Dean Rasheed
On 29 August 2011 15:40, Andrew Dunstan and...@dunslane.net 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


Re: [HACKERS] timestamptz parsing bug?

2011-08-29 Thread Tom Lane
Dean Rasheed dean.a.rash...@gmail.com writes:
 On 29 August 2011 15:40, Andrew Dunstan and...@dunslane.net 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?

2011-08-29 Thread David E. Wheeler
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?

2011-08-29 Thread Andrew Dunstan



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?

2011-08-29 Thread Tom Lane
Andrew Dunstan and...@dunslane.net 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?

2011-08-29 Thread Dean Rasheed
On 29 August 2011 20:43, Andrew Dunstan and...@dunslane.net 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