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

fix: avoid timezone conversions when sending LocalDateTime to the database #2852

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Mar 14, 2023

Previously, LocalDateTime was converted to OffsetDateTime using the default timezone, which made certain values unrepresentable. For instance, 2023-03-12T02:00 in America/New_York is hard to represent as it undergoes DST change.

It does not change resultSet.getObject(int | string), and resultSet.getString(int | string) and those methods would still have issues when accessing timestamps like 2023-03-12T02:00 when the client time zone is America/New_York

Fixes #1390
Fixes #2850
Closes #1391

@vlsi
Copy link
Member Author

vlsi commented Mar 15, 2023

This change brings back inconsistency of getString() for timestamp values in binary vs text format.
The DB sends timestamp in text format as 2000-03-26 02:00:00, and when it sends the timestamp in binary we roundtrip with Timestamp, and result in 2000-03-26 03:00:00.

So getString should be fixed as well.

Comment on lines 2339 to 2340
case Oid.TIMESTAMPTZ:
return ts.toString(ts.toOffsetDateTimeBin(value));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This yields +00:00 OffsetDateTime since PostgreSQL does not store and return the offset.

It causes the failure like

    org.junit.ComparisonFailure: tstz -> getString, binary expected:<2005-01-01 1[5:00:00+03]> but was:<2005-01-01 1[2:00:00+00]>

Any thoughts on whether we should fix it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only solution to this is to not allow sending timestamptz in binary. Or we apply the client timezone to the received value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with "apply client timezone to the received value" (see withOffsetSameInstant). It keeps compatibility regarding .getString() which might be a nice thing to have.

Comment on lines -235 to -237
String expected = localDateTime.atZone(zone)
.format(DateTimeFormatter.ISO_LOCAL_DATE_TIME)
.replace('T', ' ');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the actual test for the feature. In other words, previously, the test used atZone formatting of the local date time which was invalid, and it augmented the input date. That made the test to pass without issues. Now we avoid atZone formatting, and we use the input value as is, and we test the output is the same.

@vlsi vlsi added this to the 42.7.1 milestone Dec 6, 2023
…abase

Previously, LocalDateTime was converted to OffsetDateTime using the default
timezone, which made certain values unrepresentable.
For instance, 2023-03-12T02:00 in America/New_York is hard to represent
as it undergoes DST change.

It does not change resultSet.getObject(int | string), and resultSet.getString(int | string)
and those methods would still have issues when accessing timestamps
like 2023-03-12T02:00 when the client time zone is America/New_York

Co-authored-by: Kevin Wooten <kevin@wooten.com>

Fixes pgjdbc#1390
Fixes pgjdbc#2850
Closes pgjdbc#1391
@vlsi vlsi merged commit c1a851c into pgjdbc:master Dec 6, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants