Skip to content
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

Wrong handling of Instant timestamps #3067

Open
michalkoza opened this issue Aug 6, 2024 · 2 comments
Open

Wrong handling of Instant timestamps #3067

michalkoza opened this issue Aug 6, 2024 · 2 comments

Comments

@michalkoza
Copy link

michalkoza commented Aug 6, 2024

Version: 4.5.0-4.8.5
Module: quill-jdbc
Database: postgres 16.3

Expected behavior

Inserting timestamp of type Instant, to column of type TIMEZONE should store exact Instant value, without introducing timezone offsets

Actual behavior

being e.g. in CET/CEST timezone and
storing
Instant.ofEpochSecond(1L)
stores in DB:
1970-01-01 01:00:01.000000 instead of
1970-01-01 00:00:01.000000
1 hr difference while in CET

storing
Instant.parse("2018-04-08T19:52:29Z")
stores in DB:
2018-04-08 21:52:29.000000 instead of
2018-04-08 19:52:29.000000
2 hr difference while in CEST

Issue introduction

The issue has been introduced here:
8117a0e#diff-5161e16f0f0a107bd28613fe49aa063e2e7390ec87a61c5a8fd540ca191f67beR83

Workaround

Storing Instants in TIMESTAMP with time zone columns seems to work. But it is very confusing.

Reasoning

Instant type is supposed to be a time-zone agnostic moment in time. The same goes for the TIMESTAMP type in Postgres.
Binding time-zone agnostic type with explicitly time-zone aware TIMESTAMP with time zone type seems to be a mistake.

@getquill/maintainers

@vladimir-lu
Copy link

vladimir-lu commented Aug 7, 2024

Is your JVM running in the CEST tz? JDBC drivers typically pick the current machine timezone, which unfortunately leads to such interactions. In the commit you linked, Instant is converted to a java.sql.Timestamp. There's still an open discussion in pgjdbc about supporting Instant natively, e.g. pgjdbc/pgjdbc#1325 (comment)

Also see that in zio/zio-jdbc#182 you probably need to use TIMESTAMP WITH TIME ZONE if you want the correct behavior currently.

@michalkoza
Copy link
Author

Thank you @vladimir-lu ! But is it really converted to java.sql.Timestamp? It seems more that it is actually converted to OffsetDateTime in this line 8117a0e#diff-5161e16f0f0a107bd28613fe49aa063e2e7390ec87a61c5a8fd540ca191f67beR101

I also reached the conclusion that I need to have TIMESTAMP WITH TIME ZONE (see Workaround), but it is very disappointing and counter intuitive (see Reasoning)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants