Fix prepared statement metadata cache issue by xiangyushawn · Pull Request #543 · microsoft/mssql-jdbc

xiangyushawn · 2017-11-07T00:01:37Z

also added tests to verify the number of insertion/update executed.

… fix for BatchTriggerTest

codecov-io · 2017-11-07T00:22:41Z

Codecov Report

Merging #543 into dev will decrease coverage by 0.16%.
The diff coverage is 79.31%.

@@             Coverage Diff              @@
##                dev     #543      +/-   ##
============================================
- Coverage     46.68%   46.51%   -0.17%     
+ Complexity     2228     2214      -14     
============================================
  Files           108      108              
  Lines         25308    25321      +13     
  Branches       4175     4175              
============================================
- Hits          11815    11779      -36     
- Misses        11463    11522      +59     
+ Partials       2030     2020      -10

Flag	Coverage Δ	Complexity Δ
#JDBC41	`46.28% <79.31%> (-0.17%)`	`2201 <15> (-12)`
#JDBC42	`46.4% <79.31%> (-0.18%)`	`2210 <15> (-14)`

Impacted Files	Coverage Δ	Complexity Δ
...t/sqlserver/jdbc/SQLServerConnectionPoolProxy.java	`15.1% <100%> (+0.44%)`	`14 <0> (ø)`	⬇️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	`45.75% <57.14%> (-0.23%)`	`277 <2> (ø)`
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	`51.24% <80.59%> (-0.33%)`	`159 <10> (-5)`
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java	`59.65% <83.33%> (+0.18%)`	`133 <3> (+2)`	⬆️
...ooglecode/concurrentlinkedhashmap/LinkedDeque.java	`28.46% <0%> (-3.08%)`	`20% <0%> (-2%)`
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	`39.61% <0%> (-2.6%)`	`44% <0%> (-2%)`
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	`51.11% <0%> (-1.49%)`	`11% <0%> (-1%)`
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	`43.56% <0%> (-1.36%)`	`103% <0%> (-2%)`
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`53.49% <0%> (-0.37%)`	`0% <0%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c75147...1992eed. Read the comment docs.

…ching is disabled on the connection

…otherwise throw sql server exception

TobiasSQL · 2017-11-16T14:58:11Z

This SQLServerConnection.contextChanged thing, do we really believe that we can keep this accurate? There are lots of reasons for the context on the server to change, trying to figure this out from the client side seems like a losing proposition. This was why we went with re-try logic instead, obviously with its own issues. What are the thoughts here?

xiangyushawn · 2017-11-16T17:18:56Z

@TobiasSQL Thank you so much for checking the code. I did talk to our team about the limitation of this SQLServerConnection.contextChanged thing. I didn't want to send another query to SQL Server to retrieve the status of ANSI_NULLS, QUOTED_IDENTIFIER etc., which would affect the performance of the driver. Instead, I decided to detect keywords passed into the driver. The limitation is: if user calls 'use DB1' twice, the driver still assumes the context is changed although context didn't change; also, if user has keywords as text in a statement, the driver would think the context is changed. I think the chance of having those scenarios is low. At least, having it works with slower performance in some scenarios is acceptable in my opinion, unless we can have better solutions. Also, context is connection based. I tested it. If I establish a connection with SSMS and change the context there, it wont affect the driver (in other words, it wont cause handle not valid error on the driver), since they use different connections.

AfsanehR-zz · 2017-11-16T19:03:53Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java

-
+        if (definitionChanged || connection.contextChanged) {
+            prepStmtHandle = -1; // so that hasPreparedStatementHandle() also returns false
+            return false;


we should also add these two in here, do you agree?

if (connection.contextChanged) { connection.contextChanged = false; // to be able to use the handle after this call connection.clearCachedPreparedStatementHandle(); //making sure the cache is cleared }

Thank you so much for the suggestion, it's much better.

xiangyushawn · 2017-11-17T01:22:17Z

@TobiasSQL Hello, the team has decided to merge this PR. Please feel free to leave comments after it's merged.

xiangyushawn added 7 commits November 3, 2017 13:55

fix handle not fund bug for metadata caching

5940cfc

fix tests

97a5cfc

fix failures in requesthandling test

9ffd521

remove prepStmtHandle = -1 which disables metadata caching, and added…

2cbd065

… fix for BatchTriggerTest

remove fix for BatchTriggerTest, it breaks PreparedStatementTest

ab68580

fix hasPreparedStatementHandle()

5c74751

add the fix for batch trigger

9a54b6b

This was referenced Nov 7, 2017

Process all responses before proceed and fix retry logic for metadata caching #436

Closed

use matrix to test against both SQL Server 2016 and 2008 #428

Merged

xiangyushawn added 6 commits November 6, 2017 17:35

fix "handle is not valid" error. If context changed once, metadata ca…

705ebc1

…ching is disabled on the connection

clear cached prepared statement handle when pooled connection is removed

3ccdf0f

merge the fix of batch trigger

8559a33

change back the test to catch BatchUpdateException exception

1839edc

disable metadata caching after switching database on connection directly

875f62a

process all batch first, then throw batch exception is there is one, …

df89f02

…otherwise throw sql server exception

cheenamalhotra requested a review from AfsanehR-zz November 14, 2017 20:36

cheenamalhotra added this to the 6.3.5 milestone Nov 14, 2017

xiangyushawn requested a review from TobiasSQL November 15, 2017 21:39

AfsanehR-zz reviewed Nov 16, 2017

View reviewed changes

re-enable metadata caching after context change

1992eed

AfsanehR-zz approved these changes Nov 17, 2017

View reviewed changes

xiangyushawn merged commit 7e1f663 into microsoft:dev Nov 17, 2017

xiangyushawn deleted the fix-metadata-cache-2 branch November 17, 2017 01:22

tubbynl mentioned this pull request Jan 5, 2018

Implement PreparedStatement caching for performance #166

Closed

xiangyu-sun-789 mentioned this pull request Jan 26, 2018

Detect changed context is just ... #610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prepared statement metadata cache issue#543

Fix prepared statement metadata cache issue#543
xiangyushawn merged 14 commits intomicrosoft:devfrom
xiangyushawn:fix-metadata-cache-2

xiangyushawn commented Nov 7, 2017

Uh oh!

codecov-io commented Nov 7, 2017 •

edited

Loading

Uh oh!

TobiasSQL commented Nov 16, 2017

Uh oh!

xiangyushawn commented Nov 16, 2017 •

edited

Loading

Uh oh!

AfsanehR-zz Nov 16, 2017 •

edited by xiangyushawn

Loading

Uh oh!

xiangyushawn Nov 16, 2017

Uh oh!

xiangyushawn commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

xiangyushawn commented Nov 7, 2017

Uh oh!

codecov-io commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TobiasSQL commented Nov 16, 2017

Uh oh!

xiangyushawn commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AfsanehR-zz Nov 16, 2017 • edited by xiangyushawn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangyushawn Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

xiangyushawn commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Nov 7, 2017 •

edited

Loading

xiangyushawn commented Nov 16, 2017 •

edited

Loading

AfsanehR-zz Nov 16, 2017 •

edited by xiangyushawn

Loading