Re: 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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 "("
> LNEA 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

On Thu, Sep 25, 2014 at 12:06:56AM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 "("
> > LNEA 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.9999 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.9999 seconds' HOUR to SECOND;
interval
----------
00:01:41

and in cases where the precision made no sense:

SELECT INTERVAL (2) '100.9999 seconds' HOUR to MINUTE;
interval
----------
00:01:00

I have created the attached patch which only allows parentheses in the
first case.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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.9999 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.9999 seconds' HOUR to SECOND;
> interval
> ----------
> 00:01:41
>
> and in cases where the precision made no sense:
>
> SELECT INTERVAL (2) '100.9999 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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +