Bug 163 - Resolve Timezone ambiguity
: Resolve Timezone ambiguity
Status: RESOLVED FIXED
Product: Slony-I
slon
: devel
: All All
: low normal
Assigned To: Christopher Browne
: https://github.com/cbbrowne/slony1-en...
:
:
  Show dependency treegraph
 
Reported: 2010-11-01 14:19 PDT by Christopher Browne
Modified: 2011-01-12 13:16 PST (History)
1 user (show)

See Also:


Attachments


Note

You need to log in before you can comment on or make changes to this bug.


Description Christopher Browne 2010-11-01 14:19:31 PDT
There is a concern out there as to what happens if Slony-I is operating in a
timezone that has the Daylight Savings Time discontinuity.

Some ways of addressing this include:

a) Changing Slony-I tables that have TIMESTAMP values to, instead, have
TIMESTAMP WITH TIMEZONE.

Downsides include that this involves a number of alterations to existing
tables, with a conversion ambiguity.  (e.g. - what should be done with existing
data?  This seems notably risky if the existing timestamps surround the
discontinuity.)

b) Have the slon "health check" (see bug #156) look at the timezone in
TimeZone, and warn if it has a discontinuity.

Open question:  UTC and GMT would be acceptable timezones; what others are
potentially acceptable?

c) Have slon process expressly use UTC as its timezone.

This might change logging output, and be unpopular to users accustomed to
operating Slony in some other timezone.
Comment 1 Christopher Browne 2010-12-08 12:02:32 PST
Intent: Do a).
Comment 2 Christopher Browne 2010-12-08 14:38:23 PST
Set up a branch for this on GitHub...

https://github.com/cbbrowne/slony1-engine/commits/bug163

Took an initial swing at implementation:

https://github.com/cbbrowne/slony1-engine/commit/42a4d2ed275d4d2c1b08f49b5c079e8c6a790258

1.  Change TIMESTAMP to TIMESTAMPTZ in slony1_base.sql

2.  Add logic to upgradeSchema() that looks for tables with "timestamp without
time zone" and changes them to "timestamp with time zone"

This has a couple of complications...

The view sl_status depends on the underlying tables; if you try to alter them,
this errors out as follows:

Dec  8 17:01:23 cbbrowne postgres[32294]: [3-1] ERROR:  cannot alter type of a
column used by a view or rule
Dec  8 17:01:23 cbbrowne postgres[32294]: [3-2] DETAIL:  rule _RETURN on view
sl_status depends on column "ev_timestamp"
Dec  8 17:01:23 cbbrowne postgres[32294]: [3-3] STATEMENT:  alter table
sl_event  alter column ev_timestamp set data type timestamp with time zone;

Consequently, I added logic to pull the definition of sl_status, using
pg_get_viewdef().  So the logic is...

- Capture view definition for sl_status
- Drop view sl_status
- Do all necessary alterations
- Recreate sl_status

Note also that this requires changing the schema to the Slony cluster's schema,
so pg_get_viewdef() and the subsequent recreation are working against the same
namespaces.

Consequence: change the function to set search_path appropriately.  That seems
like something we might want to do to all the Slony functions.
Comment 3 Steve Singer 2010-12-13 14:00:27 PST
In slony1_funcs.sql log_switch_start()

The reason why the now() needed a typecast was because now() returns a 
timestampz and we were storing just a timestamp.  Since we are now 
storing the full timestampz we don't need the typecast.

upgradeSchema()

My reading of this function is that it will take ANY table in the slony 
schema that has a timestamp column and replace it with a timestampz 
column.  I don't think this query is restricting things to the set of 
slony tables that exist today.

If in the future (maybe version 2.2 or 3.5)  we add a new column/table 
and we REALLY do want a timestamp with no time zone then this 
upgradeSchema() function will still alter the table.

If anyone has added a custom table to their slony schema then this query 
will alter their table as well. (I don't want to encourage people to be 
adding their own tables to the slony schema; I just don't like the idea 
of using pattern matching to any table/column in the schema as the 
target for an alter table). I'd be more comfortable if we filtered on 
the explicit slony tables we have in 2.0


This section should also have a comment a to what it's for ie
--
-- When upgrading to 2.1 the 'timestamp' columns get changed to
-- timestampz since we now store the timezone

I have not tested this verison of the patch just reviewed it.
Comment 5 Christopher Browne 2010-12-30 09:00:21 PST
In principle, this may be committable; can folks take a poke at the branch
https://github.com/cbbrowne/slony1-engine/tree/bug163
???