about EncodeDateTime() arguments
We currently have
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style, char *str)
but tzn isn't used anywhere, only *tzn is used everywhere. Wouldn't it
be clearer to remove that one level of indirection and instead have the
signature be
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char *tzn, int style, char *str)
or better yet
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, const int *tzp, const char *tzn, int style, char *str)
Peter Eisentraut <peter_e@gmx.net> writes:
We currently have
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style, char *str)
but tzn isn't used anywhere, only *tzn is used everywhere. Wouldn't it
be clearer to remove that one level of indirection and instead have the
signature be
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char *tzn, int style, char *str)
or better yet
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, const int *tzp, const char *tzn, int style, char *str)
It appears to me that null-ness of tzp and tzn are used as a 3-way flag
to identify the style of timezone output wanted (none, numeric, or alpha).
It would probably be better yet if it went like
enum tzstyle, int tzp, const char *tzn
where tzp or tzn would be examined only if tzstyle said so.
regards, tom lane
On lör, 2012-03-10 at 18:47 -0500, Tom Lane wrote:
void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, const int *tzp, const char *tzn, int style, char *str)
It appears to me that null-ness of tzp and tzn are used as a 3-way flag
to identify the style of timezone output wanted (none, numeric, or alpha).
It would probably be better yet if it went likeenum tzstyle, int tzp, const char *tzn
where tzp or tzn would be examined only if tzstyle said so.
It's not quite a three-way flag, because it also depends on the style,
which time zone style is used. But I liked the idea of making "print
the time zone" more explicit and getting rid of the funny pointers. I
added a bit of documentation and code deduplication in the attached
patch, and it already looks much more understandable.
Attachments:
encodedatetime-api.patchtext/x-patch; charset=UTF-8; name=encodedatetime-api.patchDownload
diff --git i/src/backend/utils/adt/date.c w/src/backend/utils/adt/date.c
index 85e8fd0..2da4e04 100644
--- i/src/backend/utils/adt/date.c
+++ w/src/backend/utils/adt/date.c
@@ -1131,7 +1131,7 @@ time_out(PG_FUNCTION_ARGS)
char buf[MAXDATELEN + 1];
time2tm(time, tm, &fsec);
- EncodeTimeOnly(tm, fsec, NULL, DateStyle, buf);
+ EncodeTimeOnly(tm, fsec, false, 0, DateStyle, buf);
result = pstrdup(buf);
PG_RETURN_CSTRING(result);
@@ -1918,7 +1918,7 @@ timetz_out(PG_FUNCTION_ARGS)
char buf[MAXDATELEN + 1];
timetz2tm(time, tm, &fsec, &tz);
- EncodeTimeOnly(tm, fsec, &tz, DateStyle, buf);
+ EncodeTimeOnly(tm, fsec, true, tz, DateStyle, buf);
result = pstrdup(buf);
PG_RETURN_CSTRING(result);
diff --git i/src/backend/utils/adt/datetime.c w/src/backend/utils/adt/datetime.c
index f495c3f..56515f1 100644
--- i/src/backend/utils/adt/datetime.c
+++ w/src/backend/utils/adt/datetime.c
@@ -3679,39 +3679,54 @@ EncodeDateOnly(struct pg_tm * tm, int style, char *str)
/* EncodeTimeOnly()
* Encode time fields only.
+ *
+ * tm and fsec are the value to encode, print_tz determines whether to include
+ * a time zone (the difference between time and timetz types), tz is the
+ * numeric time zone offset, style is the date style, str is where to write the
+ * output.
*/
void
-EncodeTimeOnly(struct pg_tm * tm, fsec_t fsec, int *tzp, int style, char *str)
+EncodeTimeOnly(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, int style, char *str)
{
sprintf(str, "%02d:%02d:", tm->tm_hour, tm->tm_min);
str += strlen(str);
AppendSeconds(str, tm->tm_sec, fsec, MAX_TIME_PRECISION, true);
- if (tzp != NULL)
- EncodeTimezone(str, *tzp, style);
+ if (print_tz)
+ EncodeTimezone(str, tz, style);
}
/* EncodeDateTime()
* Encode date and time interpreted as local time.
- * Support several date styles:
+ *
+ * tm and fsec are the value to encode, print_tz determines whether to include
+ * a time zone (the difference between timestamp and timestamptz types), tz is
+ * the numeric time zone offset, tzn is the textual time zone, which if
+ * specified will be used instead of tz by some styles, style is the date
+ * style, str is where to write the output.
+ *
+ * Supported date styles:
* Postgres - day mon hh:mm:ss yyyy tz
* SQL - mm/dd/yyyy hh:mm:ss.ss tz
* ISO - yyyy-mm-dd hh:mm:ss+/-tz
* German - dd.mm.yyyy hh:mm:ss tz
* XSD - yyyy-mm-ddThh:mm:ss.ss+/-tz
- * Variants (affects order of month and day for Postgres and SQL styles):
- * US - mm/dd/yyyy
- * European - dd/mm/yyyy
*/
void
-EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style, char *str)
+EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str)
{
int day;
Assert(tm->tm_mon >= 1 && tm->tm_mon <= MONTHS_PER_YEAR);
+ /*
+ * Negative tm_isdst means we have no valid time zone translation.
+ */
+ if (tm->tm_isdst < 0)
+ print_tz = false;
+
switch (style)
{
case USE_ISO_DATES:
@@ -3729,14 +3744,8 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style,
AppendTimestampSeconds(str + strlen(str), tm, fsec);
- /*
- * tzp == NULL indicates that we don't want *any* time zone info
- * in the output string. *tzn != NULL indicates that we have alpha
- * time zone info available. tm_isdst != -1 indicates that we have
- * a valid time zone translation.
- */
- if (tzp != NULL && tm->tm_isdst >= 0)
- EncodeTimezone(str, *tzp, style);
+ if (print_tz)
+ EncodeTimezone(str, tz, style);
if (tm->tm_year <= 0)
sprintf(str + strlen(str), " BC");
@@ -3762,12 +3771,12 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style,
* TZ abbreviations in the Olson database are plain ASCII.
*/
- if (tzp != NULL && tm->tm_isdst >= 0)
+ if (print_tz)
{
- if (*tzn != NULL)
- sprintf(str + strlen(str), " %.*s", MAXTZLEN, *tzn);
+ if (tzn)
+ sprintf(str + strlen(str), " %.*s", MAXTZLEN, tzn);
else
- EncodeTimezone(str, *tzp, style);
+ EncodeTimezone(str, tz, style);
}
if (tm->tm_year <= 0)
@@ -3785,12 +3794,12 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style,
AppendTimestampSeconds(str + strlen(str), tm, fsec);
- if (tzp != NULL && tm->tm_isdst >= 0)
+ if (print_tz)
{
- if (*tzn != NULL)
- sprintf(str + strlen(str), " %.*s", MAXTZLEN, *tzn);
+ if (tzn)
+ sprintf(str + strlen(str), " %.*s", MAXTZLEN, tzn);
else
- EncodeTimezone(str, *tzp, style);
+ EncodeTimezone(str, tz, style);
}
if (tm->tm_year <= 0)
@@ -3819,10 +3828,10 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style,
sprintf(str + strlen(str), " %04d",
(tm->tm_year > 0) ? tm->tm_year : -(tm->tm_year - 1));
- if (tzp != NULL && tm->tm_isdst >= 0)
+ if (print_tz)
{
- if (*tzn != NULL)
- sprintf(str + strlen(str), " %.*s", MAXTZLEN, *tzn);
+ if (tzn)
+ sprintf(str + strlen(str), " %.*s", MAXTZLEN, tzn);
else
{
/*
@@ -3832,7 +3841,7 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style,
* the date/time parser later. - thomas 2001-10-19
*/
sprintf(str + strlen(str), " ");
- EncodeTimezone(str, *tzp, style);
+ EncodeTimezone(str, tz, style);
}
}
diff --git i/src/backend/utils/adt/nabstime.c w/src/backend/utils/adt/nabstime.c
index d35645d..dbd9e92 100644
--- i/src/backend/utils/adt/nabstime.c
+++ w/src/backend/utils/adt/nabstime.c
@@ -311,7 +311,7 @@ abstimeout(PG_FUNCTION_ARGS)
break;
default:
abstime2tm(time, &tz, tm, &tzn);
- EncodeDateTime(tm, fsec, &tz, &tzn, DateStyle, buf);
+ EncodeDateTime(tm, fsec, true, tz, tzn, DateStyle, buf);
break;
}
diff --git i/src/backend/utils/adt/timestamp.c w/src/backend/utils/adt/timestamp.c
index db434dc..edcce5f 100644
--- i/src/backend/utils/adt/timestamp.c
+++ w/src/backend/utils/adt/timestamp.c
@@ -215,13 +215,12 @@ timestamp_out(PG_FUNCTION_ARGS)
struct pg_tm tt,
*tm = &tt;
fsec_t fsec;
- char *tzn = NULL;
char buf[MAXDATELEN + 1];
if (TIMESTAMP_NOT_FINITE(timestamp))
EncodeSpecialTimestamp(timestamp, buf);
else if (timestamp2tm(timestamp, NULL, tm, &fsec, NULL, NULL) == 0)
- EncodeDateTime(tm, fsec, NULL, &tzn, DateStyle, buf);
+ EncodeDateTime(tm, fsec, false, 0, NULL, DateStyle, buf);
else
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
@@ -501,7 +500,7 @@ timestamptz_out(PG_FUNCTION_ARGS)
if (TIMESTAMP_NOT_FINITE(dt))
EncodeSpecialTimestamp(dt, buf);
else if (timestamp2tm(dt, &tz, tm, &fsec, &tzn, NULL) == 0)
- EncodeDateTime(tm, fsec, &tz, &tzn, DateStyle, buf);
+ EncodeDateTime(tm, fsec, true, tz, tzn, DateStyle, buf);
else
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
@@ -1422,7 +1421,7 @@ timestamptz_to_str(TimestampTz t)
if (TIMESTAMP_NOT_FINITE(t))
EncodeSpecialTimestamp(t, buf);
else if (timestamp2tm(t, &tz, tm, &fsec, &tzn, NULL) == 0)
- EncodeDateTime(tm, fsec, &tz, &tzn, USE_ISO_DATES, buf);
+ EncodeDateTime(tm, fsec, true, tz, tzn, USE_ISO_DATES, buf);
else
strlcpy(buf, "(timestamp out of range)", sizeof(buf));
diff --git i/src/backend/utils/adt/xml.c w/src/backend/utils/adt/xml.c
index d7b637c..b042e6b 100644
--- i/src/backend/utils/adt/xml.c
+++ w/src/backend/utils/adt/xml.c
@@ -1987,7 +1987,6 @@ map_sql_value_to_xml_value(Datum value, Oid type, bool xml_escape_strings)
Timestamp timestamp;
struct pg_tm tm;
fsec_t fsec;
- char *tzn = NULL;
char buf[MAXDATELEN + 1];
timestamp = DatumGetTimestamp(value);
@@ -1999,7 +1998,7 @@ map_sql_value_to_xml_value(Datum value, Oid type, bool xml_escape_strings)
errmsg("timestamp out of range"),
errdetail("XML does not support infinite timestamp values.")));
else if (timestamp2tm(timestamp, NULL, &tm, &fsec, NULL, NULL) == 0)
- EncodeDateTime(&tm, fsec, NULL, &tzn, USE_XSD_DATES, buf);
+ EncodeDateTime(&tm, fsec, false, 0, NULL, USE_XSD_DATES, buf);
else
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
@@ -2026,7 +2025,7 @@ map_sql_value_to_xml_value(Datum value, Oid type, bool xml_escape_strings)
errmsg("timestamp out of range"),
errdetail("XML does not support infinite timestamp values.")));
else if (timestamp2tm(timestamp, &tz, &tm, &fsec, &tzn, NULL) == 0)
- EncodeDateTime(&tm, fsec, &tz, &tzn, USE_XSD_DATES, buf);
+ EncodeDateTime(&tm, fsec, true, tz, tzn, USE_XSD_DATES, buf);
else
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
diff --git i/src/include/utils/datetime.h w/src/include/utils/datetime.h
index 358e2a6..d73cc8d 100644
--- i/src/include/utils/datetime.h
+++ w/src/include/utils/datetime.h
@@ -290,8 +290,8 @@ extern void DateTimeParseError(int dterr, const char *str,
extern int DetermineTimeZoneOffset(struct pg_tm * tm, pg_tz *tzp);
extern void EncodeDateOnly(struct pg_tm * tm, int style, char *str);
-extern void EncodeTimeOnly(struct pg_tm * tm, fsec_t fsec, int *tzp, int style, char *str);
-extern void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, int *tzp, char **tzn, int style, char *str);
+extern void EncodeTimeOnly(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, int style, char *str);
+extern void EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str);
extern void EncodeInterval(struct pg_tm * tm, fsec_t fsec, int style, char *str);
extern int DecodeSpecial(int field, char *lowtoken, int *val);
Peter Eisentraut <peter_e@gmx.net> writes:
On lör, 2012-03-10 at 18:47 -0500, Tom Lane wrote:
It appears to me that null-ness of tzp and tzn are used as a 3-way flag
to identify the style of timezone output wanted (none, numeric, or alpha).
It's not quite a three-way flag, because it also depends on the style,
which time zone style is used. But I liked the idea of making "print
the time zone" more explicit and getting rid of the funny pointers. I
added a bit of documentation and code deduplication in the attached
patch, and it already looks much more understandable.
Yeah, looks nicer to me too.
Should we propagate this fix into ecpg's copy of the code as well?
I'm not sure how far the backend has diverged from that copy.
regards, tom lane