Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 6, 2023
Merged

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

merged 1 commit into from
Dec 6, 2023

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Mar 14, 2023 •
edited
Loading

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 vlsi linked an issue Mar 14, 2023 that may be closed by this pull request
LocalDateTime conversion to timestamp without time zone is affected by JVM timezone at DST boundary #2850
Closed
@vlsi vlsi mentioned this pull request Mar 14, 2023
Open
@vlsi vlsi force-pushed the time_types branch 3 times, most recently from 61525f5 to d29cc5d Compare March 14, 2023 10:54
@vlsi vlsi mentioned this pull request Mar 14, 2023
Closed
8 tasks
@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.

@davecramer davecramer mentioned this pull request Mar 22, 2023
Closed
@vlsi vlsi force-pushed the time_types branch 2 times, most recently from ac25bff to 4bce414 Compare December 5, 2023 12:57
vlsi
vlsi commented Dec 5, 2023
View reviewed changes
pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java Outdated
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.

@vlsi vlsi added the wrong-data label Dec 5, 2023
@vlsi vlsi force-pushed the time_types branch 2 times, most recently from fea1e3f to 78636b7 Compare December 6, 2023 06:17
vlsi
vlsi commented Dec 6, 2023
View reviewed changes
pgjdbc/src/test/java/org/postgresql/test/jdbc42/SetObject310Test.java
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
@vlsi
fix: avoid timezone conversions when sending LocalDateTime to the dat…
c1a851c 
…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
@vlsi vlsi mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davecramer davecramer davecramer left review comments

Assignees
No one assigned
Labels
wrong-data
Projects
None yet
Milestone
42.7.1
Development

Successfully merging this pull request may close these issues.

LocalDateTime conversion to timestamp without time zone is affected by JVM timezone at DST boundary LocalDateTime has invalid string serialization
2 participants
@vlsi @davecramer