feat: use direct wire format -> LocalDate conversion without resorting to java.util.Date, java.util.Calendar, and default timezones

[uschindler]

This fixes #2221:

I added a new method to TimestampUtils that directly converts the epoch days to a LocalDate. Interestingly this allows to not use the cryptic proleptic seconds hack. You can pass the plain epoch days from postgres, just added a few number of days -- actually the strange constant at beginning to toJavaSecs() method:

    // postgres epoc to java epoc
    secs += 946684800L;

divided by 86400 (to get it in days). The following code in toJavaSecs is not needed and returns broken results, because LocalDate also does its own Julian/Gregorian transformation.

I also beefed the test for LocalDate to cycle through all timezones (should not be needed, as timezone is not involved). I also added dates like 0001-01-01 to tests for LocalDate and LocalDateTime.

[bokken]

I wonder if we should address /all/ of the date and time data types (not just LocalDate), as the discussion (and proposed change) seems pretty similar to where discussion on #1391 is going.

the strange constant at beginning to toJavaSecs()

This is the difference between the java/unix epoch of Jan 1, 1970 and the postgres epoch of Jan 1, 2000.

Another way to do this (which might be less cryptic) is to define a PG_EPOCH constant and do plus/minus operations from there.

private static final LocalDateTime PG_EPOCH = LocalDateTime.of(2000, 1, 1, 0, 0);

[vlsi]

I wonder if we should address /all/ of the date and time data types (not just LocalDate),

I wonder if going with java.time. causes performance degradation or not.

[uschindler]

I wonder if we should address /all/ of the date and time data types (not just LocalDate),

I wonder if going with java.time. causes performance degradation or not.

At least not for this fix. The previous code with converting between all those old date/time conversions including reflection with setAccessible() to get current timezone are not allowed in Java 9+. So the code above should really be faster! And this old-style calendar magic for guessing zones is also not the fastest. The current code does fixed here did previously:

• convert the Date to a new java.sql.Date with reflective access to get timezone, coping into calendar and modifying to local timezone
• convert the date back with the same timezone when appliyng toLocalDate

All not good and errors and bugs waiting everywhere!

[bokken]

I wonder if we should address /all/ of the date and time data types (not just LocalDate),

I wonder if going with java.time. causes performance degradation or not.

Based on a cursory glance at the implementation, I would suspect it is actually faster.

[vlsi]

At least not for this fix

That is no question. If user asks for java.time., it is just faster to create one.

However, what @bokken mentioned was "should we always build the value as java.time., and then convert to java.util if user calls regular ResultSet#getTimestamp API. That change might have performance implications (both positive and negative).

[uschindler]

I have not tested all stuff yet, so this issue is still WIP. Thanks for the comments, will update later.

I forgot to mark as Draft.

[uschindler]

I think we can't use java.time Apis for the legacy stuff, because we need to support custom calendars passed in to get date and others. Unless we can integrate that, we have to stay with the legacy code.

But we should make sure that if java.time is requested, we should directly deserialize to that. For speed and to keep time zones put of the game.

[uschindler]

I changed the integer overflow and added infinity handling. The infinity tests also pass.

@bokken: I also added the "strange constants" precalculated using java.time.Duration as static final longs.

[uschindler]

I looked into the other remianing legacy conversion (TIMETZ -> OffsetDateTime). This still uses legacy java.sql.Time at two places:

• pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

  Lines 687 to 693 in 54e5598

  	} else if (oid == Oid.TIMETZ) {
  	// JDBC spec says timetz must be supported
  	Time time = getTime(i);
  	if (time == null) {
  	return null;
  	}
  	return getTimestampUtils().toOffsetDateTime(time);

• pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

  Lines 705 to 711 in 54e5598

  	if (oid == Oid.TIMETZ) {
  	// JDBC spec says timetz must be supported
  	// If server sends us a TIMETZ, we ensure java counterpart has date of 1970-01-01
  	Calendar cal = getDefaultCalendar();
  	Time time = getTimestampUtils().toTime(cal, string);
  	return getTimestampUtils().toOffsetDateTime(time);
  	}

Both are not tested! I set a breakpoint in both if clauses and it was never hit!

The 2nd one looks obsolete, because the standard string Timestamp parser can also parse a TIME or TIMETZ only (the date will then be returned as 1970-01-01). So Lines 705 to 711 can be removed, the code below should be able to read it. But before doing that a test should be added. Especially for the special case of "24:00:00".

The first one requires a binary parser from TIMETZ to java.time.OffsetTime. This is easy to implement and I would like to also do this. But then I noticed that we have no support for doing getObject(..., OffsetTime.class) at all. What do others think, would this be a good addition?

I know, use of TIMETZ is discouraged by Postgres, but for full JDBC support of modern time APIs, OffsetTime should be supported as it also offsers a way to get the time including its timezone (as stored in database).

I could do those changes in this PR, but as alternative I would start working on it as a new PR, if this one is merged. What do others think? @bokken @vlsi

Last sentence for @bokken: At a later stage we may not fully switch to java.time totally (see my comment above), but at least the parsing of string timestamps coming from database should possibly be implemented using java.time and the value-based helper classes ParsedTimestamp and so on should be removed in favor of OffsetDateTime. This may be another cleanup.

[davecramer]

I looked into the other remianing legacy conversion (TIMETZ -> OffsetDateTime). This still uses legacy java.sql.Time at two places:

• pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

  Lines 687 to 693 in 54e5598

  	} else if (oid == Oid.TIMETZ) {
  	// JDBC spec says timetz must be supported
  	Time time = getTime(i);
  	if (time == null) {
  	return null;
  	}
  	return getTimestampUtils().toOffsetDateTime(time);

• pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

  Lines 705 to 711 in 54e5598

  	if (oid == Oid.TIMETZ) {
  	// JDBC spec says timetz must be supported
  	// If server sends us a TIMETZ, we ensure java counterpart has date of 1970-01-01
  	Calendar cal = getDefaultCalendar();
  	Time time = getTimestampUtils().toTime(cal, string);
  	return getTimestampUtils().toOffsetDateTime(time);
  	}

Both are not tested! I set a breakpoint in both if clauses and it was never hit!

The 2nd one looks obsolete, because the standard string Timestamp parser can also parse a TIME or TIMETZ only (the date will then be returned as 1970-01-01). So Lines 705 to 711 can be removed, the code below should be able to read it. But before doing that a test should be added. Especially for the special case of "24:00:00".

The first one requires a binary parser from TIMETZ to java.time.OffsetTime. This is easy to implement and I would like to also do this. But then I noticed that we have no support for doing getObject(..., OffsetTime.class) at all. What do others think, would this be a good addition?

I know, use of TIMETZ is discouraged by Postgres, but for full JDBC support of modern time APIs, OffsetTime should be supported as it also offsers a way to get the time including its timezone (as stored in database).

I could do those changes in this PR, but as alternative I would start working on it as a new PR, if this one is merged. What do others think? @bokken @vlsi

Last sentence for @bokken: At a later stage we may not fully switch to java.time totally (see my comment above), but at least the parsing of string timestamps coming from database should possibly be implemented using java.time and the value-based helper classes ParsedTimestamp and so on should be removed in favor of OffsetDateTime. This may be another cleanup.

I would prefer another PR.

• moves this one along faster
• separate discussion, etc ....

[bokken]

I seem to recall that the jdbc spec listed specific java.time classes to support (and it is not all of them).

I do not see the ability to leverage user provided timezones to be problematic or necessarily even negative.

timestamp -> LocalDateTime, which can then default TimeZone or user supplied calendar timezone applied[1] to get to a ZonedDateTime, which can be converted to an Instant[2], which can trivially be used to create a java.sql.TimeStamp.

timestamptz is the kind of tricky one. This maps directly to an Instant and/or java.sql.TimeStamp.
Presumably it should go to a ZonedDateTime with zone of UTC?

Getting from a ZonedDateTime to the other data types is pretty straightforward.

The time types take a fair amount of massaging, but mapping to the java.time data types is far more straightforward than the java.sql types.

I think my proposal is that we pick a preferred internal "native" representation (a java.time type) for each of the pg types and then map how to do the conversions.

[1] - https://docs.oracle.com/javase/8/docs/api/java/time/LocalDateTime.html#atZone-java.time.ZoneId-
[2] - https://docs.oracle.com/javase/8/docs/api/java/time/chrono/ChronoZonedDateTime.html#toInstant--

[uschindler]

I think my proposal is that we pick a preferred internal "native" representation (a java.time type) for each of the pg types and then map how to do the conversions.

OK, this is a good approach. At least it makes it easy to correctly impement the mapping from wire byte arrays. It also does not need the julian/gregorian conversions, because the java.time API does it correctly (as shown by the LocalDateTime and LocalDate conversions).

I think we still need some massaging for the legacy types, where we may need to apply local timezone for the "Local", non-timezoned types. This is how they were originally defined and what also mekes them so horrible to use. But the timezone for this conversion can be picked from the passed Calendar instance to getTimestamp() & Co.

[vlsi]

Looks good to me modulo small style issues. Thanks for the PR.

[vlsi]

Will commit the changes shortly unless someone does that earlier

[uschindler]

Thanks for review. I applied your suggestions.

[uschindler]

Thanks for merging. I will proceed with a separate PR about TIME and TIMETZ which has same problem like described here. This will also add support for OffsetTime then which is completely missing.

[vlsi]

Just in case, I see you follow the code style that was there in the classes before (I really appreciate that!), however, please, don't hesitate to use Java 8+ features like try-with-resources in the newly created code.

Historically the driver was Java 6+, so the code idioms are still from the old days. I think the newly added code could just use 8+, especially for the cases like try-with-resources, lambdas in assertions.

[uschindler]

however, please, don't hesitate to use Java 8+ features like try-with-resources in the newly created code.

The background here was: I copypasted the LocalDateTime-iterate-over-timezones test and modified it - I agree, I could have changed it! :-)