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

On Thu, Sep 18, 2008 at 6:03 AM, 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).
>

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.outTue Nov  4 14:46:34 2008
--- /home/direvus/src/postgres/src/test/regress/results/interval.outTue 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