Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
Ron Mayer <[EMAIL PROTECTED]> writes: > Once this settles I suppose I should post a ECPG patch that's based > off of these Decode/Encode interval functions too? Yeah, if you want. I think you'll find that the datetime code has drifted far enough since ecpg forked it that you'll be looking at a pretty huge diff :-( regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
Tom Lane wrote: The original INT64 coding here is exact (at least for the common case where fval is zero) but I'm not convinced that your revision can't suffer from roundoff error. Good point. I'll study this tonight; and either try to make a patch that'll be exact where fval's zero or try to come up with convincing reasons that it's harmless. Once this settles I suppose I should post a ECPG patch that's based off of these Decode/Encode interval functions too? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
Ron Mayer <[EMAIL PROTECTED]> writes:
> 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.
Applied with another round of mostly-stylistic revisions, plus a little
extra work to factor out some more code duplication (around strtod
calls, which were insufficiently error-checked too).
There was one part I left out because it worried me:
***
*** 2980,3010
switch (type)
{
case DTK_MICROSEC:
! #ifdef HAVE_INT64_TIMESTAMP
! *fsec += rint(val + fval);
! #else
! *fsec += (val + fval) * 1e-6;
! #endif
tmask = DTK_M(MICROSECOND);
break;
case DTK_MILLISEC:
! #ifdef HAVE_INT64_TIMESTAMP
! *fsec += rint((val + fval) * 1000);
! #else
! *fsec += (val + fval) * 1e-3;
! #endif
tmask = DTK_M(MILLISECOND);
break;
case DTK_SECOND:
tm->tm_sec += val;
! #ifdef HAVE_INT64_TIMESTAMP
! *fsec += rint(fval * 100);
! #else
! *fsec += fval;
! #endif
!
/*
* If any subseconds were specified, consider this
* microsecond and millisecond input as well.
--- 2897,2914
switch (type)
{
case DTK_MICROSEC:
! AdjustFractSeconds((val + fval) * 1e-6, tm, fsec, 1);
tmask = DTK_M(MICROSECOND);
break;
case DTK_MILLISEC:
! AdjustFractSeconds((val + fval) * 1e-3, tm, fsec, 1);
tmask = DTK_M(MILLISECOND);
break;
case DTK_SECOND:
tm->tm_sec += val;
! AdjustFractSeconds(fval, tm, fsec, 1);
/*
* If any subseconds were specified, consider this
* microsecond and millisecond input as well.
The original INT64 coding here is exact (at least for the common case
where fval is zero) but I'm not convinced that your revision can't
suffer from roundoff error.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer > <[EMAIL PROTECTED]> 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. > 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. diff --ignore-spaces is the best way to analyze that type of situation. (I believe this is what pg_regress does by default.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
On Wed, Nov 12, 2008 at 6:13 AM, Ron Mayer <[EMAIL PROTECTED]> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
Brendan Jurd wrote: On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer <[EMAIL PROTECTED]> 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? cleanup.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
On Wed, Nov 12, 2008 at 5:32 AM, Ron Mayer <[EMAIL PROTECTED]> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
Brendan Jurd wrote: On Sat, Nov 1, 2008 at 3:42 PM, Ron Mayer <[EMAIL PROTECTED]> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
"Brendan Jurd" <[EMAIL PROTECTED]> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
On Sat, Nov 1, 2008 at 3:42 PM, Ron Mayer <[EMAIL PROTECTED]> 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 cleanup-1.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
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. cleanup.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
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.
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***
*** 1975,1980 January 8 04:05:06 1999 PST
--- 1975,1996
+ Alternatively, interval values can be written as
+ ISO 8601 time intervals, using the "Format with time-unit designators",
+ or PnYnMnDTnHnMnS. This format always starts with the character
+ 'P', followed by a string of values followed by single
+ character time-unit designators. A 'T' separates the
+ date and time parts of the interval.
+ In this format, 'n' gets replaced by a number, and
+Y represents years,
+M (in the date part) months,
+D months,
+H hours,
+M (in the time part) minutes,
+and S seconds.
+
+
+
Internally interval values are stored as months, days,
and seconds. This is done because the number of days in a month
varies, and a day can have 23 or 25 hours if a daylight savings
***
*** 2224,2230 January 8 04:05:06 1999 PST
The output format of the interval types can be set to one of the
! three styles sql_standard,
postgres, or postgres_verbose.
The default is the postgres format.
The output format of the interval types can be set to one of the
! four styles sql_standard, iso_8601,
postgres, or postgres_verbose.
The default is the postgres format.
+ The iso_8601 style will output ISO 8601
+ time intervals using the "format with time-unit designators"
+ This format always starts with the character
+ 'P', followed by a string of values followed by single
+ character time-unit designators. A 'T' separates the
+ date and time parts of the interval.
+ In this format, 'n' gets replaced by a number, and
+Y represents years,
+M (in the date part) months,
+D months,
+H hours,
+M (in the time part) minutes,
+and S seconds.
+
+
+
The postgres style will output intervals that match
the style PostgreSQL 8.3 outputed when the
parameter was set to ISO.
***
*** 2274,2279 January 8 04:05:06 1999 PST
--- 2306,2317
-1-2 +3 -4:05:06
+ iso_8601
+ P1Y2M
+ P3DT4H5M6
+ P-1Y-2M3DT-4H-5M-6
+
+
postgres
1 year 2 mons
3 days 04:05:06
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 248,253 assign_intervalstyle(const char *value, bool doit, GucSource source)
--- 248,257
{
newIntervalStyle = INTSTYLE_SQL_STANDARD;
}
+ else if (pg_strcasecmp(value, "iso_8601") == 0)
+ {
+ newIntervalStyle = INTSTYLE_ISO_8601;
+ }
else
{
ereport(GUC_complaint_elevel(source),
*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
***
*** 2723,2728 DecodeSpecial(int field, char *lowtoken, int *val)
--- 2723,2865
}
+ /*
+ * Small helper functions to avoid cut&paste code in DecodeIso8601Interval
+ */
+ static void
+ adjust_fractional_seconds(double fval,struct pg_tm * tm, fsec_t *fsec, int scale)
+ {
+ int sec;
+ if (fval == 0) return;
+ fval *= scale;
+ sec = fval;
+ tm->tm_sec += sec;
+ #ifdef HAVE_INT64_TIMESTAMP
+ *fsec += rint((fval - sec) * 100);
+ #else
+ *fsec += (fval - sec);
+ #endif
+ }
+
+ static void
+ adjust_fractional_days(double fval,struct pg_tm * tm, fsec_t *fsec, int scale)
+ {
+ int extra_days;
+ if (fval == 0) return;
+ fval*= scale;
+ extra_days = fval;
+ tm->tm_mday += extra_days;
+ fval-= extra_days;
+ adjust_fractional_seconds(fval,tm,fsec, SECS_PER_DAY);
+ }
+
+
+ /* DecodeISO8601Interval()
+ * Decode an ISO 8601 "Representation of time-interval by
+ * duration only basic extended format" from ISO 8601 section 5.5.4.2
+
[HACKERS] Re: Updated interval patches (SQL std output, ISO8601 intervals, and interval rounding)
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/
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 4016,4021 SET XML OPTION { DOCUMENT | CONTENT };
--- 4016,4043
+
+ IntervalStyle (string)
+
+IntervalStyle configuration parameter
+
+
+
+ Sets the display format for interval values.
+ The value sql_standard will output SQL Standard
+ strings when given intervals that conform to the SQL
+ standard (either year-month only or date-time only; and no
+ mixing of positive and negative components).
+ 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'.
+
+
+
+
timezone (string)
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
***
*** 2213,2218 January 8 04:05:06 1999 PST
--- 2213,2305
+
+ Interval Output
+
+
+ interval
+ output format
+ formatting
+
+
+
+ The output format of the interval types can be set to one of the
+ three styles sql_standard,
+ postgres, or postgres_verbose.
+ The default is the postgres format.
+ shows examples of each
+ output style.
+
+
+
+ The sql_standard style will output SQL standard
+ interval literal strings where the value of the interval
+ value consists of only a year-month component or a datetime
+ component (as required by the sql standard). For an interval
+ containing both a year-month and a datetime component, the
+ output will be a SQL Standard unquoted year-month literal
+ string joined to a SQL Standard unquoted datetime literal
+ string with a space in between.
+
+
+
+ The postgres style will output intervals that match
+ the style PostgreSQL 8.3 outputed when the
+ parameter was set to ISO.
+
+
+
+ The postgres_verbose style will output intervals that match
+ the style PostgreSQL 8.3 outputed when the
+ parameter was set to SQL.
+
+
+
+ Interval Style Example
+
+
+
+ Style Specification
+ Year-Month Interval
+ DateTime Interval
+ Nonstandardrd Extended Interval
+
+
+
+
+ sql_standard
+ 1-2
+ 3 4:05:06
+ -1-2 +3 -4:05:06
+
+
+ postgres
+ 1 year 2 mons
+ 3 days 04:05:06
+ -1 year -2 mons +3 days -04:05:06
+
+
+ postgres_verbose
+ @ 1 year 2 mons
+ @ 3 days 4 hours 5 mins 6 secs
+ @ 1 year 2 mons -3 days 4 hours 5 mins 6 secs ago
+
+
+
+
+
+
+ Note that sql_standard style will only produce strictly
+ standards-conforming interval literals when given a strictly SQL-standard
+ interval value - meaning that it needs to be a pure year-month or datetime
+ interval and not mix positive and negative components.
+
+
+
+
+
+
Time Zones
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 229,234 assign_datestyle(const char *value, bool doit, GucSource source)
--- 229,271
/*
+ * assign_intervalstyle: GUC assign_hook for datestyle
+ */
+ const char *
+ assign_intervalstyle(const char *value, bool doit, GucSource source)
+ {
+ int newIntervalStyle = IntervalStyle;
+ char * result = (char *) malloc(32);
+ if (pg_strcasecmp(value
