Skip to content

Convert all timestamps to TIMESTAMP WITH TIMEZONE #1067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jyn514 opened this issue Sep 25, 2020 · 8 comments
Closed

Convert all timestamps to TIMESTAMP WITH TIMEZONE #1067

jyn514 opened this issue Sep 25, 2020 · 8 comments
Labels
A-backend Area: Webserver backend C-technical-debt Category: This makes the code harder to read and modify, but has no impact on end users

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 25, 2020

This came up while working on #874, but it's good practice anyway. We store them in Rust as DateTime<Utc>, it should be the same in the database.

@jyn514 jyn514 added C-technical-debt Category: This makes the code harder to read and modify, but has no impact on end users A-backend Area: Webserver backend labels Sep 25, 2020
@Nemo157
Copy link
Member

Nemo157 commented Sep 25, 2020

timestamp without timezone is easier to work with. Postgres doesn’t actually store timezones, it just converts between UTC and the connections timezone when reading.

@jyn514
Copy link
Member Author

jyn514 commented Sep 25, 2020

timestamp without timezone is easier to work with

Hmm, I'm curious why you say that. The first thing we always do fetching them from the database is to add the timezone: https://github.com/rust-lang/docs.rs/blob/master/src/web/releases.rs#L104

@Nemo157
Copy link
Member

Nemo157 commented Sep 25, 2020

Yep, because we're explicitly storing UTC timestamps in the database. Using timestamp with timezone doesn't get you any advantage over that (other than confusion when you have a connection with a non-UTC timezone on it).

@Nemo157
Copy link
Member

Nemo157 commented Sep 25, 2020

It looks like sqlx should be fine with converting between timestamp without timezone and DateTime<Utc>, it literally does the same Naive conversion when encoding and decoding as we do manually (though maybe the type checker won't like that).

@jyn514
Copy link
Member Author

jyn514 commented Sep 25, 2020

Unfortunately it does not:

error[E0308]: mismatched types
  --> src/web/builds.rs:52:9
   |
52 | /         query_as!(
53 | |             Build,
54 | |             "SELECT
55 | |                 builds.id,
...  |
67 | |             version,
68 | |         )
   | |_________^ expected struct `chrono::DateTime`, found struct `chrono::NaiveDateTime`
   |
   = note: expected struct `chrono::DateTime<chrono::Utc>`
              found struct `chrono::NaiveDateTime`
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@Nemo157
Copy link
Member

Nemo157 commented Oct 1, 2020

Given that launchbadge/sqlx#703 is not possible to hit in practice since the binary protocol is always used, and it not being possible to tell sqlx that our timestamp values are stored at UTC, it's probably fine to switch to timestamptz and just hope we don't run into any weirdness.

@syphar
Copy link
Member

syphar commented Dec 26, 2020

I had some time on my hands and created a first PR in #1223

@syphar
Copy link
Member

syphar commented Dec 29, 2020

we can close that one, right?

@Nemo157 Nemo157 closed this as completed Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend Area: Webserver backend C-technical-debt Category: This makes the code harder to read and modify, but has no impact on end users
Projects
None yet
Development

No branches or pull requests

3 participants