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