PR#2237: db: use timestamps with timezone - koji

koji

#2237 db: use timestamps with timezone

Merged 2 years ago by tkopecek. Opened 3 years ago by tkopecek.

tkopecek/koji issue2160 into master

db: use timestamps with timezone

Tomas Kopecek • 3 years ago

731ebbb

docs/schema-upgrade-1.21-1.22.sql

file added

+31

		`@@ -0,0 +1,31 @@`
		`+ -- upgrade script to migrate the Koji database schema`
		`+ -- from version 1.20 to 1.21`
		`+`
		`+`
		`+ BEGIN;`
		`+`
		`+ ALTER TABLE events ALTER COLUMN time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), time::timestamptz);`
		`+ ALTER TABLE sessions ALTER COLUMN start_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), start_time::timestamptz);`
		`+ ALTER TABLE sessions ALTER COLUMN update_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), update_time::timestamptz);`
		`+ ALTER TABLE task ALTER COLUMN create_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), create_time::timestamptz);`
		`+ ALTER TABLE task ALTER COLUMN start_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), start_time::timestamptz);`
		`+ ALTER TABLE task ALTER COLUMN completion_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), completion_time::timestamptz);`
		`+ ALTER TABLE build ALTER COLUMN start_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), start_time::timestamptz);`
		`+ ALTER TABLE build ALTER COLUMN completion_time TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), completion_time::timestamptz);`
		`+ ALTER TABLE build_reservations ALTER COLUMN created TYPE TIMESTAMPTZ USING`
		`+ timezone(current_setting('TIMEZONE'), created::timestamptz);`
		`+`
		`+ DROP FUNCTION IF EXISTS get_event_time;`
		`+ CREATE FUNCTION get_event_time(INTEGER) RETURNS TIMESTAMPTZ AS '`
		`+ SELECT time FROM events WHERE id=$1;`
		`+ ' LANGUAGE SQL;`
		`+`
		`+ COMMIT;`

docs/schema.sql

file modified

+10 -10

		`@@ -7,7 +7,7 @@`
		`-- in the event that the system clock rolls back, event_ids will retain proper sequencing`
		`CREATE TABLE events (`
		`id SERIAL NOT NULL PRIMARY KEY,`
		`- time TIMESTAMP NOT NULL DEFAULT clock_timestamp()`
		`+ time TIMESTAMPTZ NOT NULL DEFAULT clock_timestamp()`
		`) WITHOUT OIDS;`

		`-- A function that creates an event and returns the id, used as DEFAULT value for versioned tables`
		`@@ -18,7 +18,7 @@`

		`-- A convenience function for converting events to timestamps, useful for`
		`-- quick queries where you want to avoid JOINs.`
		`- CREATE FUNCTION get_event_time(INTEGER) RETURNS TIMESTAMP AS '`
		`+ CREATE FUNCTION get_event_time(INTEGER) RETURNS TIMESTAMPTZ AS '`
		`SELECT time FROM events WHERE id=$1;`
		`' LANGUAGE SQL;`

		`@@ -116,8 +116,8 @@`
		`authtype INTEGER,`
		`hostip VARCHAR(255),`
		`callnum INTEGER,`
		`- start_time TIMESTAMP NOT NULL DEFAULT NOW(),`
		`- update_time TIMESTAMP NOT NULL DEFAULT NOW(),`
		`+ start_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),`
		`+ update_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),`
		`exclusive BOOLEAN CHECK (exclusive),`
		`CONSTRAINT no_exclusive_subsessions CHECK (`
		`master IS NULL OR "exclusive" IS NULL),`
		`@@ -213,9 +213,9 @@`
		`CREATE TABLE task (`
		`id SERIAL NOT NULL PRIMARY KEY,`
		`state INTEGER,`
		`- create_time TIMESTAMP NOT NULL DEFAULT NOW(),`
		`- start_time TIMESTAMP,`
		`- completion_time TIMESTAMP,`
		`+ create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),`
		`+ start_time TIMESTAMPTZ,`
		`+ completion_time TIMESTAMPTZ,`
		`channel_id INTEGER NOT NULL REFERENCES channels(id),`
		`host_id INTEGER REFERENCES host (id),`
		`parent INTEGER REFERENCES task (id),`
		`@@ -279,8 +279,8 @@`
		`epoch INTEGER,`
		`source TEXT,`
		`create_event INTEGER NOT NULL REFERENCES events(id) DEFAULT get_event(),`
		`- start_time TIMESTAMP,`
		`- completion_time TIMESTAMP,`
		`+ start_time TIMESTAMPTZ,`
		`+ completion_time TIMESTAMPTZ,`
		`state INTEGER NOT NULL,`
		`task_id INTEGER REFERENCES task (id),`
		`owner INTEGER NOT NULL REFERENCES users (id),`
		`@@ -518,7 +518,7 @@`
		`CREATE TABLE build_reservations (`
		`build_id INTEGER NOT NULL REFERENCES build(id),`
		`token VARCHAR(64),`
		`- created TIMESTAMP NOT NULL,`
		`+ created TIMESTAMPTZ NOT NULL,`
		`PRIMARY KEY (build_id)`
		`) WITHOUT OIDS;`
		`CREATE INDEX build_reservations_created ON build_reservations(created);`

tkopecek commented 3 years ago

Fixes: https://pagure.io/koji/issue/2160

tkopecek commented 3 years ago

Not sure, if it solves everything we want. We still return data via API from DB, so users will get local timestamps. @mikem ?

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

2 years ago

mikem commented 2 years ago

This does seem to resolve the issue at hand. I tested with the following method.

configured my local db instance with timezone = 'America/Los_Angeles' (not utc and not my local tz) in postgresql.conf.
restarted postgres
force event creation in my test koji instance (created a new tag)
call getLastEvent and print that tz with time.localtime

Without the schema changes, the reported time is off. With them, it matches my local time.

I'm not seeing any obvious problems, but since this touches so many tables, there's always the possibility of a subtle issue.

Edited 2 years ago by mikem

mikem commented 2 years ago

At any rate, I think this is the direction we want to go and I don't see any obvious issues, so :thumbsup:

tkopecek commented 2 years ago

Commit 4675229 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago