Re: Patch for ISO-8601-Interval Input and output.

Over in that TPC-H thread, I was bemoaning once again the never-finished
support for SQL-spec interval literals. I decided to go look at exactly
how unfinished it was, and it turns out that it's actually pretty close.
Hence the attached proposed patch ;-)

The main gating factor is that coerce_type doesn't want to pass typmod
through to the datatype input function when converting a literal
constant. This is necessary for certain types like char and varchar
but loses badly for interval. I have a feeling that we changed that
behavior after Tom Lockhart left the project, which may mean that
interval wasn't quite as broken when he left it as it is today.
Anyway, the attached patch simply hardwires a special case for INTERVAL.
Given that this is reflective of a special case in the standard, and
that there's no very good reason for anyone else to design a datatype
that acts this way, I don't feel too bad about such a hack; but has
anyone got a better idea?

After that it's just a matter of getting DecodeInterval to do the
right things; and it turns out that about half the logic for SQL-spec
input syntax was there already. Almost the only thing I had to change
was the code to decide what a plain integer field at the right end of
the input means.

The patch includes regression test changes that illustrate what it does.
I am not sure about some of the corner cases --- anyone want to see if
their understanding of the spec for <interval string> is different?

There is still some unfinished business if anyone wants to make it
really exactly 100% spec compliant. In particular the spec seems to
allow a minus sign *outside* the string literal, and if I'm reading it
right, a precision spec in combination with field restrictions ought to
look like INTERVAL '...' DAY TO SECOND(3) not INTERVAL(3) '...' DAY TO
SECOND. However, for these you'll get a syntax error instead of
silently wrong answers if you try to use the other syntax, so it's not
quite as pernicious as the matters addressed here.

regards, tom lane

>>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> The patch includes regression test changes that illustrate what it
does.
> I am not sure about some of the corner cases --- anyone want to see
if
> their understanding of the spec for <interval string> is different?

The patch seems to support extensions to the standard.

(1) In the spec, an interval value or literal must be either
year-month or day-time. (I guess they didn't want to try to deal with
the sticky issues of what it means to have an interval of, for
example, seven months and three days -- since an interval has no sense
of which seven months.)

(2) The interval qualifier is not optional in the spec.

(3) It seems to me that they were requiring that there be a
one-to-one match between the fields specified in the interval
qualifier and the fields present in the interval string.

(4) I'm not 100% sure on this one, but it seemed to me that they were
requiring year to be four digits and other components (except for
fractional seconds) to be two digits.

So long as they are documented, there's nothing wrong with extensions.
Nothing I saw suggests that legal interval literals would be
misinterpreted.

> There is still some unfinished business if anyone wants to make it
> really exactly 100% spec compliant. In particular the spec seems to
> allow a minus sign *outside* the string literal,

I agree. They go so far as to point out how it should be interpreted
if the minus is present in both allowed locations (both inside and
outside the quotes):

5) The <sign> in a <signed numeric literal> or an <interval literal> is
a monadic arithmetic operator. The
monadic arithmetic operators + and * specify monadic plus and
monadic minus, respectively. If neither
monadic plus nor monadic minus are specified in a <signed numeric
literal> or an <interval literal>, or if
monadic plus is specified, then the literal is positive. If monadic
minus is specified in a <signed numeric
literal> or <interval literal>, then the literal is negative. If
<sign> is specified in both possible locations in
an <interval literal>, then the sign of the literal is determined by
normal mathematical interpretation of
multiple sign operators.

> and if I'm reading it
> right, a precision spec in combination with field restrictions ought
to
> look like INTERVAL '...' DAY TO SECOND(3) not INTERVAL(3) '...' DAY
TO
> SECOND.

Agreed.

> However, for these you'll get a syntax error instead of
> silently wrong answers if you try to use the other syntax, so it's
not
> quite as pernicious as the matters addressed here.

Agreed.

-Kevin

>>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> (4) I'm not 100% sure on this one, but it seemed to me that they
were
> requiring year to be four digits and other components (except for
> fractional seconds) to be two digits.

That can't be right. Maybe I saw that in datetime literal specs.

Apologies.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I am not sure about some of the corner cases --- anyone want to see if
>> their understanding of the spec for <interval string> is different?

> The patch seems to support extensions to the standard.

Right. All of these were extensions that already existed in PG.

> (3) It seems to me that they were requiring that there be a
> one-to-one match between the fields specified in the interval
> qualifier and the fields present in the interval string.

Yeah. I couldn't actually find any such statement in SQL92, but
the SQL:2008 draft has this at 5.3 rule 30:

30)Let N be the number of <primary datetime field>s in the precision of
the <interval literal>, as specified by <interval qualifier>.

The <interval literal> being defined shall contain N datetime components.

and at rule 34:

34)Within the definition of an <interval literal> that contains a
<year-month literal>, the <interval qualifier> shall not specify DAY,
HOUR, MINUTE, or SECOND. Within the definition of an <interval
literal> that contains a <day-time literal>, the <interval qualifier>
shall not specify YEAR or MONTH.

This seems to be requiring that not only do you give the exact number of
components, but the formatting must match the expectation. So anything
we accept beyond that is gravy. I think that most of the "extension"
cases were already being accepted in some form, and I'd be hesitant
to take them out for fear of breaking existing applications.

>> There is still some unfinished business if anyone wants to make it
>> really exactly 100% spec compliant ...

> I agree.

I committed the patch as presented, and I think I might go take a quick
look at the other two issues. What I suspect I'll find is that the
minus sign issue isn't fixable without turning INTERVAL into a fully
reserved word, which is probably a cure worse than the disease. However
it might be fairly easy to get the grammar to allow the precision in
either place. (We'd want to keep the old way working for backward
compatibility.)

regards, tom lane

> (1) In the spec, an interval value or literal must be either
> year-month or day-time. (I guess they didn't want to try to deal with
> the sticky issues of what it means to have an interval of, for
> example, seven months and three days -- since an interval has no sense
> of which seven months.)

Note that, for usability reasons, Karel some time ago try-partitioned
our intervals: year-month|day-week|hour-min-sec.

--Josh

Tom Lane wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I am not sure about some of the corner cases --- anyone want to see if
>>> their understanding of the spec for <interval string> is different?
>
>> The patch seems to support extensions to the standard.
>
> Right. All of these were extensions that already existed in PG.

Back a while ago (2003) there was some talk about replacing
some of the non-standard extensions with shorthand forms of
intervals with ISO 8601 intervals that have a similar but
not-the-same shorthand.
Interval ISO Postgres
8601 shorthand
-----------------------------------------------------
'1 year 1 minute' 'P1YT1M' '1Y1M'
'1 year 1 month' 'P1Y1M' N/A
http://archives.postgresql.org/pgsql-patches/2003-09/msg00119.php

When I proposed we support the ISO-8601 standard shorthand,
Tom recommended we rip out the old shorthand at the same time:

http://archives.postgresql.org/pgsql-patches/2003-09/msg00134.php

I've been maintaining a patch that supports these ISO-8601
intervals for a client. Back in 2003 I recall Peter said
he wanted to see SQL standard intervals first. There were
also discussions about selecting the output format. My old
patch made this depend on datestyle; but it seems Tom preferred
a separate GUC?

http://archives.postgresql.org/pgsql-patches/2003-12/msg00257.php

I see there's a TODO regarding ISO8601 intervals as well.

I have a version of this patch for 8.2; but would be happy
to bring it up-to-date if people want to re-consider it now.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Back a while ago (2003) there was some talk about replacing
> some of the non-standard extensions with shorthand forms of
> intervals with ISO 8601 intervals that have a similar but
> not-the-same shorthand.

I think *replacement* would be a hard sell, as that would tick off all
the existing users ;-). Now it seems like being able to accept either
the 8601 syntax or the existing syntaxes on input wouldn't be tough
at all, if you insist on the P prefix to distinguish; so that end of
it should be easy enough. On the output side, seems like a GUC variable
is the standard precedent here. I'd still vote against overloading
DateStyle --- it does too much already --- but a separate variable for
interval style wouldn't bother me. In fact, given that we are now
somewhat SQL-compliant on interval input, a GUC that selected
PG traditional, SQL-standard, or ISO 8601 interval output format seems
like it could be a good idea.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Back a while ago (2003) there was some talk about replacing
>> some of the non-standard extensions with shorthand forms of
>> intervals with ISO 8601 intervals that have a similar but
>> not-the-same shorthand.
>
> I think *replacement* would be a hard sell, as that would tick off all
> the existing users ;-). Now it seems like being able to accept either

I originally submitted a patch that supported both, and I think
you suggested replacing on the grounds that the old one was
never documented,

http://archives.postgresql.org/pgsql-patches/2003-09/msg00134.php
"If we're going to support the real ISO spec, I'd suggest ripping
out any not-quite-there variant.

http://archives.postgresql.org/pgsql-patches/2003-09/msg00121.php
"I doubt anyone is using it, because it's completely undocumented."

On the other hand, the company I was at was indeed originally
using it, so I prefer that it stay in as well. Perhaps if
there's a way to mark them as deprecated and post warnings in
the log file if they're used. I think they should be
removed eventually in a few releases, because they're quite
confusing as they stand:

Interval ISO Postgres
8601 shorthand
-----------------------------------------------------
'1 year 1 minute' 'P1YT1M' '1Y1M'
'1 year 1 month' 'P1Y1M' N/A

> the 8601 syntax or the existing syntaxes on input wouldn't be tough
> at all, if you insist on the P prefix to distinguish; so that end of

ISO 8601 seems to me to require the P, so I think we would.

> it should be easy enough. On the output side, seems like a GUC variable
> is the standard precedent here. I'd still vote against overloading
> DateStyle --- it does too much already --- but a separate variable for
> interval style wouldn't bother me. In fact, given that we are now
> somewhat SQL-compliant on interval input, a GUC that selected
> PG traditional, SQL-standard, or ISO 8601 interval output format seems
> like it could be a good idea.

Great. I'm bringing my patch up-to-date with CVS now
and adding the separate GUC.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> I think *replacement* would be a hard sell, as that would tick off all
>> the existing users ;-). Now it seems like being able to accept either

> I originally submitted a patch that supported both, and I think
> you suggested replacing on the grounds that the old one was
> never documented,

Yeah, but that was five years ago, and someone remedied the oversight
since then ...

The other problem is that the SQL spec clearly defines an interval
literal syntax, and it's not this ISO thing. So even without backward
compatibility issues, 8601-only doesn't seem like it would fly.

regards, tom lane

Tom Lane wrote:
>
> The other problem is that the SQL spec clearly defines an interval
> literal syntax, and it's not this ISO thing. So even without backward
> compatibility issues, 8601-only doesn't seem like it would fly.
>

Oh. I wasn't proposing 8601-only. Just the one-character
shorthands like '1Y1M'::interval that postgresql interprets
as "1 year one minute". No standard specifies anything close
to that; and any similar standards ask to interpret that M as
months instead of minutes.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Oh. I wasn't proposing 8601-only. Just the one-character
> shorthands like '1Y1M'::interval that postgresql interprets
> as "1 year one minute". No standard specifies anything close
> to that; and any similar standards ask to interpret that M as
> months instead of minutes.

Hmmm. I would say that the problem with that is not that it's
nonstandard but that it's ambiguous. Our documentation about the
interval type says:

interval values can be written with the following syntax:

[(at)] quantity unit [quantity unit...] [direction]

Where: quantity is a number (possibly signed); unit is microsecond,
millisecond, second, minute, hour, day, week, month, year, decade,
century, millennium, or abbreviations or plurals of these units;
direction can be ago or empty. The at sign (@) is optional noise. The
amounts of different units are implicitly added up with appropriate
sign accounting. ago negates all the fields.

There isn't anything there that would suggest to a user that 'm' is
well-defined as a unit, much less that it specifically means "minute"
rather than one of the other options. What if we just tweak the code to
reject ambiguous abbreviations?

[ experiments a bit... ] Another interesting point is that "mo",
which is a perfectly unique abbreviation, is rejected. Seems like
the handling of abbreviations in this code could be improved.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> '1Y1M'::interval ... minute ... month
> Hmmm. I would say that the problem with that is not that it's
> nonstandard but that it's ambiguous.

Ah yes.

> Our documentation...says..."or abbreviations".
>...What if we just tweak the code to
> reject ambiguous abbreviations?

Good idea. I'll try that.

> [ experiments a bit... ] Another interesting point is that "mo",
> which is a perfectly unique abbreviation, is rejected. Seems like
> the handling of abbreviations in this code could be improved.

It looks like rather than abbreviations being any shorter
form of a unit, it has an explicit list of abbreviations
it likes (deltatktbl) in the beginning of datetime.c that
forces "m" to "minute"? So losing the ambiguous ones
should be very easy.

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> ... ISO 8601 intervals ...
>
> On the output side, seems like a GUC variable
> is the standard precedent here. I'd still vote against overloading
> DateStyle --- it does too much already --- but a separate variable for
> interval style wouldn't bother me. In fact, given that we are now
> somewhat SQL-compliant on interval input, a GUC that selected
> PG traditional, SQL-standard, or ISO 8601 interval output format seems
> like it could be a good idea.

Is it OK that this seems to me it wouldn't be backward compatible
with the current interval_out that looks to me to be using
the DateStyle GUC?

I supposed it could be made backward compatible if the new
IntervalStyle GUC defaulted to a value of "guess_from_datestyle",
but I fear an option like that might add rather than remove
confusion.

Tom Lane wrote:
> somewhat SQL-compliant on interval input, a GUC that selected
> PG traditional, SQL-standard, or ISO 8601 interval output format seems
> like it could be a good idea.

Trying to do the SQL-standard output now, and have a question
of what to do in the SQL-standard mode when trying to output
an interval that as both a YEAR and a DAY component.

AFAICT the SQL standard doesn't let you have both, so the
"SQL-standard" output actually won't be.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Trying to do the SQL-standard output now, and have a question
> of what to do in the SQL-standard mode when trying to output
> an interval that as both a YEAR and a DAY component.

> AFAICT the SQL standard doesn't let you have both, so the
> "SQL-standard" output actually won't be.

The reason it's not SQL-standard is the data value isn't.
So not a problem. Someone conforming to the spec limits on
what he puts in will see spec-compliant output. I think all
you need is 'yyy-mm dd hh:mm:ss' where you omit yyy-mm if
zeroes, omit dd if zero, omit hh:mm:ss if zeroes (but maybe
only if dd is also 0? otherwise your output is just dd which
is uncomfortably ambiguous).

regards, tom lane

Tom Lane wrote:
> The reason it's not SQL-standard is the data value isn't.
> So not a problem. Someone conforming to the spec limits on
> what he puts in will see spec-compliant output. I think all
> you need is 'yyy-mm dd hh:mm:ss' where you omit yyy-mm if
> zeroes, omit dd if zero, omit hh:mm:ss if zeroes (but maybe
> only if dd is also 0? otherwise your output is just dd which
> is uncomfortably ambiguous).

Great. That's what I'll do.

Any convention or preference on the naming of the GUC?
I assume "intervalstyle" is reasonable?

Or thoughts regarding the current EncodeInterval() that's
already using the "datestyle" GUC?

pg82=# select interval '1';
interval
----------
00:00:01
(1 row)

pg82=# set datestyle='sql';
SET

pg82=# select interval '1';
interval
----------
@ 1 sec
(1 row)

Ron Mayer wrote:
> Tom Lane wrote:
>> you need is 'yyy-mm dd hh:mm:ss' where you omit yyy-mm if
>> zeroes, omit dd if zero, omit hh:mm:ss if zeroes (but maybe
>> only if dd is also 0? otherwise your output is just dd which
>> is uncomfortably ambiguous).

Oh, and if both parts are 0, I guess we desire
the (more comfortable than the alternatives) '0'?

Tom Lane wrote:
> I think all
> you need is 'yyy-mm dd hh:mm:ss' where you omit yyy-mm if
> zeroes, omit dd if zero, omit hh:mm:ss if zeroes (but maybe
> only if dd is also 0? otherwise your output is just dd which
> is uncomfortably ambiguous).

Cool. I think I have it pretty much working with a new
GUC "intervalstyle" that can take values of

"sql_standard" that I think will output SQL standard
interval literals when given a sql
standard interval.

"iso_8601" that will output ISO 8601 "Time Intervals" of
the "format with time-unit deignators", and

"backward_compatible" that will output the same thing
that postgres currently does that depends
on the value of the DateStyle GUC.

I'll add the documentation and regression tests and
can submit a patch early next week. Oh. One more
question is that under ecpg there seems to be a fair
amount of near-duplicated code (EncodeDateTime,
EncodeInterval) for turning dates and times and
intervals to strings.

Should those ECPG functions be made identical to
the ones in the backend?
Could those somehow share code with the backend for
some of their work?

Anyway - here's a quick test of the
SQL Standard and ISO interval output as it stands
right now...

regression=# drop table test_intervals;
DROP TABLE
regression=# create temporary table test_intervals (i interval);
CREATE TABLE
regression=# insert into test_intervals values
regression-# ('0 years'),
regression-# ('1 year 1 month'),
regression-# ('1 day 2 hours 3 minutes 4 seconds'),
regression-# ('1 year 1 minute');
INSERT 0 4
regression=#
regression=# insert into test_intervals values
regression-# ('1-1'),
regression-# ('1'),
regression-# (interval '1' year),
regression-# ('1:00:00'),
regression-# ('1 1:02:03');
INSERT 0 5
regression=#
regression=# insert into test_intervals values
regression-# ('P1Y1M'),
regression-# ('P1DT1H1M1S'),
regression-# ('PT1S');
INSERT 0 3
regression=#
regression=# set intervalstyle to sql_standard;
SET
regression=# select * from test_intervals;
i
-------------
0
1-1
1 2:3:4
1-0 0 0:1:0
1-1
0:0:1
1-0
1:0:0
1 1:2:3
1-1
1 1:1:1
0:0:1
(12 rows)

regression=#
regression=# set intervalstyle to iso_8601;
SET
regression=# select * from test_intervals;
i
------------
PT0S
P1Y1M
P1DT2H3M4S
P1YT1M
P1Y1M
PT1S
P1Y
PT1H
P1DT1H2M3S
P1Y1M
P1DT1H1M1S
PT1S
(12 rows)

regression=#
regression=# set intervalstyle to backward_compatible;
SET
regression=# set datestyle to sql;
SET
regression=# select * from test_intervals;
i
-------------------------------
@ 0
@ 1 year 1 mon
@ 1 day 2 hours 3 mins 4 secs
@ 1 year 1 min
@ 1 year 1 mon
@ 1 sec
@ 1 year
@ 1 hour
@ 1 day 1 hour 2 mins 3 secs
@ 1 year 1 mon
@ 1 day 1 hour 1 min 1 sec
@ 1 sec
(12 rows)

regression=# set datestyle to iso;
SET
regression=# select * from test_intervals;
i
-----------------
00:00:00
1 year 1 mon
1 day 02:03:04
1 year 00:01:00
1 year 1 mon
00:00:01
1 year
01:00:00
1 day 01:02:03
1 year 1 mon
1 day 01:01:01
00:00:01
(12 rows)

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Cool. I think I have it pretty much working with a new
> GUC "intervalstyle" that can take values of

> "sql_standard" that I think will output SQL standard
> interval literals when given a sql
> standard interval.

> "iso_8601" that will output ISO 8601 "Time Intervals" of
> the "format with time-unit deignators", and

> "backward_compatible" that will output the same thing
> that postgres currently does that depends
> on the value of the DateStyle GUC.

Actually, we have never considered that new releases need to preserve
the behavior of postgresql.conf settings. So the above seems
unnecessarily baroque. How about decoupling interval_out's behavior
from DateStyle altogether, and instead providing values of IntervalStyle
that match all the previous behaviors?

> Should those ECPG functions be made identical to
> the ones in the backend?

The ECPG situation is a mess :-(. That code was forked off from the
backend some time ago, and has not been well maintained at all. If you
are brave enough to tackle that mess, more power to you; but I strongly
suggest doing it as an independent patch.

> Could those somehow share code with the backend for
> some of their work?

The palloc and elog dependencies seem to be the hard part.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> interval ... "sql_standard"..."iso_8601"...
>> "backward_compatible" ...depends... on ... DateStyle...
>
> ...How about decoupling interval_out's behavior
> from DateStyle altogether, and instead providing values of IntervalStyle
> that match all the previous behaviors?

Great. That seems much more sane.

Any desired names for the existing interval styles?

Currently we output intervals in these two styles:
'1 year 2 mons 3 days 04:05:06'
when the DateStyle is iso.
and
'@ 1 year 2 mons 3 days 4 hours 5 mins 6 secs'
when the DateStyle is sql or postgres, etc.

I'm not quite sure where those styles came from so
don't know what good names for them might be.

>> Should those ECPG functions be made identical ...
> ...
> The palloc and elog dependencies seem to be the hard part.

Interesting. So EncodeDateTime and EncodeInterval, guessing 400 or
so lines, seem sharable since at first glance they either already do or
easily could make their callers deal with all allocation and logging.

Agreed that it's a independent patch that I'll try separately.

Unless I'm compiling stuff wrong, it seems HEAD is giving me
slightly different output on Intervals than 8.3 in the roundoff
of seconds. 8.3 was rounding to the nearest fraction of a second,
HEAD seems to be truncating.

In the psql output below it shows 8.3.1 outputting "6.70 secs"
while the similar output for head is showing "6.69 secs".

At first glance it seems this might be because HEAD defaults
to USE_INTEGER_DATETIMES, which leads to HAVE_INT64_TIMESTAMP
which leads to
sprintf(cp, "%s%d.%02d secs", is_nonzero ? " " : "",
tm->tm_sec, ((int) sec) / 10000);
in EncodeInterval in datetime.c which doesn't seem to be
doing any rounding.

Am I interpreting this right? If so, shall I submit a patch
that rounds it to hundredths of a second (hundredths seems
hardcoded in the sprintf), or perhaps just silently add that
fix to the EncodeInterval patch I'm doing any for SQL Standard
and ISO intervals?

========================================================================
psql (8.4devel)
Type "help" for help.
regression=# set datestyle to sql;
SET
regression=# select '1 year 2 mons 3 days 04:05:06.699999'::interval;
interval
-------------------------------------------------
@ 1 year 2 mons 3 days 4 hours 5 mins 6.69 secs
(1 row)

========================================================================
Welcome to psql 8.3.1, the PostgreSQL interactive terminal.
...
pg83=# set datestyle to sql;
SET
pg83=# select '1 year 2 mons 3 days 04:05:06.699999'::interval;
interval
-------------------------------------------------
@ 1 year 2 mons 3 days 4 hours 5 mins 6.70 secs
(1 row)

Unless I'm compiling stuff wrong, it seems HEAD is giving me
slightly different output on Intervals than 8.3 in the roundoff
of seconds. 8.3 was rounding to the nearest fraction of a second,
HEAD seems to be truncating.

In the psql output below it shows 8.3.1 outputting "6.70 secs"
while the similar output for head is showing "6.69 secs".

At first glance it seems this might be because HEAD defaults
to USE_INTEGER_DATETIMES, which leads to HAVE_INT64_TIMESTAMP
which leads to
sprintf(cp, "%s%d.%02d secs", is_nonzero ? " " : "",
tm->tm_sec, ((int) sec) / 10000);
in EncodeInterval in datetime.c which doesn't seem to be
doing any rounding.

Am I interpreting this right? If so, shall I submit a patch
that rounds it to hundredths of a second (hundredths seems
hardcoded in the sprintf), or perhaps just silently add that
fix to the EncodeInterval patch I'm doing any for SQL Standard
and ISO intervals?

========================================================================
psql (8.4devel)
Type "help" for help.
regression=# set datestyle to sql;
SET
regression=# select '1 year 2 mons 3 days 04:05:06.699999'::interval;
interval
-------------------------------------------------
@ 1 year 2 mons 3 days 4 hours 5 mins 6.69 secs
(1 row)

========================================================================
Welcome to psql 8.3.1, the PostgreSQL interactive terminal.
...
pg83=# set datestyle to sql;
SET
pg83=# select '1 year 2 mons 3 days 04:05:06.699999'::interval;
interval
-------------------------------------------------
@ 1 year 2 mons 3 days 4 hours 5 mins 6.70 secs
(1 row)

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Unless I'm compiling stuff wrong, it seems HEAD is giving me
> slightly different output on Intervals than 8.3 in the roundoff
> of seconds. 8.3 was rounding to the nearest fraction of a second,
> HEAD seems to be truncating.

Yeah, that's surely because of the change to integer timestamps.

> Am I interpreting this right? If so, shall I submit a patch
> that rounds it to hundredths of a second (hundredths seems
> hardcoded in the sprintf), or perhaps just silently add that
> fix to the EncodeInterval patch I'm doing any for SQL Standard
> and ISO intervals?

This is not the only place where the float-timestamps code has rounding
behavior that doesn't appear in the integer-timestamps code. I don't
know if we want to buy into making them act the same ... after all,
if they acted exactly the same, we'd not have bothered with writing the
integer code. In this example, rounding to hundredths doesn't seem like
a particularly good idea; it seems to me it should give you the exact
down-to-the-microsecond value.

regards, tom lane

>>> On Mon, Sep 15, 2008 at 4:58 PM, in message
<20219(dot)1221515913(at)sss(dot)pgh(dot)pa(dot)us>,
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Unless I'm compiling stuff wrong, it seems HEAD is giving me
>> slightly different output on Intervals than 8.3 in the roundoff
>> of seconds. 8.3 was rounding to the nearest fraction of a second,
>> HEAD seems to be truncating.
>
> Yeah, that's surely because of the change to integer timestamps.
>
>> Am I interpreting this right? If so, shall I submit a patch
>> that rounds it to hundredths of a second (hundredths seems
>> hardcoded in the sprintf), or perhaps just silently add that
>> fix to the EncodeInterval patch I'm doing any for SQL Standard
>> and ISO intervals?
>
> This is not the only place where the float-timestamps code has
rounding
> behavior that doesn't appear in the integer-timestamps code. I
don't
> know if we want to buy into making them act the same ... after all,
> if they acted exactly the same, we'd not have bothered with writing
the
> integer code. In this example, rounding to hundredths doesn't seem
like
> a particularly good idea; it seems to me it should give you the
exact
> down-to-the-microsecond value.

I find the results on 8.3.3 with integer timestamps surprising:

ccdev=# select version();
version
------------------------------------------------------------------------------------------------------------------
PostgreSQL 8.3.3 on x86_64-unknown-linux-gnu, compiled by GCC gcc
(GCC) 4.1.2 20070115 (prerelease) (SUSE Linux)
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval;
interval
--------------------------------------
1 year 2 mons 3 days 04:05:06.699999
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(1);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.70
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(2);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.70
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(3);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.70
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(4);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.70
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(5);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.70
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.699999'::interval(6);
interval
--------------------------------------
1 year 2 mons 3 days 04:05:06.699999
(1 row)

-Kevin

>>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

> I find the results on 8.3.3 with integer timestamps surprising:

Even more surprising is the behavior for interval(1) here:

ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval;
interval
-------------------------------------
1 year 2 mons 3 days 04:05:06.64321
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(1);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.60
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(2);
interval
----------------------------------
1 year 2 mons 3 days 04:05:06.64
(1 row)

ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(3);
interval
-----------------------------------
1 year 2 mons 3 days 04:05:06.643
(1 row)

etc.

That trailing zero should be considered a bug.

-Kevin

Kevin Grittner wrote:
>...not the only place where the float-timestamps code has
> rounding behavior that doesn't appear in the integer-timestamps
> code. ...
> I find the results on 8.3.3 with integer timestamps surprising:

Agreed it's surprising and agreed there are more places.

Sounds like I should keep that separate and perhaps later
submit a separate patch to identify and/or remove surprising
rounding behavior.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Sounds like I should keep that separate and perhaps later
> submit a separate patch to identify and/or remove surprising
> rounding behavior.

Agreed. It's easier to review and get consensus on patches if you
keep separate issues separate.

regards, tom lane

Tom Lane wrote:
>
> This is not the only place where the float-timestamps code has rounding
> behavior that doesn't appear in the integer-timestamps code.

Yeah... For that matter, I find this surprising as well:

regression=# select '0.7 secs'::interval, ('7 secs'::interval/10);
interval | ?column?
-----------------+-------------
00:00:00.699999 | 00:00:00.70
(1 row)

I'll start making a list of them for a future patch down the road.

Tom Lane wrote:
> support for SQL-spec interval literals. I decided to go look at exactly
> how unfinished it was, and it turns out that it's actually pretty close.
> Hence the attached proposed patch ;-)

Is this code handling negative interval literals right?
I think I quote the relevant spec part at the bottom.

If I'm reading the spec right, I find this surprising.

regression=# select interval '-1-1';
interval
-------------------
-1 days -01:00:00

regression=# select interval '1-1';
interval
--------------
1 year 1 mon

Also if I read the spec right, ISTM the <sign> should
apply to the entire interval literal. But if I compiled
the patch right, the negative sign applies only to the
first part it encounters?

regression=# select interval '-1 2:3:4';
interval
-------------------
-1 days +02:03:04
(1 row)

If I understand right, this'll add confusion to
SQL "standard" output for mixed year-month and
day-time intervals that have both negative and
positive components too. :(

This looks to me like the relevant part of SQL 200N(8?),
which seems to me to allow a sign inside the quotes:
====================================================================
<interval literal> ::=
INTERVAL [ <sign> ] <interval string> <interval qualifier>
<interval string> ::=
<quote> <unquoted interval string> <quote>
<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }
====================================================================

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Is this code handling negative interval literals right?
> I think I quote the relevant spec part at the bottom.

We support independent signs for the different components of the
interval. I'm not sure how well the spec copes with that.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Is this code handling negative interval literals right?
>> I think I quote the relevant spec part at the bottom.
>
> We support independent signs for the different components of the

Even so it surprises me that:
'-1-1'::interval gives me a day-hour interval while
'1-1'::interval gives me a year-month interval.

>I'm not sure how well the spec copes with that.

If I'm read the spec right, SQL 2008 expects "-1 12:34:56" to
be what we'd see as "-1 -12:34:56", and doesn't handle with
different signs for different components at all.

SQL 92 seems to have been simpler, apparently requiring
the negative sign to stay outside the quotes.
==SQL 92===========================================================
<interval literal> ::=
INTERVAL [ <sign> ] <interval string> <interval qualifier>
<interval string> ::=
<quote> { <year-month literal> | <day-time literal> } <quote>
===================================================================

SQL 200N seems to allow a sign inside the string:
==SQL 200N=========================================================
<interval literal> ::=
INTERVAL [ <sign> ] <interval string> <interval qualifier>
<interval string> ::=
<quote> <unquoted interval string> <quote>
<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }
===================================================================

Tom Lane wrote:
> ... SQL-spec interval literals. I decided to go look at exactly
> how unfinished it was, and it turns out that it's actually pretty close.
> Hence the attached proposed patch ;-)

Short summary:

I think this patch fixes a bug with sql-spec negative interval literals.

Longer.

I believe this short (4 lines of code & 1 line of comment) patch (below)
fixes the way we handle SQL-standard negative-valued year-month interval
strings.

In particular, if I read the spec right (relevant excerpts below), the
string '-1-1' is a vaild SQL-200N "year-month" interval meaning a
negative year and a month - because the spec only allows a <sign>
in the beginning of the unquoted interval string:
<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }
Current HEAD interprets '-1-1' as "-1 days -1 hours". 8.3 doesn't
recognize it at all.

Assuming I read the spec right, are there any problems with this,
and if not, could I ask that the patch at the end of this email
be applied?

Ron

===============================================================================
== with this patch
===============================================================================

regression=# select interval '-1-1';
interval
------------------
-1 years -1 mons
(1 row)

===============================================================================
== without this patch
===============================================================================

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

===============================================================================
== 8.3
===============================================================================

regression=# select interval '-1-1';
ERROR: invalid input syntax for type interval: "-1-1"

===============================================================================
== I think the relevant part of SQL 200N
===============================================================================

<interval string> ::=
<quote> <unquoted interval string> <quote>

<unquoted interval string> ::=
[ <sign> ] { <year-month literal> | <day-time literal> }

<year-month literal> ::=
<years value> [ <minus sign> <months value> ]
| <months value>

<years value> ::=
<datetime value>
<months value> ::=
<datetime value>

<datetime value> ::=
<unsigned integer>

... If SV is a negative interval, then <sign> shall be specified
within <unquoted interval string> in the literal Y.

===============================================================================
== The patch
===============================================================================

*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
***************
*** 609,621 **** ParseDateTime(const char *timestr, char *workbuf, size_t buflen,
/* soak up leading whitespace */
while (isspace((unsigned char) *cp))
cp++;
! /* numeric timezone? */
if (isdigit((unsigned char) *cp))
{
ftype[nf] = DTK_TZ;
APPEND_CHAR(bufp, bufend, *cp++);
while (isdigit((unsigned char) *cp) ||
! *cp == ':' || *cp == '.')
APPEND_CHAR(bufp, bufend, *cp++);
}
/* special? */
--- 609,621 ----
/* soak up leading whitespace */
while (isspace((unsigned char) *cp))
cp++;
! /* numeric timezone? or sql year-month interval?*/
if (isdigit((unsigned char) *cp))
{
ftype[nf] = DTK_TZ;
APPEND_CHAR(bufp, bufend, *cp++);
while (isdigit((unsigned char) *cp) ||
! *cp == ':' || *cp == '.' || *cp == '-')
APPEND_CHAR(bufp, bufend, *cp++);
}
/* special? */
***************
*** 2876,2889 **** DecodeInterval(char **field, int *ftype, int nf, int range,
{
/* SQL "years-months" syntax */
int val2;
!
val2 = strtoi(cp + 1, &cp, 10);
if (errno == ERANGE || val2 < 0 || val2 >= MONTHS_PER_YEAR)
return DTERR_FIELD_OVERFLOW;
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
! val = val * MONTHS_PER_YEAR + val2;
fval = 0;
}
else if (*cp == '.')
--- 2876,2890 ----
{
/* SQL "years-months" syntax */
int val2;
! int sign;
! sign = val < 0 ? -1 : 1;
val2 = strtoi(cp + 1, &cp, 10);
if (errno == ERANGE || val2 < 0 || val2 >= MONTHS_PER_YEAR)
return DTERR_FIELD_OVERFLOW;
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
! val = val * MONTHS_PER_YEAR + val2*sign;
fval = 0;
}
else if (*cp == '.')
================================================================================

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Short summary:
> I think this patch fixes a bug with sql-spec negative interval literals.

Hmm. I'm a bit concerned about possible side-effects on other cases:
what had been seen as two separate tokens will now become one token
for *all* datetime types, not just interval. However, I can't
immediately think of any likely input formats where this would be a
problem.

Something else I noticed while poking at it is this inconsistency:

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

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

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

regression=# select interval '1' day;
interval
----------
1 day
(1 row)

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

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

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

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

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

As soon as you throw in a sign, it gets wacky :-(.

The reason for this bizarreness is this chunk of code at the end of
DecodeInterval's DTK_TZ case:

else if (type == IGNORE_DTF)
{
if (*cp == '.')
{
/*
* Got a decimal point? Then assume some sort of
* seconds specification
*/
type = DTK_SECOND;
}
else if (*cp == '\0')
{
/*
* Only a signed integer? Then must assume a
* timezone-like usage
*/
type = DTK_HOUR;
}
}

which means that a signed integer gets forced to be "hours" if there
isn't an explicit unit spec in the literal, while a signed float gets
forced to be "seconds". I can't see any reason why that's a good idea,
and propose that while we're making incompatible changes in corner cases,
we should just drop the code quoted above.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Short summary:
>> I think this patch fixes a bug with sql-spec negative interval literals.
>
> Hmm. I'm a bit concerned about possible side-effects on other cases:
> what had been seen as two separate tokens will now become one token
> for *all* datetime types, not just interval. However, I can't

If it's a concern, I could make interval_in first look for the
SQL-standard patterns before even parsing the string into fields.
If we want to handle the SQL standard negative datetime intervals
(see below) the way the spec looks to me,

> immediately think of any likely input formats where this would be a
> problem.
> Something else I noticed while poking at it is this inconsistency:...

Yes. I saw some of those too (and '-1 1:00:00'); but didn't have a
patch ready (yet). I'm happy to work on it.

> As soon as you throw in a sign, it gets wacky :-(.

Oh. And looking more closely; there's a potential bad incompatibility.

If I read SQL 200N's spec correctly

select interval '-1 1:00:00';

should mean "-1 days -1 hours",
yet 8.3 sees it as "-1 days +1 hours".

Scary to touch that one, but since a standard's a standard, I think
eventually we should get there. Perhaps we need a GUC to choose
standards or backward compatible behavior for that one? Or perhaps
keep parsing it the old way but with a WARNING for 8.4 and switch in 8.5?

> The reason for this bizarreness is this chunk of code at the end of
> DecodeInterval's DTK_TZ case:
>
> else if (type == IGNORE_DTF)
> {...} }
>
> which means that a signed integer gets forced to be "hours" if there
> isn't an explicit unit spec in the literal, while a signed float gets
> forced to be "seconds". I can't see any reason why that's a good idea,
> and propose that while we're making incompatible changes in corner cases,
> we should just drop the code quoted above.

+1. Shall I try for a patch that handles those and possibly the (more
frightening) SQL 200N signed day-time interval mentioned above.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> Hmm. I'm a bit concerned about possible side-effects on other cases:
>> what had been seen as two separate tokens will now become one token
>> for *all* datetime types, not just interval. However, I can't

> If it's a concern, I could make interval_in first look for the
> SQL-standard patterns before even parsing the string into fields.

I don't think it's worth the trouble unless someone points out a
real-world format that would be broken by the change. We certainly
don't document anything that would be. I've applied a patch along
these lines and we'll see if anyone complains.

> If I read SQL 200N's spec correctly

> select interval '-1 1:00:00';

> should mean "-1 days -1 hours",
> yet 8.3 sees it as "-1 days +1 hours".

I think we are kind of stuck on this one. If we change it, then how
would one represent -1 days +1 hours? The spec's format is only sane
if you assume all the fields must have the same sign, which is not
the case for PG.

> Scary to touch that one, but since a standard's a standard, I think
> eventually we should get there.

The SQL spec's date/time handling is, and always has been, broken enough
that I feel no great compulsion to follow every last detail. Especially
details that make it impossible to support our extensions...

regards, tom lane

Tom Lane wrote:
>> If I read SQL 200N's spec correctly
>> select interval '-1 1:00:00';
>> should mean "-1 days -1 hours",
>> yet 8.3 sees it as "-1 days +1 hours".
>
> I think we are kind of stuck on this one. If we change it, then how
> would one represent -1 days +1 hours? The spec's format is only sane

I'm not proposing this, but I could imagine making
"-1 -1:00:00" and/or "-1 +1:00:00" mean -1 days +1 hours.

I think if it weren't for backward compatibility issues I'd even
support such an idea - since now we're oh-so-very-close to accepting
to-spec literals. Unfortunately I fear old users might assume the
opposite meaning.

>> Scary to touch that one, but since a standard's a standard, I think
>> eventually we should get there.
>
> The SQL spec's date/time handling is, and always has been, broken enough
> that I feel no great compulsion to follow every last detail. Especially
> details that make it impossible to support our extensions...

In this case we're so very close to meeting the spec, though. And
it's ashame where we accept the to-spec syntax ("-1 1:00:00") but
return a different answer.

And since it's not quite impossible to support our extensions (force
putting the sign on the h:m:s part for mixed sign cases) I feels nice
to me to try.

Tom Lane wrote:
>Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Tom Lane wrote:
>> If I read SQL 200N's spec correctly

>> select interval '-1 1:00:00';

>> should mean "-1 days -1 hours",
>> yet 8.3 sees it as "-1 days +1 hours".

>I think we are kind of stuck on this one. If we change it, then how
>would one represent -1 days +1 hours? The spec's format is only sane
>if you assume all the fields must have the same sign, which is not
>the case for PG.

-1 days +1 hours = interval '-0 23:00:00'

Intervals are a scalar, not an addition of assorted values, alternating signs
between fields would be wrong.
--
Sincerely,
Stephen R. van den Berg.

He did a quarter of the work in *half* the time!

"Stephen R. van den Berg" <srb(at)cuci(dot)nl> writes:
> Intervals are a scalar, not an addition of assorted values, alternating signs
> between fields would be wrong.

Sorry, you're the one who's wrong on that. We've treated intervals as
three independent fields for years now (and before that it was two
independent fields). We're not going to throw away that capability.

regards, tom lane

Tom Lane wrote:
> "Stephen R. van den Berg" <srb(at)cuci(dot)nl> writes:
>> Intervals are a scalar, not an addition of assorted values, alternating signs
>> between fields would be wrong.
>
> Sorry, you're the one who's wrong on that. We've treated intervals as
> three independent fields for years now (and before that it was two
> independent fields). We're not going to throw away that capability.

+1 It's very useful.

Currently our terse input format that's similar to the SQL standard
rejects more mixed-sign intervals than I'd like. I'd be quite
happy if:
'1 2:03:-04'
gave me
'1 day 2 hours 3 minutes -4 seconds'
but currently we reject that mixed-sign-literal.

I'd just like to find a way to have SQL-standard input produce SQL-standard
output in the cases where the input happened to match the standard.

If we had a blank slate, my vote would be that
'-1 2:03:04' should mean what the SQL standard says it should.
'-1 +2:03:04' should mean negative 1 days, plus 2 hours 3 minutes 4 sec
'1 2:03:-04' should mean 1 day 2 hours 3 minutes minus 4 seconds
'-1 2:03:+04' should mean negative 1 day 2 hours 3 minutes plus 4 seconds
but I'm aware that there are backward compatibility issues.

Tom Lane wrote:
>"Stephen R. van den Berg" <srb(at)cuci(dot)nl> writes:
>> Intervals are a scalar, not an addition of assorted values, alternating signs
>> between fields would be wrong.

>Sorry, you're the one who's wrong on that. We've treated intervals as
>three independent fields for years now (and before that it was two
>independent fields).

Ok, didn't know that.
Let's put it differently then: I can understand that the standard
considers it a scalar and not an addition, but apparently the addition
characteristic is being used in Postgres code already; that makes it
undesirable to change it indeed.
--
Sincerely,
Stephen R. van den Berg.

He did a quarter of the work in *half* the time!

Tom Lane wrote:
> In fact, given that we are now
> somewhat SQL-compliant on interval input, a GUC that selected
> PG traditional, SQL-standard, or ISO 8601 interval output format seems
> like it could be a good idea.

Short summary:

The attached patch
(1) adds a new GUC called "IntervalStyle" that decouples interval
output from the "DateStyle" GUC, and
(2) adds a new interval style that will match the SQL standards
for interval literals when given interval data that meets the
sql standard (year-month or date-time only; and no mixed sign).

Background:

Currently Postgres outputs Intervals in one of two formats
depending on DateStyle. When DateStyle is 'ISO', it outputs
intervals like '1 year 2 mons 3 days -04:05:06.78' (though I
know of no ISO interval standards that look like that). When
DateStyle is 'SQL' it outputs intervals like
'@ 1 year 2 mons 3 days -4 hours -5 mins -6.78 secs' (though
I know of no SQL interval standards that look like that).

The feature:

The SQL standard only specifies interval literal strings
for the two kinds of intervals (year-month and day-time)
that are defined by the SQL spec. It also doesn't account
for postgres's intervals that can have mixed-sign components.
I tried to make the output here be a logical extension
of the spec, concatenating the year-month and day-time
interval strings and forcing signs in the output that could
otherwise have been ambiguous.

This little table shows an example of the output of this
new style compared to the existing postgres output for
(a) a year-month interval, (b) a day-time interval, and
(c) a not-quite-standard postgres interval with year-month
and day-time components of varying signs.

'1-2' | '@ 1 year 2 mons'
'3 4:05:06.78' | '@ 3 days 4 hours 5 mins 6.78 secs'
'+1-2 -3 +4:05:06.78' | '@ 1 year 2 mons -3 days 4 hours 5 mins 6.78 secs'

The patch:

Seems to work for me; and I believe I updated the docs.
Many regression tests fail, though, because they assume
the existing coupling of DateStyle and interval output
styles. If people like where this is going I can update
those regression tests and add ones to test this new style.

Ron Mayer wrote:
> Tom Lane wrote:
>> ...GUC that selected PG traditional, SQL-standard... interval output
>> format seems like it could be a good idea.
>

This is an update to the earlier SQL-standard-interval-literal output
patch that I submitted here:
http://archives.postgresql.org/message-id/48D15471.6080305@cheapcomplexdevices.com

This version fixes a couple bugs in my last patch related to reltime output and
with the new GUC variable, and updated the regression tests to adjust the
new IntervalStyle guc to match the output of the previous regression tests
where the interval output depended on DateStyle.

I've also added it to the Nov CommitFest wiki page.

Tom Lane wrote:
> >> There is still some unfinished business if anyone wants to make it
> >> really exactly 100% spec compliant ...
>
> > I agree.
>
> I committed the patch as presented, and I think I might go take a quick

Tom, which Interval TODO items did you complete with this patch?

http://wiki.postgresql.org/wiki/Todo#Dates_and_Times

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

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom, which Interval TODO items did you complete with this patch?
> http://wiki.postgresql.org/wiki/Todo#Dates_and_Times

I think we've at least mostly fixed

* Support ISO INTERVAL syntax if units cannot be determined from the string, and are supplied after the string

* Add support for year-month syntax, INTERVAL '50-6' YEAR TO MONTH

There might be a few glitches left but they are at much smaller grain
than the TODO is talking about.

... while I'm looking: I am not sure that I think either of these TODO
items are sane or standards-compliant:

* Interpret INTERVAL '1 year' MONTH as CAST (INTERVAL '1 year' AS INTERVAL MONTH), and this should return '12 months'

* Support precision, CREATE TABLE foo (a INTERVAL MONTH(3))

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom, which Interval TODO items did you complete with this patch?
> > http://wiki.postgresql.org/wiki/Todo#Dates_and_Times
>
> I think we've at least mostly fixed
>
> * Support ISO INTERVAL syntax if units cannot be determined from the string, and are supplied after the string
>
> * Add support for year-month syntax, INTERVAL '50-6' YEAR TO MONTH
>
> There might be a few glitches left but they are at much smaller grain
> than the TODO is talking about.

Thanks, marked as done.

> ... while I'm looking: I am not sure that I think either of these TODO
> items are sane or standards-compliant:
>
> * Interpret INTERVAL '1 year' MONTH as CAST (INTERVAL '1 year' AS INTERVAL MONTH), and this should return '12 months'
>
> * Support precision, CREATE TABLE foo (a INTERVAL MONTH(3))

OK, I have removed the items; we can always re-add them if they are
requested. Thanks for the review.

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

+ If your life is a hard drive, Christ can be your backup. +

Ron Mayer wrote:
> Ron Mayer wrote:
>> Tom Lane wrote:
>>> ...GUC that selected PG traditional, SQL-standard... interval output
>>> format seems like it could be a good idea.
> This is an update to the earlier SQL-standard-interval-literal output
> patch that I submitted here:
> http://archives.postgresql.org/message-id/48D15471.6080305@cheapcomplexdevices.com

Yet another update - mostly bringing the patch current with HEAD
now that the previous commit fest is over; and also posting it again
since I have a followup patch (for ISO 8601 interval input and output)
that is based on this one and I want the patches lines-of-code to match.

Ron

Ron Mayer wrote:
> Tom Lane wrote:
>> In fact, given that we are now
>> somewhat SQL-compliant on interval input, a GUC that selected
>> PG traditional, SQL-standard, or ISO 8601 interval output format seems
>> like it could be a good idea.

This patch (that works on top of the IntervalStyle patch I
posted earlier today) adds support for ISO8601 standard[0]
"Time Interval" "Durations" of the "format with designators"
(section 4.4.4.2.1). The other ISO 8601 types of intervals
deal with start and end points, so this one seemed most relevant.

It builds on a patch I had earlier submitted back in 2003[1],
where people noted that we wanted sql-standard intervals
first; but I see that ISO 8601 intervals did make it to the
todo list.

I updated the docs, but I still need to add regression tests,
so it's still a WIP, but I think the code's ready (I've been
using an older version of this patch internally since '03) so
I'd appreciate feedback.

[0] http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetch&nodeid=4021199
[1] http://archives.postgresql.org/pgsql-patches/2003-09/msg00121.php
[2] http://wiki.postgresql.org/wiki/Todo

Kevin Grittner wrote:
>>>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>
> Even more surprising is the behavior for interval(1) here:
> [.... some context with nonsurprising examples removed ...]
> ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(1);
> interval
> ----------------------------------
> 1 year 2 mons 3 days 04:05:06.60
> (1 row)
>
> That trailing zero should be considered a bug.

Is there a consensus that we don't want that trailing zero?
I notice that datetime.c's "TrimTrailingZeros(char *str)" has
the comment:
/* chop off trailing zeros... but leave at least 2 fractional digits */
that suggests that the trailing zero was intentional, but I
can't find any reasons why 2 fractional disgits were left.

The same function's also used for timestamps, so if we remove that
trailing zero in both places we'll see some regression differences
where we get
! | Mon Feb 10 17:32:01.5 1997 PST | 1997 | 7 | 1
instead of
! | Mon Feb 10 17:32:01.50 1997 PST | 1997 | 7 | 1

IMHO we don't want the extra zero for timestamps either.

If people agree I'll fold it into the patch dealing with
the other interval rounding eccentricities I have.

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> [some other interval rounding example]
>
> I don't much like the forced rounding to two digits here, but changing
> that doesn't seem like material for back-patching. Are you going to
> fix that up while working on your other patches?

On Thu, Oct 09, 2008 at 11:50:17AM -0700, Ron Mayer wrote:
> Kevin Grittner wrote:
>>>>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> Even more surprising is the behavior for interval(1) here:
>> [.... some context with nonsurprising examples removed ...]
>> ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(1);
>> interval
>> ----------------------------------
>> 1 year 2 mons 3 days 04:05:06.60
>> (1 row)
>> That trailing zero should be considered a bug.
>
> Is there a consensus that we don't want that trailing zero?
> I notice that datetime.c's "TrimTrailingZeros(char *str)" has
> the comment:
> /* chop off trailing zeros... but leave at least 2 fractional digits */
> that suggests that the trailing zero was intentional, but I
> can't find any reasons why 2 fractional disgits were left.
>
> The same function's also used for timestamps, so if we remove that
> trailing zero in both places we'll see some regression differences
> where we get
> ! | Mon Feb 10 17:32:01.5 1997 PST | 1997 | 7 | 1
> instead of
> ! | Mon Feb 10 17:32:01.50 1997 PST | 1997 | 7 | 1
>
> IMHO we don't want the extra zero for timestamps either.
>
There is a difference between the result 0.6 and 0.60 in rounding.
The first is accurate +-0.05 and the second is +-0.005. Certainly,
it does not seem unreasonable that machines can calulate intervals
to the nearest 100th of a second. What is not clear to me is how the
decision to stop at the 2nd decimal digit was reached. If timestamps
are accurate to 1/100th, intervals should be returned to that level
of accuracy as well. Trailing digits definitely have meaning.

My 2 cents,
Ken

>
> If people agree I'll fold it into the patch dealing with
> the other interval rounding eccentricities I have.
>
> Tom Lane wrote:
>> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>>> [some other interval rounding example]
>> I don't much like the forced rounding to two digits here, but changing
>> that doesn't seem like material for back-patching. Are you going to
>> fix that up while working on your other patches?
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Kenneth Marshall <ktm(at)rice(dot)edu> writes:
> There is a difference between the result 0.6 and 0.60 in rounding.
> The first is accurate +-0.05 and the second is +-0.005. Certainly,
> it does not seem unreasonable that machines can calulate intervals
> to the nearest 100th of a second. What is not clear to me is how the
> decision to stop at the 2nd decimal digit was reached.

Probably by flipping a coin ;-). You have to remember that all this
behavior was designed around floating-point intervals, so there's
inherent imprecision in there; and the extent depends on the size of
the interval which makes it pretty hard to choose a display precision.

In the integer-timestamp world we know that the number is exact in
microseconds. We clearly ought to be prepared to display up to six
fractional digits, but suppressing trailing zeroes in that seems
appropriate.

We could try to do the same in the float case, but I'm a bit worried
about finding ourselves showing "1234567.799999" where it should be
"1234567.8".

regards, tom lane

>>> Kenneth Marshall <ktm(at)rice(dot)edu> wrote:

>>> Even more surprising is the behavior for interval(1) here:
>>> [.... some context with nonsurprising examples removed ...]
>>> ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(1);
>>> interval
>>> ----------------------------------
>>> 1 year 2 mons 3 days 04:05:06.60
>>> (1 row)
>>> That trailing zero should be considered a bug.

> What is not clear to me is how the
> decision to stop at the 2nd decimal digit was reached.

See this posting and others on the thread:

http://archives.postgresql.org/pgsql-hackers/2008-09/msg00999.php

The current rules seem to be:

(1) If precision is specified, round to that precision.

(2) If result has only zeros in the fraction, show no fraction, else
show at least two digits in the fraction, adding a trailing zero if
needed to get to two digits, but don't show any trailing zeros in the
fraction beyond the second position.

I think it would be ideal if we could track how many digits of
accuracy we have in a value, and show them all, even if that involves
trailing zeros. If that's not feasible, let's consistently not show
trailing zeros. Rounding .64 to .6 and then showing .60 is just plain
wrong.

-Kevin

On Thu, Oct 09, 2008 at 02:47:24PM -0500, Kevin Grittner wrote:
> >>> Kenneth Marshall <ktm(at)rice(dot)edu> wrote:
>
> >>> Even more surprising is the behavior for interval(1) here:
> >>> [.... some context with nonsurprising examples removed ...]
> >>> ccdev=# select '1 year 2 mons 3 days 04:05:06.64321'::interval(1);
> >>> interval
> >>> ----------------------------------
> >>> 1 year 2 mons 3 days 04:05:06.60
> >>> (1 row)
> >>> That trailing zero should be considered a bug.
>
> > What is not clear to me is how the
> > decision to stop at the 2nd decimal digit was reached.
>
> See this posting and others on the thread:
>
> http://archives.postgresql.org/pgsql-hackers/2008-09/msg00999.php
>
> The current rules seem to be:
>
> (1) If precision is specified, round to that precision.
>
> (2) If result has only zeros in the fraction, show no fraction, else
> show at least two digits in the fraction, adding a trailing zero if
> needed to get to two digits, but don't show any trailing zeros in the
> fraction beyond the second position.
>
> I think it would be ideal if we could track how many digits of
> accuracy we have in a value, and show them all, even if that involves
> trailing zeros. If that's not feasible, let's consistently not show
> trailing zeros. Rounding .64 to .6 and then showing .60 is just plain
> wrong.
>
> -Kevin
>
+1

Ken

Tom Lane wrote:
> In the integer-timestamp world we know that the number is exact in
> microseconds. We clearly ought to be prepared to display up to six
> fractional digits, but suppressing trailing zeroes in that seems
> appropriate.

Great.

> We could try to do the same in the float case, but I'm a bit worried
> about finding ourselves showing "1234567.799999" where it should be
> "1234567.8".

If I understand the code right fsec should mostly be values
between -1 and 1 anyway, because even in the floating point
case seconds are carried in the tm->tm_sec part.
It looks to me that a double should be plenty to do
microseconds so long as we don't put big numbers into fsec.
printf("%.99f\n",59.111111111111111111);
59.11111111111111426907882 ...

Anyway - I'll try showing up-to-6-digits in both cases and
seeing if I can find a test case that breaks.

I guess this rounding trickiness with rounding explains some of
the bizarre code like this too:
#if 0
/* chop off trailing one to cope with interval rounding */
if (strcmp(str + len - 4, "0001") == 0)
{
len -= 4;
*(str + len) = '\0';
}
#endif

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> We could try to do the same in the float case, but I'm a bit worried
>> about finding ourselves showing "1234567.799999" where it should be
>> "1234567.8".

> If I understand the code right fsec should mostly be values
> between -1 and 1 anyway, because even in the floating point
> case seconds are carried in the tm->tm_sec part.

The problem is that that's a decomposed representation. In the stored
form, there's a floating-point seconds field that includes hours,
minutes, seconds, and fractional seconds, and therefore large values
of the H/M/S fields degrade the accuracy of the fraction part.

Here's an example (testing in 8.3, since HEAD defaults to integer):

regression=# select '1234567890 hours 0.123 sec'::interval;
interval
-------------------------
1234567890:00:00.123047
(1 row)

Since there's a (somewhat arbitrary) limitation of the hours to 2^31,
this is close to the worst possible case. (Hm, maybe someone actually
did the math and decided that 2 fractional digits were the most they
could promise given that? No, because this code dates from a time
when we included days in the same field too ... back then there might
have been no accuracy at all in the fraction part.)

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Tom Lane wrote:
>>> We could try to do the same in the float case, but I'm a bit worried
>>> about finding ourselves showing "1234567.799999" ...
>> If I understand the code right [I didn't...]
>
> The problem is ... seconds field that includes hours,
> minutes, seconds, and fractional seconds...Here's an example...
> regression=# select '1234567890 hours 0.123 sec'::interval;
> ... 1234567890:00:00.123047

Hmm. That's also an existence proof that we're not too concerned
about showing 6 imprecise digits anyway (at least for some 8.3
DateStyles). Doesn't seem like it'd hurt too much if we show
them for all the IntervalStyles.

> Since there's a (somewhat arbitrary) limitation of the hours to 2^31,
> this is close to the worst possible case. (Hm, maybe someone actually
> did the math and decided that 2 fractional digits ...

Or I guess we could truncate to 2 digits only in the float case;
or truncate to 2 digits only if we're using the float case and
have large values. But that extra complexity doesn't seem
worth it to me - especially since it seems to only affect
people who do two non-default things (pick a date/interval style
that used to truncate to 2, and --disable-integer-datetimes).

I put a patch up at http://0ape.com/postgres_interval_patches
that does what I think seems getting reasonable. For better
or worse, it depends on the other two interval patches I was
working on, but I could make a version that doesn't depend on
those as well if people prefer that.

Ron Mayer wrote:
> Tom Lane wrote:
>> In fact, given that we are now
>> somewhat SQL-compliant on interval input, a GUC that selected
>> PG traditional, SQL-standard, or ISO 8601 interval output format seems
>> like it could be a good idea.

Attached are updated versions of the Interval patches (SQL-standard interval
output, ISO8601 intervals, and interval rounding) I posted earlier upthread.
I mostly brought it up-to-date with HEAD, cleaned up comments and regression
tests, and fixed a couple bugs. [Sorry if people get this twice. I tried
attaching all 4 patches earlier today, but didn't notice it on the
list perhaps because of the combined size.]

# Patch 1: "stdintervaloutput.patch"
SQL Standard Interval Literal Output

Description: This patch adds an IntervalStyle GUC to control
the style of intervals. Previously the interval style was a
side-effect of the DateStyle GUC. IntervalStyle can be set to
"sql_standard" to output SQL Standard Interval Literals.

Reason for the patch: Now that we support SQL-standard interval
inputs, it's nice to be able to output intervals in that style as well.

During the commit-fest I'll post versions of these that are regularly
synced with CVS HEAD here: http://0ape.com/postgres_interval_patches/

Ron Mayer wrote:
> Ron Mayer wrote:
> > Tom Lane wrote:
> >> In fact, given that we are now
> >> somewhat SQL-compliant on interval input, a GUC that selected
> >> PG traditional, SQL-standard, or ISO 8601 interval output format seems
> >> like it could be a good idea.
>
> Attached are updated versions of the Interval patches ...

# Patch 2:
ISO 8601 Formatted Interval Input and Output

This patch adds another IntervalStyle 'iso_8601' to output ISO 8601
Time Intervals of the "format with designators". These are a bit
more flexible than Sql Standard intervals in that (like postgres)
they can express both years and days in the same interval value.

Reason for the patch:SQL Standard Intervals are limited compared to
postgres in what they allow (no mixed year-month and day-time
components). ISO8601 intervals allow such intervals and are easier
for machines to parse than the traditional postgres formats.

This patch depends on the IntervalStyle patch mentioned above.

Ron Mayer wrote:
> Ron Mayer wrote:
>> Ron Mayer wrote:
>> > Tom Lane wrote:
>> >> In fact, given that we are now
>> >> somewhat SQL-compliant on interval input, a GUC that selected
>> >> PG traditional, SQL-standard, or ISO 8601 interval output format
>> seems
>> >> like it could be a good idea.
>>
>> Attached are updated versions of the Interval patches ...

# Patch 3: "cleanup.patch"
Fix rounding inconsistencies and refactor interval input/output code

This patch removes a lot of copy & paste with gratuitous rounding
inconsistencies in the old interval code. This patch refactors it to save
about 300 lines in datetime.c, and by reusing code (instead of the
near-copy-paste that was there before) helps insure that rounding
inconsistancies are avoided.

It removes almost as much code as the other two patches added.

This patch applies on top of patch 1 and 2 posted up-thread..

My apologies if you got these twice, my mailer seems to be rejecting
even slightly large attachments so I tried a couple times.

During the commit-fest I'll post versions of these that are regularly
synced with CVS HEAD here: http://0ape.com/postgres_interval_patches/
along with a combined patch that includes all 3 of these in a single patch.

On Thu, Sep 18, 2008 at 6:03 AM, Ron Mayer
<rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> The attached patch
> (1) adds a new GUC called "IntervalStyle" that decouples interval
> output from the "DateStyle" GUC, and
> (2) adds a new interval style that will match the SQL standards
> for interval literals when given interval data that meets the
> sql standard (year-month or date-time only; and no mixed sign).
>

Hi Ron,

I've been assigned to do an initial review of your interval patches.
I'm going to be reviewing them one at a time, starting with this one
(the introduction of the new IntervalStyle GUC).

I grabbed the latest version of the patch from the URL posted up on
the CF wiki page:
http://0ape.com/postgres_interval_patches/stdintervaloutput.patch

Nice site you've got set up for the patches, BTW. It certainly makes
it all a lot more approachable.

On with the review then ...

The patch applied cleanly to the latest version of HEAD in the git
repository. I was able to build both postgres and the documentation
without complaint on x86_64 gentoo.

When I ran the regression tests, I got one failure in the new interval
tests. Looks like the "nonstandard extended" format gets a bit
confused when the seconds are negative:

*** /home/direvus/src/postgres/src/test/regress/expected/interval.out
Tue Nov 4 14:46:34 2008
--- /home/direvus/src/postgres/src/test/regress/results/interval.out
Tue Nov 4 15:19:53 2008
***************
*** 629,634 ****
- interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds';
interval | ?column?
----------------------+----------------------
! +1-2 -3 +4:05:06.789 | -1-2 +3 -4:05:06.789
(1 row)

--- 629,634 ----
- interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds';
interval | ?column?
----------------------+----------------------
! +1-2 -3 +4:05:06.789 | -1-2 +3 -4:05:-6.789
(1 row)

Otherwise, the feature seemed to behave as advertised. I tried
throwing a few bizarre intervals at it, but didn't manage to break
anything.

The C code has some small stylistic inconsistencies; in some cases the
spaces around binary operators are missing (e.g., "(fsec<0)"). See
src/backend/utils/adt/datetime.c lines 3691, 3694, 3697, 3729-3731.
There are also a lot of function calls missing the space after the
argument separator (e.g., "sprintf(cp,"%d %d:%02d:",mday,hour,min)").
Apart from not merging well with the style of the surrounding code, I
respectfully suggest that omitting the spaces really does make the
code harder to read.

The new documentation is good in terms of content, but there are some
minor stylistic and spelling cleanups I would suggest.

The standard is referred to variously as "SQL standard",
"SQL-standard" and "SQL Standard" in the patch. The surrounding
documentation seems to use "SQL standard", so that's probably the way
to go.

These sentences in datatype.sgml are a bit awkward:

"The postgres style will output intervals that match the style
PostgreSQL 8.3 outputed when the DateStyle parameter was set to ISO.

The postgres_verbose style will output intervals that match the style
PostgreSQL 8.3 outputed when the DateStyle parameter was set to SQL."

As far as I know, "outputed" isn't a word, and singling out 8.3 in
particular is a bit misleading, since the statement applies to earlier
versions as well. I would go with something more along the lines of:

"The postgres style will output intervals matching those output by
PostgreSQL prior to version 8.4, with the DateStyle parameter set to
ISO."

Likewise in config.sgml, the patch has:

"The value postgres will output intervals in a format that matches
what old releases had output when the DateStyle was set to 'ISO'. The
value postgres_verbose will output intervals in a format that matches
what old releases had output when the DateStyle was set to 'SQL'."

I don't think "old releases" is specific enough. Most folks reading
the documentation aren't going to know what is meant by "old". Better
to be precise. Again I would suggest phrasing like "... releases
prior to 8.4, with the DateStyle set to ...".

That's all the feedback I have for the moment. I hope you found my
comments helpful. I'll be setting the status of this patch to
"Returned with Feedback" and wait for your reponses before I move
forward with reviewing the other patches.

Cheers,
BJ

I think we need to distinguish between patches that are clearly not
going in, and patches that are not going in in their present form but
might be able to be reworked. My suggestion would be that only the
first category be moved to the Returned with feedback section, and the
others just have their status changed to "Waiting for new version" or
similar. Alternatively, we could create a separate section for
patches in this category.

...Robert

On Tue, Nov 4, 2008 at 12:22 AM, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> On Thu, Sep 18, 2008 at 6:03 AM, Ron Mayer
> <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> The attached patch
>> (1) adds a new GUC called "IntervalStyle" that decouples interval
>> output from the "DateStyle" GUC, and
>> (2) adds a new interval style that will match the SQL standards
>> for interval literals when given interval data that meets the
>> sql standard (year-month or date-time only; and no mixed sign).
>>
>
> Hi Ron,
>
> I've been assigned to do an initial review of your interval patches.
> I'm going to be reviewing them one at a time, starting with this one
> (the introduction of the new IntervalStyle GUC).
>
> I grabbed the latest version of the patch from the URL posted up on
> the CF wiki page:
> http://0ape.com/postgres_interval_patches/stdintervaloutput.patch
>
> Nice site you've got set up for the patches, BTW. It certainly makes
> it all a lot more approachable.
>
> On with the review then ...
>
> The patch applied cleanly to the latest version of HEAD in the git
> repository. I was able to build both postgres and the documentation
> without complaint on x86_64 gentoo.
>
> When I ran the regression tests, I got one failure in the new interval
> tests. Looks like the "nonstandard extended" format gets a bit
> confused when the seconds are negative:
>
> *** /home/direvus/src/postgres/src/test/regress/expected/interval.out
> Tue Nov 4 14:46:34 2008
> --- /home/direvus/src/postgres/src/test/regress/results/interval.out
> Tue Nov 4 15:19:53 2008
> ***************
> *** 629,634 ****
> - interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds';
> interval | ?column?
> ----------------------+----------------------
> ! +1-2 -3 +4:05:06.789 | -1-2 +3 -4:05:06.789
> (1 row)
>
> --- 629,634 ----
> - interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds';
> interval | ?column?
> ----------------------+----------------------
> ! +1-2 -3 +4:05:06.789 | -1-2 +3 -4:05:-6.789
> (1 row)
>
> Otherwise, the feature seemed to behave as advertised. I tried
> throwing a few bizarre intervals at it, but didn't manage to break
> anything.
>
> The C code has some small stylistic inconsistencies; in some cases the
> spaces around binary operators are missing (e.g., "(fsec<0)"). See
> src/backend/utils/adt/datetime.c lines 3691, 3694, 3697, 3729-3731.
> There are also a lot of function calls missing the space after the
> argument separator (e.g., "sprintf(cp,"%d %d:%02d:",mday,hour,min)").
> Apart from not merging well with the style of the surrounding code, I
> respectfully suggest that omitting the spaces really does make the
> code harder to read.
>
> The new documentation is good in terms of content, but there are some
> minor stylistic and spelling cleanups I would suggest.
>
> The standard is referred to variously as "SQL standard",
> "SQL-standard" and "SQL Standard" in the patch. The surrounding
> documentation seems to use "SQL standard", so that's probably the way
> to go.
>
> These sentences in datatype.sgml are a bit awkward:
>
> "The postgres style will output intervals that match the style
> PostgreSQL 8.3 outputed when the DateStyle parameter was set to ISO.
>
> The postgres_verbose style will output intervals that match the style
> PostgreSQL 8.3 outputed when the DateStyle parameter was set to SQL."
>
> As far as I know, "outputed" isn't a word, and singling out 8.3 in
> particular is a bit misleading, since the statement applies to earlier
> versions as well. I would go with something more along the lines of:
>
> "The postgres style will output intervals matching those output by
> PostgreSQL prior to version 8.4, with the DateStyle parameter set to
> ISO."
>
> Likewise in config.sgml, the patch has:
>
> "The value postgres will output intervals in a format that matches
> what old releases had output when the DateStyle was set to 'ISO'. The
> value postgres_verbose will output intervals in a format that matches
> what old releases had output when the DateStyle was set to 'SQL'."
>
> I don't think "old releases" is specific enough. Most folks reading
> the documentation aren't going to know what is meant by "old". Better
> to be precise. Again I would suggest phrasing like "... releases
> prior to 8.4, with the DateStyle set to ...".
>
> That's all the feedback I have for the moment. I hope you found my
> comments helpful. I'll be setting the status of this patch to
> "Returned with Feedback" and wait for your reponses before I move
> forward with reviewing the other patches.
>
> Cheers,
> BJ
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Brendan Jurd wrote:
> ...Sep 18, 2008... Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> The attached patch
>> (1) adds a new GUC called "IntervalStyle" that decouples interval
>> output from the "DateStyle" GUC, and
>> (2) adds a new interval style that will match the SQL standards
>> for interval literals when given interval data that meets the
>> sql standard (year-month or date-time only; and no mixed sign).
>
> I've been assigned to do an initial review of your interval patches.
> I'm going to be reviewing them one at a time, starting with this one
> (the introduction of the new IntervalStyle GUC).

Great! Thanks much!

> I grabbed the latest version of the patch from the URL posted up on
> the CF wiki page:
> http://0ape.com/postgres_interval_patches/stdintervaloutput.patch
>
> Nice site you've got set up for the patches, BTW. It certainly makes
> it all a lot more approachable.

Ah. If you're using GIT, you might find it more convenient to pull/merge
from
http://git.0ape.com/postgresql/
or browse through gitweb:
http://git.0ape.com/?p=postgresql;a=shortlog;h=refs/heads/cleanup
http://git.0ape.com/git-browser/by-commit.html?r=postgresql
though this is the first time I've set up gitweb so it might have rough edges.

> The patch applied cleanly to the latest version of HEAD in the git
> repository. I was able to build both postgres and the documentation
> without complaint on x86_64 gentoo.
>
> When I ran the regression tests, I got one failure in the new interval
> tests. Looks like the "nonstandard extended" format gets a bit
> confused when the seconds are negative:

Ah yes. Let me guess, HAVE_INT64_TIMESTAMP was defined. I believe
the later refactoring patch also avoids that bug; but yes, I obviously
should have had it working in this patch.

This fix was simple (can be seen on gitweb here: http://tinyurl.com/5fxeyw)
and I think I've pushed the updated patches to my website.

Once I fix the stylistic points you mentioned below I'll post
the resulting patch to the mailing list.

> Otherwise, the feature seemed to behave as advertised. I tried
> throwing a few bizarre intervals at it, but didn't manage to break
> anything.
>
> The C code has some small stylistic inconsistencies....
> ...documentation...some
> minor stylistic and spelling cleanups I would suggest.
>
Totally agree with all your style suggestions. Will send an update
a bit later today.

Ah. And one final question regarding functionality.

It seems to me that the last remaining place where we input
a SQL-2008 standard literal and do something different from
what the standard suggests is with the string:
'-1 2:03:04'
The standard seems to say that the "-" affects both the
days and hour/min/sec part; while PostgreSQL historically,
and the patch as I first submitted it only apply the negative
sign to the days part.

IMHO when the IntervalStyle GUC is set to "sql_standard",
it'd be better if the parsing of this literal matched the
standard. We already have the precedent where DateStyle
is used to interpret otherwise ambiguous output.

If the IntervalStyle is set to anything other than sql_standard
we'll keep parsing them the old way; so I think backwards
compatibility issues would be minimized. And those
using the sql_standard mode are likely to be standards
fanatics anyway, and would probably appreciate following the
standard rather than the backward compatible mode.

Thoughts?
I have a version of each alternative working here,
and I'd be happy to submit the final patch either way.

>>> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> It seems to me that the last remaining place where we input
> a SQL-2008 standard literal and do something different from
> what the standard suggests is with the string:
> '-1 2:03:04'
> The standard seems to say that the "-" affects both the
> days and hour/min/sec part;

Agreed.

> while PostgreSQL historically,
> and the patch as I first submitted it only apply the negative
> sign to the days part.
>
> IMHO when the IntervalStyle GUC is set to "sql_standard",
> it'd be better if the parsing of this literal matched the
> standard. We already have the precedent where DateStyle
> is used to interpret otherwise ambiguous output.
>
> If the IntervalStyle is set to anything other than sql_standard
> we'll keep parsing them the old way; so I think backwards
> compatibility issues would be minimized. And those
> using the sql_standard mode are likely to be standards
> fanatics anyway, and would probably appreciate following the
> standard rather than the backward compatible mode.
>
> Thoughts?

I think it would be good to be able to configure PostgreSQL such that
it didn't take standards-compliant literals and silently treat them in
a non-standard way. What you propose here seems sane to me, but if
someone objects, it would be good for some other value or other GUC to
provide compliant behavior.

-Kevin

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Ah. And one final question regarding functionality.
> It seems to me that the last remaining place where we input
> a SQL-2008 standard literal and do something different from
> what the standard suggests is with the string:
> '-1 2:03:04'
> The standard seems to say that the "-" affects both the
> days and hour/min/sec part; while PostgreSQL historically,
> and the patch as I first submitted it only apply the negative
> sign to the days part.

> IMHO when the IntervalStyle GUC is set to "sql_standard",
> it'd be better if the parsing of this literal matched the
> standard.

Then how would you input a value that had different signs for the
day and the h/m/s? I don't think "you can't" is an acceptable
answer there, because it would mean that interval_out has to fail
on such values when IntervalStyle is "sql_standard". Which is
very clearly not gonna do.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Ah. And one final question regarding functionality.
>> It seems to me that the last remaining place where we input
>> a SQL-2008 standard literal and do something different from
>> what the standard suggests is with the string:
>> '-1 2:03:04'
>> The standard seems to say that the "-" affects both the
>> days and hour/min/sec part; while PostgreSQL historically,
>> and the patch as I first submitted it only apply the negative
>> sign to the days part.
>
>> IMHO when the IntervalStyle GUC is set to "sql_standard",
>> it'd be better if the parsing of this literal matched the
>> standard.
>
> Then how would you input a value that had different signs for the
> day and the h/m/s? I don't think "you can't" is an acceptable
> answer there, because it would mean that interval_out has to fail
> on such values when IntervalStyle is "sql_standard". Which is
> very clearly not gonna do.

In the patch I submitted:
"-1 +2:03:04" always means negative day, positive hours/min/sec
"+1 -2:03:04" always means positive day, negative hours/min/sec

When given a non-standard interval value, EncodeInterval is
always outputting all the signs ("+" and "-") to force it
to be unambiguous.

-- test a couple non-standard interval values too
SELECT interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds',
- interval '1 years 2 months -3 days 4 hours 5 minutes 6.789 seconds';
interval | ?column?
----------------------+----------------------
+1-2 -3 +4:05:06.789 | -1-2 +3 -4:05:06.789
(1 row)

Brendan Jurd wrote:
> ...Sep 18, 2008...Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> (1) ...GUC called "IntervalStyle"...
>> (2) ...interval style that will match the SQL standards...
>
> ...an initial review...
>
> When I ran the regression tests, I got one failure in the new interval

Fixed, and I did a bit more testing both with and without HAVE_INT64_TIMESTAMP.

> The C code has some small stylistic inconsistencies; ...
> ... spaces around binary operators are missing (e.g., "(fsec<0)").

Thanks. Fixed these.

> ...function calls missing the space after the argument separator...

I think I fixed all these now too.

> The new documentation is good in terms of content, but there are some
> minor stylistic and spelling cleanups I would suggest.
> ...variously..."SQL standard", "SQL-standard" and "SQL Standard"...

Got it. There are a few inconsistencies elsewhere in the file
talking about other data types. I wonder if I should fix those
as well.

> These sentences in datatype.sgml are a bit awkward ...
> I would go with something more along the lines of...

Yes. Thanks for the better wording.

> I don't think "old releases" is specific enough.

Yup - fixed that too.

> That's all the feedback I have for the moment. I hope you found my
> comments helpful. I'll be setting the status of this patch to
> "Returned with Feedback" and wait for your responses before I move
> forward with reviewing the other patches.

Great. I've tried to update the style on my remaining patches as well.

In addition, I've added to the docs describing how I use
explicit '+' and '-' signs to disambiguate the mixed-sign
non-standard intervals when in the sql_standard mode.

As before the 3 patches are at: http://0ape.com/postgres_interval_patches/
and http://git.forensiclogic.com/postgresql/ and
http://git.forensiclogic.com/?p=postgresql;a=shortlog;h=refs/heads/cleanup

I'm attaching the patch dealing with sql standard intervals here for the archives.

On Wed, Nov 5, 2008 at 7:34 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> Brendan Jurd wrote:
>> When I ran the regression tests, I got one failure in the new interval
>
> Fixed, and I did a bit more testing both with and without
> HAVE_INT64_TIMESTAMP.

Confirmed, all regression tests now pass on my system with the updated patch.

>> The C code has some small stylistic inconsistencies; ...
>> ... spaces around binary operators are missing (e.g., "(fsec<0)").
>
> Thanks. Fixed these.
>
>> ...function calls missing the space after the argument separator...
>
> I think I fixed all these now too.

Awesome. As far as I can tell, you got them all. I don't have any
further nits to pick about the code style.

The changes to the documentation all look good. I did notice one
final typo that I think was introduced in the latest version.
doc/src/sgml/datatype.sgml:2270 has "Nonstandardrd" instead of
"Nonstandard".

But, apart from that I have no further feedback.

I will sign off on this one and mark it "Ready for committer" in the commitfest.

Review of the other two patches coming soon to a mail client near you.

Cheers,
BJ

Brendan Jurd wrote:
> The changes to the documentation all look good. I did notice one
> final typo that I think was introduced in the latest version.
> doc/src/sgml/datatype.sgml:2270 has "Nonstandardrd" instead of
> "Nonstandard".

Just checked in a fix to that one; and updated my website at
http://0ape.com/postgres_interval_patches/
and pushed it to my (hopefully fixed now) git server.

If this'll be the final update to this patch should I be
posting it to the mailing list too for the archives?

> But, apart from that I have no further feedback.
> I will sign off on this one and mark it "Ready for committer" in the commitfest.

Cool. I'm not sure if anyone still wants to weigh in on the
approach I took for mixed-sign intervals, where '-1 2:03:04'
gets interpreted differently depending on the date style,
and '-1 +2:03:04' and '-1 -2:03:04' are the way I'm using
to disambiguate them.

> Review of the other two patches coming soon to a mail client near you.

Feel free to do them one-at-a-time too; since no doubt any issues
with the first one will probably affect the second one too.

I think I updated the other patches for the missing whitespace
style issues my first patch had; but no doubt there could be
other bad habits I have as well.

Ron

Brendan Jurd wrote:
> On Wed, Nov 5, 2008 at 7:34 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> Brendan Jurd wrote:
>>> ...new interval
> Review of the other two patches coming soon to a mail client near you.
>

Oh - and for review of the next patch, ISO 8601's spec would no doubt
be useful.

I think this is the right place to get it:
http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetch&nodeid=4021199

Is there anywhere in the comments, docs, or wiki where such links
and/or excerpts from specs belong?

On Thu, Oct 2, 2008 at 9:31 PM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> Ron Mayer wrote:
> This patch (that works on top of the IntervalStyle patch I
> posted earlier today) adds support for ISO8601 standard[0]
> "Time Interval" "Durations" of the "format with designators"
> (section 4.4.4.2.1). The other ISO 8601 types of intervals
> deal with start and end points, so this one seemed most relevant.
>

Hi Ron,

Reviewing this patch now; I'm working from the 'iso8601' branch in
your git repository.

Compile, regression tests and my own ad hoc tests in psql all worked
without a hitch.

As I was reading through the new documentation introduced by the
patch, I thought of various improvements and decided to try them out
for myself. In the end, rather than try to explain all of my changes
in detail, I thought I'd post a patch of my own (against your branch)
and accompany it with a few explanatory notes.

The patch includes some minor changes to the code style in
src/backend/utils/adt/datetime.c, similar to my recommendations for
the IntervalStyle patch.

As for the documentation, I ended up making quite a few changes.
Explanations and rationales to follow.

ISO section numbers -- in a couple of places, the documentation
referred to the wrong parts of ISO 8601. I found references to
5.5.4.2.1 and 5.5.4.2.2, whereas in both cases the first two digits
should be '4'.

Interval Input syntax -- I thought that the paragraph explaining the
ISO input syntax was not verbose enough. One paragraph didn't seem
sufficient to explain the syntaxes involved. In particular, the
original paragraph didn't mention that units could be omitted or
reordered in the "designators" format. I expanded a bit more on how
the syntaxes worked, using the same pseudogrammar style used to
explain the postgres interval format, and included a table showing the
available time-unit designators.

Interval Output section structure -- the new section for Interval
Output was added at level 2, right below Date/Time Output. I felt
this was uncomfortably assymetric with the Input section, which has
Date/Time Input as a level 2 section, with Interval Input as a level 3
underneath it. I demoted Interval Output to a level 3 and moved it
underneath Date/Time Output.

The rest of my doc changes were adding extra linkage, and some minor
wording and consistency tweaks.

Most of these changes are of a highly subjective nature. I think they
are improvements, but someone else might prefer the way it was before
I got my hands dirty. Please consider the changes in my patch a set
of suggestions for making the documentation on this feature a little
easier to digest. You're welcome to take or leave them as you see
fit.

Cheers,
BJ

Brendan Jurd wrote:
> Reviewing this patch now; I'm working from the 'iso8601' branch in
> ... I thought I'd post a patch of my own (against your branch)
> and accompany it with a few explanatory notes.

Wow thanks! That's very helpful (though it might have been more
fair to your time if you just kicked it back to me saying "rewrite
the docs" so they make sense)!

I've applied them with a couple minor changes.

* If ISO 8601 5.5.3.1.d's statement "The designator T shall be
absent if all of the time components are absent." also applies
to 5.5.4.2.2; then I think the 'T' needed to be inside the
<optional> tags, so I moved it there. The link to the spec's
below[1].

* There was a <sect2> that the patch changed to a <sect3>, and
with that change I get an error:
openjade:datatype.sgml:2306:31:E: document type does not allow element "SECT3" here
so I changed it back to a <sect2>.

You can see my changes to your patch on gitweb here: http://tinyurl.com/6crks6
and see how they got applied after your patch here http://tinyurl.com/685hla

I think I've updated my website, my git, and the cleanup patch to include
these changes now.

> Most of these changes are of a highly subjective nature. I think they
> are improvements, but someone else might prefer the way it was before
> I got my hands dirty. Please consider the changes in my patch a set
> of suggestions for making the documentation on this feature a little
> easier to digest. You're welcome to take or leave them as you see
> fit.

They're clearly improvements. I'm a total novice when it comes to
writing docs.

[1]
http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetch&nodeid=4021199

On Thu, Nov 6, 2008 at 3:36 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> Wow thanks! That's very helpful (though it might have been more
> fair to your time if you just kicked it back to me saying "rewrite
> the docs" so they make sense)!
>

Maybe ... but I figured it would take more time to fully explain what
I meant by "rewrite the docs" in an email than it would to actually
rewrite them. =)

>
> I've applied them with a couple minor changes.
>
> * If ISO 8601 5.5.3.1.d's statement "The designator T shall be
> absent if all of the time components are absent." also applies
> to 5.5.4.2.2; then I think the 'T' needed to be inside the
> <optional> tags, so I moved it there. The link to the spec's
> below[1].

Hmm, okay. When I was running my tests in psql I came away with the
impression that the T was required in the "alternative format". I
might be mistaken. I'll run some further tests a little later on.

> * There was a <sect2> that the patch changed to a <sect3>, and
> with that change I get an error:
> openjade:datatype.sgml:2306:31:E: document type does not allow element
> "SECT3" here
> so I changed it back to a <sect2>.
>

Ah, the <sect3> needs to go _inside_ the <sect2>, whereas I originally
had just changed the tags and left it in the same position (after the
<sect2>). I fixed this in my working copy but I must have forgotten
to produce a fresh patch after doing so.

Cheers,
BJ

Brendan Jurd wrote:
>> I've applied them with a couple minor changes.
>>
>> * If ISO 8601 5.5.3.1.d's statement "The designator T shall be
>> absent if all of the time components are absent." also applies
>> to 5.5.4.2.2; then I think the 'T' needed to be inside the
>> <optional> tags, so I moved it there. The link to the spec's
>> below[1].
>
> Hmm, okay. When I was running my tests in psql I came away with the
> impression that the T was required in the "alternative format". I
> might be mistaken. I'll run some further tests a little later on.

Indeed that's a bug in my code; where I was sometimes
requiring the 'T' (in the ISO8601 "alternative format") and
sometimes not (in the ISO8601 format from 5.5.4.2.1).

Below's a test case. If I read the spec[1] right both of those
should mean 1 day. I'll update git and post a new patch now.
If people think I read the specs wrong, I'll undo this change
and fix the docs.

==========================================================
[2]lt:/home/ramayer/proj/pg% ./psql regression
psql (8.4devel)
Type "help" for help.

regression=# select interval 'P1D';
interval
----------
1 day
(1 row)

regression=# select interval 'P0000-00-01';
ERROR: invalid input syntax for type interval: "P0000-00-01"
LINE 1: select interval 'P0000-00-01';
^
==========================================================

[1]
http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetch&nodeid=4021199

Ron Mayer wrote:
> Brendan Jurd wrote:
>>> 'T' ... <optional>
>
> Indeed that's a bug in my code; where I was sometimes
> requiring the 'T' (in the ISO8601 "alternative format") and
> sometimes not (in the ISO8601 format from 5.5.4.2.1).
>
> Below's a test case. If I read the spec[1] right both of those
> should mean 1 day. I'll update git and post a new patch now.
> If people think I read the specs wrong, I'll undo this change
> and fix the docs.

I think I updated the web site and git now, and
'P0000-00-01' is now accepted. It might be useful if
someone double checked my reading of the spec, tho.

> [1]
> http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetch&nodeid=4021199
>

On Fri, Nov 7, 2008 at 3:35 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> I think I updated the web site and git now, and
> 'P0000-00-01' is now accepted. It might be useful if
> someone double checked my reading of the spec, tho.
>

Hi Ron,

I've tested out your latest revision and read the spec more closely,
and pretty much everything works as expected. 'P0000-00-01' yields 1
day, and various other combinations like 'P0000-01-00' and 'P0000-01'
work correctly too.

I agree with your interpretation of the spec, it clearly says that 'T'
can be omitted when there are no time components. This should apply
equally to both the designators format and the alternative format.
The examples in Annex B confirm this.

I did run into one potential bug:

postgres=# select interval 'P0001';
interval
----------
1 day

Whereas, I expected to get '1 year', since the format allows you to
omit lower-order components from right-to-left:

P0001-01-01 => 1 year 1 month 1 day
P0001-01 => 1 year 1 month
P0001 => should be 1 year?

On the documentation front, I have a few final cleanups to suggest
(patch attached).

* After giving the spec a closer look, I thought that 4.4.3.2 and
4.4.3.3 were the proper spec references to use for the two formats.

* I removed the identation from a <programlisting> element. These
elements are left at the start of the line to prevent the initial
spacing from being interpreted as part of the listing itself.

* I made "Interval Output" a level 3 section again, and this time I
corrected that structural issue in my earlier patch. It's now
contained within the level 2 section.

* There was a sentence in the docs about the 'T' separator being
mandatory in the "alternative format". I deleted it.

* Changed "format with time-unit designators" to just "format with
designators", as that is how the spec refers to it.

* Some tabs had crept into the indentation, and there were a few
indentation errors in the new tables. I corrected those (the tabs may
have been my fault; I sometimes forget to change my vi settings when
switching between C code and SGML).

Cheers,
BJ

Brendan Jurd wrote:
> On Fri, Nov 7, 2008 at 3:35 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> I think I updated the web site and git now, and
>> 'P0000-00-01' is now accepted. It might be useful if
>> someone double checked my reading of the spec, tho.
>
> I've tested out your latest revision and read the spec more closely,
> and pretty much everything works as expected. ...
> I agree with your interpretation of the spec, it clearly says that 'T'
> can be omitted when there are no time components. ...
> The examples in Annex B confirm this.

Cool. Thanks.

> I did run into one potential bug:
> postgres=# select interval 'P0001';
> ...
> Whereas, I expected to get '1 year', since the format allows you to
> omit lower-order components from right-to-left:
> P0001-01-01 => 1 year 1 month 1 day
> P0001-01 => 1 year 1 month
> P0001 => should be 1 year?

Indeed, that's right. Thanks for catching another one.
I just checked in (to my git) a patch that I believe fixes it.

regression=# select interval 'P0001',interval 'P00010000',interval 'PT01';
interval | interval | interval
----------+----------+----------
1 year | 1 year | 01:00:00
(1 row)

> On the documentation front, I have a few final cleanups to suggest
> (patch attached).
> * After giving the spec a closer look, I thought that 4.4.3.2 and
> 4.4.3.3 were the proper spec references to use for the two formats.

Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm
now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear?

Totally agree with the rest of your docs changes and applied those.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Brendan Jurd wrote:
>> The changes to the documentation all look good. I did notice one
>> final typo that I think was introduced in the latest version.
>> doc/src/sgml/datatype.sgml:2270 has "Nonstandardrd" instead of
>> "Nonstandard".

> Just checked in a fix to that one; and updated my website at
> http://0ape.com/postgres_interval_patches/
> and pushed it to my (hopefully fixed now) git server.

> If this'll be the final update to this patch should I be
> posting it to the mailing list too for the archives?

Since it's only one line different from your previous, probably no need.

I've started reviewing this patch for commit, and I find myself a bit
disturbed by its compatibility properties. The SQL_STANDARD output
style is simply ambiguous: what is meant by
-1 1:00:00
? What you get from that will depend on the intervalstyle setting at
the recipient. Either of the traditional Postgres styles are
non-ambiguous and will be interpreted correctly regardless of receiver's
intervalstyle --- in particular, Postgres mode always puts an explicit
sign on the time part if the days or months part was negative. What
this means is that SQL_STANDARD mode is unsafe for dumping data, and
*pg_dump had better force Postgres mode*. We can certainly do that with
a couple more lines added to the patch, but it's a bit troublesome that
we are boxed into using a nonstandard dump-data format until forever.

I don't immediately see any way around that, though. Anyone have a
bright idea?

regards, tom lane

Tom Lane wrote:
>
> I've started reviewing this patch for commit, and I find myself a bit
> disturbed by its compatibility properties. The SQL_STANDARD output
> style is simply ambiguous: what is meant by
> -1 1:00:00
> ? What you get from that will depend on the intervalstyle setting at
> the recipient.

Nope. The SQL Standard style avoids the ambiguity by following
the SQL Standard's rules when the input value complied with the
standard's restrictions on intervals.

For example - given the sql standard compliant value of negative
one days and negative one hours you get "-1 1;00:00".

If you give it a non-sql-standard-compliant value like negative
one days plus one hours it will force outputting all the signs
both positive and negative:

regression=# select interval '-1 days +1 hours';
interval
------------------
+0-0 -1 +1:00:00
(1 row)

I agree that there's an ambiguity on input - in much the same way
that date order can affect ambiguous inputs.

> Either of the traditional Postgres styles are
> non-ambiguous and will be interpreted correctly regardless of receiver's
> intervalstyle --- in particular, Postgres mode always puts an explicit
> sign on the time part if the days or months part was negative. What
> this means is that SQL_STANDARD mode is unsafe for dumping data, and

So long as the SQL Standard style is chosen both on dumping and loading,
I think it will preserve any values given to it.

> *pg_dump had better force Postgres mode*. We can certainly do that with
> a couple more lines added to the patch, but it's a bit troublesome that
> we are boxed into using a nonstandard dump-data format until forever.
>
> I don't immediately see any way around that, though. Anyone have a
> bright idea?

Are you concerned about someone dumping in SQL_STANDARD mode and
then importing in POSTGRES mode? If so, how's the similar case
handled with date order?

Ron Mayer wrote:
> Tom Lane wrote:
>> *pg_dump had better force Postgres mode*. We can certainly do that with
>> a couple more lines added to the patch, but it's a bit troublesome that
>> we are boxed into using a nonstandard dump-data format until forever.

Ok. I see that is the concern..

Rather than forcing Postgres mode; couldn't it put a
"set intervalstyle = [whatever the current interval style is]"
in the dump file?

That doesn't force us to using a nonstandard dump-data format (except
for the nonstandard "set intervalstyle" line).

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> I've started reviewing this patch for commit, and I find myself a bit
>> disturbed by its compatibility properties. The SQL_STANDARD output
>> style is simply ambiguous: what is meant by
>> -1 1:00:00
>> ? What you get from that will depend on the intervalstyle setting at
>> the recipient.

> Nope. The SQL Standard style avoids the ambiguity by following
> the SQL Standard's rules when the input value complied with the
> standard's restrictions on intervals.

You're missing the point: the same string can mean different things
to different recipients depending on their intervalstyle setting.
This means it's unsafe to use that representation in pg_dump output,
unless we take steps to force the interpretation.

> Are you concerned about someone dumping in SQL_STANDARD mode and
> then importing in POSTGRES mode?

Exactly.

> If so, how's the similar case handled with date order?

ISO date format is read the same regardless of recipient's datestyle,
so pg_dump solves this by forcing the dump to be made in ISO style.
The corresponding solution for intervals will be to dump in POSTGRES
style, not SQL_STANDARD style, which seems a bit unfortunate.

regards, tom lane

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Rather than forcing Postgres mode; couldn't it put a
> "set intervalstyle = [whatever the current interval style is]"
> in the dump file?

This would work for loading into a PG >= 8.4 server, and fail miserably
for loading into pre-8.4 servers. Even though we don't guarantee
backwards compatibility of dump files, I'm loath to adopt a solution
that will successfully load wrong data into an older server.

regards, tom lane

Tom Lane wrote:
>
> ISO date format is read the same regardless of recipient's datestyle,
> so pg_dump solves this by forcing the dump to be made in ISO style.
> The corresponding solution for intervals will be to dump in POSTGRES
> style, not SQL_STANDARD style, which seems a bit unfortunate.

[reading pg_dump.c now]

I wonder if it could be similar to standard_conforming_strings
where it appears to be reading the current value and setting it
to whatever the user chose in the beginning of pg_dump. Then we
could dump in whichever intervalstyle the user prefers. Or,
for 8.4+ dumps we could even force "set intervalstyle = sql_standard;"
in the top of the dump file. For dumps of 8.3 or less we'd need
the non-standard style anyway it seems.

If this seems sane, I can try experimenting with it tonight.

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Rather than forcing Postgres mode; couldn't it put a
>> "set intervalstyle = [whatever the current interval style is]"
>> in the dump file?
>
> This would work for loading into a PG >= 8.4 server, and fail miserably
> for loading into pre-8.4 servers. Even though we don't guarantee
> backwards compatibility of dump files, I'm loath to adopt a solution
> that will successfully load wrong data into an older server.

'k.
So the options seem to be:

(1) Don't output a SQL-standard interval literal for the
value "negative one days and negative one hours"; perhaps
by sticking an extra '+' sign in there?

(2) Force pg_dump to a non-standard mode, at least until 8.3's
deprecated in many years?
After that, pg_dump can use any intervalstyle so long as
it says which one it uses.

(3) Put something into the dump file that will make the old
server reject the file rather than successfully loading
wrong data? (Some "if intervalstyle==std and version<8.3
abort loading the restore" logic?)

I don't much like the first one, because it seems we're oh-so-close
to meeting the standard.

I don't know how to do the third one; but if pg_dump had
an "assert(version>=8.4)" feature it might be useful if
we ever had other non-backward-compatible changes. pg_dump
would only put that assert-like-thing in the dump file if
the sql_standard mode (or iso mode, if that gets approved)
was chosen.

The second one doesn't really seem that scary to me; since
the uglyness can go away when we eventually stop restoring
into 83.

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Rather than forcing Postgres mode; couldn't it put a
>> "set intervalstyle = [whatever the current interval style is]"
>> in the dump file?
>
> This would work for loading into a PG >= 8.4 server, and fail miserably
> for loading into pre-8.4 servers. Even though we don't guarantee
> backwards compatibility of dump files, I'm loath to adopt a solution
> that will successfully load wrong data into an older server.

How is the case different from standard_conforming_strings; where ISTM
depending on postgresql.conf 8.4 will happily dump either

SET standard_conforming_strings = off;
...
INSERT INTO dumptest VALUES ('\\\\');

or

SET standard_conforming_strings = on;
...
INSERT INTO dumptest VALUES ('\\');

and AFAICT the latter will happily load wrong data into 8.1 with
only the error message

ERROR: parameter "standard_conforming_strings" cannot be changed

I imagine the use-case for "standard_conforming_strings = 0 " and
"intervalstyle = sql_standard" are petty similar as well.

I wonder if the best solution is that any dump file with
standard_conforming_strings=on include some SQL that will refuse
to load in pre-8.2 systems; and that any dump file with
intervalstyle=sql_standard refuse to load in pre-8.4 systems.
It seems pretty easy to add a sql fragment that checks version()
and put that in the beginning of a dump file that uses these GUCs
to enforce this.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> So the options seem to be:

> (1) Don't output a SQL-standard interval literal for the
> value "negative one days and negative one hours"; perhaps
> by sticking an extra '+' sign in there?

This is pretty much what the postgres style does...

> (2) Force pg_dump to a non-standard mode, at least until 8.3's
> deprecated in many years?

IOW, same as above.

> (3) Put something into the dump file that will make the old
> server reject the file rather than successfully loading
> wrong data? (Some "if intervalstyle==std and version<8.3
> abort loading the restore" logic?)

There isn't any way to do that, unless you have a time machine in
your hip pocket. The trouble with putting
set intervalstyle = something;
into the dump script is that older servers will (by default) report
an error on that line and keep right on chugging. The same is true
of standard_conforming_strings BTW, which is one of the reasons why
that's not a very good solution. But at least you're reasonably likely
to get additional errors later in the dump if you try to load it into a
server that doesn't handle standard_conforming_strings. What's scaring
me about the interval stuff is that it will *silently* adopt the wrong
reading of ambiguous interval strings. A DBA who missed seeing that
one bleat early in the restore would not know anything was wrong.

You're right that we don't have to be frozen into this forever, but
I fear that any change is going to be a long way off. We couldn't
really change pg_dump's output style until we have obsoleted all
pre-8.4 releases.

regards, tom lane

BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec
literals:

regression=# select interval '-2008-10';
interval
----------------------
-2008 years -10 mons
(1 row)

regression=# select interval '-0000-10';
interval
----------
10 mons
(1 row)

Surely the latter must mean -10 months. This is orthogonal to the
current patch ...

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>
>> (3) Put something into the dump file that will make the old
>> server reject the file rather than successfully loading
>> wrong data? (Some "if intervalstyle==std and version<8.3
>> abort loading the restore" logic?)
>
> There isn't any way to do that, unless you have a time machine in
> your hip pocket. The trouble with putting
> set intervalstyle = something;
> into the dump script is that older servers will (by default) report
> an error on that line and keep right on chugging.

Not necessarily. Couldn't we put

select * from (select substring(version() from '[0-9\.]+') as version) as a
join (select generate_series(0,100000000000)) as b on(version<'8.4');

set intervalstyle = something;

Or something similar in the dump file.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> There isn't any way to do that, unless you have a time machine in
>> your hip pocket. The trouble with putting
>> set intervalstyle = something;
>> into the dump script is that older servers will (by default) report
>> an error on that line and keep right on chugging.

> Not necessarily. Couldn't we put

> select * from (select substring(version() from '[0-9\.]+') as version) as a
> join (select generate_series(0,100000000000)) as b on(version<'8.4');
> set intervalstyle = something;

> Or something similar in the dump file.

[ shrug... ] It's still just one easily missable bleat.

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Tom Lane wrote:
>>> The trouble is that older servers will (by default) report
>>> an error on that line and keep right on chugging.
>
>> Not necessarily. Couldn't we put
>
>> select * from (select substring(version() from '[0-9\.]+') as version) as a
>> join (select generate_series(0,100000000000)) as b on(version<'8.4');
>> set intervalstyle = something;
>
> [ shrug... ] It's still just one easily missable bleat.

Not here.

On my system it hangs forever on 8.3 or less and proceeds
harmlessly with 8.4.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>>> select * from (select substring(version() from '[0-9\.]+') as version) as a
>>> join (select generate_series(0,100000000000)) as b on(version<'8.4');
>>> set intervalstyle = something;
>>
>> [ shrug... ] It's still just one easily missable bleat.

> Not here.

> On my system it hangs forever on 8.3 or less and proceeds
> harmlessly with 8.4.

Oh, I see what you're trying to do. The answer is no. We're not going
to totally destroy back-portability of dumps, especially not for a
problem that won't even affect most people (negative intervals are
hardly common).

regards, tom lane

Tom Lane wrote:
> Oh, I see what you're trying to do. The answer is no. We're not going
> to totally destroy back-portability of dumps, especially not for a
> problem that won't even affect most people (negative intervals are
> hardly common).

Similarly I wonder if pg_dump should add a "fail if version < 8.2" right
before it outputs
SET standard_conforming_strings = on;
which IMHO is far more common than negative intervals and AFAICT
has the same risk.

For intervals, we would only add the fail code if intervalstyle was set
to one of the new interval styles (if the ISO8601 interval's accepted
it'll have the problem too).

For backward compatible patches, they could still have their
GUC settingse specify standard_conforming_strings and interval_style
values that are supported by whichever versions they want to support.

Tom Lane wrote:
> BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec
> literals:
> regression=# select interval '-2008-10';
> regression=# select interval '-0000-10';
> Surely the latter must mean -10 months. This is orthogonal to the
> current patch ...

Perhaps the below patch fixes that?
(though line numbers probably won't match since this was based off
of the patched version)

*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
***************
*** 2879,2885 **** DecodeInterval(char **field, int *ftype, int nf, int range,
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
! if (val < 0)
val2 = -val2;
val = val * MONTHS_PER_YEAR + val2;
fval = 0;
--- 2879,2885 ----
if (*cp != '\0')
return DTERR_BAD_FORMAT;
type = DTK_MONTH;
! if (field[0][0] == '-')
val2 = -val2;
val = val * MONTHS_PER_YEAR + val2;
fval = 0;
[5]lt:/home/ramayer/proj/pg/postgresql/src/backend/utils/adt%

Another thought here ... I'm looking at the sign hack

+ if (IntervalStyle == INTSTYLE_SQL_STANDARD &&
+ field[0][0] == '-' && i == 1 &&
+ field[i][0] != '-' && field[i][0] != '+')
+ {
+ /*----------
+ * The SQL Standard defines the interval literal
+ * '-1 1:00:00'
+ * to mean "negative 1 days and negative one hours"
+ * while Postgres traditionally treated this as
+ * meaning "negative 1 days and positive one hours".
+ * In SQL_STANDARD style, flip the sign to conform
+ * to the standard's interpretation.

and not liking it very much. Yes, it does the intended thing for strict
SQL-spec input, but it seems to produce a bunch of weird corner cases
for non-spec input. Consider

-1 1:00:00 flips the sign
- 1 1:00:00 doesn't flip the sign
-1 day 1:00:00 doesn't flip the sign
-2008-10 1:00:00 flips the sign
-2008-10 1 doesn't flip the sign
-2008 years 1:00:00 doesn't flip the sign

If the rule were that it never flipped the sign for non-SQL-spec input
then I think that'd be okay, but case 4 here puts the lie to that.
I'm also not entirely sure if case 2 is allowed by SQL spec or not,
but if it is then we've got a problem with that; and even if it isn't
it's awfully hard to explain why it's treated differently from case 1.

I'm inclined to think we need a more semantically-based instead of
syntactically-based rule. For instance, if first field is negative and
no other field has an explicit sign, then force all fields to be <= 0.
This would probably have to be applied at the end of DecodeInterval
instead of on-the-fly within the loop.

Thoughts?

regards, tom lane

Tom Lane wrote:
> Another thought here ... I'm looking at the sign hack
> + if (IntervalStyle == INTSTYLE_SQL_STANDARD &&
>....
> and not liking it very much. Yes, it does the intended thing for strict
> SQL-spec input, but it seems to produce a bunch of weird corner cases
> for non-spec input. Consider [... many examples ...]
>
> I'm inclined to think we need a more semantically-based instead of
> syntactically-based rule. For instance, if first field is negative and
> no other field has an explicit sign, then force all fields to be <= 0.
> This would probably have to be applied at the end of DecodeInterval
> instead of on-the-fly within the loop.
>
> Thoughts?

I'll take a step back and think about that....

Yes, at first glance I think that approach is better; but we'd need
to make sure not to apply the rule too enthusiastically on traditional
postgres intervals; or worse, ones that mix sql standardish and postgres
values For example
dish=# select interval '-1 1:1 1 years';
interval
--------------------------
1 year -1 days +01:01:00
(1 row)
that 8.3 accepts. (or do we not care about those)?

In some ways I wonder if we should have 2 totally separate parsing
one for the SQL standard ones, and one for the postgres.
That would avoid some other confusing inputs like:
select interval '-00-01 1 years';
select interval '1-1 hours';
select interval '1:1 years';
select interval '1 hours 1-1 1 years'
that are currently accepted.

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Tom Lane wrote:
>> BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec
>> literals:

> Perhaps the below patch fixes that?

Actually I think it should be
if (*field[i] == '-')
as in the comparable case for fractional seconds just below.
Will apply.

regards, tom lane

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Yes, at first glance I think that approach is better; but we'd need
> to make sure not to apply the rule too enthusiastically on traditional
> postgres intervals;

Well, of course we'd only apply it in SQL_STANDARD mode. The idea here
is that intervalstyle helps us resolve an ambiguity about what the signs
are, more or less independently of syntactic details. If you consider
that the issue is
sql standard: leading sign applies to all fields
postgres: each field is independently signed
this applies perfectly well without any restrictions on syntax.

> In some ways I wonder if we should have 2 totally separate parsing
> one for the SQL standard ones, and one for the postgres.

No, I think we want the style to be a hint for resolving ambiguous
cases, not to cause complete failure if the input doesn't conform to the
style. That's certainly how DateStyle has always worked.

regards, tom lane

I wrote:
> ... Consider

> -1 1:00:00 flips the sign
> - 1 1:00:00 doesn't flip the sign
> -1 day 1:00:00 doesn't flip the sign
> -2008-10 1:00:00 flips the sign
> -2008-10 1 doesn't flip the sign
> -2008 years 1:00:00 doesn't flip the sign

> If the rule were that it never flipped the sign for non-SQL-spec input
> then I think that'd be okay, but case 4 here puts the lie to that.
> I'm also not entirely sure if case 2 is allowed by SQL spec or not,
> but if it is then we've got a problem with that; and even if it isn't
> it's awfully hard to explain why it's treated differently from case 1.

Actually case 2 is a red herring --- I now see that ParseDateTime()
collapses out the whitespace and makes it just like case 1. So never
mind that. Case 4 is still bogus though.

regards, tom lane

Doesn't ANSI standard interval syntax have the minus sign before the quotes?

Select interval -'2008-10';

???

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org [mailto:pgsql-hackers-
> owner(at)postgresql(dot)org] On Behalf Of Tom Lane
> Sent: Saturday, November 08, 2008 11:39 AM
> To: Ron Mayer
> Cc: Brendan Jurd; Kevin Grittner; pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] Patch for SQL-Standard Interval output and
> decoupling DateStyle from IntervalStyle
>
> BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-
> spec
> literals:
>
> regression=# select interval '-2008-10';
> interval
> ----------------------
> -2008 years -10 mons
> (1 row)
>
> regression=# select interval '-0000-10';
> interval
> ----------
> 10 mons
> (1 row)
>
> Surely the latter must mean -10 months. This is orthogonal to the
> current patch ...
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
> Brendan Jurd wrote:
>> The changes to the documentation all look good. I did notice one
>> final typo that I think was introduced in the latest version.
>> doc/src/sgml/datatype.sgml:2270 has "Nonstandardrd" instead of
>> "Nonstandard".

> Just checked in a fix to that one; and updated my website at
> http://0ape.com/postgres_interval_patches/
> and pushed it to my (hopefully fixed now) git server.

Applied with some revisions: I changed the rule for negating input
fields in SQL_STANDARD style as we discussed, made IntervalStyle into
an "enum" GUC, and did some pretty heavy editorialization on the docs
changes.

regards, tom lane

Chuck McDevitt <cmcdevitt(at)greenplum(dot)com> writes:
> Doesn't ANSI standard interval syntax have the minus sign before the quotes?
> Select interval -'2008-10';

They allow it either there or inside the quotes.

We can't support outside-the-quotes unless we make INTERVAL a fully
reserved word (and even then there are syntactic problems :-().

Putting the minus sign before INTERVAL works fine btw ...

regards, tom lane

Tom Lane wrote:
> Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> writes:
>> Brendan Jurd wrote:
>>> ...I did notice one final ...
>> Just checked in a fix to that one; and updated my website at
>> http://0ape.com/postgres_interval_patches/
>> and pushed it to my (hopefully fixed now) git server.
>
> Applied with some revisions: I changed the rule for negating input
> fields in SQL_STANDARD style as we discussed, made IntervalStyle into
> an "enum" GUC, and did some pretty heavy editorialization on the docs
> changes.

Cool. Thanks!

Brendan and anyone else looking at these, I've done an initial merge
between the new CVS head and my other interval patches and pushed
them to my website link mentioned above; but I probably still need
to cleanup some of the docs merges. I'm a bit tied up today; so
will probably cleanup the merge and study Tom's changes and post an
update when the other patches are ready to continue reviewing, probably
late tomorrow.

On Sat, Nov 8, 2008 at 2:19 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>
> Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm
> now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear?
>

Sorry, I don't understand what you mean by "5.5.4.2.1". In the spec
you linked to, clause 5 "Date and time format representations" doesn't
have any numbered subsections at all. It's just a half-page saying,
basically, that if applications want to interchange information about
datetime formats, they can. Much like the ents, spec authors don't
like to say anything unless it's worth taking a very long time to say.

So, to the best of my knowledge, there is no 5.5.4.2.1. There is no 5.5.

Originally I assumed that when you wrote 5.5.4.2.1, you meant
4.4.4.2.1. However, when I looked closer I noticed that this section
is about a textual "representation" of the format, not about the
format itself. Therefore I suggested 4.4.3.2, which does specify the
format.

Cheers,
BJ

Brendan Jurd wrote:
> On Sat, Nov 8, 2008 at 2:19 AM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm
>> now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear?
>
> Sorry, I don't understand what you mean by "5.5.4.2.1". In the spec

Ah! That 5.5.4.2.1 comes from apparently an old Oct 2000
draft version of the spec titled ISO/FDIS 8601. (For now you can
see it here: http://0ape.com/postgres_interval_patches/ISO-FDIS-8601.pdf )

I'll fix all the links to point to the 2004 spec.

Ron Mayer wrote:
> Ah! That 5.5.4.2.1 comes from apparently an old Oct 2000
> draft version of the spec titled ISO/FDIS 8601. (For now you can
> see it here: http://0ape.com/postgres_interval_patches/ISO-FDIS-8601.pdf )
>
> I'll fix all the links to point to the 2004 spec.

I updated my web site[1] with the latest version of this patch.

Main differences since last time
* Merged with the IntervalStyle patch as it was checked into CVS.
* Fixed references to consistently refer to the same version of
the ISO 8601 spec (ISO 8601:2004(E))

[1] http://0ape.com/postgres_interval_patches/

PS: I realize that this patch makes datetime.c a bit longer that it needs
to be; and that some of the functions added in this patch can be used by the
other interval styles as well. "patch 3" that can be found on the same HTML
page does this refactoring.

On Tue, Nov 11, 2008 at 2:36 AM, Ron Mayer
<rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> I updated my web site[1] with the latest version of this patch.

I'm just testing this latest version out now.

I get the expected result from 'P0001', but oddly enough if I specify
only the year and month, it pukes:

postgres=# select interval 'P0001-01';
ERROR: invalid input syntax for type interval: "P0001-01"
LINE 1: select interval 'P0001-01';

I'm attaching a patch to clean up a few more code style issues and fix
broken spec references within C code comments in datetime.c.

Cheers,
BJ

Brendan Jurd wrote:
> On Tue, Nov 11, 2008 at 2:36 AM, Ron Mayer
> I get the expected result from 'P0001', but oddly enough if I specify
> only the year and month, it pukes:
> postgres=# select interval 'P0001-01';

Indeed. Thanks again.

I've fixed this and added regression tests to check the handling of
optional fields of the "alternative format" which my patch has
been so very bad at handling.

> I'm attaching a patch to clean up a few more code style issues and fix
> broken spec references within C code comments in datetime.c.

Applied and pushed to the website http://0ape.com/postgres_interval_patches/

On Tue, Nov 11, 2008 at 5:51 AM, R Mayer
<pg_cert(at)cheapcomplexdevices(dot)com> wrote:
> Applied and pushed to the website http://0ape.com/postgres_interval_patches/
>

This latest version works as expected and I don't detect any other
issues with the code or documentation ... seems I've run out of things
to gripe about!

I'm ready to sign off on this patch now and move on to the final
cleanup patch. I'll update the commitfest to show this one as "ready
for committer".

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> I'm ready to sign off on this patch now and move on to the final
> cleanup patch. I'll update the commitfest to show this one as "ready
> for committer".

OK, I'll pick this one up now.

regards, tom lane

On Sat, Nov 1, 2008 at 3:42 PM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> # Patch 3: "cleanup.patch"
> Fix rounding inconsistencies and refactor interval input/output code
>

Compile, testing and regression tests all checked out.

I've picked up on a few code style issues, fixes attached.

If I'm reading the patch correctly, it seems you've renamed two of the
functions in datetime.c:

* AdjustFractionalSeconds => AdjustFractSeconds
* AdjustFractionalDays => AdjustFractDays

To be frank, this doesn't really seem worthwhile. It only saves five
characters in the name. What was your reason for renaming them?

I *was* going to question the inconsistent use of a space between the
pointer qualifier and the argument name, for example:

static char *
AddVerboseIntPart(char * cp, int value, char *units,
bool * is_zero, bool *is_before)

But then I noticed that there's a lot of this going on in datetime.c,
some of it appears to predate your patches. So I guess cleaning this
up in your function definitions would be a bit of a bolted-horse,
barn-door affair. Unless you felt like cleaning it up throughout the
file, it's probably not worth worrying about.

There are some very large-scale changes to the regression tests. I'm
finding it difficult to grok the nature of the changes from reading a
diff. If possible, could you post some quick notes on the
purpose/rationale of these changes?

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> I *was* going to question the inconsistent use of a space between the
> pointer qualifier and the argument name, for example:
> ...
> But then I noticed that there's a lot of this going on in datetime.c,
> some of it appears to predate your patches.

Any inconsistency there is pg_indent's fault (caused by not knowing that
some words are typedefs). There's no great value in messing with it
manually, because pg_indent will set the spacing to suit itself anyway.

regards, tom lane

R Mayer <pg_cert(at)cheapcomplexdevices(dot)com> writes:
> Applied and pushed to the website http://0ape.com/postgres_interval_patches/

I ran into an interesting failure case:

regression=# select interval 'P-1Y-2M3DT-4H-5M-6';
interval
-------------------
P-1Y-2M3DT-10H-5M
(1 row)

This isn't the result I'd expect, and AFAICS the ISO spec does *not*
allow any unit markers to be omitted in the format with designators.
I think the problem is that the code will accept a value as being
alternative format even when it's already read some
format-with-designator fields. I propose adding a flag to remember
that we've seen a field in the current part (date or time) and rejecting
an apparent alternative-format input if the flag is set.

regards, tom lane

R Mayer <pg_cert(at)cheapcomplexdevices(dot)com> writes:
> Applied and pushed to the website http://0ape.com/postgres_interval_patches/

Applied with nontrivial revisions --- I fear I probably broke your third
patch again :-(. There were still a number of parsing bugs, and I also
didn't like the lack of error checking around the strtod() call ...

regards, tom lane

Tom Lane wrote:
> ...failure case ... interval 'P-1Y-2M3DT-4H-5M-6';
> This isn't the result I'd expect, and AFAICS the ISO spec does *not*
> allow any unit markers to be omitted in the format with designators.

Yes, this is true. I see you already made the change.

Tom Lane wrote:
> Applied with nontrivial revisions --- I fear I probably broke your third
> patch again :-(.

No problem. It wasn't hard to update. Attached is an updated patch (as
well as being updated on my website; but since it applies to HEAD it's
as easy to get here).

The bulk of the changes are in regression tests where rounding of
fractional seconds was changed as discussed up-thread back in Sep.

Seems I should also submit one more patch that merge the newest
DecodeInterval, EncodeInterval and related functions into
/ecpg/pgtypeslib/interval.c?

And beyond that, there's still some eccentricities with the interval code
(why's "interval '1 year 1 year'" ok but "interval '1 second 1 second'" not)
but I don't know if I'd do more harm or good trying to look at those.

Brendan Jurd wrote:
> On Sat, Nov 1, 2008 at 3:42 PM, Ron Mayer <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> # Patch 3: "cleanup.patch"
>> Fix rounding inconsistencies and refactor interval input/output code
>
> Compile, testing and regression tests all checked out.
> I've picked up on a few code style issues, fixes attached.
>
> If I'm reading the patch correctly, it seems you've renamed two of the
> functions in datetime.c:
> * AdjustFractionalSeconds => AdjustFractSeconds
> * AdjustFractionalDays => AdjustFractDays
> To be frank, this doesn't really seem worthwhile. It only saves five
> characters in the name. What was your reason for renaming them?

Otherwise many lines were over 80 chars long.
And it happened often enough I thought the shorter name
was less ugly than splitting the arguments in many of the
places where it's called.

I'm happy either way, tho.

> I *was* going to question the inconsistent use of a space between the
> pointer qualifier and the argument name, for example:
>
> static char *
> AddVerboseIntPart(char * cp, int value, char *units,
> bool * is_zero, bool *is_before)
>
> But then I noticed that there's a lot of this going on in datetime.c,
> some of it appears to predate your patches. So I guess cleaning this
> up in your function definitions would be a bit of a bolted-horse,
> barn-door affair. Unless you felt like cleaning it up throughout the
> file, it's probably not worth worrying about.

I don't mindn cleaning it up; but someone could point me to which direction.

> There are some very large-scale changes to the regression tests. I'm
> finding it difficult to grok the nature of the changes from reading a
> diff. If possible, could you post some quick notes on the
> purpose/rationale of these changes?

Previously, much (but IIRC not quite all) of the interval output stuff
rounded to the hundredths place regardless of how many significant digits
there were. So, for example, the interval "1.699" seconds would usually
appear as "1.70" for most but not all combinations of DateStyle
and HAVE_INT64_TIMESTAMP. After a few discussions on the mailing list
I think people decided to simply show the digits that were

http://archives.postgresql.org/pgsql-hackers/2008-09/msg00998.php

On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer
<rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> Brendan Jurd wrote:
>> * AdjustFractionalSeconds => AdjustFractSeconds
>> * AdjustFractionalDays => AdjustFractDays
>
> Otherwise many lines were over 80 chars long.
> And it happened often enough I thought the shorter name
> was less ugly than splitting the arguments in many of the
> places where it's called.

Fair enough. I don't have a strong opinion about that.

>> I *was* going to question the inconsistent use of a space between the
>> pointer qualifier and the argument name, for example:
>>
> I don't mindn cleaning it up; but someone could point me to which direction.
>

Per Tom's comments, the space is sometimes inserted by pg_indent,
because it doesn't know all of the typedefs. So there's no point
trying to clean it up at the moment; next time pg_indent is run over
the code, it will just insert those spaces again.

>> There are some very large-scale changes to the regression tests. ...
>
> Previously, much (but IIRC not quite all) of the interval output stuff
> rounded to the hundredths place regardless of how many significant digits
> there were.

I understood about the rounding issues. I was a bit confused by the
fact that the patch shows differences for an entire table of results
from the horology test, but I've just now realised that the whole
table is different because changing the output precision in some of
the rows has altered the column width.

Makes me wonder whether an unaligned psql output format would be a
better choice for the regression tests. It would certainly make for
clearer diffs. But that's a tangent for another email.

I don't have any further gripes regarding this patch, apart from the
code style stuff I sent through in my previous post. Did you have any
response to those?

Cheers,
BJ

Brendan Jurd wrote:
> On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer
> <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> Brendan Jurd wrote:
>>> * AdjustFractionalSeconds => AdjustFractSeconds
>>> * AdjustFractionalDays => AdjustFractDays
>> Otherwise many lines were over 80 chars long.
>> And it happened often enough I thought the shorter name
>> was less ugly than splitting the arguments in many of the
>> places where it's called.
>
> Fair enough. I don't have a strong opinion about that.

Cool. If anyone does have an opinion on that, let me know
and I can change it whichever way people prefer.

>>> There are some very large-scale changes to the regression tests. ...
>> Previously, much (but IIRC not quite all) of the interval output stuff
>> rounded to the hundredths place regardless of how many significant digits
>> there were.
>
> I understood about the rounding issues. I was a bit confused by the
> fact that the patch shows differences for an entire table of results
> from the horology test, but I've just now realised that the whole
> table is different because changing the output precision in some of
> the rows has altered the column width.
>
> Makes me wonder whether an unaligned psql output format would be a
> better choice for the regression tests. It would certainly make for
> clearer diffs. But that's a tangent for another email.

Yeah. And that's what made the patch so big I had to gzip it.

> I don't have any further gripes regarding this patch, apart from the
> code style stuff I sent through in my previous post. Did you have any
> response to those?

Yup - you were right again.
Applied them and updated the website and attaching the patch.

I wonder what's the best way for myself to get out of those habits
in the future? Some lint flags or similar?

On Wed, Nov 12, 2008 at 6:13 AM, Ron Mayer
<rm_pg(at)cheapcomplexdevices(dot)com> wrote:
> Brendan Jurd wrote:
>> I don't have any further gripes regarding this patch, apart from the
>> code style stuff I sent through in my previous post. Did you have any
>> response to those?
>
> Yup - you were right again.
> Applied them and updated the website and attaching the patch.
>

Cool. In that case I'm ready to kick this up to a committer.

> I wonder what's the best way for myself to get out of those habits
> in the future? Some lint flags or similar?

Can't help you there I'm afraid. vi takes care of much of the
indentation and such, but my patches have had their fair share of code
and error message style problems too. On the bright side I've found
that switching between code conventions does get easier with practice.
Currently my strategy consists largely of "read the patch over
several times before submitting it". Even so, mistakes sometimes slip
through.

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer
> <rm_pg(at)cheapcomplexdevices(dot)com> wrote:
>> Brendan Jurd wrote:
>>> There are some very large-scale changes to the regression tests. ...
>>
>> Previously, much (but IIRC not quite all) of the interval output stuff
>> rounded to the hundredths place regardless of how many significant digits
>> there were.