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

Data corruption caused by prepared statement cache #373

Closed
dfex55 opened this Issue Sep 28, 2015 · 33 comments

Comments

Projects
None yet
4 participants
@dfex55

dfex55 commented Sep 28, 2015

Hello,

fetching "timestamp without time zone" columns using ResultSet.getDate(…) leads to data corruption when prepared statement cache is used and active.

The following output is generated by a loop using the same PreparedStatement and printing a timestamp column as String, Timestamp and Date (see code below).
"prepareThreshold" has been set to 2.

as String: 2015-09-03 12:00:00
as Timestamp: 2015-09-03 12:00:00.0
as Date: 2015-09-03

---
as String: 2015-09-03 12:00:00
as Timestamp: 2015-09-03 12:00:00.0
as Date: 2015-09-03

---
as String: 2015-09-03 12:00:00.0
as Timestamp: 2015-09-03 12:00:00.0
as Date: 2015-09-02

---

The first two iterations are OK.
The third iteration show a little difference in the String representation:

 2015-09-03 12:00:00 vs. 2015-09-03 12:00:00.0

but more critical is the invalid value of the Date as returned by ResultSet.getDate(…)

2015-09-03 vs. 2015-09-02

Note: getTimestamp has no corruption.

The test is executed with:

  • postgresql-9.4-1203.jdbc41.jar
  • postgresql 9.3.9
    String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
    Properties props = new Properties();
    props.setProperty("user", DB_USER);
    props.setProperty("password", DB_PWD);
    props.setProperty("prepareThreshold", "2");
    Connection conn = DriverManager.getConnection(url, props);

    Statement stmt = conn.createStatement();

    final String tableTestName = "_jdbctest1";

    //@formatter:off
    stmt.execute("CREATE TABLE " + tableTestName
                + "("
                + "  datecol timestamp without time zone,"
                + "  rowno integer"
                + ")");
    //@formatter:on

    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-09-03 12:00:00', 1)");

    PreparedStatement pstmt =
        conn.prepareStatement("SELECT datecol FROM " + tableTestName + " WHERE (rowno = ?)");

    for (int i = 0; i <= 2; i++) {

      pstmt.setInt(1, 1);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        System.out.println("as String:\t" + rs.getString("datecol"));
        System.out.println("as Timestamp:\t" + rs.getTimestamp("datecol"));
        System.out.println("as Date:\t" + rs.getDate("datecol"));
        System.out.println("---");
      }

    }

    stmt.execute("drop table " + tableTestName);
@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 28, 2015

Member

Hi, Thanks for the report. I have a pull request PR #376 and or you can try the snapshots here https://oss.sonatype.org/content/repositories/snapshots/org/postgresql/postgresql/

In general the date/time/timestamp stuff needs a rewrite... anyone ?

Member

davecramer commented Sep 28, 2015

Hi, Thanks for the report. I have a pull request PR #376 and or you can try the snapshots here https://oss.sonatype.org/content/repositories/snapshots/org/postgresql/postgresql/

In general the date/time/timestamp stuff needs a rewrite... anyone ?

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 28, 2015

Member

TL;DR: I think #376 is broken and proper Timestamp -> Date conversion should use Calendar.

Frankly speaking, Timestamp/Date support from JDBC point of view is broken (as was discussed in #340).

I am not sure which sense makes using of without time zone and date at the same time.
Well, it could make sense if use something like https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html, however I am not sure if we can please all the users since some of the cases want Date behave like LocalDate and some of them might want Date to honor time zones.

Having said that, I wish some of the data_type vs getter_method combinations are forbidden.

Current bug-report uses Date.toString() as a decision-maker of the "string representation of the Date" value.
Well, it might be not that bad, but we should consider that this is one more moving part.

As per my understanding, server-side always sends an instant (e.g. something like number of milliseconds since 1970), so if dealing with without time zone things while client gives no Calendar object we should offset the provided server-side value, so it is rendered "as in server-side" if using "java default time zone".
I believe it is done by

millis -= tz.getOffset(millis);

So far so good.

When it comes to "Timestamp -> Date" conversion (#376), I think it should just truncate "time" part.
I do not feel #376 gets it right, since it does just millis = millis / 86400000 * 86400000;, however this operation assumes "date is represented in UTC". In other words, this operation rounds to a UTC day, and it does not account for things like "leap seconds".

I am not sure what is the "fastest" way to truncate time part, but I think the proper one is to spin a Calendar instance, use setTime(long), then Calendar.set(Calendar.HOUR_OF_DAY, 0), etc.
For that sake we can reuse Calendar instance sitting and waiting inside TimestampUtils.

Member

vlsi commented Sep 28, 2015

TL;DR: I think #376 is broken and proper Timestamp -> Date conversion should use Calendar.

Frankly speaking, Timestamp/Date support from JDBC point of view is broken (as was discussed in #340).

I am not sure which sense makes using of without time zone and date at the same time.
Well, it could make sense if use something like https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html, however I am not sure if we can please all the users since some of the cases want Date behave like LocalDate and some of them might want Date to honor time zones.

Having said that, I wish some of the data_type vs getter_method combinations are forbidden.

Current bug-report uses Date.toString() as a decision-maker of the "string representation of the Date" value.
Well, it might be not that bad, but we should consider that this is one more moving part.

As per my understanding, server-side always sends an instant (e.g. something like number of milliseconds since 1970), so if dealing with without time zone things while client gives no Calendar object we should offset the provided server-side value, so it is rendered "as in server-side" if using "java default time zone".
I believe it is done by

millis -= tz.getOffset(millis);

So far so good.

When it comes to "Timestamp -> Date" conversion (#376), I think it should just truncate "time" part.
I do not feel #376 gets it right, since it does just millis = millis / 86400000 * 86400000;, however this operation assumes "date is represented in UTC". In other words, this operation rounds to a UTC day, and it does not account for things like "leap seconds".

I am not sure what is the "fastest" way to truncate time part, but I think the proper one is to spin a Calendar instance, use setTime(long), then Calendar.set(Calendar.HOUR_OF_DAY, 0), etc.
For that sake we can reuse Calendar instance sitting and waiting inside TimestampUtils.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 28, 2015

Member

On 28 September 2015 at 14:56, Vladimir Sitnikov notifications@github.com
wrote:

TL;DR: I think #376 #376 is broken
and proper Timestamp -> Date conversion should use Calendar.

Frankly speaking, Timestamp/Date support from JDBC point of view is broken
(as was discussed in #340 #340).

I am not sure which sense makes using of without time zone and date at
the same time.
Well, it could make sense if use something like
https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html,
however I am not sure if we can please all the users since some of the
cases want Date behave like LocalDate and some of them might want Date to
honor time zones.

Having said that, I wish some of the data_type vs getter_method
combinations are forbidden.

Current bug-report uses Date.toString() as a decision-maker of the "string
representation of the Date" value.
Well, it might be not that bad, but we should consider that this is one
more moving part.

As per my understanding, server-side always sends an instant (e.g.
something like number of milliseconds since 1970), so if dealing with without
time zone things while client gives no Calendar object we should offset
the provided server-side value, so it is rendered "as in server-side" if
using "java default time zone".
I believe it is done by

millis -= tz.getOffset(millis);

So far so good.

When it comes to "Timestamp -> Date" conversion (#376
#376), I think it should just
truncate "time" part.

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not feel #376 #376 gets it
right, since it does just millis = millis / 86400000 * 86400000;, however
this operation assumes "date is represented in UTC". In other words, this
operation rounds to a UTC day, and it does not account for things like
"leap seconds".

Actually no, by the time it gets here the offset has already been added in
once see
https://github.com/pgjdbc/pgjdbc/blob/master/org/postgresql/jdbc2/TimestampUtils.java#L816

I am not sure what is the "fastest" way to truncate time part, but I think
the proper one is to spin a Calendar instance, use setTime(long), then Calendar.set(Calendar.HOUR_OF_DAY,
0), etc.
For that sake we can reuse Calendar instance sitting and waiting inside
TimestampUtils.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 28, 2015

On 28 September 2015 at 14:56, Vladimir Sitnikov notifications@github.com
wrote:

TL;DR: I think #376 #376 is broken
and proper Timestamp -> Date conversion should use Calendar.

Frankly speaking, Timestamp/Date support from JDBC point of view is broken
(as was discussed in #340 #340).

I am not sure which sense makes using of without time zone and date at
the same time.
Well, it could make sense if use something like
https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html,
however I am not sure if we can please all the users since some of the
cases want Date behave like LocalDate and some of them might want Date to
honor time zones.

Having said that, I wish some of the data_type vs getter_method
combinations are forbidden.

Current bug-report uses Date.toString() as a decision-maker of the "string
representation of the Date" value.
Well, it might be not that bad, but we should consider that this is one
more moving part.

As per my understanding, server-side always sends an instant (e.g.
something like number of milliseconds since 1970), so if dealing with without
time zone things while client gives no Calendar object we should offset
the provided server-side value, so it is rendered "as in server-side" if
using "java default time zone".
I believe it is done by

millis -= tz.getOffset(millis);

So far so good.

When it comes to "Timestamp -> Date" conversion (#376
#376), I think it should just
truncate "time" part.

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not feel #376 #376 gets it
right, since it does just millis = millis / 86400000 * 86400000;, however
this operation assumes "date is represented in UTC". In other words, this
operation rounds to a UTC day, and it does not account for things like
"leap seconds".

Actually no, by the time it gets here the offset has already been added in
once see
https://github.com/pgjdbc/pgjdbc/blob/master/org/postgresql/jdbc2/TimestampUtils.java#L816

I am not sure what is the "fastest" way to truncate time part, but I think
the proper one is to spin a Calendar instance, use setTime(long), then Calendar.set(Calendar.HOUR_OF_DAY,
0), etc.
For that sake we can reuse Calendar instance sitting and waiting inside
TimestampUtils.


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 28, 2015

Member

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not get you. How can it use "client calendar" provided no calendar was given?

Actually no, by the time it gets here the offset has already been added in
once see

Are you sure that is enough to make millis = millis / 86400000 * 86400000 bulletproof?

Member

vlsi commented Sep 28, 2015

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not get you. How can it use "client calendar" provided no calendar was given?

Actually no, by the time it gets here the offset has already been added in
once see

Are you sure that is enough to make millis = millis / 86400000 * 86400000 bulletproof?

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 28, 2015

Member

On 28 September 2015 at 15:10, Vladimir Sitnikov notifications@github.com
wrote:

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not get you. How can it use "client calendar" provided no calendar
was given?

It is supposed to use the default calendar for the client. so
Calendar.getInstance()

Actually no, by the time it gets here the offset has already been added in
once see

Are you sure that is enough to make millis = millis / 86400000 * 86400000
bulletproof?

I'd wondered about whether the compiler might optimize that out. We may
have to create a function to return milis in a day. It works in Oracle's
compiler


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 28, 2015

On 28 September 2015 at 15:10, Vladimir Sitnikov notifications@github.com
wrote:

Unfortunately the spec says if no Calendar is provided then it uses the
client calendar to create the date with

I do not get you. How can it use "client calendar" provided no calendar
was given?

It is supposed to use the default calendar for the client. so
Calendar.getInstance()

Actually no, by the time it gets here the offset has already been added in
once see

Are you sure that is enough to make millis = millis / 86400000 * 86400000
bulletproof?

I'd wondered about whether the compiler might optimize that out. We may
have to create a function to return milis in a day. It works in Oracle's
compiler


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 28, 2015

Member

It is supposed to use the default calendar for the client. so Calendar.getInstance()

So we both agree here. I meant "just truncate time part as if it was in instant in java default timezone"

I'd wondered about whether the compiler might optimize that out

It must not optimize it.
However, number of millis in a day varies from day to day.
Do you think 86'400'000 accounts for that properly?

Member

vlsi commented Sep 28, 2015

It is supposed to use the default calendar for the client. so Calendar.getInstance()

So we both agree here. I meant "just truncate time part as if it was in instant in java default timezone"

I'd wondered about whether the compiler might optimize that out

It must not optimize it.
However, number of millis in a day varies from day to day.
Do you think 86'400'000 accounts for that properly?

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 28, 2015

Member

On 28 September 2015 at 15:18, Vladimir Sitnikov notifications@github.com
wrote:

It is supposed to use the default calendar for the client. so
Calendar.getInstance()

So we both agree here.

I'd wondered about whether the compiler might optimize that out

It must not optimize it.
However, number of millis in a day varies from day to day
https://en.wikipedia.org/wiki/Leap_second
.
Do you think 86'400'000 accounts for that properly?

I don't know enough about how seconds since epoch is actually used. Perhaps
it makes more sense to use a Calendar object and simply zero out the hour,
minutes and seconds ?


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 28, 2015

On 28 September 2015 at 15:18, Vladimir Sitnikov notifications@github.com
wrote:

It is supposed to use the default calendar for the client. so
Calendar.getInstance()

So we both agree here.

I'd wondered about whether the compiler might optimize that out

It must not optimize it.
However, number of millis in a day varies from day to day
https://en.wikipedia.org/wiki/Leap_second
.
Do you think 86'400'000 accounts for that properly?

I don't know enough about how seconds since epoch is actually used. Perhaps
it makes more sense to use a Calendar object and simply zero out the hour,
minutes and seconds ?


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 28, 2015

Member

Perhaps it makes more sense to use a Calendar object and simply zero out the hour, minutes and seconds ?

Bingo!
This is exactly why I write

TL;DR: I think #376 is broken and proper Timestamp -> Date conversion should use Calendar.

Would you please reconsider #376 and add a case like June 30, 2015 at 23:59:60 UTC?
In other words,

  • June 30, 2015 at 23:59:58 UTC
  • June 30, 2015 at 23:59:59 UTC
  • June 30, 2015 at 23:59:60 UTC
  • July 31, 2015 at 00:00:00 UTC
  • July 31, 2015 at 00:00:01 UTC
Member

vlsi commented Sep 28, 2015

Perhaps it makes more sense to use a Calendar object and simply zero out the hour, minutes and seconds ?

Bingo!
This is exactly why I write

TL;DR: I think #376 is broken and proper Timestamp -> Date conversion should use Calendar.

Would you please reconsider #376 and add a case like June 30, 2015 at 23:59:60 UTC?
In other words,

  • June 30, 2015 at 23:59:58 UTC
  • June 30, 2015 at 23:59:59 UTC
  • June 30, 2015 at 23:59:60 UTC
  • July 31, 2015 at 00:00:00 UTC
  • July 31, 2015 at 00:00:01 UTC
@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 28, 2015

Member

I'm not married to the PR. Which is why I din't just commit it.

Thanks for your feedback.

Dave Cramer

On 28 September 2015 at 15:39, Vladimir Sitnikov notifications@github.com
wrote:

Perhaps it makes more sense to use a Calendar object and simply zero out
the hour, minutes and seconds ?

Bingo!
This is exactly why I write

TL;DR: I think #376 #376 is broken
and proper Timestamp -> Date conversion should use Calendar.

Would you please reconsider #376
#376 and add a case like June 30,
2015 at 23:59:60 UTC?
In other words,

  • June 30, 2015 at 23:59:58 UTC
  • June 30, 2015 at 23:59:59 UTC
  • June 30, 2015 at 23:59:60 UTC
  • July 31, 2015 at 00:00:00 UTC
  • July 31, 2015 at 00:00:01 UTC


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 28, 2015

I'm not married to the PR. Which is why I din't just commit it.

Thanks for your feedback.

Dave Cramer

On 28 September 2015 at 15:39, Vladimir Sitnikov notifications@github.com
wrote:

Perhaps it makes more sense to use a Calendar object and simply zero out
the hour, minutes and seconds ?

Bingo!
This is exactly why I write

TL;DR: I think #376 #376 is broken
and proper Timestamp -> Date conversion should use Calendar.

Would you please reconsider #376
#376 and add a case like June 30,
2015 at 23:59:60 UTC?
In other words,

  • June 30, 2015 at 23:59:58 UTC
  • June 30, 2015 at 23:59:59 UTC
  • June 30, 2015 at 23:59:60 UTC
  • July 31, 2015 at 00:00:00 UTC
  • July 31, 2015 at 00:00:01 UTC


Reply to this email directly or view it on GitHub
#373 (comment).

@dfex55

This comment has been minimized.

Show comment
Hide comment
@dfex55

dfex55 Sep 29, 2015

I have added the dates from vlsi to the code and rerun it with #376.

As you can see, my example Date works, but those from vlsi not:

2015-07-01 00:00:00.0 vs. 2015-06-30
2015-07-31 00:00:00.0 vs. 2015-07-30
2015-07-31 00:00:01.0 vs. 2015-07-30

Maybe it will be a good idea to write some unit tests when fixing TimestampUtils.java as you mentioned a possible rewrite in the future (eg. with jodatime or java8) to avoid regressions.

I also vote for Calendar (or jodatime if you can consider repackaging jodatime).

Some background on the importance of this bug/fix:

pgjdbc changed the way the executions of prepared statements are counted. Previously it was based on the prepared statement instance, but in the current version the actual query is taken into account. see #319

Many frameworks (eg. JPA - eclipselink and others) are using prepared statements for a long time, but because they create new instances for their prepared statements each time, the (default) threshold was never reached. After upgrading the underlying pgjdbc driver, the threshold will be reached.

For JPA it is a common practice to use the Temporal annotation with TemporalType.DATE:

  @Basic
  @Temporal(TemporalType.DATE)
  @Column(name = "some_date")
  private Date someDate;

Some JPA implementations (e.g. eclipselink) are calling ResultSet.getDate(…) to fill the field, which leads to data corruption in your app.

As this is no obvious bug, it could be introduced slowly by the masses without noticing it as the pgjdbc upgrade might survive (most) test cases.

It could also make sense to directly mention this fix in the release announcement.

raw output:

as String:  2015-09-03 12:00:00.0
as Timestamp:   2015-09-03 12:00:00.0
as Date:    2015-09-03
---
as String:  2015-06-30 23:59:58.0
as Timestamp:   2015-06-30 23:59:58.0
as Date:    2015-06-30
---
as String:  2015-06-30 23:59:59.0
as Timestamp:   2015-06-30 23:59:59.0
as Date:    2015-06-30
---
as String:  2015-07-01 00:00:00.0
as Timestamp:   2015-07-01 00:00:00.0
as Date:    2015-06-30
---
as String:  2015-07-31 00:00:00.0
as Timestamp:   2015-07-31 00:00:00.0
as Date:    2015-07-30
---
as String:  2015-07-31 00:00:01.0
as Timestamp:   2015-07-31 00:00:01.0
as Date:    2015-07-30
---
    String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
    Properties props = new Properties();
    props.setProperty("user", DB_USER);
    props.setProperty("password", DB_PWD);
    props.setProperty("prepareThreshold", "2");
    Connection conn = DriverManager.getConnection(url, props);

    Statement stmt = conn.createStatement();

    final String tableTestName = "_jdbctest1";

    //@formatter:off
    stmt.execute("CREATE TABLE " + tableTestName
                + "("
                + "  datecol timestamp without time zone,"
                + "  rowno integer"
                + ")");
    //@formatter:on

    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-09-03 12:00:00', 1)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:58', 2)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:59', 3)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:60', 4)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-07-31 00:00:00', 5)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-07-31 00:00:01', 6)");

    PreparedStatement pstmt =
        conn.prepareStatement("SELECT datecol FROM " + tableTestName + " WHERE (rowno = ?)");

    System.out.print("warmup to reach prepareThreshold .. ");

    for (int i = 0; i < 2; i++) {

      pstmt.setInt(1, 1);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        // do nothing
      }
      rs.close();

    }

    System.out.println("finished\n");

    for (int i = 1; i <= 6; i++) {

      pstmt.setInt(1, i);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        System.out.println("as String:\t" + rs.getString("datecol"));
        System.out.println("as Timestamp:\t" + rs.getTimestamp("datecol"));
        System.out.println("as Date:\t" + rs.getDate("datecol"));
        System.out.println("---");
      }
      rs.close();

    }

    stmt.execute("drop table " + tableTestName);
    stmt.close();
    conn.close();

dfex55 commented Sep 29, 2015

I have added the dates from vlsi to the code and rerun it with #376.

As you can see, my example Date works, but those from vlsi not:

2015-07-01 00:00:00.0 vs. 2015-06-30
2015-07-31 00:00:00.0 vs. 2015-07-30
2015-07-31 00:00:01.0 vs. 2015-07-30

Maybe it will be a good idea to write some unit tests when fixing TimestampUtils.java as you mentioned a possible rewrite in the future (eg. with jodatime or java8) to avoid regressions.

I also vote for Calendar (or jodatime if you can consider repackaging jodatime).

Some background on the importance of this bug/fix:

pgjdbc changed the way the executions of prepared statements are counted. Previously it was based on the prepared statement instance, but in the current version the actual query is taken into account. see #319

Many frameworks (eg. JPA - eclipselink and others) are using prepared statements for a long time, but because they create new instances for their prepared statements each time, the (default) threshold was never reached. After upgrading the underlying pgjdbc driver, the threshold will be reached.

For JPA it is a common practice to use the Temporal annotation with TemporalType.DATE:

  @Basic
  @Temporal(TemporalType.DATE)
  @Column(name = "some_date")
  private Date someDate;

Some JPA implementations (e.g. eclipselink) are calling ResultSet.getDate(…) to fill the field, which leads to data corruption in your app.

As this is no obvious bug, it could be introduced slowly by the masses without noticing it as the pgjdbc upgrade might survive (most) test cases.

It could also make sense to directly mention this fix in the release announcement.

raw output:

as String:  2015-09-03 12:00:00.0
as Timestamp:   2015-09-03 12:00:00.0
as Date:    2015-09-03
---
as String:  2015-06-30 23:59:58.0
as Timestamp:   2015-06-30 23:59:58.0
as Date:    2015-06-30
---
as String:  2015-06-30 23:59:59.0
as Timestamp:   2015-06-30 23:59:59.0
as Date:    2015-06-30
---
as String:  2015-07-01 00:00:00.0
as Timestamp:   2015-07-01 00:00:00.0
as Date:    2015-06-30
---
as String:  2015-07-31 00:00:00.0
as Timestamp:   2015-07-31 00:00:00.0
as Date:    2015-07-30
---
as String:  2015-07-31 00:00:01.0
as Timestamp:   2015-07-31 00:00:01.0
as Date:    2015-07-30
---
    String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
    Properties props = new Properties();
    props.setProperty("user", DB_USER);
    props.setProperty("password", DB_PWD);
    props.setProperty("prepareThreshold", "2");
    Connection conn = DriverManager.getConnection(url, props);

    Statement stmt = conn.createStatement();

    final String tableTestName = "_jdbctest1";

    //@formatter:off
    stmt.execute("CREATE TABLE " + tableTestName
                + "("
                + "  datecol timestamp without time zone,"
                + "  rowno integer"
                + ")");
    //@formatter:on

    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-09-03 12:00:00', 1)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:58', 2)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:59', 3)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-06-30 23:59:60', 4)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-07-31 00:00:00', 5)");
    stmt.execute("insert into " + tableTestName
        + " (datecol, rowno) values ('2015-07-31 00:00:01', 6)");

    PreparedStatement pstmt =
        conn.prepareStatement("SELECT datecol FROM " + tableTestName + " WHERE (rowno = ?)");

    System.out.print("warmup to reach prepareThreshold .. ");

    for (int i = 0; i < 2; i++) {

      pstmt.setInt(1, 1);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        // do nothing
      }
      rs.close();

    }

    System.out.println("finished\n");

    for (int i = 1; i <= 6; i++) {

      pstmt.setInt(1, i);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        System.out.println("as String:\t" + rs.getString("datecol"));
        System.out.println("as Timestamp:\t" + rs.getTimestamp("datecol"));
        System.out.println("as Date:\t" + rs.getDate("datecol"));
        System.out.println("---");
      }
      rs.close();

    }

    stmt.execute("drop table " + tableTestName);
    stmt.close();
    conn.close();
@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 29, 2015

Member

Well this code has been around for quite a while. This is not new code.
When you say upgrade what exactly are you upgrading from to and where did
you not see the issue ?

Dave Cramer

On 29 September 2015 at 03:16, dfex55 notifications@github.com wrote:

I have added the dates from vlsi to the code and rerun it with #376
#376.

As you can see, my example Date works, but those from vlsi not:

2015-07-01 00:00:00.0 vs. 2015-06-30
2015-07-31 00:00:00.0 vs. 2015-07-30
2015-07-31 00:00:01.0 vs. 2015-07-30

Maybe it will be a good idea to write some unit tests when fixing
TimestampUtils.java as you mentioned a possible rewrite in the future (eg.
with jodatime or java8) to avoid regressions.

I also vote for Calendar (or jodatime if you can consider repackaging
jodatime).

Some background on the importance of this bug/fix:

pgjdbc changed the way the executions of prepared statements are counted.
Previously it was based on the prepared statement instance, but in the
current version the actual query is taken into account.

Many frameworks (eg. JPA - eclipselink and others) are using prepared
statements for a long time, but because they create new instances for their
prepared statements each time, the (default) threshold was never reached.
After upgrading the underlying pgjdbc driver, the threshold will be reached.

For JPA it is a common practice to use the Temporal annotation with
TemporalType.DATE:

@basic
@TeMPOraL(TemporalType.DATE)
@column(name = "some_date")
private Date someDate;

Some JPA implementations (e.g. eclipselink) are calling
ResultSet.getDate(…) to fill the field, which leads to data corruption in
your app.

As this is no obvious bug, it could be introduced slowly by the masses
without noticing it as the pgjdbc upgrade might survive (most) test cases.

It could also make sense to directly mention this fix in the release
announcement.

raw output:

as String: 2015-09-03 12:00:00.0
as Timestamp: 2015-09-03 12:00:00.0

as Date: 2015-09-03

as String: 2015-06-30 23:59:58.0
as Timestamp: 2015-06-30 23:59:58.0

as Date: 2015-06-30

as String: 2015-06-30 23:59:59.0
as Timestamp: 2015-06-30 23:59:59.0

as Date: 2015-06-30

as String: 2015-07-01 00:00:00.0
as Timestamp: 2015-07-01 00:00:00.0

as Date: 2015-06-30

as String: 2015-07-31 00:00:00.0
as Timestamp: 2015-07-31 00:00:00.0

as Date: 2015-07-30

as String: 2015-07-31 00:00:01.0
as Timestamp: 2015-07-31 00:00:01.0

as Date: 2015-07-30

String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
Properties props = new Properties();
props.setProperty("user", DB_USER);
props.setProperty("password", DB_PWD);
props.setProperty("prepareThreshold", "2");
Connection conn = DriverManager.getConnection(url, props);

Statement stmt = conn.createStatement();

final String tableTestName = "_jdbctest1";

//@formatter:off
stmt.execute("CREATE TABLE " + tableTestName
            + "("
            + "  datecol timestamp without time zone,"
            + "  rowno integer"
            + ")");
//@formatter:on

stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-09-03 12:00:00', 1)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:58', 2)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:59', 3)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:60', 4)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-07-31 00:00:00', 5)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-07-31 00:00:01', 6)");

PreparedStatement pstmt =
    conn.prepareStatement("SELECT datecol FROM " + tableTestName + " WHERE (rowno = ?)");

System.out.print("warmup to reach prepareThreshold .. ");

for (int i = 0; i < 2; i++) {

  pstmt.setInt(1, 1);
  ResultSet rs = pstmt.executeQuery();
  while (rs.next()) {
    // do nothing
  }
  rs.close();

}

System.out.println("finished\n");

for (int i = 1; i <= 6; i++) {

  pstmt.setInt(1, i);
  ResultSet rs = pstmt.executeQuery();
  while (rs.next()) {
    System.out.println("as String:\t" + rs.getString("datecol"));
    System.out.println("as Timestamp:\t" + rs.getTimestamp("datecol"));
    System.out.println("as Date:\t" + rs.getDate("datecol"));
    System.out.println("---");
  }
  rs.close();

}

stmt.execute("drop table " + tableTestName);
stmt.close();
conn.close();


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 29, 2015

Well this code has been around for quite a while. This is not new code.
When you say upgrade what exactly are you upgrading from to and where did
you not see the issue ?

Dave Cramer

On 29 September 2015 at 03:16, dfex55 notifications@github.com wrote:

I have added the dates from vlsi to the code and rerun it with #376
#376.

As you can see, my example Date works, but those from vlsi not:

2015-07-01 00:00:00.0 vs. 2015-06-30
2015-07-31 00:00:00.0 vs. 2015-07-30
2015-07-31 00:00:01.0 vs. 2015-07-30

Maybe it will be a good idea to write some unit tests when fixing
TimestampUtils.java as you mentioned a possible rewrite in the future (eg.
with jodatime or java8) to avoid regressions.

I also vote for Calendar (or jodatime if you can consider repackaging
jodatime).

Some background on the importance of this bug/fix:

pgjdbc changed the way the executions of prepared statements are counted.
Previously it was based on the prepared statement instance, but in the
current version the actual query is taken into account.

Many frameworks (eg. JPA - eclipselink and others) are using prepared
statements for a long time, but because they create new instances for their
prepared statements each time, the (default) threshold was never reached.
After upgrading the underlying pgjdbc driver, the threshold will be reached.

For JPA it is a common practice to use the Temporal annotation with
TemporalType.DATE:

@basic
@TeMPOraL(TemporalType.DATE)
@column(name = "some_date")
private Date someDate;

Some JPA implementations (e.g. eclipselink) are calling
ResultSet.getDate(…) to fill the field, which leads to data corruption in
your app.

As this is no obvious bug, it could be introduced slowly by the masses
without noticing it as the pgjdbc upgrade might survive (most) test cases.

It could also make sense to directly mention this fix in the release
announcement.

raw output:

as String: 2015-09-03 12:00:00.0
as Timestamp: 2015-09-03 12:00:00.0

as Date: 2015-09-03

as String: 2015-06-30 23:59:58.0
as Timestamp: 2015-06-30 23:59:58.0

as Date: 2015-06-30

as String: 2015-06-30 23:59:59.0
as Timestamp: 2015-06-30 23:59:59.0

as Date: 2015-06-30

as String: 2015-07-01 00:00:00.0
as Timestamp: 2015-07-01 00:00:00.0

as Date: 2015-06-30

as String: 2015-07-31 00:00:00.0
as Timestamp: 2015-07-31 00:00:00.0

as Date: 2015-07-30

as String: 2015-07-31 00:00:01.0
as Timestamp: 2015-07-31 00:00:01.0

as Date: 2015-07-30

String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
Properties props = new Properties();
props.setProperty("user", DB_USER);
props.setProperty("password", DB_PWD);
props.setProperty("prepareThreshold", "2");
Connection conn = DriverManager.getConnection(url, props);

Statement stmt = conn.createStatement();

final String tableTestName = "_jdbctest1";

//@formatter:off
stmt.execute("CREATE TABLE " + tableTestName
            + "("
            + "  datecol timestamp without time zone,"
            + "  rowno integer"
            + ")");
//@formatter:on

stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-09-03 12:00:00', 1)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:58', 2)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:59', 3)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-06-30 23:59:60', 4)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-07-31 00:00:00', 5)");
stmt.execute("insert into " + tableTestName
    + " (datecol, rowno) values ('2015-07-31 00:00:01', 6)");

PreparedStatement pstmt =
    conn.prepareStatement("SELECT datecol FROM " + tableTestName + " WHERE (rowno = ?)");

System.out.print("warmup to reach prepareThreshold .. ");

for (int i = 0; i < 2; i++) {

  pstmt.setInt(1, 1);
  ResultSet rs = pstmt.executeQuery();
  while (rs.next()) {
    // do nothing
  }
  rs.close();

}

System.out.println("finished\n");

for (int i = 1; i <= 6; i++) {

  pstmt.setInt(1, i);
  ResultSet rs = pstmt.executeQuery();
  while (rs.next()) {
    System.out.println("as String:\t" + rs.getString("datecol"));
    System.out.println("as Timestamp:\t" + rs.getTimestamp("datecol"));
    System.out.println("as Date:\t" + rs.getDate("datecol"));
    System.out.println("---");
  }
  rs.close();

}

stmt.execute("drop table " + tableTestName);
stmt.close();
conn.close();


Reply to this email directly or view it on GitHub
#373 (comment).

@dfex55

This comment has been minimized.

Show comment
Hide comment
@dfex55

dfex55 Sep 29, 2015

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319 was released on 2015-08-27 (Version 9.4-1202). And now frameworks that did not reuse PreparedStatement instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit the prepareThreshold you will also hit the code. But that never happened (to me) because the used framework always created new PreparedStatement instances.

So you will hit the bug more likely if you upgrade from a version prior 1202 to 1202 or later.

dfex55 commented Sep 29, 2015

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319 was released on 2015-08-27 (Version 9.4-1202). And now frameworks that did not reuse PreparedStatement instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit the prepareThreshold you will also hit the code. But that never happened (to me) because the used framework always created new PreparedStatement instances.

So you will hit the bug more likely if you upgrade from a version prior 1202 to 1202 or later.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 29, 2015

Member

So the real issue here is that we subtract the offset twice.

Once in getTimestamp() and again in getDate()

Even the simple code using /day*day would work.

As I said the whole date handling is borked in the current code and needs a
rewrite.

Dave Cramer

On 29 September 2015 at 07:12, dfex55 notifications@github.com wrote:

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319
#319 was released on 2015-08-27
(Version 9.4-1202). And now frameworks that did not reuse PreparedStatement
instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit
the prepareThreshold you will also hit the code. But that never happened
(to me) because the used framework always created new PreparedStatement
instances.

So you will hit the bug more likely if you upgrade from a version prior
1202 to 1202 or later.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 29, 2015

So the real issue here is that we subtract the offset twice.

Once in getTimestamp() and again in getDate()

Even the simple code using /day*day would work.

As I said the whole date handling is borked in the current code and needs a
rewrite.

Dave Cramer

On 29 September 2015 at 07:12, dfex55 notifications@github.com wrote:

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319
#319 was released on 2015-08-27
(Version 9.4-1202). And now frameworks that did not reuse PreparedStatement
instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit
the prepareThreshold you will also hit the code. But that never happened
(to me) because the used framework always created new PreparedStatement
instances.

So you will hit the bug more likely if you upgrade from a version prior
1202 to 1202 or later.


Reply to this email directly or view it on GitHub
#373 (comment).

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 29, 2015

Member

This now passes all of the dates proposed by visi

Dave Cramer

On 29 September 2015 at 10:16, Dave Cramer davecramer@gmail.com wrote:

So the real issue here is that we subtract the offset twice.

Once in getTimestamp() and again in getDate()

Even the simple code using /day*day would work.

As I said the whole date handling is borked in the current code and needs
a rewrite.

Dave Cramer

On 29 September 2015 at 07:12, dfex55 notifications@github.com wrote:

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319
#319 was released on 2015-08-27
(Version 9.4-1202). And now frameworks that did not reuse PreparedStatement
instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit
the prepareThreshold you will also hit the code. But that never happened
(to me) because the used framework always created new PreparedStatement
instances.

So you will hit the bug more likely if you upgrade from a version prior
1202 to 1202 or later.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 29, 2015

This now passes all of the dates proposed by visi

Dave Cramer

On 29 September 2015 at 10:16, Dave Cramer davecramer@gmail.com wrote:

So the real issue here is that we subtract the offset twice.

Once in getTimestamp() and again in getDate()

Even the simple code using /day*day would work.

As I said the whole date handling is borked in the current code and needs
a rewrite.

Dave Cramer

On 29 September 2015 at 07:12, dfex55 notifications@github.com wrote:

You are right. The code in TimestampUtils has been there for a long time.

But: According to your changelog #319
#319 was released on 2015-08-27
(Version 9.4-1202). And now frameworks that did not reuse PreparedStatement
instances will or may run into the bug.

If you are reusing the PreparedStatement before Version 9.4-1202 and hit
the prepareThreshold you will also hit the code. But that never happened
(to me) because the used framework always created new PreparedStatement
instances.

So you will hit the bug more likely if you upgrade from a version prior
1202 to 1202 or later.


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 29, 2015

Member
  1. Dave, could you please add the tests?
  2. I do not see how could it pass tests provided you are using 86400 and true day durations are different. It might be something broken in in the test itself.
Member

vlsi commented Sep 29, 2015

  1. Dave, could you please add the tests?
  2. I do not see how could it pass tests provided you are using 86400 and true day durations are different. It might be something broken in in the test itself.
@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 29, 2015

Member

Vladimir,

Sure, I'll add them, you can try the snapshots I built this morning though
if you like.

Dave Cramer

On 29 September 2015 at 12:21, Vladimir Sitnikov notifications@github.com
wrote:

  1. Dave, could you please add the tests?
  2. I do not see how could it pass tests provided you are using 86400 and
    true day durations are different. It might be something broken in in the
    test itself.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 29, 2015

Vladimir,

Sure, I'll add them, you can try the snapshots I built this morning though
if you like.

Dave Cramer

On 29 September 2015 at 12:21, Vladimir Sitnikov notifications@github.com
wrote:

  1. Dave, could you please add the tests?
  2. I do not see how could it pass tests provided you are using 86400 and
    true day durations are different. It might be something broken in in the
    test itself.


Reply to this email directly or view it on GitHub
#373 (comment).

@dfex55

This comment has been minimized.

Show comment
Hide comment
@dfex55

dfex55 Sep 30, 2015

Your snapshot is working for me.

executed with:

  • postgresql-9.4-1203-jdbc41-20150929.150610-2.jar
  • postgres 9.3.9
  @Test
  public final void testTimestampToDateConversion() throws SQLException, ParseException {

    final int prepareThreshold = 2;
    final String dateColName = "dateCol";

    final String testDateFormat = "yyyy-MM-dd HH:mm:ss";
    final List<String> datesToTest =
        Arrays.asList(
            "2015-09-03 12:00:00", "2015-06-30 23:59:58", "1997-06-30 23:59:59",
            "1997-07-01 00:00:00", "2012-06-30 23:59:59", "2012-07-01 00:00:00",
            "2015-06-30 23:59:59", "2015-07-01 00:00:00", "2005-12-31 23:59:59",
            "2006-01-01 00:00:00", "2008-12-31 23:59:59", "2009-01-01 00:00:00",
            "2015-06-30 23:59:60", "2015-07-31 00:00:00", "2015-07-31 00:00:01");

    Date[] resultingDates = new Date[datesToTest.size()];
    Date[] resultingTimestamps = new Date[datesToTest.size()];

    String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
    Properties props = new Properties();
    props.setProperty("user", DB_USER);
    props.setProperty("password", DB_PWD);
    props.setProperty("prepareThreshold", String.valueOf(prepareThreshold));
    Connection conn = DriverManager.getConnection(url, props);

    Statement stmt = conn.createStatement();

    final String tableTestName =
        "_jdbc_test_timestamp_date_conversion_" + System.currentTimeMillis();

    //@formatter:off
    stmt.execute("CREATE TABLE " + tableTestName
                + "("
                + "  " + dateColName + " timestamp without time zone,"
                + "  rowno integer"
                + ")");
    //@formatter:on

    for (int i = 0; i < datesToTest.size(); i++) {
      stmt.execute("insert into " + tableTestName + " (" + dateColName + ", rowno) values ('"
          + datesToTest.get(i) + "', " + i + ")");
    }

    PreparedStatement pstmt =
        conn.prepareStatement("SELECT " + dateColName + " FROM " + tableTestName
            + " WHERE (rowno = ?)");

    System.out.print("warmup to reach prepareThreshold .. ");

    for (int i = 0; i < prepareThreshold; i++) {

      pstmt.setInt(1, 1);
      ResultSet rs = pstmt.executeQuery();
      rs.close();

    }

    System.out.println("finished\n");

    for (int i = 0; i < datesToTest.size(); i++) {

      pstmt.setInt(1, i);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        System.out.println("to test:\t" + datesToTest.get(i));
        System.out.println("as String:\t" + rs.getString(dateColName));
        System.out.println("as Timestamp:\t" + rs.getTimestamp(dateColName));
        System.out.println("as Date:\t" + rs.getDate(dateColName));
        System.out.println("---");
        resultingDates[i] = rs.getDate(dateColName);
        resultingTimestamps[i] = rs.getTimestamp(dateColName);
      }
      rs.close();

    }

    stmt.execute("drop table " + tableTestName);
    stmt.close();
    pstmt.close();
    conn.close();

    Calendar dbTimestamp = Calendar.getInstance();
    Calendar dbDate = Calendar.getInstance();
    Calendar expectedDate = Calendar.getInstance();

    SimpleDateFormat sdf = new SimpleDateFormat(testDateFormat);
    for (int i = 0; i < datesToTest.size(); i++) {
      expectedDate.setTime(sdf.parse(datesToTest.get(i)));
      dbTimestamp.setTime(resultingTimestamps[i]);
      dbDate.setTime(resultingDates[i]);

      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(), expectedDate.get(Calendar.YEAR),
          dbDate.get(Calendar.YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(), expectedDate.get(Calendar.MONTH),
          dbDate.get(Calendar.MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(),
          expectedDate.get(Calendar.DAY_OF_MONTH), dbDate.get(Calendar.DAY_OF_MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(),
          expectedDate.get(Calendar.DAY_OF_YEAR), dbDate.get(Calendar.DAY_OF_YEAR));

      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.YEAR), dbTimestamp.get(Calendar.YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.MONTH), dbTimestamp.get(Calendar.MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.DAY_OF_MONTH), dbTimestamp.get(Calendar.DAY_OF_MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.DAY_OF_YEAR), dbTimestamp.get(Calendar.DAY_OF_YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.HOUR_OF_DAY), dbTimestamp.get(Calendar.HOUR_OF_DAY));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.MINUTE), dbTimestamp.get(Calendar.MINUTE));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.SECOND), dbTimestamp.get(Calendar.SECOND));
    }

  }

dfex55 commented Sep 30, 2015

Your snapshot is working for me.

executed with:

  • postgresql-9.4-1203-jdbc41-20150929.150610-2.jar
  • postgres 9.3.9
  @Test
  public final void testTimestampToDateConversion() throws SQLException, ParseException {

    final int prepareThreshold = 2;
    final String dateColName = "dateCol";

    final String testDateFormat = "yyyy-MM-dd HH:mm:ss";
    final List<String> datesToTest =
        Arrays.asList(
            "2015-09-03 12:00:00", "2015-06-30 23:59:58", "1997-06-30 23:59:59",
            "1997-07-01 00:00:00", "2012-06-30 23:59:59", "2012-07-01 00:00:00",
            "2015-06-30 23:59:59", "2015-07-01 00:00:00", "2005-12-31 23:59:59",
            "2006-01-01 00:00:00", "2008-12-31 23:59:59", "2009-01-01 00:00:00",
            "2015-06-30 23:59:60", "2015-07-31 00:00:00", "2015-07-31 00:00:01");

    Date[] resultingDates = new Date[datesToTest.size()];
    Date[] resultingTimestamps = new Date[datesToTest.size()];

    String url = "jdbc:postgresql://" + DB_HOST + "/" + DB_NAME;
    Properties props = new Properties();
    props.setProperty("user", DB_USER);
    props.setProperty("password", DB_PWD);
    props.setProperty("prepareThreshold", String.valueOf(prepareThreshold));
    Connection conn = DriverManager.getConnection(url, props);

    Statement stmt = conn.createStatement();

    final String tableTestName =
        "_jdbc_test_timestamp_date_conversion_" + System.currentTimeMillis();

    //@formatter:off
    stmt.execute("CREATE TABLE " + tableTestName
                + "("
                + "  " + dateColName + " timestamp without time zone,"
                + "  rowno integer"
                + ")");
    //@formatter:on

    for (int i = 0; i < datesToTest.size(); i++) {
      stmt.execute("insert into " + tableTestName + " (" + dateColName + ", rowno) values ('"
          + datesToTest.get(i) + "', " + i + ")");
    }

    PreparedStatement pstmt =
        conn.prepareStatement("SELECT " + dateColName + " FROM " + tableTestName
            + " WHERE (rowno = ?)");

    System.out.print("warmup to reach prepareThreshold .. ");

    for (int i = 0; i < prepareThreshold; i++) {

      pstmt.setInt(1, 1);
      ResultSet rs = pstmt.executeQuery();
      rs.close();

    }

    System.out.println("finished\n");

    for (int i = 0; i < datesToTest.size(); i++) {

      pstmt.setInt(1, i);
      ResultSet rs = pstmt.executeQuery();
      while (rs.next()) {
        System.out.println("to test:\t" + datesToTest.get(i));
        System.out.println("as String:\t" + rs.getString(dateColName));
        System.out.println("as Timestamp:\t" + rs.getTimestamp(dateColName));
        System.out.println("as Date:\t" + rs.getDate(dateColName));
        System.out.println("---");
        resultingDates[i] = rs.getDate(dateColName);
        resultingTimestamps[i] = rs.getTimestamp(dateColName);
      }
      rs.close();

    }

    stmt.execute("drop table " + tableTestName);
    stmt.close();
    pstmt.close();
    conn.close();

    Calendar dbTimestamp = Calendar.getInstance();
    Calendar dbDate = Calendar.getInstance();
    Calendar expectedDate = Calendar.getInstance();

    SimpleDateFormat sdf = new SimpleDateFormat(testDateFormat);
    for (int i = 0; i < datesToTest.size(); i++) {
      expectedDate.setTime(sdf.parse(datesToTest.get(i)));
      dbTimestamp.setTime(resultingTimestamps[i]);
      dbDate.setTime(resultingDates[i]);

      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(), expectedDate.get(Calendar.YEAR),
          dbDate.get(Calendar.YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(), expectedDate.get(Calendar.MONTH),
          dbDate.get(Calendar.MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(),
          expectedDate.get(Calendar.DAY_OF_MONTH), dbDate.get(Calendar.DAY_OF_MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbDate.getTime(),
          expectedDate.get(Calendar.DAY_OF_YEAR), dbDate.get(Calendar.DAY_OF_YEAR));

      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.YEAR), dbTimestamp.get(Calendar.YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.MONTH), dbTimestamp.get(Calendar.MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.DAY_OF_MONTH), dbTimestamp.get(Calendar.DAY_OF_MONTH));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.DAY_OF_YEAR), dbTimestamp.get(Calendar.DAY_OF_YEAR));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.HOUR_OF_DAY), dbTimestamp.get(Calendar.HOUR_OF_DAY));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.MINUTE), dbTimestamp.get(Calendar.MINUTE));
      Assert.assertEquals(
          expectedDate.getTime() + " vs. " + dbTimestamp.getTime(),
          expectedDate.get(Calendar.SECOND), dbTimestamp.get(Calendar.SECOND));
    }

  }
@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 30, 2015

Member

I do not think the test is properly crafted.

SimpleDateFormat should use UTC time zone.
I would like to explicitly see "unix time" (long value) for 23:59:60 since I am afraid this value is improperly parsed from string representation.

Member

vlsi commented Sep 30, 2015

I do not think the test is properly crafted.

SimpleDateFormat should use UTC time zone.
I would like to explicitly see "unix time" (long value) for 23:59:60 since I am afraid this value is improperly parsed from string representation.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 30, 2015

Member

Vladimir,

Is there a way to see if the number of seconds for a leap day is actually
greater than 86400 in Calendar ?

Alternatively can you provide the number of milliseconds to use to
construct the date for testing ?

Dave Cramer

On 30 September 2015 at 05:57, Vladimir Sitnikov notifications@github.com
wrote:

I do not think the test is properly crafted.

SimpleDateFormat should use UTC time zone.
I would like to explicitly see "unix time" (long value) for 23:59:60
since I am afraid this value is improperly parsed from string
representation.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 30, 2015

Vladimir,

Is there a way to see if the number of seconds for a leap day is actually
greater than 86400 in Calendar ?

Alternatively can you provide the number of milliseconds to use to
construct the date for testing ?

Dave Cramer

On 30 September 2015 at 05:57, Vladimir Sitnikov notifications@github.com
wrote:

I do not think the test is properly crafted.

SimpleDateFormat should use UTC time zone.
I would like to explicitly see "unix time" (long value) for 23:59:60
since I am afraid this value is improperly parsed from string
representation.


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Sep 30, 2015

Member

I'm wrong here.
In fact, there is no long value for 23:59:60. When that happens, the same number of millis is reused (in other words, the clocks just move backward). Well, that depends on the implementation, OS, etc, but anyway java 8 does not have a dedicated long value for :60.

https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html: The Java Time-Scale divides each calendar day into exactly 86400 subdivisions, known as seconds.

So, 86400 is in line with java8's time API.

I think it makes sense adding a couple of tests around DST boundary with using explicit time zone (TimeZone.setDefault(...), etc.)

Member

vlsi commented Sep 30, 2015

I'm wrong here.
In fact, there is no long value for 23:59:60. When that happens, the same number of millis is reused (in other words, the clocks just move backward). Well, that depends on the implementation, OS, etc, but anyway java 8 does not have a dedicated long value for :60.

https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html: The Java Time-Scale divides each calendar day into exactly 86400 subdivisions, known as seconds.

So, 86400 is in line with java8's time API.

I think it makes sense adding a couple of tests around DST boundary with using explicit time zone (TimeZone.setDefault(...), etc.)

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Sep 30, 2015

Member

Thanks for putting up such a vigorous offence though. It is good to discuss
these things.

Dave

Dave Cramer

On 30 September 2015 at 06:52, Vladimir Sitnikov notifications@github.com
wrote:

I'm wrong here.
In fact, there is no long value for 23:59:60. When that happens, the same
number of millis is reused (in other words, the clocks just move backward).

https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html: The
Java Time-Scale divides each calendar day into exactly 86400 subdivisions,
known as seconds.

So, 86400 is in line with java8's time API.

I think it makes sense adding a couple of tests around DST boundary with
using explicit time zone (TimeZone.setDefault(...), etc.)


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Sep 30, 2015

Thanks for putting up such a vigorous offence though. It is good to discuss
these things.

Dave

Dave Cramer

On 30 September 2015 at 06:52, Vladimir Sitnikov notifications@github.com
wrote:

I'm wrong here.
In fact, there is no long value for 23:59:60. When that happens, the same
number of millis is reused (in other words, the clocks just move backward).

https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html: The
Java Time-Scale divides each calendar day into exactly 86400 subdivisions,
known as seconds.

So, 86400 is in line with java8's time API.

I think it makes sense adding a couple of tests around DST boundary with
using explicit time zone (TimeZone.setDefault(...), etc.)


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Oct 8, 2015

Member

I've incorporated this and some more test data into #387.
I think we can close this issue and continue discussion there.

Member

vlsi commented Oct 8, 2015

I've incorporated this and some more test data into #387.
I think we can close this issue and continue discussion there.

@talios

This comment has been minimized.

Show comment
Hide comment
@talios

talios Nov 23, 2015

We've just encountered a remarkably similar using postgresql-9.4-1203.jdbc41.jar to this but involving COUNT(*) queries rather than dates/timezones. Our use case is coming via Hibernate and an SQLQuery instance. The first two invocations returned an expected count of 6 whilst the third and subsequent returned 60. Rolling back to the previous JDBC driver we were using ( 9.1-901-1.jdbc4 ) does not exhibit this behaviour.

I notice all the comments on this ticket focus solely on the date/timestamp issues also uncovered here, but is there possibly an underlying issue that's unrelated to timestamps?

We hope to extract a reproducible test case soon to submit, but I thought I'd query this first in the mean time.

talios commented Nov 23, 2015

We've just encountered a remarkably similar using postgresql-9.4-1203.jdbc41.jar to this but involving COUNT(*) queries rather than dates/timezones. Our use case is coming via Hibernate and an SQLQuery instance. The first two invocations returned an expected count of 6 whilst the third and subsequent returned 60. Rolling back to the previous JDBC driver we were using ( 9.1-901-1.jdbc4 ) does not exhibit this behaviour.

I notice all the comments on this ticket focus solely on the date/timestamp issues also uncovered here, but is there possibly an underlying issue that's unrelated to timestamps?

We hope to extract a reproducible test case soon to submit, but I thought I'd query this first in the mean time.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Nov 23, 2015

Member

The problem is related to using named statements which usually kicks in at
the 6th invocation.

As 1203 is rather old, can you try 1205 ?

Dave

Dave Cramer

On 23 November 2015 at 17:55, Mark Derricutt notifications@github.com
wrote:

We've just encountered a remarkably similar using
postgresql-9.4-1203.jdbc41.jar to this but involving COUNT() queries
rather than dates/timezones. Our use case is coming via Hibernate and an
SQLQuery instance. The first *two
invocations returned an expected count
of 6 whilst the third and subsequent returned 60. Rolling back to the
previous JDBC driver we were using ( 9.1-901-1.jdbc4 ) does not exhibit
this behaviour.

I notice all the comments on this ticket focus solely on the
date/timestamp issues also uncovered here, but is there possibly an
underlying issue that's unrelated to timestamps?

We hope to extract a reproducible test case soon to submit, but I thought
I'd query this first in the mean time.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Nov 23, 2015

The problem is related to using named statements which usually kicks in at
the 6th invocation.

As 1203 is rather old, can you try 1205 ?

Dave

Dave Cramer

On 23 November 2015 at 17:55, Mark Derricutt notifications@github.com
wrote:

We've just encountered a remarkably similar using
postgresql-9.4-1203.jdbc41.jar to this but involving COUNT() queries
rather than dates/timezones. Our use case is coming via Hibernate and an
SQLQuery instance. The first *two
invocations returned an expected count
of 6 whilst the third and subsequent returned 60. Rolling back to the
previous JDBC driver we were using ( 9.1-901-1.jdbc4 ) does not exhibit
this behaviour.

I notice all the comments on this ticket focus solely on the
date/timestamp issues also uncovered here, but is there possibly an
underlying issue that's unrelated to timestamps?

We hope to extract a reproducible test case soon to submit, but I thought
I'd query this first in the mean time.


Reply to this email directly or view it on GitHub
#373 (comment).

@talios

This comment has been minimized.

Show comment
Hide comment
@talios

talios Nov 24, 2015

Ewps - my bad - we were actually using 1205. I'm working on a reproducible test now.

talios commented Nov 24, 2015

Ewps - my bad - we were actually using 1205. I'm working on a reproducible test now.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Nov 24, 2015

Member

in which case try building from source as there was a related bug fixed
since

Dave Cramer

On 23 November 2015 at 21:16, Mark Derricutt notifications@github.com
wrote:

Ewps - my bad - we were actually using 1205. I'm working on a reproducible
test now.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Nov 24, 2015

in which case try building from source as there was a related bug fixed
since

Dave Cramer

On 23 November 2015 at 21:16, Mark Derricutt notifications@github.com
wrote:

Ewps - my bad - we were actually using 1205. I'm working on a reproducible
test now.


Reply to this email directly or view it on GitHub
#373 (comment).

@talios

This comment has been minimized.

Show comment
Hide comment
@talios

talios Nov 25, 2015

@davecramer Was unable to reproduce using straight JDBC ( either via 1mil connections/PreparedStatements run accross a parrallel stream ).

We use the JTA Driver via Atomikos, via Hibernate which have been exhausting caches much more aggressively.

That being said however - building HEAD@master seems to no longer exhibit the issue ( likely solved by either fad7f52 or e6f1beb ).

Unfortunately this caused some nasty data-corruption with wrong values being returned from sequences etc. etc.

IMHO it'd be worth pushing a release containing these changes and recommending people to NOT use 1205 ( this probably isn't the right place to continue this discussion tho ).

Mark

talios commented Nov 25, 2015

@davecramer Was unable to reproduce using straight JDBC ( either via 1mil connections/PreparedStatements run accross a parrallel stream ).

We use the JTA Driver via Atomikos, via Hibernate which have been exhausting caches much more aggressively.

That being said however - building HEAD@master seems to no longer exhibit the issue ( likely solved by either fad7f52 or e6f1beb ).

Unfortunately this caused some nasty data-corruption with wrong values being returned from sequences etc. etc.

IMHO it'd be worth pushing a release containing these changes and recommending people to NOT use 1205 ( this probably isn't the right place to continue this discussion tho ).

Mark

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Nov 25, 2015

Member

@davecramer, should we release 1206? The regression in 1205 is not that rare.

Member

vlsi commented Nov 25, 2015

@davecramer, should we release 1206? The regression in 1205 is not that rare.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Nov 25, 2015

Member

@vlsi yes, today

Member

davecramer commented Nov 25, 2015

@vlsi yes, today

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Nov 25, 2015

Member

sonatype is being belligerent today. Seems to be very slow

Dave Cramer

On 25 November 2015 at 03:45, Vladimir Sitnikov notifications@github.com
wrote:

@davecramer https://github.com/davecramer, should we release 1206? The
regression in 1205 is not that rare.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Nov 25, 2015

sonatype is being belligerent today. Seems to be very slow

Dave Cramer

On 25 November 2015 at 03:45, Vladimir Sitnikov notifications@github.com
wrote:

@davecramer https://github.com/davecramer, should we release 1206? The
regression in 1205 is not that rare.


Reply to this email directly or view it on GitHub
#373 (comment).

@talios

This comment has been minimized.

Show comment
Hide comment
@talios

talios Nov 25, 2015

Seems to be in central now - tho search.maven.org is not updated.

talios commented Nov 25, 2015

Seems to be in central now - tho search.maven.org is not updated.

@davecramer

This comment has been minimized.

Show comment
Hide comment
@davecramer

davecramer Nov 25, 2015

Member

nexus was being belligerent today. Give it time

Dave Cramer

On 25 November 2015 at 16:56, Mark Derricutt notifications@github.com
wrote:

Seems to be in central now - tho search.maven.org is not updated.


Reply to this email directly or view it on GitHub
#373 (comment).

Member

davecramer commented Nov 25, 2015

nexus was being belligerent today. Give it time

Dave Cramer

On 25 November 2015 at 16:56, Mark Derricutt notifications@github.com
wrote:

Seems to be in central now - tho search.maven.org is not updated.


Reply to this email directly or view it on GitHub
#373 (comment).

@vlsi

This comment has been minimized.

Show comment
Hide comment
@vlsi

vlsi Dec 21, 2015

Member

I believe this one is not reproduced in 1206

Member

vlsi commented Dec 21, 2015

I believe this one is not reproduced in 1206

@vlsi vlsi closed this Dec 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment