Thread: int64/double for time/timestamp

Hi!

I work on memory leaks during creation index on time/timestamp column using GiST 
and found follow problem (?):

For timestamp storage and defines are defined as (from utils/timestamp.h):
#ifdef HAVE_INT64_TIMESTAMP

typedef int64 Timestamp;
#define TimestampGetDatum(X) Int64GetDatum(X)
#define DatumGetTimestamp(X)  ((Timestamp) DatumGetInt64(X))

#else

typedef double Timestamp;
#define TimestampGetDatum(X) Float8GetDatum(X)
#define DatumGetTimestamp(X)  ((Timestamp) DatumGetFloat8(X))

#endif

It looks consistently, but for time (from utils/date.h):

ifdef HAVE_INT64_TIMESTAMP
typedef int64 TimeADT;

#else
typedef float8 TimeADT;
#endif

#define TimeADTGetDatum(X)        Float8GetDatum(X)
#define DatumGetTimeADT(X)        ((TimeADT) DatumGetFloat8(X))

So, in case HAVE_INT64_TIMESTAMP int64 may use as float8.  Is it correct?


It seems to me, that my last changes in btree_gist produce a error for 
btree_time on some architectures for this reason, but the same changes for 
timestamp doesn't produce ones.


-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/
 

Teodor Sigaev <teodor@sigaev.ru> writes:
> It looks consistently, but for time (from utils/date.h):

> ifdef HAVE_INT64_TIMESTAMP
> typedef int64 TimeADT;
> #else
> typedef float8 TimeADT;
> #endif

> #define TimeADTGetDatum(X)        Float8GetDatum(X)
> #define DatumGetTimeADT(X)        ((TimeADT) DatumGetFloat8(X))

> So, in case HAVE_INT64_TIMESTAMP int64 may use as float8.  Is it correct?

Urgh.  This is clearly a bug.  All the code in utils/adt seems to be
correctly set up to treat TimeADT as an integral value, but then the two
macros quoted are converting the value to float8 and back again ... so
what's actually on disk is the float8 equivalent of what the int64 value
is supposed to be :-(.  As long as the macros are used *consistently* to
fetch and store time datums, no one would notice --- you could only see
a difference if the int64 values got large enough to not be represented
completely accurately as floats, which I believe is impossible for type
time.

So the fact that you're seeing a bug in btree_gist suggests that
someplace you're cheating and bypassing the FooGetDatum/DatumGetFoo
macros.

We'll obviously want to fix this going forward for efficiency reasons,
but it's an initdb-forcer because it'll change the on-disk
representation of time columns.  So we can't change it in 8.0 or before.
        regards, tom lane

> Urgh.  This is clearly a bug.  All the code in utils/adt seems to be
> correctly set up to treat TimeADT as an integral value, but then the two
> macros quoted are converting the value to float8 and back again ... so
> what's actually on disk is the float8 equivalent of what the int64 value
> is supposed to be :-(.  As long as the macros are used *consistently* to
> fetch and store time datums, no one would notice --- you could only see
> a difference if the int64 values got large enough to not be represented
> completely accurately as floats, which I believe is impossible for type
> time.
> 
> So the fact that you're seeing a bug in btree_gist suggests that
> someplace you're cheating and bypassing the FooGetDatum/DatumGetFoo
> macros.
> 
> We'll obviously want to fix this going forward for efficiency reasons,
> but it's an initdb-forcer because it'll change the on-disk
> representation of time columns.  So we can't change it in 8.0 or before.

So, will we do it? I can do, but I don't know: Is there a place which contains 
storage version (except file PG_VERSION)?


-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/
 

Teodor Sigaev <teodor@sigaev.ru> writes:
>> We'll obviously want to fix this going forward for efficiency reasons,
>> but it's an initdb-forcer because it'll change the on-disk
>> representation of time columns.  So we can't change it in 8.0 or before.

> So, will we do it? I can do, but I don't know: Is there a place which
> contains storage version (except file PG_VERSION)?

catversion.h would need to be advanced for such a change.  We haven't
got anything finer-grained than that.
        regards, tom lane

Teodor Sigaev wrote:
>> Urgh.  This is clearly a bug.  All the code in utils/adt seems to be
>> correctly set up to treat TimeADT as an integral value, but then the two
>> macros quoted are converting the value to float8 and back again ... so
>> what's actually on disk is the float8 equivalent of what the int64 value
>> is supposed to be :-(.  As long as the macros are used *consistently* to
>> fetch and store time datums, no one would notice --- you could only see
>> a difference if the int64 values got large enough to not be represented
>> completely accurately as floats, which I believe is impossible for type
>> time.
>>
>> So the fact that you're seeing a bug in btree_gist suggests that
>> someplace you're cheating and bypassing the FooGetDatum/DatumGetFoo
>> macros.
>>
>> We'll obviously want to fix this going forward for efficiency reasons,
>> but it's an initdb-forcer because it'll change the on-disk
>> representation of time columns.  So we can't change it in 8.0 or before.
> 
> 
> So, will we do it? I can do, but I don't know: Is there a place which 
> contains storage version (except file PG_VERSION)?
> 
> 
When making PL/Java dynamically adapt to the setting of 
integer-datetimes, I too was bitten by this bug. Is it safe to assume 
that the fix for this will arrive in 8.1.0?

Regards,
Thomas Hallgren

Thomas Hallgren <thhal@mailblocks.com> writes:
> When making PL/Java dynamically adapt to the setting of 
> integer-datetimes, I too was bitten by this bug. Is it safe to assume 
> that the fix for this will arrive in 8.1.0?

I believe Teodor already committed the change in CVS HEAD.
        regards, tom lane