Skip to content
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

Timestamp to Date Rounding Error #1499

Closed
trevoreckersley opened this issue Jun 12, 2019 · 20 comments

Comments

Projects
None yet
3 participants
@trevoreckersley
Copy link

commented Jun 12, 2019

I'm submitting a ...

  • bug report
  • feature request

Describe the issue
When requesting a date prior to the Unix epoch with a non-midnight time (e.g. via PgResultSet#getDate(String)), if the JVM time is UTC, GMT, or an offset variant (e.g. UTC-7:00), the date will be one day higher than expected.

Driver Version?
42.2.5

Java Version?
N/A (developer box 1.8.0_202)

OS Version?
N/A (developer box MacOS 10.14.5)

PostgreSQL Version?
N/A (production database 10.6)

To Reproduce
Steps to reproduce the behavior:

  1. Set the JVM time zone to UTC, GMT, or an offset variant (e.g. UTC-7:00).
  2. Request a column from the database which is of type timestamp as a date (e.g. PgResultSet#getDate(String)). This date must be prior to the Unix epoch (1970-01-01) and have a time greater than midnight (00:00:00.0). For example: 1948-01-10 12:00:00.0.

Expected behavior
With the example date provided above (1948-01-10 12:00:00.0), I would have expected the resulting date to be 1948-01-10; however, due to a rounding error in TimestampUtils#convertToDate(long, TimeZone), the resulting date is 1948-01-11--a whole day off. Dates on or after 1970-01-01 are not impacted by this issue, nor are dates prior to 1970-01-01 if the time is midnight.

Unit Test Showing the Failure

// JVM time zone needs to be set to UTC, GMT, or an offset variant (e.g. UTC-7:00) prior to execution.
// create an instance of timestampUtils
timestampUtils.convertToDate(Timestamp.valueOf("1948-01-10 12:00:00").getTime(), TimeZone.getDefault());
assertEquals(LocalDate.of(1948, 1, 10), date.toLocalDate());  // this will fail

Location of Error
https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/TimestampUtils.java#L1244

Diagnosis of Code
This method of truncation works for positive millisecond values (1970-01-01 and above) and negative millisecond values where there is no time. Unfortunately, in negative cases where there are time, after dividing by a day in milliseconds, dropping the remainder causes the date to shift closer to zero, resulting in the wrong day. Example:

Step Midnight prior to epoch date After midnight prior to epoch date
Human readable start values 1948-01-10 00:00:00.0 1948-01-10 00:00:00.001
Milliseconds -693532800000 -693532799999
Divide by 86400000 -8027 -8026.999999988425926
After truncation -8027 -8026
Multiply by 86400000 -693532800000 -693446400000
Human readable end values 1948-01-10 00:00:00.0 1948-01-11 00:00:00.0
@vlsi

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Just one question: how did you know that?

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

I'm not quite sure I'm following your question. We were seeing instances of this happening in our production environment. An admin for our system noticed that some dates they were seeing were different from what they were stored as in the database. After a few hours of diagnosis, this was the only point I could determine as causing the issue.

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Unfortunately, the project we're using is an internal company project, so I can't point to our specific source code. What I can say is that we are using Hibernate for ORM mapping, HikariCP for database connection pooling, and the Postgres 42.2.5 driver. When we try getting an object through Hibernate that has a LocalDate value mapped to a timestamp in our Postgres database, we see this issue happen under the previously mentioned conditions.

As mentioned in the initial post, I have a short test which demonstrates the issue in question at a the lowest level I could manage. If needed, I can try writing up a small project to demonstrate the issue.

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Here's a small Spring Boot project that can be executed to mimic the problem I'm seeing: https://github.com/trevoreckersley/date-bug-demo

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Hmmmm small would be

public static main
{
...
3 or 4 lines which show me the problem
}

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Do you mean like this?

  public static void main(String... args) {
    TimeZone.setDefault(TimeZone.getTimeZone(ZoneOffset.UTC));
    Date date = new TimestampUtils(false, TimeZone::getDefault)
        .convertToDate(Timestamp.valueOf("1948-01-10 12:00:00").getTime(), TimeZone.getDefault());
    System.out.println("Expected 1948-01-10, instead was " + date.toLocalDate());
  }
@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

So actually this doesn't work for any date before 1970. I'm not even sure the timezone math is correct either.

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

When I've got a date with a time set to midnight, it seems to work okay for UTC time.

As for the timezone math, at least part of it seems okay. Java includes the default timezone as part of it's calculation when creating a millisecond value from Timestamp#getTime(). Example:

TimeZone.setDefault(TimeZone.getTimeZone("America/Denver"));
long denverMillis = Timestamp.valueOf("1948-01-10 12:00:00").getTime();
TimeZone.setDefault(TimeZone.getTimeZone("UTC"));
long utcMillis = Timestamp.valueOf("1948-01-10 12:00:00").getTime();
assertEquals(denverMillis, utcMillis); // expected: <-693464400000> but was: <-693489600000>

As a result, adding the value of the timezone offset negates the change Java makes (e.g. converts the Denver millisecond value above to be the same as the UTC value); however, adding the offset back in seems counter-intuitive, since there should be no time included in the date value.

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

Then again, maybe new Date(long) expects the milliseconds to offset according to the current system offset. I can't say for sure, as I haven't dug into that yet, but if it is the case, then the handling seems correct.

davecramer added a commit to davecramer/pgjdbc that referenced this issue Jun 14, 2019

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

That should work. Another option I discovered would be to use millis = Math.floorDiv(millis, ONEDAY) * ONEDAY; instead of millis = millis / ONEDAY * ONEDAY. That will force the date toward negative infinity for both positive and negative numbers. Either option works for me.

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@trevoreckersley Well as I said I'm not sure the timezone adjustment actually works with negative numbers.

@vlsi

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

Math.floorDiv seems to be a nice way to go.

@davecramer

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@trevoreckersley

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Thank you for your help. Looking forward to using the fix when it's available.

@vlsi

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

There is still code like

    long secs = toPgSecs(millis / 1000);
    ByteConverter.int4(bytes, 0, (int) (secs / 86400));

I'm not sure if it is ok or not though.

vlsi added a commit that referenced this issue Jun 18, 2019

fix: date rounding errors for dates before 1970
millis % ONEDAY should be reworked to floorMod(millis, ONEDAY)
Regular division "rounds" towards zero, however that is not what we want for dates
Division "towards zero" results to adding a day for dates with "negative" long values.

So we should use floorDiv/floorMod to get date/time components

see #1499
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.