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

fix: ensure executeBatch() does not use server-side prepared statements when prepareThreshold=0 #690

Merged
merged 4 commits into from Jan 20, 2017

Conversation

Projects
None yet
5 participants
@scubasau
Contributor

scubasau commented Nov 16, 2016

Prevents usage of preparedStatements in executeBatch when prepareThreshold is set to 0

@codecov-io

This comment has been minimized.

codecov-io commented Nov 16, 2016

Current coverage is 64.08% (diff: 100%)

Merging #690 into master will increase coverage by <.01%

@@             master       #690   diff @@
==========================================
  Files           165        165          
  Lines         15129      15129          
  Methods           0          0          
  Messages          0          0          
  Branches       2978       2978          
==========================================
+ Hits           9694       9695     +1   
+ Misses         4201       4196     -5   
- Partials       1234       1238     +4   

Powered by Codecov. Last update f48c6bb...6a6c21c

pstmt.executeUpdate();
pstmt.close();
// When using a prepareThreshold of 0, a batch update should not use server-side prepare

This comment has been minimized.

@Gordiychuk

Gordiychuk Nov 17, 2016

Contributor

Why not split this test into 2?

This comment has been minimized.

@scubasau

scubasau Nov 17, 2016

Contributor

No strong reason for keeping them together. I can split them apart if desired.

This comment has been minimized.

@Gordiychuk

Gordiychuk Nov 18, 2016

Contributor

It will be more readable, when one test test only one scenario. Thanks.

@vlsi vlsi changed the title from Fix for Issue 669 to fix: ensure executeBatch() does not use server-side prepared statements when prepareThreshold=0 Nov 25, 2016

@vlsi vlsi added this to the 9.4.1213 milestone Nov 25, 2016

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 25, 2016

@scubasau would you please check why the tests failed?

@scubasau

This comment has been minimized.

Contributor

scubasau commented Nov 25, 2016

@vlsi 88016d5 should have corrected the tests but build 1711 states it built from bd4109118bb693258aab4c54f187f16ce8830441 (which doesn't even seem to exist ??). The line numbers of the test failures match the failures in build 1709. Is there a way to force Travis to rebuild? I believe the branch should pass all tests if it builds from 88016d5

@Gordiychuk

This comment has been minimized.

Contributor

Gordiychuk commented Nov 25, 2016

@scubasau you can rebase you changes and force push to bransh, it trigger travis

@scubasau scubasau force-pushed the scubasau:issue669 branch from 88016d5 to bc2b605 Nov 25, 2016

@scubasau

This comment has been minimized.

Contributor

scubasau commented Nov 25, 2016

@Gordiychuk all set, thanks!

@@ -736,7 +736,7 @@ protected BatchResultHandler createBatchHandler(Query[] queries,
// If executing the same query twice in a batch, make sure the statement
// is server-prepared. In other words, "oneshot" only if the query is one in the batch
// or the queries are different
&& isOneShotQuery(null)) {
|| isOneShotQuery(null)) {
flags |= QueryExecutor.QUERY_ONESHOT;

This comment has been minimized.

@bd-infor

bd-infor Nov 29, 2016

Contributor

I don't think this change has the intended effect.
The first check in isOneShotQuery(CachedQuery cachedQuery) is "if (cachedQuery == null) { return true; }".
In other words, the entire "if (!sameQueryAhead || isOneShotQuery(null)) { flags |= QueryExecutor.QUERY_ONESHOT; } else { ... }" block is equivalent to an unconditional "flags |= QueryExecutor.QUERY_ONESHOT;" statement. The else block will never execute now.
Also, the original code is misleading because "if (!sameQueryAhead && isOneShotQuery(null))" is equivalent to "if (!sameQueryAhead && true)".
Note: If this change is not causing a test failure for queries that require "flags |= QueryExecutor.QUERY_FORCE_DESCRIBE_PORTAL", then there is a coverage gap that should be filled or there are no queries that need that flag set or the flag gets set somewhere else.

This comment has been minimized.

@vlsi

vlsi Nov 29, 2016

Member

@bd-infor thanks for reviewing, however you seem to miss that 'isOneShotQuery' is overridden at pg prepared statement level.

This comment has been minimized.

@bd-infor

bd-infor Nov 29, 2016

Contributor

Yes, I did. Sorry.

@vlsi vlsi merged commit aca26a0 into pgjdbc:master Jan 20, 2017

2 checks passed

codecov/project 64.08% (+<.01%) compared to f48c6bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment