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
setTimestamp use timestamptz db type #3220
base: master
Are you sure you want to change the base?
Conversation
old pull request pgjdbc#2835
at the moment I do not know the best way to implement that. Need help.
Why do another PR ? |
I did some mistake on the original github fork and it closes the pull request. So I created a new one, sorry. |
SQL_TIMESTAMPTZ_ALWAYS( | ||
"sqlTimestamptzAlways", | ||
"false", | ||
"If true it uses time stamp with time zone type for java Timestamp. The default is (@code false)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/time stamp/timestamp
@@ -0,0 +1,245 @@ | |||
/* | |||
* Copyright (c) 2004, PostgreSQL Global Development Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2004/2024
} else if (in instanceof Instant) { | ||
tmpts = Timestamp.from( (Instant) in ); | ||
} else if (in instanceof ZonedDateTime) { | ||
tmpts = Timestamp.valueOf( ((ZonedDateTime) in).toLocalDateTime()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ZonedDateTime.toLocalDateTime()
would just forget the timezone of the zdt?
Also, why would it support ZonedDateTime
, but not OffsetDateTime
?
I might be wrong with my assumptions here of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your assumption is correct, so I changed it to use OffsetDateTime type so that time zone information is not lost but stored into the timestamptz.
Anyway, if you use OffsetDateTime to fetch data back form the db, the time zone info is lost as illustrated by the test case TimestamptzTest.test_ZonedDateTime_StoreAndSelect().
How is the correct way to get back the information of the timezone (without using string) ?
NOTE: in our application we always store dates in UTC time zone in the db to avoid any problem. If we need to store information about the time zone to use for some business logic that is stored into a separate fields independent to any stored timestamptz.
fail("no result"); | ||
} | ||
} catch (SQLException e) { | ||
fail(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid fail(e.getMessage());
pattern as it adds no extra value, and it hides the stacktrace. In cases like this just let the exception propagate, and do not catch it.
@arons , can you please clarify which issue does this change solve? I am afraid, it is hard to review the change without understanding the problem it solves. |
The problem it solves is to correctly pass the proper OID type for timestamps to the database, instead of just as text, which is the current behavior. This is useful in many different cases as it allows SQL and PL/SQL to use the parameter properly without requiring an explicit cast. |
It would be great. I would suggest we solve problem a time rather than combine everything possible within a single discussion.
There are cases like |
How is this different than #2715 ? |
testTypeOnDbSite_functionOverloading and testTypeOnDbSite_select are less trivial once already. In the current implementation: It should fix also the issue #1325 (also in the test case, no error any more in invoking the setObject method ) |
If I understand the patch correctly, that is more about the returned meta data type and not about parameter set. |
in which case these PR's should be combined. |
Currently it defines a function with I would suggest adding Frankly speaking, I think |
I agree with that: https://wiki.postgresql.org/wiki/Don%27t_Do_This#Don.27t_use_timestamp_.28without_time_zone.29 So if you prefer to use timestamp over timestamptz I think we can close the discussion and close this pull request cause my motiviation is only about timestamptz. |
setTimestamp use timestamptz db type
old pull request #2835
this pull request should fix also the issue #1325 ( test cases added into class org.postgresql.test.jdbc2.TimestamptzTest )
New Feature Submissions:
./gradlew styleCheck
pass ?Changes to Existing Features:
it breaks currently behaviour only in case user set sqlTimestamptzAlways
explained already in pull request setTimestamp use timestamptz db type #2835