timestamp datatype cleanup
PosgreSQL hackers,
Here's an initial bit of my attempt at cleaning up the the timestamp datatype.
I have gone through the backend and made a couple small changes to stop using
the HAVE_INT64_TIMESTAMP define to select a type in code by creating typedefs
in a header and using the typedef in the code. I think this small bit is ready
for inclusion for this small bit, but I have a couple questions for further
work.
1) Is there a reason that header information is duplicated between normal
posgresql include and ecpg includes instead of defining the info in one place
and #including it into the files that need it?
2) Would it be reasonable to change timestamp.h into a file that includes other
files that define the specific parts depending on HAVE_INT64_TIMESTAMP instead
of testing for HAVE_INT64_TIMESTAMP many times throughout timestamp.h? I think
this might more cleanly separate the logic for the different timestamp types.
Thanks,
wt
I added TimeOffset and DateOffset typedefs to get rid of the instances
using the HAVE_INT64_TIMESTAMP define being used to determine the
types of variables or functions in timestamp.c.
---
src/backend/utils/adt/timestamp.c | 77 +++++++-----------------------------
src/include/utils/timestamp.h | 4 ++
2 files changed, 19 insertions(+), 62 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 9b90873..4759e22 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -44,11 +44,7 @@
TimestampTz PgStartTime;-#ifdef HAVE_INT64_TIMESTAMP
-static int64 time2t(const int hour, const int min, const int sec, const fsec_t fsec);
-#else
-static double time2t(const int hour, const int min, const int sec, const fsec_t fsec);
-#endif
+static TimeOffset time2t(const int hour, const int min, const int sec, const fsec_t fsec);
static int EncodeSpecialTimestamp(Timestamp dt, char *str);
static Timestamp dt2local(Timestamp dt, int timezone);
static void AdjustTimestampForTypmod(Timestamp *time, int32 typmod);
@@ -977,11 +973,7 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod)
}
else if (range == INTERVAL_MASK(MINUTE))
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 hour;
-#else
- double hour;
-#endif
+ TimeOffset hour; interval->month = 0;
interval->day = 0;
@@ -998,11 +990,7 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod)
}
else if (range == INTERVAL_MASK(SECOND))
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 minute;
-#else
- double minute;
-#endif
+ TimeOffset minute; interval->month = 0;
interval->day = 0;
@@ -1076,11 +1064,7 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod)
else if (range == (INTERVAL_MASK(MINUTE) |
INTERVAL_MASK(SECOND)))
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 hour;
-#else
- double hour;
-#endif
+ TimeOffset hour; interval->month = 0;
interval->day = 0;
@@ -1342,11 +1326,7 @@ timestamptz_to_str(TimestampTz t)
void
dt2time(Timestamp jd, int *hour, int *min, int *sec, fsec_t *fsec)
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 time;
-#else
- double time;
-#endif
+ TimeOffset time;time = jd;
@@ -1547,13 +1527,8 @@ recalc_t:
int
tm2timestamp(struct pg_tm * tm, fsec_t fsec, int *tzp, Timestamp *result)
{
-#ifdef HAVE_INT64_TIMESTAMP
- int date;
- int64 time;
-#else
- double date,
- time;
-#endif
+ DateOffset date;
+ TimeOffset time; /* Julian day routines are not correct for negative Julian days */
if (!IS_VALID_JULIAN(tm->tm_year, tm->tm_mon, tm->tm_mday))
@@ -1596,13 +1571,8 @@ tm2timestamp(struct pg_tm * tm, fsec_t fsec, int *tzp, Timestamp *result)
int
interval2tm(Interval span, struct pg_tm * tm, fsec_t *fsec)
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 time;
- int64 tfrac;
-#else
- double time;
- double tfrac;
-#endif
+ TimeOffset time;
+ TimeOffset tfrac; tm->tm_year = span.month / MONTHS_PER_YEAR;
tm->tm_mon = span.month % MONTHS_PER_YEAR;
@@ -1658,19 +1628,15 @@ tm2interval(struct pg_tm * tm, fsec_t fsec, Interval *span)
return 0;
}
-#ifdef HAVE_INT64_TIMESTAMP
-static int64
+static TimeOffset
time2t(const int hour, const int min, const int sec, const fsec_t fsec)
{
+#ifdef HAVE_INT64_TIMESTAMP
return (((((hour * MINS_PER_HOUR) + min) * SECS_PER_MINUTE) + sec) * USECS_PER_SEC) + fsec;
-} /* time2t() */
#else
-static double
-time2t(const int hour, const int min, const int sec, const fsec_t fsec)
-{
return (((hour * MINS_PER_HOUR) + min) * SECS_PER_MINUTE) + sec + fsec;
-} /* time2t() */
#endif
+} /* time2t() */ static Timestamp
dt2local(Timestamp dt, int tz)
@@ -2042,13 +2008,8 @@ timestamptz_cmp_timestamp(PG_FUNCTION_ARGS)
static int
interval_cmp_internal(Interval *interval1, Interval *interval2)
{
-#ifdef HAVE_INT64_TIMESTAMP
- int64 span1,
+ TimeOffset span1,
span2;
-#else
- double span1,
- span2;
-#endif span1 = interval1->time;
span2 = interval2->time;
@@ -2388,11 +2349,7 @@ interval_justify_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(0);
Interval *result;
-#ifdef HAVE_INT64_TIMESTAMP
- int64 wholeday;
-#else
- double wholeday;
-#endif
+ TimeOffset wholeday;
int32 wholemonth; result = (Interval *) palloc(sizeof(Interval));
@@ -2460,11 +2417,7 @@ interval_justify_hours(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(0);
Interval *result;
-#ifdef HAVE_INT64_TIMESTAMP
- int64 wholeday;
-#else
- double wholeday;
-#endif
+ TimeOffset wholeday; result = (Interval *) palloc(sizeof(Interval));
result->month = span->month;
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index 34e6644..63e033c 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -36,9 +36,13 @@
#ifdef HAVE_INT64_TIMESTAMP
typedef int64 Timestamp;
typedef int64 TimestampTz;
+typedef int64 TimeOffset;
+typedef int DateOffset;
#else
typedef double Timestamp;
typedef double TimestampTz;
+typedef double TimeOffset;
+typedef double DateOffset;
#endif typedef struct
--
1.5.2.5
I have removed all instances of using HAVE_INT64_TIMESTAMP to determine the
type of a variable in files depending on timestamp.h.
---
src/backend/utils/adt/nabstime.c | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/nabstime.c b/src/backend/utils/adt/nabstime.c
index 992f7ab..5a662f3 100644
--- a/src/backend/utils/adt/nabstime.c
+++ b/src/backend/utils/adt/nabstime.c
@@ -832,11 +832,7 @@ interval_reltime(PG_FUNCTION_ARGS)
month,
day;-#ifdef HAVE_INT64_TIMESTAMP
- int64 span;
-#else
- double span;
-#endif
+ TimeOffset span; year = interval->month / MONTHS_PER_YEAR;
month = interval->month % MONTHS_PER_YEAR;
--
1.5.2.5
Warren Turkal <turkal@google.com> writes:
I added TimeOffset and DateOffset typedefs to get rid of the instances
using the HAVE_INT64_TIMESTAMP define being used to determine the
types of variables or functions in timestamp.c.
Applied with minor revisions. I dropped DateOffset since it didn't seem
to be pulling its weight --- there was only one use and that was
probably better declared as TimeOffset anyway. We can always add it
later if we really do need it. Also I knocked over one or two other
places (in files beyond the ones you touched) where there was a
conditional declaration that could be eliminated.
There sure are a lot of #ifdef HAVE_INT64_TIMESTAMP's left, though,
aren't there :-(. It looked like the next thing to think about was
how to unify the scale-dependent calculations. We could make some
headway by defining a conversion constant that was either 1000000
or 1.0, but I'm worried about whether the C compiler is always smart
enough to optimize away a floating point multiplication or division
by 1.0 (there are at least some contexts where it *shouldn't* do
that, I think). Might be better to make macros that either
multiply/divide by 1000000 or do nothing.
BTW, not sure if you're aware of this, but pgindent tends to add
and subtract blank lines around #if/#else/#endif commands in weird,
inconsistent ways. If the vertical spacing seems a bit odd after
you've removed a conditional, feel free to fix it. I fixed a number
of places like that in this patch.
regards, tom lane
On Thu, Mar 20, 2008 at 6:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Warren Turkal <turkal@google.com> writes:
I added TimeOffset and DateOffset typedefs to get rid of the instances
using the HAVE_INT64_TIMESTAMP define being used to determine the
types of variables or functions in timestamp.c.Applied with minor revisions. I dropped DateOffset since it didn't seem
to be pulling its weight --- there was only one use and that was
probably better declared as TimeOffset anyway. We can always add it
later if we really do need it. Also I knocked over one or two other
places (in files beyond the ones you touched) where there was a
conditional declaration that could be eliminated.There sure are a lot of #ifdef HAVE_INT64_TIMESTAMP's left, though,
aren't there :-(. It looked like the next thing to think about was
how to unify the scale-dependent calculations. We could make some
headway by defining a conversion constant that was either 1000000
or 1.0, but I'm worried about whether the C compiler is always smart
enough to optimize away a floating point multiplication or division
by 1.0 (there are at least some contexts where it *shouldn't* do
that, I think). Might be better to make macros that either
multiply/divide by 1000000 or do nothing.BTW, not sure if you're aware of this, but pgindent tends to add
and subtract blank lines around #if/#else/#endif commands in weird,
inconsistent ways. If the vertical spacing seems a bit odd after
you've removed a conditional, feel free to fix it. I fixed a number
of places like that in this patch.
Thanks for the info. I will look over the conversions. I have had them
in the back of my mind for a little while.
wt