inappropriate statement sharing #674

igelbox · 2016-10-30T11:51:44Z

Add more parameter type checks into SimpleQuery.isPreparedFor for Oid.UNSPECIFIED parameter type.
This commit addresses issue #648

when parameter type changed

Check original parameter type when current parameter type is Oid.UNSPECIFIED. fixes pgjdbc#648

codecov-io · 2016-10-30T12:14:14Z

Current coverage is 64.12% (diff: 60.00%)

Merging #674 into master will increase coverage by 0.05%

@@             master       #674   diff @@
==========================================
  Files           165        164     -1   
  Lines         15129      15131     +2   
  Methods           0          0          
  Messages          0          0          
  Branches       2978       2982     +4   
==========================================
+ Hits           9694       9703     +9   
+ Misses         4201       4193     -8   
- Partials       1234       1235     +1

Powered by Codecov. Last update f48c6bb...7938dce

vlsi

Please update test code. Please add batch insert into t(...) values(?) kind of test.

insert values is needed for two reasons:

test insert case;
test how the fix plays with "insert batch rewritter".

vlsi · 2016-11-21T10:37:15Z

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

+    PreparedStatement ps = con.prepareStatement("SELECT ?::timestamp");
+    try {
+      Timestamp ts = new Timestamp(1474997614836L);
+      for (int i = 0; i < 3; ++i) {


Why is it required to have a loop here?

Because PreparedStatement is not cached after first executions. But after 5-th executions (which happens on 3-rd iteration) it become cached.

vlsi · 2016-11-21T10:38:11Z

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

+        rs = ps.executeQuery();
+        try {
+          assertTrue(rs.next());
+          assertNull(rs.getObject(1));


Please follow https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#test guidelines for writing tests and assertions

I read the rules for the tests writing. But not figured, what I'm doing wrong.
Also, there is (into pgjdbc test classes) a lot of code that uses assertTrue for rs.next() and assertEquals without a message.

a lot of code that uses assertTrue for rs.next() and assertEquals without a message.

That is unfortunate. New code should explain what it is trying to test.
I agree assertTrue(rs.next()); might be fine (I would agree even with plain rs.next() without assert), however assertNull(rs.getObject(1)); should explain why does it expect null there.

The thing is when test fails, it takes lots of time to figure out what was the intention behind the test and why did it fail.
Having decent messages greatly reduces maintenance costs.

igelbox · 2016-11-21T21:54:46Z

I added comments into the PreparedStatementTest#testInappropriateStatementSharing source code (for readability purposes).
Could you tell me, why should I add tests for data inserting, if this bug was already reproduced using select-statement? Or are you concerned about a lack of insert-statement tests?

vlsi · 2016-11-25T06:21:34Z

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

+        rs = ps.executeQuery();
+        try {
+          assertTrue(rs.next());
+          assertNull(rs.getObject(1));


a lot of code that uses assertTrue for rs.next() and assertEquals without a message.

That is unfortunate. New code should explain what it is trying to test.
I agree assertTrue(rs.next()); might be fine (I would agree even with plain rs.next() without assert), however assertNull(rs.getObject(1)); should explain why does it expect null there.

The thing is when test fails, it takes lots of time to figure out what was the intention behind the test and why did it fail.
Having decent messages greatly reduces maintenance costs.

vlsi · 2016-11-25T06:22:31Z

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

+        try {
+          assertTrue(rs.next());
+          // If an inappropriate caching occurred, then we'll get the data narrowing (TIMESTAMP -> DATE)
+          assertEquals(ts, rs.getObject(1));


The comment above this line should be moved into message argument of assertEquals. That would make error message much cleaner.

…haring test

igelbox · 2016-11-27T23:14:34Z

@vlsi thanks for you detailed answers.
I've added messages to assertNull and assertEquals methods.

macobo · 2016-12-09T10:36:52Z

Ping on this - I think we might be running into this scenario under some rare conditions.

vlsi · 2017-05-03T18:24:19Z

I'm sorry to say that, however I need postpone the review to the release after 42.1.0.

vlsi · 2018-03-08T19:30:29Z

I've been postponing this fix for quite a while now, and now I think the whole notion of "resolving parameter types" is odd (

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

Line 1770 in e442db1

params.setResolvedType(i + 1, queryOIDs[i]);

)
Current pgjdbc implementation parses the statement with Oid.UNSPECIFIED, receives the updated types from the backend and overwrites UNSPECIFIED with the DB results.

I would try the following approach:

On bind/"isGoodFor" side, use exact oids specified by the client. For instance, if the statement was prepared for Oid.UNSPECIFIED, then the statement would suit for Oid.UNSPECIFIED parameter. After statement execution, backend might send more proper datatype for the bind (e.g. Oid.DATE), then Oid.DATE is good as well for the bind.
On ParameterMetadata side, the types send by the DB should be used.

…ypes timestamp, date, are sent as UNSPECIFIED, and pgjdbc did not verify the actual parameter types at parse time. This might cause wrong results if the query was parsed with explicit type (e.g. setString(...)), and then re-executed with TIMESTAMP parameter (timestamps are sent as UNSPECIFIED, thus pgjdbc silently accepted the statement even though it should reparse) fixes pgjdbc#648 closes pgjdbc#674

…ypes (#1137) Timestamp, date (and some other types), are sent as UNSPECIFIED, and pgjdbc did not verify the actual parameter types at parse time. This might cause wrong results if the query was parsed with explicit type (e.g. setString(...)), and then re-executed with TIMESTAMP parameter (timestamps are sent as UNSPECIFIED, thus pgjdbc silently accepted the statement even though it should reparse) fixes #648 closes #674

igelbox added 2 commits October 30, 2016 07:52

test: inappropriate statement sharing

c167c16

when parameter type changed

fix: prevent inappropriate statement sharing

6f888f9

Check original parameter type when current parameter type is Oid.UNSPECIFIED. fixes pgjdbc#648

vlsi requested changes Nov 21, 2016

View reviewed changes

test: improve readability of the inappropriate statement sharing test

534c05a

igelbox force-pushed the issues/inappropriate-statement-sharing branch from 200061c to 534c05a Compare November 21, 2016 21:42

vlsi requested changes Nov 25, 2016

View reviewed changes

test: add more clear error messages for the inappropriate statement s…

7938dce

…haring test

vlsi added this to the 42.0.1 milestone Feb 12, 2017

vlsi modified the milestones: 42.2.0, 42.1.0 May 3, 2017

vlsi modified the milestones: 42.2.0, 42.2.1 Jan 16, 2018

vlsi force-pushed the master branch from 4a6d1d1 to 5819f31 Compare January 17, 2018 17:17

vlsi modified the milestones: 42.2.1, 42.2.2 Jan 23, 2018

vlsi mentioned this pull request Mar 9, 2018

fix: wrong results when a single statement is used with UNSPECIFIED types #1137

Merged

vlsi closed this in #1137 Mar 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inappropriate statement sharing #674

inappropriate statement sharing #674

igelbox commented Oct 30, 2016

codecov-io commented Oct 30, 2016 •

edited

Loading

vlsi left a comment

vlsi Nov 21, 2016

igelbox Nov 21, 2016

vlsi Nov 21, 2016

igelbox Nov 21, 2016

vlsi Nov 25, 2016 •

edited

Loading

igelbox commented Nov 21, 2016

vlsi Nov 25, 2016 •

edited

Loading

vlsi Nov 25, 2016 •

edited

Loading

igelbox commented Nov 27, 2016

macobo commented Dec 9, 2016

vlsi commented May 3, 2017

vlsi commented Mar 8, 2018

inappropriate statement sharing #674

inappropriate statement sharing #674

Conversation

igelbox commented Oct 30, 2016

codecov-io commented Oct 30, 2016 • edited Loading

Current coverage is 64.12% (diff: 60.00%)

vlsi left a comment

Choose a reason for hiding this comment

vlsi Nov 21, 2016

Choose a reason for hiding this comment

igelbox Nov 21, 2016

Choose a reason for hiding this comment

vlsi Nov 21, 2016

Choose a reason for hiding this comment

igelbox Nov 21, 2016

Choose a reason for hiding this comment

vlsi Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

igelbox commented Nov 21, 2016

vlsi Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

vlsi Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

igelbox commented Nov 27, 2016

macobo commented Dec 9, 2016

vlsi commented May 3, 2017

vlsi commented Mar 8, 2018

codecov-io commented Oct 30, 2016 •

edited

Loading

vlsi Nov 25, 2016 •

edited

Loading

vlsi Nov 25, 2016 •

edited

Loading

vlsi Nov 25, 2016 •

edited

Loading