More test and fix for issues discovered by #2476 #2488

uschindler · 2022-04-13T13:20:39Z

This is a PR related to issues found in #2476, maybe caused by #2464 or #2467

uschindler · 2022-04-13T13:29:02Z

This is basically a copy of the other test for LocalDateTime and OffsetDateTime.

…zone! I also added timestamp without TZ

pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java

uschindler · 2022-04-13T13:52:01Z

In my last commit I copied over the modified test from #2476 to check if it fails.

On my local timezone (UTC+2) it passes.

uschindler · 2022-04-13T14:09:50Z

I was able to break the test in @davecramer 's timezone (I set my computer to US Eastern time). The issue happens when the time zone is converted multiple times.

uschindler · 2022-04-13T14:19:29Z

I reverted in my local branch both changes that I did. The test still fails for me in US Eastern Timezone. So it looks like it is unrelated to #2464 and #2467.

Digging more...

uschindler · 2022-04-13T15:01:03Z

Ok now I figured out:
(1) With #2467 applied in US Eastern Timezone the test has two failures:

> Task :postgresql:test
FAILURE   1,0sec, org.postgresql.test.jdbc2.DateTest > testGetDate
    java.lang.AssertionError: expected:<0101-01-01> but was:<0102-12-31>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:276)
        at org.postgresql.test.jdbc2.DateTest.testGetDate(DateTest.java:83)

FAILURE   0,5sec, org.postgresql.test.jdbc2.DateTest > testSetDate
    java.lang.AssertionError: expected:<0101-01-01> but was:<0102-12-31>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:276)
        at org.postgresql.test.jdbc2.DateTest.testSetDate(DateTest.java:169)

If I revert #2467, there is only one test failure:

> Task :postgresql:test
FAILURE   0,6sec, org.postgresql.test.jdbc2.DateTest > testGetDate
    java.lang.AssertionError: expected:<0101-01-01> but was:<0102-12-31>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.junit.Assert.assertEquals(Assert.java:146)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:276)
        at org.postgresql.test.jdbc2.DateTest.testGetDate(DateTest.java:83)

The same happens when I revert all changes and go back to Postgresql tag of last Maven release (42.3.2). Only one test fails (the getDate one). It looks like theres something wrong with setting dates already.

Now the fix for the whole problem looks like is caused by one change in #2467 and the fix is already mentioned there:
https://github.com/pgjdbc/pgjdbc/pull/2467/files#r833415824

It looks like theres a difference between SimpleTimeZone class in JDK and using the ZoneOffset (which uses a real CDLR Timezone). After applying the fix there the testSetDate method passes.

It is still unclear how to fix testGetDate() because it also fails in US Eastern Timezone on the release Tag.

uschindler · 2022-04-13T15:48:12Z

Further investigations (printing the String values of all transmitted dates) it looks like this is caused by the combination of timezones and BC dates.

If I set my local timezone (server and also client) to US Eastern Timezone on Microsoft Windows 10, Postgres 13, all BC Dates by postgres are returned as a string value like this one: 0101-01-01 00:03:58-04:56:02 BC

Look at specifically the time offset? It is totally off, I would have expected it to be -5

So this looks like a server problem. But also Java seems to have a difference between SimpleTimeZone and Timezone.get(ZoneOffset) which returns a different zone.

M yproposal would be to revert the change getting the cleandar in #2467 and use this code in TimestampUtils as suggested in that issue:

  private Calendar getCalendar(ZoneOffset offset) {
    if (calCache != null && Objects.equals(offset, calCacheZone)) {
      return calCache;
    }

    final String tzid = (offset.getTotalSeconds() == 0) ? "UTC" : "GMT".concat(offset.getId());
    final TimeZone syntheticTZ = new SimpleTimeZone(offset.getTotalSeconds() * 1000, tzid);
    calCache = new GregorianCalendar(syntheticTZ);
    calCacheZone = offset;
    return calCache;
  }

This restores the same behaviour as last Postgresql JDBC driver (as seen on Maven Central). That the test still fails here is then a different thing.

What do you think?

uschindler · 2022-04-13T15:50:26Z

What is completely broken on Postgres 13 seems to be when you set a timestamptz column with a plain BC date as string. It then returns some very strange zone offset (at least on my windows computer).

…ous pgjdbc version.

uschindler · 2022-04-13T16:06:26Z

I pushed a commit to work around the strange server behaviour with totally whacky timezone offsets returned by Postgres 13 and also 14 (just different) for BC dates. The reason for the whole issue looks like TimeZone.getTimeZone(offset) for offsets with seconds returns a strange implementation.

This restores the old behaviour.

We should still ask the server people why the timezones BC are returned like that. The other test in this issue also shows that LocalDate works correct with BC dates.

uschindler · 2022-04-13T16:51:17Z

Hi,
it looks like the job "CI / Test - JDK 11 on self-hosted (pull_request)" fails with the same issue like I see locally.
without my last commit it fails with 2 tests.

So there's still something fishy. Maybe we should comment out the "timestamptz" column in the testGetDate test.

My fix at least restores original behaviour.

uschindler · 2022-04-13T17:00:16Z

Hi, it looks like the job "CI / Test - JDK 11 on self-hosted (pull_request)" fails with the same issue like I see locally. without my last commit it fails with 2 tests.

So there's still something fishy. Maybe we should comment out the "timestamptz" column in the testGetDate test.

My fix at least restores original behaviour.

This job runs on postgres 13, so it looks like same setup like I have. There seems to be a problem. But previous runs have shown that without my fix 2 tests fail.

Maybe add assumpion to test to only run testGetDate in Postgres 14+? @davecramer

uschindler · 2022-04-13T17:01:59Z

I also have the feeling the test fails depending on time of day!

…mezones like Aperica/New_York it fails while America/Toronto it works. This explains the problems.

uschindler · 2022-04-13T18:35:02Z

OK, I improved the test to make a parameterized on cycling over timezones. Without the SimpleTimeZone fix that was broken since #2467, many tests failed, so this has to do with SimpleTimeZone work better than TimeZone.get(ZoneOffset). For this I will try to file a bug with OpenJDK after nailing this down to a simple test case with GregorianCalendar and timezones (not today!).

With the bugxfix applied and restoring the behaviour of pgjdbc 42.3.2 now only 3 timezones fail:

> Task :postgresql:test
FAILURE   0,5sec, org.postgresql.test.jdbc2.DateTest > testGetDate[zoneId = Africa/Casablanca]
    java.lang.AssertionError: testtstz expected:<3456-01-01> but was:<3455-12-31>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:302)
        at org.postgresql.test.jdbc2.DateTest.testGetDate(DateTest.java:114)

FAILURE   0,4sec, org.postgresql.test.jdbc2.DateTest > testGetDate[zoneId = America/New_York]
    java.lang.AssertionError: testtstz expected:<0101-01-01> but was:<0102-12-31>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:307)
        at org.postgresql.test.jdbc2.DateTest.testGetDate(DateTest.java:114)

FAILURE   0,3sec, org.postgresql.test.jdbc2.DateTest > testGetDate[zoneId = Pacific/Niue]
    java.lang.AssertionError: testtstz expected:<1950-02-07> but was:<1950-02-06>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.postgresql.test.jdbc2.DateTest.dateTest(DateTest.java:222)
        at org.postgresql.test.jdbc2.DateTest.testGetDate(DateTest.java:114)

As you see "America/Toronto" does not fail (this is why @davecramer does not see an issue on testGetDate() while I saw one with "America/New_York".

What is funny in Niobe the test also fails in 1950 :-)

So to conclude: There is something wrong with the server side (Postgres 13 and possibly also 14, @davecramer can you test this if you get same output?). The issue comes from the following fact: If you insert a string-formatted date to a table with timestamptz with client time zone in the list of above, the server converts the date to some timestamp with a strange time of day and a broken offset (for historic dates the offset was based on geo-coordinates). When parsing this returned string on client and not using SimpleTimeZone it fails because of the offset with minutes as seconds. Because of the server bugs, the time part of the timestamp also has some non-00 hours/min/seconds and this will fail the test for some timezones due to shifting times around and then sometimes crossing begin of day.

To fix this, I have the feeling we should fix Postgres client side string date parsing to not parse the whole timestamp including time and timezone when you only want to get a date out of it. It should stop parsing after the day of month. Or we should set all fields to zero after parsing and before passing it to Calendar API.

…ue/false

uschindler · 2022-04-13T22:05:14Z

Hi,
I rewrote the test and made everything a parameter:

type
zoneId
binary=force, regular

I also limited the GMT zones (which are unproblematic) to not have tooooooo many test executions.

As noted before all is symmetric only the problems around some "local timezones" in combination with passing the date as string to server and later reading it. The problem here is that few timezones have bugs (wich also depends on server version). For this I added an assume to the test. We may change the assume to allow all local timezones except the 3 mentioned above, but I don't know which other timezone bugs are there in other server versions than 13 and 14..

@davecramer what do you think. We can try to fix the issues or report them to the Postgres people where the symmetry between input/output types does not seem to be give.

…s did not exist at that time), so removing them. Instead use a more common timezone

uschindler · 2022-04-13T23:50:54Z

I changed away from the test time zones Azores and Niue, because those are special: we have extra tests for them. Those break with some dates we have:

Azores change DST at start of day; because of this it is not symmetric
Niue seems to have issues only in Postgres 14, not 13

We already have quite good tests for time zone conversions, but this one uses paraeters so it tests many more combinations. But this testing has more problems, because daylight saving is unpredictable.

This is my personal problem with the legacy datetime types: they allow to do shit nobody would or should ever do! And this brings those non-symmetric cases.

uschindler · 2022-04-13T23:53:40Z

I am done with that now. Legacy time types are a never-ending story. Plesae don't touch them! Sorry for doing this in my cleanup #2467. 😱😱😱

uschindler · 2022-04-13T23:56:19Z

FYI, with java.time we no longer allow to do bad conversions like converting between tz and non-tz types.

What's also bad: the server sometimes adds a time zone, too.

uschindler · 2022-04-14T00:17:20Z

It finally passed all tests! Time to go to bed.

TODO: Use this also in TimestampUtils

uschindler · 2022-04-14T08:38:48Z

As mentioned in other issue, I rewrote the (Offset|Local)Date(Time)? tests to use ISOChronology directly instead of hacking the ERA. In a future PR we should also do this in TimestampUtils, but that's a nother point for discussion.

davecramer · 2022-04-14T13:15:56Z

pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java

- calCache = new GregorianCalendar(TimeZone.getTimeZone(offset));
+ // normally we would use:
+ // calCache = new GregorianCalendar(TimeZone.getTimeZone(offset));
+ // But this seems to cause issues for some crazy offsets as returned by server for BC dates!


I don't think this is a postgres specific thing java does the same

jshell> java.time.ZoneId.of("America/Toronto"). ...> getRules(). ...> nextTransition(java.time.Instant.parse("0101-01-01T00:00:00Z")) $1 ==> Transition[Gap at 1895-01-01T00:00-05:17:32 to -05:00]

The problem is more that the conversion from ZoneOffset to Timezone using Timezone.get(ZoneOffset) is somehow broken. That's why I want to open a bug report at JDK. Not sure how this comes together. I will do some detailed analysis. At least this fixes the problem, although it is not really what the new Java API suggests. This code just restores the old behaviour. The comment was added that there's something fishiy and not somebody else tries to "simplify" it.

It is still strange why different Postgres versions fail differently.

05:17:32 is the geographic timezone based on latitude/longitude. Interestingly not all timezones have this rule in 1895.

The problem is more that the conversion from ZoneOffset to Timezone using Timezone.get(ZoneOffset) is somehow broken.

Just guessing from my observations: To me it looks like Timezone.get() with those offsets down to second return the default Timezone! It should better throw exception when it cannot parse the ZoneOffset.

I opened a JDK bug report about converting a ZoneOffset to TimeZone, which is buggy for offsets with seconds: https://bugs.openjdk.java.net/browse/JDK-8285844

We should maybe add a link to this issue here! @davecramer can you add the above issue to the comment above? Or do I need to open a bug report.

pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java

uschindler · 2022-04-15T15:37:29Z

Thanks @davecramer

davecramer · 2022-04-15T15:40:19Z

The thanks goes to you for all the work you did on this. Really appreciate it!

uschindler · 2022-04-28T17:22:36Z

I opened a JDK bug report about converting a ZoneOffset to TimeZone, which is buggy for offsets with seconds: https://bugs.openjdk.java.net/browse/JDK-8285844

This adds some tests for issues discovered by pgjdbc#2476

3f72bc0

uschindler mentioned this pull request Apr 13, 2022

fix: allow binary transfer or date parameters #2476

Open

9 tasks

Port over DateTest from the other PR. It passes for me on German time…

c0291c4

…zone! I also added timestamp without TZ

davecramer reviewed Apr 13, 2022

View reviewed changes

pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java Outdated Show resolved Hide resolved

Fix the offset handling in pgjdbc#2467 to restore how it was in previ…

c2951cf

…ous pgjdbc version.

uschindler mentioned this pull request Apr 13, 2022

feat: add support for ResultSet#getObject(OffsetTime.class) and PreparedStatement#setObject(OffsetTime.class) #2467

Merged

5 tasks

Improve test to cycle over all timezones. This shows that for some ti…

6f4ac5f

…mezones like Aperica/New_York it fails while America/Toronto it works. This explains the problems.

Rewrite test and make everything a parameter: type, zoneId, binary=tr…

7f5f96b

…ue/false

The Azores and Niue timezones have some problems in 1950 (e.g., Azore…

e2e1774

…s did not exist at that time), so removing them. Instead use a more common timezone

uschindler requested a review from davecramer April 14, 2022 07:10

uschindler changed the title ~~This adds some tests for issues discovered by #2476~~ More test and fix for issues discovered by #2476 Apr 14, 2022

Use ISOChronology in the test to make the BC dates more readable

5f301fd

TODO: Use this also in TimestampUtils

davecramer reviewed Apr 14, 2022

View reviewed changes

uschindler commented Apr 15, 2022

View reviewed changes

pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java Outdated Show resolved Hide resolved

uschindler commented Apr 15, 2022

View reviewed changes

pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java Show resolved Hide resolved

davecramer force-pushed the add_more_tests_for_BC_dates branch from e031510 to 5f301fd Compare April 15, 2022 12:14

davecramer merged commit 97d9106 into pgjdbc:master Apr 15, 2022

uschindler deleted the add_more_tests_for_BC_dates branch April 15, 2022 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More test and fix for issues discovered by #2476 #2488

More test and fix for issues discovered by #2476 #2488

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022 •

edited

Loading

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022 •

edited

Loading

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 14, 2022

uschindler commented Apr 14, 2022

davecramer Apr 14, 2022 •

edited

Loading

uschindler Apr 14, 2022 •

edited

Loading

uschindler Apr 14, 2022

uschindler Apr 14, 2022

uschindler Apr 28, 2022

uschindler commented Apr 15, 2022

davecramer commented Apr 15, 2022

uschindler commented Apr 28, 2022

More test and fix for issues discovered by #2476 #2488

More test and fix for issues discovered by #2476 #2488

Conversation

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022 • edited Loading

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022 • edited Loading

uschindler commented Apr 13, 2022

uschindler commented Apr 13, 2022

uschindler commented Apr 14, 2022

uschindler commented Apr 14, 2022

davecramer Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

uschindler Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

uschindler Apr 14, 2022

Choose a reason for hiding this comment

uschindler Apr 14, 2022

Choose a reason for hiding this comment

uschindler Apr 28, 2022

Choose a reason for hiding this comment

uschindler commented Apr 15, 2022

davecramer commented Apr 15, 2022

uschindler commented Apr 28, 2022

uschindler commented Apr 13, 2022 •

edited

Loading

uschindler commented Apr 13, 2022 •

edited

Loading

davecramer Apr 14, 2022 •

edited

Loading

uschindler Apr 14, 2022 •

edited

Loading