AugustNagro / magnum

A 'new look' for database access in Scala
Apache License 2.0
153 stars 10 forks source link

fix UTC time for timstamp decoder/encoder #9

Closed EvgenyAfanasev closed 1 year ago

EvgenyAfanasev commented 1 year ago

Hi. Today I wrote integration tests for my app. Currently my postgres table have column with type 'timstamp', which means that column have value without timezone info. But when I try insert timestamp value to table I see value +n hours from UTC (in my case it was +3 hours). I think, app should store value in UTC format, because if app will launch on different servers in different regions, we can have a lot of problems with it.

lbialy commented 1 year ago

While I get the rationale for this change you really should just use timestamptz 😆

EvgenyAfanasev commented 1 year ago

@lbialy may be you're right, but I want store all data with UTC offset. IMHO, create timestampz only for storing UTC offset isn't clean solution. timstamp without timezone usually used for this, but currently it has non transparently

lbialy commented 1 year ago

Why isn't using the timestamptz the right solution here? It's has exactly the same size as timestamp. The only thing that is different is the fact that when you pass a timezone-aware data to Postgres for a timestamptz column it normalizes it to UTC on write and stores data in UTC. timestamptz is the correct choice in 99% of cases because in 99% of cases you are storing point in time and that exactly calls for timezone-awareness. I'd be against this change (because it changes dumb timestamps for the user transparently and it might be confusing when he/she queries database later with a different tool) if not for the fact that I literally don't see much reasons to ever use timestamp over timestamptz so maybe doing the reasonable thing as default is a good choice here, I don't know. Either way, if you're storing points in time you should use timestamptz.

EvgenyAfanasev commented 1 year ago

@lbialy yeah, you're right. Yesterday I researched some best practices and currently I agree with you. May be usage in companies where I worked was not correct and clean. I will close this PR

AugustNagro commented 1 year ago

Great conversation; I agree timestamptz should always be used. In Magnum you can map it to OffsetDateTime.

In my applications, I generally set up a configurable java.time.Clock and use it like OffsetDateTime.now(myClock).

If you wanted your app to always use UTC, then you can use the [utc clock](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/time/Clock.html#systemUTC())