Re: [HACKERS] pgsql: Remove internal uses of CTimeZone/HasCTZSet.
On Mon, Nov 4, 2013 at 8:17 PM, Noah Misch n...@leadboat.com wrote: On Mon, Nov 04, 2013 at 02:34:02PM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: On Fri, Nov 01, 2013 at 04:51:34PM +, Tom Lane wrote: Remove internal uses of CTimeZone/HasCTZSet. This changed EncodeDateTime() output for USE_SQL_DATES and USE_GERMAN_DATES styles, because it inserts a space before tzn but does not insert a space before EncodeTimezone() output. Example: set datestyle = sql,mdy; select '2013-01-01'::timestamptz; old output: timestamptz 01/01/2013 00:00:00+00 new output: timestamptz - 01/01/2013 00:00:00 +00 I assume this was unintended. Yeah, I had seen some cases of that. I don't find it of great concern. We'd have to insert some ugly special-case code that looks at the zone name to decide whether to insert a space or not, and I don't think it'd actually be an improvement to do so. (Arguably, these formats are more consistent this way.) Still, this change is probably a sufficient reason not to back-patch this part of the fix. I was prepared to suppose that no substantial clientele relies on the to_char() TZ format code expanding to blank, the other behavior that changed with this patch. It's more of a stretch to figure applications won't stumble over this new whitespace. I do see greater consistency in the new behavior, but changing type output is a big deal. While preserving the old output does require ugly special-case code, such code was in place for years until removal by this commit and the commit following it. Perhaps making the behavior change is best anyway, but we should not conclude that decision on the basis of its origin as a side effect of otherwise-desirable refactoring. I have to admit I fear this will break a huge amount of application code. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] pgsql: Remove internal uses of CTimeZone/HasCTZSet.
Robert Haas robertmh...@gmail.com writes: On Mon, Nov 4, 2013 at 8:17 PM, Noah Misch n...@leadboat.com wrote: I was prepared to suppose that no substantial clientele relies on the to_char() TZ format code expanding to blank, the other behavior that changed with this patch. It's more of a stretch to figure applications won't stumble over this new whitespace. I do see greater consistency in the new behavior, but changing type output is a big deal. While preserving the old output does require ugly special-case code, such code was in place for years until removal by this commit and the commit following it. Perhaps making the behavior change is best anyway, but we should not conclude that decision on the basis of its origin as a side effect of otherwise-desirable refactoring. I have to admit I fear this will break a huge amount of application code. I don't believe that. You are talking about the intersection of (1) people who use a brute force timezone (which we already know is a small set, else the original bug would've been noticed long ago). (2) people who use SQL or German datestyle, neither of which is default. (3) people whose apps will fail if there's whitespace at a specific place in timestamp output, even though in many other cases there will be whitespace at that place anyway, notably including the case where they select any regular timezone. It's possible that this intersection is nonempty, but huge amount? Come on. This is minor compared to the application compatibility issues we come up with in every major release. More, I would argue that the number of failing applications is likely to be exceeded by the number that no longer fail upon selection of a brute-force zone, because they'd only ever been coded or tested with the normal-zone-name formatting. 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] pgsql: Remove internal uses of CTimeZone/HasCTZSet.
On Fri, Nov 01, 2013 at 04:51:34PM +, Tom Lane wrote: Remove internal uses of CTimeZone/HasCTZSet. The only remaining places where we actually look at CTimeZone/HasCTZSet are abstime2tm() and timestamp2tm(). Now that session_timezone is always valid, we can remove these special cases. The caller-visible impact of this is that these functions now always return a valid zone abbreviation if requested, whereas before they'd return a NULL pointer if a brute-force timezone was in use. In the existing code, the only place I can find that changes behavior is to_char(), whose TZ format code will now print something useful rather than nothing for such zones. (In the places where the returned zone abbreviation is passed to EncodeDateTime, the lack of visible change is because we've chosen the abbreviation used for these zones to match what EncodeTimezone would have printed.) This changed EncodeDateTime() output for USE_SQL_DATES and USE_GERMAN_DATES styles, because it inserts a space before tzn but does not insert a space before EncodeTimezone() output. Example: set datestyle = sql,mdy; select '2013-01-01'::timestamptz; old output: timestamptz 01/01/2013 00:00:00+00 new output: timestamptz - 01/01/2013 00:00:00 +00 I assume this was unintended. This could be back-patched if we decide we'd like to fix to_char()'s behavior in the back branches, but there doesn't seem to be much enthusiasm for that at present. Note that for the corresponding scenario in Oracle, the TZD format code yields blank and the TZR format code yields -01:30. (Oracle does not have a plain TZ escape.) So there's precedent for each choice of behavior, and I don't see this as a back-patch candidate. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- 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] pgsql: Remove internal uses of CTimeZone/HasCTZSet.
Noah Misch n...@leadboat.com writes: On Fri, Nov 01, 2013 at 04:51:34PM +, Tom Lane wrote: Remove internal uses of CTimeZone/HasCTZSet. This changed EncodeDateTime() output for USE_SQL_DATES and USE_GERMAN_DATES styles, because it inserts a space before tzn but does not insert a space before EncodeTimezone() output. Example: set datestyle = sql,mdy; select '2013-01-01'::timestamptz; old output: timestamptz 01/01/2013 00:00:00+00 new output: timestamptz - 01/01/2013 00:00:00 +00 I assume this was unintended. Yeah, I had seen some cases of that. I don't find it of great concern. We'd have to insert some ugly special-case code that looks at the zone name to decide whether to insert a space or not, and I don't think it'd actually be an improvement to do so. (Arguably, these formats are more consistent this way.) Still, this change is probably a sufficient reason not to back-patch this part of the fix. 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] pgsql: Remove internal uses of CTimeZone/HasCTZSet.
On Mon, Nov 04, 2013 at 02:34:02PM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: On Fri, Nov 01, 2013 at 04:51:34PM +, Tom Lane wrote: Remove internal uses of CTimeZone/HasCTZSet. This changed EncodeDateTime() output for USE_SQL_DATES and USE_GERMAN_DATES styles, because it inserts a space before tzn but does not insert a space before EncodeTimezone() output. Example: set datestyle = sql,mdy; select '2013-01-01'::timestamptz; old output: timestamptz 01/01/2013 00:00:00+00 new output: timestamptz - 01/01/2013 00:00:00 +00 I assume this was unintended. Yeah, I had seen some cases of that. I don't find it of great concern. We'd have to insert some ugly special-case code that looks at the zone name to decide whether to insert a space or not, and I don't think it'd actually be an improvement to do so. (Arguably, these formats are more consistent this way.) Still, this change is probably a sufficient reason not to back-patch this part of the fix. I was prepared to suppose that no substantial clientele relies on the to_char() TZ format code expanding to blank, the other behavior that changed with this patch. It's more of a stretch to figure applications won't stumble over this new whitespace. I do see greater consistency in the new behavior, but changing type output is a big deal. While preserving the old output does require ugly special-case code, such code was in place for years until removal by this commit and the commit following it. Perhaps making the behavior change is best anyway, but we should not conclude that decision on the basis of its origin as a side effect of otherwise-desirable refactoring. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
