Patch: AdjustIntervalForTypmod shouldn't discard high-order data

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Cc: Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com>, Sam Mason <sam(at)samason(dot)me(dot)uk>
Subject: Patch: AdjustIntervalForTypmod shouldn't discard high-order data
Date: 2009-05-31 22:32:53
Message-ID: 16963.1243809173@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

As I mentioned a bit ago
http://archives.postgresql.org/pgsql-hackers/2009-05/msg01505.php
there seems to be a definite problem still remaining with our handling
of interval literals. To wit, this behavior is absolutely not per spec:

regression=# select '999'::interval second;
interval
----------
00:00:39
(1 row)

The correct interpretation of the input value is certainly 999 seconds.
The spec would allow us to throw error if it exceeds the available range
of the field, but a silent modulo operation is not per spec and seems
against our general design principle of not silently discarding data.
I propose the attached patch to make the code not throw away high-order
values in this fashion.

A somewhat more debatable case is this:

regression=# select '1 day 1 hour'::interval hour;
interval
----------
01:00:00
(1 row)

which with the attached patch we would render as

regression=# select '1 day 1 hour'::interval hour;
interval
----------------
1 day 01:00:00
(1 row)

There is some case to be made that we should throw error here,
which we could do by putting error tests where the attached patch
has comments suggesting an error test. However I'm inclined to think
that doing that would expose an implementation dependency rather more
than we should. It is usually not clear to novices that '1 day 1 hour'
is different from '25 hours', and it would be even less clear why the
latter would be acceptable input for an INTERVAL HOUR field when the
former isn't. So I'm proposing the patch as-is rather than with the
extra error tests, but am open to being convinced otherwise.

The reason I'm bringing this up now is that we've already changed the
behavior of interval literals quite a bit in 8.4. I would rather try to
finish getting it right in this release than have the behavior change
twice in successive releases.

Comments?

regards, tom lane

Attachment Content-Type Size
unknown_filename text/plain 11.0 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Stark 2009-05-31 22:47:16 Re: search_path improvements
Previous Message Tom Lane 2009-05-31 21:35:33 Re: INTERVAL SECOND limited to 59 seconds?