interval typmodout is broken

Alvaro Herrera <alvherre@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

Tom Lane wrote:

Alvaro Herrera <alvherre@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 Herrera http://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

Alvaro Herrera <alvherre@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

On Thu, Sep 25, 2014 at 12:06:56AM -0400, Tom Lane wrote:

Alvaro Herrera <alvherre@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.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@momjian.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@momjian.us> 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