Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix prepared statement metadata cache issue #543

Merged
merged 14 commits into from
Nov 17, 2017

Conversation

xiangyushawn
Copy link
Contributor

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

@codecov-io
Copy link

codecov-io commented Nov 7, 2017

Codecov Report

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

Impacted file tree graph

@@             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.

@cheenamalhotra cheenamalhotra added this to the 6.3.5 milestone Nov 14, 2017
@TobiasSQL
Copy link
Contributor

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
Copy link
Contributor Author

xiangyushawn commented Nov 16, 2017

@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.


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

@AfsanehR-zz AfsanehR-zz Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xiangyushawn
Copy link
Contributor Author

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

@xiangyushawn xiangyushawn merged commit 7e1f663 into microsoft:dev Nov 17, 2017
@xiangyushawn xiangyushawn deleted the fix-metadata-cache-2 branch November 17, 2017 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants