Bug 163

Summary: Resolve Timezone ambiguity
Product: Slony-I Reporter: Christopher Browne <cbbrowne>
Component: slonAssignee: Christopher Browne <cbbrowne>
Severity: normal CC: slony1-bugs
Priority: low    
Version: devel   
Hardware: All   
OS: All   
URL: https://github.com/cbbrowne/slony1-engine/commits/bug163

Description Christopher Browne 2010-11-01 14:19:31 UTC
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 UTC
Intent: Do a).
Comment 2 Christopher Browne 2010-12-08 14:38:23 UTC
Set up a branch for this on GitHub...


Took an initial swing at implementation:


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 UTC
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.


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 UTC
In principle, this may be committable; can folks take a poke at the branch