-
Notifications
You must be signed in to change notification settings - Fork 839
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: binary processing of Date/Time/Timestamps #387
Conversation
e1cd4af
to
1110c84
Compare
ef8035d
to
e15fb91
Compare
fcd3ec7
to
1fe0255
Compare
interestingly this travis-ci is having issues with this |
It does not. I'm a bit lazy and I do not always run full test suite. I expect 1fe0255466154150c01637a901acfc72ca3f6689 to pass in Travis. |
I did alter previous behavior a bit: In two words: when backend responds with |
No, that's not the issue. It is taking a very long time to run. Much longer Dave Cramer On 8 October 2015 at 08:22, Vladimir Sitnikov notifications@github.com
|
Is this in one of the tests ? Dave Cramer On 7 October 2015 at 17:45, Vladimir Sitnikov notifications@github.com
|
The test was already there, however I altered "expected" value. |
OK, so to answer what I think your question is. If you use getString that implies no Calendar. Without a calendar we have Dave Cramer On 8 October 2015 at 08:36, Vladimir Sitnikov notifications@github.com
|
I think you answer to some other question. My current question is: what are the missing tests. |
Comments about why we are adding the offset in before zeroing out the time Dave Cramer On 8 October 2015 at 08:45, Vladimir Sitnikov notifications@github.com
|
06827de
to
488e5df
Compare
I've updated implementation, added comments, added more tests. Please review. |
57ad065
to
22f85e3
Compare
This makes sure TimezoneTest works in both text and binary mode. This is a follow-up on pgjdbc#376
fix: binary processing of Date/Time/Timestamps
This is a follow-up on #376.
I force TimezoneTest to test both text and binary encoding and it makes
It looks like this PR fixes all known problems with Timestamp/Date/Time parsing.
I wish someone could add more tests.
Here are the measurements (jdk 1.8u60, OS X 10.11, Intel(R) Core(TM) i7-4960HQ CPU @ 2.60GHz, time zone == Europe/Moscow, local postgresql 9.4).
The test is to perform and fully fetch a query like
select localtimestamp c0, localtimestamp c1, ... from generate_series(1, ?) as t(x)
: https://github.com/pgjdbc/pgjdbc/pull/387/files#diff-962be0de5c547f8cb43ea8efd768dccaR88Notes:
getTimestamp
when selectinglocaltimestamp
if compared with 1203. I believe the degradation is due to more complex logic inTimestampUtil.guessTimestamp
.I believe it is not a showstopper as I see no alternative solution there: https://github.com/pgjdbc/pgjdbc/pull/387/files#diff-d3ce26f4020b4504cf0a8ef2e019b316R699
GMT+-XX:...
), the performance is better: 130us vs 350us for 100 rows x 5 columns test. No degradation here if compared with 1203getBigDecimal
for1234567890123456789
TimeZone.getDefault()
does not buy much, thus I switched toTimeZone.getDefault()
when needed. So pgjdbc supports "default timezone" changes.With current fix:
With caching of
TimeZone.getDefault()
(like in previous pgjdbc versions):1203 (2014-09-18, a0bafcd):
1201 (2015-02-28, a4f8c9e):
pgjdbc-ng 0.6-SNAPSHOT (2015-10-09, impossibl/pgjdbc-ng@1f8cdcb)
pgjdbc-ng 0.6, "do not close preparedstatement test"
pgjdbc-ng 0.5 (2015-04-30, impossibl/pgjdbc-ng@9cefd79)