Re: API bug in DetermineTimeZoneOffset()

I wrote:
> So what I'm thinking we should do is internally translate SET TIMEZONE
> with an interval value into a POSIX-style zone name in the above format,
> and then just flush HasCTZSet/CTimeZone and all the special case logic
> around them.

Attached is a set of proposed patches for this.

> 1. The zone abbreviation printed by timeofday(), to_char(), etc would now
> become "GMT+-hh[:mm[:ss]]" (or whatever we choose to stick in the angle
> brackets, but that's what I'm thinking).

After experimentation it seems that the best idea is to make the displayed
abbreviation be just "+-hh[:mm[:ss]]", using ISO sign convention. This
avoids changes in behavior in places where the code already prints
something reasonable for the timezone name.

The first attached patch just causes session_timezone to point to a pg_tz
struct defined along these lines whenever HasCTZSet is true, plus removing
the very bogus lookaside check in DetermineTimeZoneOffset(). As exhibited
in the added regression tests, this fixes the behavior complained of in
bug #8572. It also makes timeofday() return self-consistent data when a
brute-force timezone is in use. AFAICT it doesn't have any other visible
effects, and it shouldn't break any third-party modules. So I propose
back-patching this to all active branches.

The second attached patch, to be applied after the first, removes the
existing checks of HasCTZSet in the backend. The only visible effect of
this, AFAICT, is that to_char's TZ format spec now delivers something
useful instead of an empty string when a brute-force timezone is in use.
I could be persuaded either way as to whether to back-patch this part.
From one standpoint, this to_char behavioral change is clearly a bug fix
--- but it's barely possible that somebody out there thought that
returning an empty string for TZ was actually the intended/desirable
behavior.

The third patch, to be applied last, nukes HasCTZSet/CTimeZone entirely.
The only consequence I can see at SQL level is that SHOW TIMEZONE will
return a POSIX zone spec instead of an interval value for a brute-force
zone setting (cf change in regression test output). Since the bare
interval value isn't actually something that SET TIMEZONE will accept,
this is clearly a step forward in self-consistency, but it's not something
I would propose to back-patch. In any case we probably don't want to
remove these variables in back branches, on the off chance that some
third-party code is looking at them.

Comments?

regards, tom lane