Re: [HACKERS] interval typmodout is broken
On Mon, Oct 13, 2014 at 07:38:39PM -0400, Bruce Momjian wrote: > I think the basic problem is that the original author had the idea of > doing: > > SELECT INTERVAL (2) '100. seconds'; >interval > -- >00:01:41 > > and using (2) in that location as a short-hand when the interval > precision units were not specified, which seems logical. However, they > allowed it even when the units were specified: > > SELECT INTERVAL (2) '100. seconds' HOUR to SECOND; >interval > -- >00:01:41 > > and in cases where the precision made no sense: > > SELECT INTERVAL (2) '100. seconds' HOUR to MINUTE; >interval > -- >00:01:00 > > I have created the attached patch which only allows parentheses in the > first case. Patch applied. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] interval typmodout is broken
On Thu, Sep 25, 2014 at 12:06:56AM -0400, Tom Lane wrote: > Alvaro Herrera writes: > > Tom Lane wrote: > >> You sure about that? The grammar for INTERVAL is weird. > > > Well, I tested what is taken on input, and yes I agree the grammar is > > weird (but not more weird than timestamp/timestamptz, mind). The input > > function only accepts the precision just after the INTERVAL keyword, not > > after the fieldstr: > > > alvherre=# create table str (a interval(2) hour to minute); > > CREATE TABLE > > > alvherre=# create table str2 (a interval hour to minute(2)); > > ERROR: syntax error at or near "(" > > L�NEA 1: create table str2 (a interval hour to minute(2)); > > ^ > > No, that's not about where it is, it's about what the field is: only > "second" can have a precision. Our grammar is actually allowing stuff > here that it shouldn't. According to the SQL spec, you could write > interval hour(2) to minute > but this involves a "leading field precision", which we do not support > and should definitely not be conflating with trailing-field precision. > Or you could write > interval hour to second(2) > which is valid and we support it. You can *not* write > interval hour to minute(2) > either per spec or per our implementation; and > interval(2) hour to minute > is 100% invalid per spec, even though our grammar goes out of its > way to accept it. > > In short, the typmodout function is doing what it ought to. It's the > grammar that's broken. It looks to me like Tom Lockhart coded the > grammar to accept a bunch of cases that he never got round to actually > implementing reasonably. In particular, per SQL spec these are > completely different animals: > interval hour(2) to second > interval hour to second(2) > but our grammar transforms them into the same thing. > > We ought to fix that... I did not find any cases where we support 'INTERVAL HOUR(2) to SECOND'. I think the basic problem is that the original author had the idea of doing: SELECT INTERVAL (2) '100. seconds'; interval -- 00:01:41 and using (2) in that location as a short-hand when the interval precision units were not specified, which seems logical. However, they allowed it even when the units were specified: SELECT INTERVAL (2) '100. seconds' HOUR to SECOND; interval -- 00:01:41 and in cases where the precision made no sense: SELECT INTERVAL (2) '100. seconds' HOUR to MINUTE; interval -- 00:01:00 I have created the attached patch which only allows parentheses in the first case. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y new file mode 100644 index c98c27a..0de9584 *** a/src/backend/parser/gram.y --- b/src/backend/parser/gram.y *** zone_value: *** 1552,1578 t->typmods = $3; $$ = makeStringConstCast($2, @2, t); } ! | ConstInterval '(' Iconst ')' Sconst opt_interval { TypeName *t = $1; ! if ($6 != NIL) ! { ! A_Const *n = (A_Const *) linitial($6); ! if ((n->val.val.ival & ~(INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE))) != 0) ! ereport(ERROR, ! (errcode(ERRCODE_SYNTAX_ERROR), ! errmsg("time zone interval must be HOUR or HOUR TO MINUTE"), ! parser_errposition(@6))); ! if (list_length($6) != 1) ! ereport(ERROR, ! (errcode(ERRCODE_SYNTAX_ERROR), ! errmsg("interval precision specified twice"), ! parser_errposition(@1))); ! t->typmods = lappend($6, makeIntConst($3, @3)); ! } ! else ! t->typmods = list_make2(makeIntConst(INTERVAL_FULL_RANGE, -1), ! makeIntConst($3, @3)); $$ = makeStringConstCast($5, @5, t); } | NumericOnly { $$ = makeAConst($1, @1); } --- 1552,1562 t->typmods = $3; $$ = makeStringConstCast($2, @2, t); } ! | ConstInterval '(' Iconst ')' Sconst { TypeName *t = $1; ! t->typmods = list_make2(makeIntConst(INTERVAL_FULL_RANGE, -1), ! makeIntConst($3, @3)); $$ = makeStringConstCast($5, @5, t); } | NumericOnly { $$ = makeAConst($1, @1); } *** SimpleTypename: *** 10582,10602 $$ = $1; $$->typmods = $2; } ! | ConstInterval '(' Iconst ')' opt_interval { $$ = $1; ! if ($5 != NIL) ! { ! if (list_length($5) != 1) ! ereport(ERROR, ! (errcode(ERRCODE_SYNTAX_ERROR), ! errmsg("interval precision specified twice"), ! parser_errposition(@1))); ! $$->typmods = lappend($5, makeIntConst($3, @3)); ! } ! else ! $$->typmods = list_
Re: [HACKERS] interval typmodout is broken
Alvaro Herrera writes: > Tom Lane wrote: >> You sure about that? The grammar for INTERVAL is weird. > Well, I tested what is taken on input, and yes I agree the grammar is > weird (but not more weird than timestamp/timestamptz, mind). The input > function only accepts the precision just after the INTERVAL keyword, not > after the fieldstr: > alvherre=# create table str (a interval(2) hour to minute); > CREATE TABLE > alvherre=# create table str2 (a interval hour to minute(2)); > ERROR: syntax error at or near "(" > LÍNEA 1: create table str2 (a interval hour to minute(2)); > ^ No, that's not about where it is, it's about what the field is: only "second" can have a precision. Our grammar is actually allowing stuff here that it shouldn't. According to the SQL spec, you could write interval hour(2) to minute but this involves a "leading field precision", which we do not support and should definitely not be conflating with trailing-field precision. Or you could write interval hour to second(2) which is valid and we support it. You can *not* write interval hour to minute(2) either per spec or per our implementation; and interval(2) hour to minute is 100% invalid per spec, even though our grammar goes out of its way to accept it. In short, the typmodout function is doing what it ought to. It's the grammar that's broken. It looks to me like Tom Lockhart coded the grammar to accept a bunch of cases that he never got round to actually implementing reasonably. In particular, per SQL spec these are completely different animals: interval hour(2) to second interval hour to second(2) but our grammar transforms them into the same thing. We ought to fix that... 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] interval typmodout is broken
Tom Lane wrote: > Alvaro Herrera writes: > > I just noticed when working on DDL deparsing that the typmodout routine > > for intervals is broken. The code uses > > > if (precision != INTERVAL_FULL_PRECISION) > > snprintf(res, 64, "%s(%d)", fieldstr, precision); > > else > > snprintf(res, 64, "%s", fieldstr); > > > which puts the parenthised number after the textual name; but the > > grammar only takes it the other way around. > > You sure about that? The grammar for INTERVAL is weird. I believe > the output we're trying to produce here is something like > > INTERVAL HOUR TO SECOND(2) > > where "fieldstr" would be " HOUR TO SECOND" and "precision" would > give the fractional-second precision. Well, I tested what is taken on input, and yes I agree the grammar is weird (but not more weird than timestamp/timestamptz, mind). The input function only accepts the precision just after the INTERVAL keyword, not after the fieldstr: alvherre=# create table str (a interval(2) hour to minute); CREATE TABLE alvherre=# create table str2 (a interval hour to minute(2)); ERROR: syntax error at or near "(" LÍNEA 1: create table str2 (a interval hour to minute(2)); ^ -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] interval typmodout is broken
Alvaro Herrera writes: > I just noticed when working on DDL deparsing that the typmodout routine > for intervals is broken. The code uses > if (precision != INTERVAL_FULL_PRECISION) > snprintf(res, 64, "%s(%d)", fieldstr, precision); > else > snprintf(res, 64, "%s", fieldstr); > which puts the parenthised number after the textual name; but the > grammar only takes it the other way around. You sure about that? The grammar for INTERVAL is weird. I believe the output we're trying to produce here is something like INTERVAL HOUR TO SECOND(2) where "fieldstr" would be " HOUR TO SECOND" and "precision" would give the fractional-second precision. 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
[HACKERS] interval typmodout is broken
I just noticed when working on DDL deparsing that the typmodout routine for intervals is broken. The code uses if (precision != INTERVAL_FULL_PRECISION) snprintf(res, 64, "%s(%d)", fieldstr, precision); else snprintf(res, 64, "%s", fieldstr); which puts the parenthised number after the textual name; but the grammar only takes it the other way around. This has been wrong since commit 5725b9d9afc8 dated Dec 30 2006, which introduced the whole notion of type-specific typmod output functions. I don't understand how come nobody has noticed this in eight years. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers