Codecov Report

Merging #648 into dev will decrease coverage by <.01%.
The diff coverage is 54.28%.

@@             Coverage Diff              @@
##                dev     #648      +/-   ##
============================================
- Coverage     47.96%   47.95%   -0.01%     
- Complexity     2580     2582       +2     
============================================
  Files           113      114       +1     
  Lines         26596    26592       -4     
  Branches       4454     4455       +1     
============================================
- Hits          12756    12752       -4     
- Misses        11703    11706       +3     
+ Partials       2137     2134       -3

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 4f1b3e1...f713515. Read the comment docs.

@cheenamalhotra , I took a look at this. Have two questions and a suggestion.

Question 1: I am missing something on the isBatch addition you made. If needsPrepare is true we never want to re-try given that the error cannot be due to missing handle, no? What am I missing here?

Question 2: Why are we adding dbName to the source for the statement handle hash? This seems closely akin to trying to interpret the SET-options. Are we sure that dbName will always have an impact here, and that it can be trusted to be accurate?

On the suggestion side I think we can simplify the remove handle ref. logic. This isn't something new in your pull request but given we are starting to have more and more checks on cachedPreparedStatementHandle with regards to clean-up I thought it may be a reasonable thing to do. Basically my suggestion is to make PreparedStatementHandle.removeReference private given it is all handled by SQLServerConnection.returnCachedPreparedStatementHandle and then on the SQLServerPreparedStatement side of the house move all clean-up logic into resetPrepStmtHandle.

Something along these lines:

    private boolean resetPrepStmtHandle(boolean discardCurrentCacheItem) {

        PreparedStatementHandle handle = cachedPreparedStatementHandle;

        boolean statementPoolingUsed = null != handle; 

        // Return to pool and decrement reference count
        if (statementPoolingUsed) {
            // Make sure the cached handle does not get re-used more.
            if(discardCurrentCacheItem)
                handle.setIsExplicitlyDiscarded();
    
            connection.returnCachedPreparedStatementHandle(handle);
        }

        // Remove references
        cachedPreparedStatementHandle = null; 			
        prepStmtHandle = 0;

        return statementPoolingUsed;
    }

Now in closePreparedHandle we will simply do this for statement pooling:

           // Handle unprepare actions through statement pooling.
            if (resetPrepStmtHandle(false)) {
                // Done
            }
            // If no reference to a statement pool cache item is found handle unprepare actions through batching @ connection level. 
            else ...

And in reuseCachedHandle we can do:

// New type definitions and existing cached handle reference then deregister cached handle.
// If current cache item should be discarded make sure it is not used again.
if(hasNewTypeDefinitions || discardCurrentCacheItem) {
        resetPrepStmtHandle(discardCurrentCacheItem);
        if(discardCurrentCacheItem)
                return false;
}
		
// Check for new cache reference.

Thoughts?

Ping?

Hi Tobias, I was away last week and looking into the changes now. Will update you soon!

Hi @TobiasSQL

Sorry it took a while to get this working. Answers to your questions:

• Question 1: Allowing retry if batch is processed by isBatch check.
  So the reason why I added this check was because we were getting some exceptions when batches were being processed with caching enabled. As you know when we discussed Detect changed context is just ... #610 the reason why we wanted to throw exception if needsPrepare is true is in this test case where exec sp_execute or RAISEERROR is called inside SQL query that should go for retry of handle creation. Except that if a batch call is sent internally, with a prepared handle to statement, during executeBatch call, that still qualifies for retrying handle creation. The use case that fails is very complex but I can say allowing this retry does fix all scenarios where exceptions are thrown in batch processing. I also tested different possibilities where this would fail, but till now I haven't discovered anything like that. Please let me know if you find a scenario that would fail with this change.
• Question 2: This was added to address exception scenarios, by limiting handle sharing between different databases but since we have a better fix of not discarding handles, I tested with removed 'dbName' and everything looks good.

I did push changes as suggested by you recently, I could not incorporate the exact method as you mentioned above due to different conditions where resetPrepStmtHandle was called, but most of it I tried to taken in.

Request you to review the PR changes once more, and share your thoughts.

@TobiasSQL Please review changes if you find some time!

Thanks, Cheena.