Re: [HACKERS] [BUGS] BUG #3431: age() gets the days wrong

2007-07-17 Thread Bruce Momjian

I don't see this as applied yet.

---

Tom Lane wrote:
 Pelle Johansson [EMAIL PROTECTED] writes:
  The age() function seem to work by first counting months until less than a
  month remains to to the second argument, then counting days left. This
  doesn't give the correct result, as shown by this example:
 
  # select column1, age(column1, '2006-11-02'), date '2006-11-02' +
  age(column1, '2006-11-02') from (values ('2007-01-31'::date),
  ('2007-02-01')) as alias;
column1   |  age   |  ?column?   
  ++-
   2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
   2007-02-01 | 2 mons 29 days | 2007-01-31 00:00:00
  (2 rows)
 
 I took another look at this example.  I believe what is actually going
 wrong here is that when timestamp_age converts a month into an
 equivalent number of days, it uses the number of days in the first
 month of the interval it's dealing with (ie, the month containing
 the earlier of the two dates).  This is just wrong, because interval
 addition adds months first and then days.  The appropriate conversion
 to use is actually the length of the next-to-last month of the interval.
 
 As an example, 8.2 and CVS HEAD produce
 
 regression=# select age('2007-03-14', '2007-02-15');
age   
 -
  27 days
 (1 row)
 
 which is reasonable, but
 
 regression=# select age('2007-04-14', '2007-02-15');
   age  
 ---
  1 mon 27 days
 (1 row)
 
 is not so reasonable, nor is
 
 regression=# select age('2007-03-14', '2007-01-15');
   age  
 ---
  1 mon 30 days
 (1 row)
 
 If we change the code to use the next-to-last month of the interval
 then these two cases produce '1 mon 30 days' and '1 mon 27 days'
 respectively.
 
 Another problem is that the code isn't doing the propagate-to-next-field
 bit for negative fractional seconds.  Hence it produces
 
 regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:00.4');
  age  
 --
  30 days -00:00:00.40
 (1 row)
 
 which is maybe not incorrect, but certainly fairly inconsistent with
 
 regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:01');
age
 --
  29 days 23:59:59
 (1 row)
 
 
 Hence I propose the attached patch.  This does not change any existing
 regression test outputs, but it does change the example given in the
 documentation: age(timestamp '2001-04-10', timestamp '1957-06-13')
 will now produce '43 years 9 mons 28 days' not 27 days.  Which actually
 is correct if you try to add back the result to timestamp '1957-06-13'.
 It also appears to fix Palle's example:
 
 regression=# select column1, age(column1, '2006-11-02'), date '2006-11-02' +
 age(column1, '2006-11-02') from (values ('2007-01-31'::date),
 ('2007-02-01')) as alias;
   column1   |  age   |  ?column?   
 ++-
  2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
  2007-02-01 | 2 mons 30 days | 2007-02-01 00:00:00
 (2 rows)
 
 As I said earlier, I'm worried about changing the behavior of a function
 that's been around for so long, so I'm disinclined to back-patch this.
 But it seems like a reasonable change to make in 8.3.  Comments?
 
   regards, tom lane
 


-- 
  Bruce Momjian  [EMAIL PROTECTED]  http://momjian.us
  EnterpriseDB   http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate


Re: [HACKERS] [BUGS] BUG #3431: age() gets the days wrong

2007-07-17 Thread Bruce Momjian

Sorry, I see there was later discussion.

---

Tom Lane wrote:
 Pelle Johansson [EMAIL PROTECTED] writes:
  The age() function seem to work by first counting months until less than a
  month remains to to the second argument, then counting days left. This
  doesn't give the correct result, as shown by this example:
 
  # select column1, age(column1, '2006-11-02'), date '2006-11-02' +
  age(column1, '2006-11-02') from (values ('2007-01-31'::date),
  ('2007-02-01')) as alias;
column1   |  age   |  ?column?   
  ++-
   2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
   2007-02-01 | 2 mons 29 days | 2007-01-31 00:00:00
  (2 rows)
 
 I took another look at this example.  I believe what is actually going
 wrong here is that when timestamp_age converts a month into an
 equivalent number of days, it uses the number of days in the first
 month of the interval it's dealing with (ie, the month containing
 the earlier of the two dates).  This is just wrong, because interval
 addition adds months first and then days.  The appropriate conversion
 to use is actually the length of the next-to-last month of the interval.
 
 As an example, 8.2 and CVS HEAD produce
 
 regression=# select age('2007-03-14', '2007-02-15');
age   
 -
  27 days
 (1 row)
 
 which is reasonable, but
 
 regression=# select age('2007-04-14', '2007-02-15');
   age  
 ---
  1 mon 27 days
 (1 row)
 
 is not so reasonable, nor is
 
 regression=# select age('2007-03-14', '2007-01-15');
   age  
 ---
  1 mon 30 days
 (1 row)
 
 If we change the code to use the next-to-last month of the interval
 then these two cases produce '1 mon 30 days' and '1 mon 27 days'
 respectively.
 
 Another problem is that the code isn't doing the propagate-to-next-field
 bit for negative fractional seconds.  Hence it produces
 
 regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:00.4');
  age  
 --
  30 days -00:00:00.40
 (1 row)
 
 which is maybe not incorrect, but certainly fairly inconsistent with
 
 regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:01');
age
 --
  29 days 23:59:59
 (1 row)
 
 
 Hence I propose the attached patch.  This does not change any existing
 regression test outputs, but it does change the example given in the
 documentation: age(timestamp '2001-04-10', timestamp '1957-06-13')
 will now produce '43 years 9 mons 28 days' not 27 days.  Which actually
 is correct if you try to add back the result to timestamp '1957-06-13'.
 It also appears to fix Palle's example:
 
 regression=# select column1, age(column1, '2006-11-02'), date '2006-11-02' +
 age(column1, '2006-11-02') from (values ('2007-01-31'::date),
 ('2007-02-01')) as alias;
   column1   |  age   |  ?column?   
 ++-
  2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
  2007-02-01 | 2 mons 30 days | 2007-02-01 00:00:00
 (2 rows)
 
 As I said earlier, I'm worried about changing the behavior of a function
 that's been around for so long, so I'm disinclined to back-patch this.
 But it seems like a reasonable change to make in 8.3.  Comments?
 
   regards, tom lane
 

Content-Description: age.patch

 Index: timestamp.c
 ===
 RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
 retrieving revision 1.179
 diff -c -r1.179 timestamp.c
 *** timestamp.c   6 Jul 2007 04:15:59 -   1.179
 --- timestamp.c   8 Jul 2007 19:45:04 -
 ***
 *** 3044,3050 
   if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
   timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
   {
 ! fsec = (fsec1 - fsec2);
   tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
   tm-tm_min = tm1-tm_min - tm2-tm_min;
   tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
 --- 3044,3051 
   if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
   timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
   {
 ! /* form the symbolic difference */
 ! fsec = fsec1 - fsec2;
   tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
   tm-tm_min = tm1-tm_min - tm2-tm_min;
   tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
 ***
 *** 3064,3069 
 --- 3065,3081 
   tm-tm_year = -tm-tm_year;
   }
   
 + /* propagate any negative fields into the next higher field */
 + while (fsec  0)
 + {
 + #ifdef HAVE_INT64_TIMESTAMP
 + fsec += USECS_PER_SEC;
 + #else
 + fsec += 1.0;
 + #endif
 + tm-tm_sec--;
 + }
 + 
   

Re: [HACKERS] [BUGS] BUG #3431: age() gets the days wrong

2007-07-08 Thread Tom Lane
Pelle Johansson [EMAIL PROTECTED] writes:
 The age() function seem to work by first counting months until less than a
 month remains to to the second argument, then counting days left. This
 doesn't give the correct result, as shown by this example:

 # select column1, age(column1, '2006-11-02'), date '2006-11-02' +
 age(column1, '2006-11-02') from (values ('2007-01-31'::date),
 ('2007-02-01')) as alias;
   column1   |  age   |  ?column?   
 ++-
  2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
  2007-02-01 | 2 mons 29 days | 2007-01-31 00:00:00
 (2 rows)

I took another look at this example.  I believe what is actually going
wrong here is that when timestamp_age converts a month into an
equivalent number of days, it uses the number of days in the first
month of the interval it's dealing with (ie, the month containing
the earlier of the two dates).  This is just wrong, because interval
addition adds months first and then days.  The appropriate conversion
to use is actually the length of the next-to-last month of the interval.

As an example, 8.2 and CVS HEAD produce

regression=# select age('2007-03-14', '2007-02-15');
   age   
-
 27 days
(1 row)

which is reasonable, but

regression=# select age('2007-04-14', '2007-02-15');
  age  
---
 1 mon 27 days
(1 row)

is not so reasonable, nor is

regression=# select age('2007-03-14', '2007-01-15');
  age  
---
 1 mon 30 days
(1 row)

If we change the code to use the next-to-last month of the interval
then these two cases produce '1 mon 30 days' and '1 mon 27 days'
respectively.

Another problem is that the code isn't doing the propagate-to-next-field
bit for negative fractional seconds.  Hence it produces

regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:00.4');
 age  
--
 30 days -00:00:00.40
(1 row)

which is maybe not incorrect, but certainly fairly inconsistent with

regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:01');
   age
--
 29 days 23:59:59
(1 row)


Hence I propose the attached patch.  This does not change any existing
regression test outputs, but it does change the example given in the
documentation: age(timestamp '2001-04-10', timestamp '1957-06-13')
will now produce '43 years 9 mons 28 days' not 27 days.  Which actually
is correct if you try to add back the result to timestamp '1957-06-13'.
It also appears to fix Palle's example:

regression=# select column1, age(column1, '2006-11-02'), date '2006-11-02' +
age(column1, '2006-11-02') from (values ('2007-01-31'::date),
('2007-02-01')) as alias;
  column1   |  age   |  ?column?   
++-
 2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
 2007-02-01 | 2 mons 30 days | 2007-02-01 00:00:00
(2 rows)

As I said earlier, I'm worried about changing the behavior of a function
that's been around for so long, so I'm disinclined to back-patch this.
But it seems like a reasonable change to make in 8.3.  Comments?

regards, tom lane

Index: timestamp.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.179
diff -c -r1.179 timestamp.c
*** timestamp.c 6 Jul 2007 04:15:59 -   1.179
--- timestamp.c 8 Jul 2007 19:45:04 -
***
*** 3044,3050 
if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
{
!   fsec = (fsec1 - fsec2);
tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
tm-tm_min = tm1-tm_min - tm2-tm_min;
tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
--- 3044,3051 
if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
{
!   /* form the symbolic difference */
!   fsec = fsec1 - fsec2;
tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
tm-tm_min = tm1-tm_min - tm2-tm_min;
tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
***
*** 3064,3069 
--- 3065,3081 
tm-tm_year = -tm-tm_year;
}
  
+   /* propagate any negative fields into the next higher field */
+   while (fsec  0)
+   {
+ #ifdef HAVE_INT64_TIMESTAMP
+   fsec += USECS_PER_SEC;
+ #else
+   fsec += 1.0;
+ #endif
+   tm-tm_sec--;
+   }
+ 
while (tm-tm_sec  0)
{
tm-tm_sec += SECS_PER_MINUTE;
***
*** 3082,3097 
tm-tm_mday--;
}
  
!   while (tm-tm_mday  0)
{

Re: [HACKERS] [BUGS] BUG #3431: age() gets the days wrong

2007-07-08 Thread Tom Lane
Pelle Johansson [EMAIL PROTECTED] writes:
 If you were to use the result for subtracting from the first value,  
 instead of adding to the second, the conditions are reversed. It's  
 not really as obvious as I first thought whether there's 2 months and  
 29 days or 2 months and 30 days between 2006-11-02 and 2007-02-01...  

Hmm, that's a really good point; perhaps the original author was
thinking of it in those terms, in which case using the first month of
the interval is indeed sane.  (Almost: I believe that the loop can
iterate more than once, and then you need to look to the second month
etc.  The code's not doing that, so there's still a corner-case bug,
plus the fsec issue.)

Other than that corner case, it seems the behavior we currently have is
if x  y, age() produces a positive interval such that
x - age(x, y) = y
if x  y, age() produces a negative interval such that
y + age(x, y) = x

Are we satisfied with just documenting that, or do we want to change it,
and if so to what?

As the code currently stands, we have the symmetry property
age(x,y) = - age(y,x)
for all x,y.  I don't think we can preserve that if we try to simplify
the relationship to interval addition/subtraction.

Comments?

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


Re: [HACKERS] [BUGS] BUG #3431: age() gets the days wrong

2007-07-08 Thread Pelle Johansson

Hi Tom,

I only have one remark really, which I first thought of after sending  
the report.
If you were to use the result for subtracting from the first value,  
instead of adding to the second, the conditions are reversed. It's  
not really as obvious as I first thought whether there's 2 months and  
29 days or 2 months and 30 days between 2006-11-02 and 2007-02-01...  
If one want mathematical correctness, it will have to be defined  
precisely.

--
Pelle Johansson

8 jul 2007 kl. 22.07 skrev Tom Lane:


Pelle Johansson [EMAIL PROTECTED] writes:
The age() function seem to work by first counting months until  
less than a
month remains to to the second argument, then counting days left.  
This

doesn't give the correct result, as shown by this example:



# select column1, age(column1, '2006-11-02'), date '2006-11-02' +
age(column1, '2006-11-02') from (values ('2007-01-31'::date),
('2007-02-01')) as alias;
  column1   |  age   |  ?column?
++-
 2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
 2007-02-01 | 2 mons 29 days | 2007-01-31 00:00:00
(2 rows)


I took another look at this example.  I believe what is actually going
wrong here is that when timestamp_age converts a month into an
equivalent number of days, it uses the number of days in the first
month of the interval it's dealing with (ie, the month containing
the earlier of the two dates).  This is just wrong, because interval
addition adds months first and then days.  The appropriate conversion
to use is actually the length of the next-to-last month of the  
interval.


As an example, 8.2 and CVS HEAD produce

regression=# select age('2007-03-14', '2007-02-15');
   age
-
 27 days
(1 row)

which is reasonable, but

regression=# select age('2007-04-14', '2007-02-15');
  age
---
 1 mon 27 days
(1 row)

is not so reasonable, nor is

regression=# select age('2007-03-14', '2007-01-15');
  age
---
 1 mon 30 days
(1 row)

If we change the code to use the next-to-last month of the interval
then these two cases produce '1 mon 30 days' and '1 mon 27 days'
respectively.

Another problem is that the code isn't doing the propagate-to-next- 
field

bit for negative fractional seconds.  Hence it produces

regression=# select age('2007-02-14 01:00:00', '2007-01-15  
01:00:00.4');

 age
--
 30 days -00:00:00.40
(1 row)

which is maybe not incorrect, but certainly fairly inconsistent with

regression=# select age('2007-02-14 01:00:00', '2007-01-15 01:00:01');
   age
--
 29 days 23:59:59
(1 row)


Hence I propose the attached patch.  This does not change any existing
regression test outputs, but it does change the example given in the
documentation: age(timestamp '2001-04-10', timestamp '1957-06-13')
will now produce '43 years 9 mons 28 days' not 27 days.  Which  
actually
is correct if you try to add back the result to timestamp  
'1957-06-13'.

It also appears to fix Palle's example:

regression=# select column1, age(column1, '2006-11-02'), date  
'2006-11-02' +

age(column1, '2006-11-02') from (values ('2007-01-31'::date),
('2007-02-01')) as alias;
  column1   |  age   |  ?column?
++-
 2007-01-31 | 2 mons 29 days | 2007-01-31 00:00:00
 2007-02-01 | 2 mons 30 days | 2007-02-01 00:00:00
(2 rows)

As I said earlier, I'm worried about changing the behavior of a  
function

that's been around for so long, so I'm disinclined to back-patch this.
But it seems like a reasonable change to make in 8.3.  Comments?

regards, tom lane

Index: timestamp.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/timestamp.c,v
retrieving revision 1.179
diff -c -r1.179 timestamp.c
*** timestamp.c 6 Jul 2007 04:15:59 -   1.179
--- timestamp.c 8 Jul 2007 19:45:04 -
***
*** 3044,3050 
if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
{
!   fsec = (fsec1 - fsec2);
tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
tm-tm_min = tm1-tm_min - tm2-tm_min;
tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
--- 3044,3051 
if (timestamp2tm(dt1, NULL, tm1, fsec1, NULL, NULL) == 0 
timestamp2tm(dt2, NULL, tm2, fsec2, NULL, NULL) == 0)
{
!   /* form the symbolic difference */
!   fsec = fsec1 - fsec2;
tm-tm_sec = tm1-tm_sec - tm2-tm_sec;
tm-tm_min = tm1-tm_min - tm2-tm_min;
tm-tm_hour = tm1-tm_hour - tm2-tm_hour;
***
*** 3064,3069 
--- 3065,3081 
tm-tm_year = -tm-tm_year;
}

+   /* propagate any negative fields into the next higher field */
+   while (fsec  0)
+