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

Brendan Jurd wrote:
> ...Sep 18, 2008... Ron Mayer <rm_pg@cheapcomplexdevices.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.