Re: Patch for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle

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.