Re: [PATCHES] possible ecpg vpath build error

When trying to improve the rounding in interval_div and interval_mul,
I came across some behavior that seems counterintuitive to me:

test=# select '1.5 mon'::interval;
interval
-----------------
1 mon 360:00:00
(1 row)

With the time/day/month interval struct introduced in 8.1, I'd expect
this to return '1 mon 15 days'. The reason is that the DecodeInterval
converts fractional months to time directly, rather than cascading
first to days.

Similar behavior happens with weeks:

select '1.5 week'::interval;
interval
-----------------
7 days 84:00:00
(1 row)

Similarly, I believe should return 10 days 12 hours (7 days + 3.5 days).

I've patched DecodeInterval and the regression tests to check this. I
think tmask lines need to be updated, but I'm not sure how these work
so I've left them as is. I'd appreciate it if someone could look at
these areas in particular.

I think this is a behavior changing bug fix, as it was the intention
of the Interval struct change to treat days and time differently.
This patch brings the DecodeInterval function more in line with that
intention.

Thanks for your consideration.

Michael Glaesemann
grzm seespotcode net

Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.169
diff -c -r1.169 datetime.c
*** src/backend/utils/adt/datetime.c 25 Jul 2006 03:51:21 -0000 1.169
--- src/backend/utils/adt/datetime.c 27 Aug 2006 23:25:53 -0000
***************
*** 2920,2935 ****
tm->tm_mday += val * 7;
if (fval != 0)
{
! int sec;
!
! fval *= 7 * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
--- 2920,2942 ----
tm->tm_mday += val * 7;
if (fval != 0)
{
! int extra_days;
! fval *= 7;
! extra_days = (int32) fval;
! tm->tm_mday += extra_days;
! fval -= extra_days;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
***************
*** 2938,2953 ****
tm->tm_mon += val;
if (fval != 0)
{
! int sec;
!
! fval *= DAYS_PER_MONTH * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = DTK_M(MONTH);
break;
--- 2945,2967 ----
tm->tm_mon += val;
if (fval != 0)
{
! int day;
! fval *= DAYS_PER_MONTH;
! day = fval;
! tm->tm_mday += day;
! fval -= day;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = DTK_M(MONTH);
break;
Index: src/test/regress/expected/interval.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/
interval.out,v
retrieving revision 1.15
diff -c -r1.15 interval.out
*** src/test/regress/expected/interval.out 6 Mar 2006 22:49:17 -0000
1.15
--- src/test/regress/expected/interval.out 27 Aug 2006 23:25:56 -0000
***************
*** 39,44 ****
--- 39,56 ----
-1 days +02:03:00
(1 row)

+ SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
+ Ten days twelve hours
+ -----------------------
+ 10 days 12:00:00
+ (1 row)
+
+ SELECT INTERVAL '1.5 months' AS "One month 15 days";
+ One month 15 days
+ -------------------
+ 1 mon 15 days
+ (1 row)
+
SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";
9 years...
----------------------------------
Index: src/test/regress/sql/interval.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/interval.sql,v
retrieving revision 1.9
diff -c -r1.9 interval.sql
*** src/test/regress/sql/interval.sql 6 Mar 2006 22:49:17 -0000 1.9
--- src/test/regress/sql/interval.sql 27 Aug 2006 23:25:56 -0000
***************
*** 11,16 ****
--- 11,18 ----
SELECT INTERVAL '-05' AS "Five hours";
SELECT INTERVAL '-1 +02:03' AS "22 hours ago...";
SELECT INTERVAL '-1 days +02:03' AS "22 hours ago...";
+ SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
+ SELECT INTERVAL '1.5 months' AS "One month 15 days";
SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";

CREATE TABLE INTERVAL_TBL (f1 interval);

The masks don't need changing.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Michael Glaesemann wrote:
> When trying to improve the rounding in interval_div and interval_mul,
> I came across some behavior that seems counterintuitive to me:
>
> test=# select '1.5 mon'::interval;
> interval
> -----------------
> 1 mon 360:00:00
> (1 row)
>
> With the time/day/month interval struct introduced in 8.1, I'd expect
> this to return '1 mon 15 days'. The reason is that the DecodeInterval
> converts fractional months to time directly, rather than cascading
> first to days.
>
> Similar behavior happens with weeks:
>
> select '1.5 week'::interval;
> interval
> -----------------
> 7 days 84:00:00
> (1 row)
>
> Similarly, I believe should return 10 days 12 hours (7 days + 3.5 days).
>
> I've patched DecodeInterval and the regression tests to check this. I
> think tmask lines need to be updated, but I'm not sure how these work
> so I've left them as is. I'd appreciate it if someone could look at
> these areas in particular.
>
> I think this is a behavior changing bug fix, as it was the intention
> of the Interval struct change to treat days and time differently.
> This patch brings the DecodeInterval function more in line with that
> intention.
>
> Thanks for your consideration.
>
> Michael Glaesemann
> grzm seespotcode net
>
>
> Index: src/backend/utils/adt/datetime.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
> retrieving revision 1.169
> diff -c -r1.169 datetime.c
> *** src/backend/utils/adt/datetime.c 25 Jul 2006 03:51:21 -0000 1.169
> --- src/backend/utils/adt/datetime.c 27 Aug 2006 23:25:53 -0000
> ***************
> *** 2920,2935 ****
> tm->tm_mday += val * 7;
> if (fval != 0)
> {
> ! int sec;
> !
> ! fval *= 7 * SECS_PER_DAY;
> ! sec = fval;
> ! tm->tm_sec += sec;
> #ifdef HAVE_INT64_TIMESTAMP
> ! *fsec += (fval - sec) * 1000000;
> #else
> ! *fsec += fval - sec;
> #endif
> }
> tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
> break;
> --- 2920,2942 ----
> tm->tm_mday += val * 7;
> if (fval != 0)
> {
> ! int extra_days;
> ! fval *= 7;
> ! extra_days = (int32) fval;
> ! tm->tm_mday += extra_days;
> ! fval -= extra_days;
> ! if (fval != 0)
> ! {
> ! int sec;
> ! fval *= SECS_PER_DAY;
> ! sec = fval;
> ! tm->tm_sec += sec;
> #ifdef HAVE_INT64_TIMESTAMP
> ! *fsec += (fval - sec) * 1000000;
> #else
> ! *fsec += fval - sec;
> #endif
> + }
> }
> tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
> break;
> ***************
> *** 2938,2953 ****
> tm->tm_mon += val;
> if (fval != 0)
> {
> ! int sec;
> !
> ! fval *= DAYS_PER_MONTH * SECS_PER_DAY;
> ! sec = fval;
> ! tm->tm_sec += sec;
> #ifdef HAVE_INT64_TIMESTAMP
> ! *fsec += (fval - sec) * 1000000;
> #else
> ! *fsec += fval - sec;
> #endif
> }
> tmask = DTK_M(MONTH);
> break;
> --- 2945,2967 ----
> tm->tm_mon += val;
> if (fval != 0)
> {
> ! int day;
> ! fval *= DAYS_PER_MONTH;
> ! day = fval;
> ! tm->tm_mday += day;
> ! fval -= day;
> ! if (fval != 0)
> ! {
> ! int sec;
> ! fval *= SECS_PER_DAY;
> ! sec = fval;
> ! tm->tm_sec += sec;
> #ifdef HAVE_INT64_TIMESTAMP
> ! *fsec += (fval - sec) * 1000000;
> #else
> ! *fsec += fval - sec;
> #endif
> + }
> }
> tmask = DTK_M(MONTH);
> break;
> Index: src/test/regress/expected/interval.out
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/
> interval.out,v
> retrieving revision 1.15
> diff -c -r1.15 interval.out
> *** src/test/regress/expected/interval.out 6 Mar 2006 22:49:17 -0000
> 1.15
> --- src/test/regress/expected/interval.out 27 Aug 2006 23:25:56 -0000
> ***************
> *** 39,44 ****
> --- 39,56 ----
> -1 days +02:03:00
> (1 row)
>
> + SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
> + Ten days twelve hours
> + -----------------------
> + 10 days 12:00:00
> + (1 row)
> +
> + SELECT INTERVAL '1.5 months' AS "One month 15 days";
> + One month 15 days
> + -------------------
> + 1 mon 15 days
> + (1 row)
> +
> SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";
> 9 years...
> ----------------------------------
> Index: src/test/regress/sql/interval.sql
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/interval.sql,v
> retrieving revision 1.9
> diff -c -r1.9 interval.sql
> *** src/test/regress/sql/interval.sql 6 Mar 2006 22:49:17 -0000 1.9
> --- src/test/regress/sql/interval.sql 27 Aug 2006 23:25:56 -0000
> ***************
> *** 11,16 ****
> --- 11,18 ----
> SELECT INTERVAL '-05' AS "Five hours";
> SELECT INTERVAL '-1 +02:03' AS "22 hours ago...";
> SELECT INTERVAL '-1 days +02:03' AS "22 hours ago...";
> + SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
> + SELECT INTERVAL '1.5 months' AS "One month 15 days";
> SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";
>
> CREATE TABLE INTERVAL_TBL (f1 interval);
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> I came across some behavior that seems counterintuitive to me:

> test=# select '1.5 mon'::interval;
> interval
> -----------------
> 1 mon 360:00:00
> (1 row)

> With the time/day/month interval struct introduced in 8.1, I'd expect
> this to return '1 mon 15 days'. The reason is that the DecodeInterval
> converts fractional months to time directly, rather than cascading
> first to days.

I agree that this seems like an oversight in the original
months/days/seconds patch, rather than behavior we want to keep.
But is DecodeInterval the only place with the problem? My recollection
is that there's a certain amount of redundancy in the datetime code ...

regards, tom lane

On Sep 1, 2006, at 9:12 , Tom Lane wrote:

> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
>> I came across some behavior that seems counterintuitive to me:
>
>> test=# select '1.5 mon'::interval;
>> interval
>> -----------------
>> 1 mon 360:00:00
>> (1 row)
>
>> With the time/day/month interval struct introduced in 8.1, I'd expect
>> this to return '1 mon 15 days'. The reason is that the DecodeInterval
>> converts fractional months to time directly, rather than cascading
>> first to days.
>
> I agree that this seems like an oversight in the original
> months/days/seconds patch, rather than behavior we want to keep.
> But is DecodeInterval the only place with the problem? My
> recollection
> is that there's a certain amount of redundancy in the datetime
> code ...

I'll check on this tonight. Any idea where I might start to look?

Michael Glaesemann
grzm seespotcode net

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> On Sep 1, 2006, at 9:12 , Tom Lane wrote:
>> I agree that this seems like an oversight in the original
>> months/days/seconds patch, rather than behavior we want to keep.
>> But is DecodeInterval the only place with the problem?

> I'll check on this tonight. Any idea where I might start to look?

I'd look at the input routines for all the datetime types and see where
they go. It's entirely possible that DecodeInterval is the only place
with the problem, but I'd not assume that without looking.

regards, tom lane

On Sep 1, 2006, at 9:32 , Tom Lane wrote:

> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
>> On Sep 1, 2006, at 9:12 , Tom Lane wrote:
>>> I agree that this seems like an oversight in the original
>>> months/days/seconds patch, rather than behavior we want to keep.
>>> But is DecodeInterval the only place with the problem?
>
>> I'll check on this tonight. Any idea where I might start to look?
>
> I'd look at the input routines for all the datetime types and see
> where
> they go. It's entirely possible that DecodeInterval is the only place
> with the problem, but I'd not assume that without looking.

AFAICS, DecodeInterval is the only place that needed changing. I've
looked through datetime.c, timestamp.c, date.c, and nabstime.c, and
don't see anything else. It makes sense, too, as the only place where
you could have weeks or non-integer months is during Interval input
or interval multiplication/division. The pg_tm struct, which is used
in time(stamp)?(tz)?/interval arithmetic only has integral months and
no weeks component, so that shouldn't cause any problems. So, I think
that's about it.

Michael Glaesemann
grzm seespotcode net

On Sep 3, 2006, at 20:00 , Michael Glaesemann wrote:

>
> On Sep 1, 2006, at 9:32 , Tom Lane wrote:
>
>> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
>>> On Sep 1, 2006, at 9:12 , Tom Lane wrote:
>>>> I agree that this seems like an oversight in the original
>>>> months/days/seconds patch, rather than behavior we want to keep.
>>>> But is DecodeInterval the only place with the problem?
>>
>>> I'll check on this tonight. Any idea where I might start to look?
>>
>> I'd look at the input routines for all the datetime types and see
>> where
>> they go. It's entirely possible that DecodeInterval is the only
>> place
>> with the problem, but I'd not assume that without looking.
>
> AFAICS, DecodeInterval is the only place that needed changing. I've
> looked through datetime.c, timestamp.c, date.c, and nabstime.c, and
> don't see anything else. It makes sense, too, as the only place
> where you could have weeks or non-integer months is during Interval
> input or interval multiplication/division. The pg_tm struct, which
> is used in time(stamp)?(tz)?/interval arithmetic only has integral
> months and no weeks component, so that shouldn't cause any
> problems. So, I think that's about it.

I realized there might be something in ecpg, and there was. I've
updated the ecpg DecodeInterval to match. However, I haven't been
able to get ecpg make check to work, so that part's untested.

Michael Glaesemann
grzm seespotcode net

Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.169
diff -c -r1.169 datetime.c
*** src/backend/utils/adt/datetime.c 25 Jul 2006 03:51:21 -0000 1.169
--- src/backend/utils/adt/datetime.c 3 Sep 2006 23:55:34 -0000
***************
*** 2920,2935 ****
tm->tm_mday += val * 7;
if (fval != 0)
{
! int sec;
!
! fval *= 7 * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
--- 2920,2942 ----
tm->tm_mday += val * 7;
if (fval != 0)
{
! int extra_days;
! fval *= 7;
! extra_days = (int32) fval;
! tm->tm_mday += extra_days;
! fval -= extra_days;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
***************
*** 2938,2953 ****
tm->tm_mon += val;
if (fval != 0)
{
! int sec;
!
! fval *= DAYS_PER_MONTH * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = DTK_M(MONTH);
break;
--- 2945,2967 ----
tm->tm_mon += val;
if (fval != 0)
{
! int day;
! fval *= DAYS_PER_MONTH;
! day = fval;
! tm->tm_mday += day;
! fval -= day;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = DTK_M(MONTH);
break;
Index: src/interfaces/ecpg/pgtypeslib/interval.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/ecpg/pgtypeslib/
interval.c,v
retrieving revision 1.32
diff -c -r1.32 interval.c
*** src/interfaces/ecpg/pgtypeslib/interval.c 6 Jun 2006 11:31:55
-0000 1.32
--- src/interfaces/ecpg/pgtypeslib/interval.c 3 Sep 2006 23:55:34 -0000
***************
*** 307,322 ****
tm->tm_mday += val * 7;
if (fval != 0)
{
! int sec;
!
! fval *= 7 * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
--- 307,329 ----
tm->tm_mday += val * 7;
if (fval != 0)
{
! int extra_days;
! fval *= 7;
! extra_days = (int32) fval;
! tm->tm_mday += extra_days;
! fval -= extra_days;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = (fmask & DTK_M(DAY)) ? 0 : DTK_M(DAY);
break;
***************
*** 325,340 ****
tm->tm_mon += val;
if (fval != 0)
{
! int sec;
!
! fval *= DAYS_PER_MONTH * SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
}
tmask = DTK_M(MONTH);
break;
--- 332,354 ----
tm->tm_mon += val;
if (fval != 0)
{
! int day;
! fval *= DAYS_PER_MONTH;
! day = fval;
! tm->tm_mday += day;
! fval -= day;
! if (fval != 0)
! {
! int sec;
! fval *= SECS_PER_DAY;
! sec = fval;
! tm->tm_sec += sec;
#ifdef HAVE_INT64_TIMESTAMP
! *fsec += (fval - sec) * 1000000;
#else
! *fsec += fval - sec;
#endif
+ }
}
tmask = DTK_M(MONTH);
break;
Index: src/test/regress/expected/interval.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/
interval.out,v
retrieving revision 1.16
diff -c -r1.16 interval.out
*** src/test/regress/expected/interval.out 3 Sep 2006 03:34:04 -0000
1.16
--- src/test/regress/expected/interval.out 3 Sep 2006 23:55:35 -0000
***************
*** 39,44 ****
--- 39,56 ----
-1 days +02:03:00
(1 row)

+ SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
+ Ten days twelve hours
+ -----------------------
+ 10 days 12:00:00
+ (1 row)
+
+ SELECT INTERVAL '1.5 months' AS "One month 15 days";
+ One month 15 days
+ -------------------
+ 1 mon 15 days
+ (1 row)
+
SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";
9 years...
----------------------------------
Index: src/test/regress/sql/interval.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/interval.sql,v
retrieving revision 1.9
diff -c -r1.9 interval.sql
*** src/test/regress/sql/interval.sql 6 Mar 2006 22:49:17 -0000 1.9
--- src/test/regress/sql/interval.sql 3 Sep 2006 23:55:35 -0000
***************
*** 11,16 ****
--- 11,18 ----
SELECT INTERVAL '-05' AS "Five hours";
SELECT INTERVAL '-1 +02:03' AS "22 hours ago...";
SELECT INTERVAL '-1 days +02:03' AS "22 hours ago...";
+ SELECT INTERVAL '1.5 weeks' AS "Ten days twelve hours";
+ SELECT INTERVAL '1.5 months' AS "One month 15 days";
SELECT INTERVAL '10 years -11 month -12 days +13:14' AS "9 years...";

CREATE TABLE INTERVAL_TBL (f1 interval);

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> I realized there might be something in ecpg, and there was. I've
> updated the ecpg DecodeInterval to match. However, I haven't been
> able to get ecpg make check to work, so that part's untested.

This patch fails to apply --- looks like whitespace got mangled in
transit. Please resend as an attachment.

regards, tom lane

On Sep 4, 2006, at 9:41 , Tom Lane wrote:

> This patch fails to apply --- looks like whitespace got mangled in
> transit. Please resend as an attachment.

Please let me know if you have any problems with this one.

Michael Glaesemann
grzm seespotcode net

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> On Sep 4, 2006, at 9:41 , Tom Lane wrote:
>> This patch fails to apply --- looks like whitespace got mangled in
>> transit. Please resend as an attachment.

> Please let me know if you have any problems with this one.

Ah, that one works --- applied. A few comments:

* You worried about the "tmask" coding in your original message, but
I think that's OK as-is. The point of that code, IIUC, is to reject
multiple specifications of the same field type, eg '1 day 2 days'.
If we changed it then we'd reject '1.5 month 2 days', whereas I think
least surprise would dictate adding the components to give 1 month
17 days.

* AFAICT the ecpg regression tests are not affected by this change.

* You mentioned being unable to get the ecpg tests to run on your
machine. I'm sure Michael and Joachim would like the details. The
ecpg regression tests are pretty new and some portability problems
are to be expected, but they seem to be passing on all the machines
Michael and Joachim and I have access to.

regards, tom lane

Tom Lane wrote:
> You mentioned being unable to get the ecpg tests to run on your
> machine. I'm sure Michael and Joachim would like the details. The
> ecpg regression tests are pretty new and some portability problems
> are to be expected, but they seem to be passing on all the machines
> Michael and Joachim and I have access to.
>
>
>

I have just today released a new version of the buildfarm client that
includes ECPG regression testing for HEAD (until now that was in our CVS
tip but not in a released version).

cheers

andrew

Tom Lane wrote:
> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> > On Sep 4, 2006, at 9:41 , Tom Lane wrote:
> >> This patch fails to apply --- looks like whitespace got mangled in
> >> transit. Please resend as an attachment.
>
> > Please let me know if you have any problems with this one.
>
> Ah, that one works --- applied. A few comments:
>
> * You worried about the "tmask" coding in your original message, but
> I think that's OK as-is. The point of that code, IIUC, is to reject
> multiple specifications of the same field type, eg '1 day 2 days'.
> If we changed it then we'd reject '1.5 month 2 days', whereas I think
> least surprise would dictate adding the components to give 1 month
> 17 days.
>
> * AFAICT the ecpg regression tests are not affected by this change.
>
> * You mentioned being unable to get the ecpg tests to run on your
> machine. I'm sure Michael and Joachim would like the details. The
> ecpg regression tests are pretty new and some portability problems
> are to be expected, but they seem to be passing on all the machines
> Michael and Joachim and I have access to.

When I tried the ecpg regression tests it complained there was no
results/ directory. I created one and it worked.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

[Removing -patches]

On Sep 4, 2006, at 10:33 , Tom Lane wrote:

> * AFAICT the ecpg regression tests are not affected by this change.

Yeah, it doesn't look like there's any tests for interval at all. I
suppose there should be. There's a lot of duplicate code in ecpg. Is
there any way to pull that in from the "main" sections of the source
code? Maybe the ecpg regression tests could run some relevant
sections of the main regression tests as well?

> * You mentioned being unable to get the ecpg tests to run on your
> machine. I'm sure Michael and Joachim would like the details. The
> ecpg regression tests are pretty new and some portability problems
> are to be expected, but they seem to be passing on all the machines
> Michael and Joachim and I have access to.

Good to hear.

I'm using a script to handle my vpath builds, so I'll take a look at
them later. There's a good chance the problem is with the script
rather than the build itself.

For the record, the error I'm getting is
Makefile:3: ../../../src/Makefile.global: No such file or directory
make: *** No rule to make target `../../../src/Makefile.global'. Stop.

Michael Glaesemann
grzm seespotcode net

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> There's a lot of duplicate code in ecpg.

No kidding :-(. The parser is bad enough but the datatype library is
an order of magnitude worse. I don't have a great solution at hand
though. The backend utils/adt/ code gets to rely on the backend's
error handling and memory management protocols, which I surely do
not propose to remove, but how could we keep common sources when
ecpglib has to work in a far less friendly environment?

regards, tom lane

Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> For the record, the error I'm getting is
> Makefile:3: ../../../src/Makefile.global: No such file or directory
> make: *** No rule to make target `../../../src/Makefile.global'. Stop.

From which Makefile exactly? Sounds like a pretty vanilla VPATH support
bug, but can't chase it down with no context...

regards, tom lane

On Sun, Sep 03, 2006 at 10:21:11PM -0400, Bruce Momjian wrote:
> When I tried the ecpg regression tests it complained there was no
> results/ directory. I created one and it worked.

Hmm, anyone else experiencing this? The pg_regress.sh has this code that
should create it:

outputdir="results/"

if [ ! -d "$outputdir" ]; then
mkdir -p "$outputdir" || { (exit 2); exit; }
fi

Michael
--
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes(at)jabber(dot)org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

On Sep 4, 2006, at 13:12 , Tom Lane wrote:

> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
>> For the record, the error I'm getting is
>> Makefile:3: ../../../src/Makefile.global: No such file or directory
>> make: *** No rule to make target `../../../src/Makefile.global'.
>> Stop.
>
> From which Makefile exactly? Sounds like a pretty vanilla VPATH
> support
> bug, but can't chase it down with no context...

As I suspected, it was the script I was using. I had it trying to do
make check in the source directory rather than the build directory.

As always, thanks for the offer of help :)

Michael Glaesemann
grzm seespotcode net

Michael Meskes <meskes(at)postgresql(dot)org> writes:
> On Sun, Sep 03, 2006 at 10:21:11PM -0400, Bruce Momjian wrote:
>> When I tried the ecpg regression tests it complained there was no
>> results/ directory. I created one and it worked.

> Hmm, anyone else experiencing this? The pg_regress.sh has this code that
> should create it:

> outputdir="results/"

> if [ ! -d "$outputdir" ]; then
> mkdir -p "$outputdir" || { (exit 2); exit; }
> fi

I'll bet you should lose the slash in $outputdir. test(1) might or
might not be "friendly" about stripping that off.

regards, tom lane

On Mon, Sep 04, 2006 at 12:06:02AM -0400, Tom Lane wrote:
> Michael Glaesemann <grzm(at)seespotcode(dot)net> writes:
> > There's a lot of duplicate code in ecpg.
>
> No kidding :-(. The parser is bad enough but the datatype library is
> an order of magnitude worse. I don't have a great solution at hand
> though. The backend utils/adt/ code gets to rely on the backend's

Neither have I.

> error handling and memory management protocols, which I surely do
> not propose to remove, but how could we keep common sources when
> ecpglib has to work in a far less friendly environment?

We could modify the backend code to use pgtypeslib, but that would cost
at least a little bit of performance I would guess.

Michael
--
Michael Meskes
Email: Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes(at)jabber(dot)org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)org> writes:
> On Mon, Sep 04, 2006 at 12:06:02AM -0400, Tom Lane wrote:
>> The backend utils/adt/ code gets to rely on the backend's
>> error handling and memory management protocols, which I surely do
>> not propose to remove, but how could we keep common sources when
>> ecpglib has to work in a far less friendly environment?

> We could modify the backend code to use pgtypeslib, but that would cost
> at least a little bit of performance I would guess.

I'd prefer to go in the other direction: provide enough infrastructure
in ecpglib that it can use the unmodified backend sources. It would
probably not take too much code to provide minimal elog and palloc
support ... the question is what else would we need?

(BTW, if anyone is working on making that pie-in-the-sky TODO list,
here's a pet peeve for it: ecpg's bison parser should be auto-generated
from the backend's, instead of derived manually.)

regards, tom lane

Tom Lane wrote:
> Michael Meskes <meskes(at)postgresql(dot)org> writes:
> > On Sun, Sep 03, 2006 at 10:21:11PM -0400, Bruce Momjian wrote:
> >> When I tried the ecpg regression tests it complained there was no
> >> results/ directory. I created one and it worked.
>
> > Hmm, anyone else experiencing this? The pg_regress.sh has this code that
> > should create it:
>
> > outputdir="results/"
>
> > if [ ! -d "$outputdir" ]; then
> > mkdir -p "$outputdir" || { (exit 2); exit; }
> > fi
>
> I'll bet you should lose the slash in $outputdir. test(1) might or
> might not be "friendly" about stripping that off.

Yep, I saw this error:

mkdir: results/: No such file or directory
gmake: *** [installcheck] Error 2

I have removed the trailing slash from CVS; tests run fine now.
Thanks.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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