Thread: Re: pgsql: Remove internal uses of CTimeZone/HasCTZSet.
On Fri, Nov 01, 2013 at 04:51:34PM +0000, 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
Noah Misch <noah@leadboat.com> writes:
> On Fri, Nov 01, 2013 at 04:51:34PM +0000, 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
On Mon, Nov 04, 2013 at 02:34:02PM -0500, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Fri, Nov 01, 2013 at 04:51:34PM +0000, 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
On Mon, Nov 4, 2013 at 8:17 PM, Noah Misch <noah@leadboat.com> wrote: > On Mon, Nov 04, 2013 at 02:34:02PM -0500, Tom Lane wrote: >> Noah Misch <noah@leadboat.com> writes: >> > On Fri, Nov 01, 2013 at 04:51:34PM +0000, 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Nov 4, 2013 at 8:17 PM, Noah Misch <noah@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