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

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
>