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 br...@momjian.ushttp://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 alvhe...@2ndquadrant.com 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 br...@momjian.ushttp://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 =
[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
Re: [HACKERS] interval typmodout is broken
Alvaro Herrera alvhe...@2ndquadrant.com 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
Re: [HACKERS] interval typmodout is broken
Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com 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 alvhe...@2ndquadrant.com 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
