Re: [HACKERS] about EncodeDateTime() arguments

2012-03-13 Thread Tom Lane
Peter Eisentraut  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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] about EncodeDateTime() arguments

2012-03-13 Thread Peter Eisentraut
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 like
> 
> enum 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.
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  tz
  *	SQL - mm/dd/ hh:mm:ss.ss tz
  *	ISO - -mm-dd hh:mm:ss+/-tz
  *	German - dd.mm. hh:mm:ss tz
  *	XSD - -mm-ddThh:mm:ss.ss+/-tz
- * Variants (affects order of month and day for Postgres and SQL styles):
- *	US - mm/dd/
- *	European - dd/mm/
  */
 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 @@

Re: [HACKERS] about EncodeDateTime() arguments

2012-03-10 Thread Tom Lane
Peter Eisentraut  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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] about EncodeDateTime() arguments

2012-03-10 Thread Peter Eisentraut
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)




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers