Detect changed context is just ...

[TobiasSQL]

https://github.com/Microsoft/mssql-jdbc/blob/e0e70a429aa6f84d151e5a18aef8b92f3337e269/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerStatement.java#L806

+@brettwooldridge to keep me honest.

The checkIfContextChanged method in SQLServerStatement has a bunch of issues. My understanding is that this was added to handle the case where prepared handles cannot be re-used because the server context has changed. However, first off the implementation is very flaky, for ex. if I have a stored procedure called AddUser it will be detected as a context change. This also adds a bunch of overhead in terms of string manipulation & comparison with very little gain (most of the time the context will not have changed).

The previous implementation (that I did) attempted to re-create the handle if the context changed. There are pros and cons here, but I think that or simply throwing an error are the only reasonable alternatives. IMHO, the cons against this current approach are:

1. Not guaranteed to work, driver will not have the context and logic of the server to be accurate
2. Adds a bunch of overhead for a fairly edge case scenario

Thoughts?
/Tobias

[brettwooldridge]

@TobiasSQL My eyes are burning! 😱

Let see:

• How many times can we call toUpperCase()? Each call horribly inefficient if the SQL is several hundred lines (not uncommon in some apps).
• How many times can we call contains() on each one of those? Again each call horribly inefficient, because contains() as no choice but to scan the entire string to the end.

The answer is apparently 4x toUpperCase() and 4x contains(), which will generally all execute because it is a chain of || operators.

I see no reason not to kill it. The proposal outlined above, of simply throwing, seems reasonable.

[TobiasSQL]

Thanks @brettwooldridge , I share your sentiment exactly.

[TobiasSQL]

I will add that the reason I went with re-try in the first implementation is that otherwise it would be a breaking change. I.e. someone not using cached handles would not have gotten and exception and now they do. I verified internally that we can make it safe to re-try.

[xiangyu-sun-789]

Hello @TobiasSQL The reason to remove the re-try logic was due to errors that were introduced by it in some scenarios. The new implementation is definitely slower but prevents from inconsistent behaviors and errors. In the original PR #543 it's also mentioned that this implementation is definitely slower in some cases. In my humble opinion, having something working slower is better than causing errors... But yes, it will be much better if there will be a way to fix the previous re-try logic without performance impact.

[cheenamalhotra]

Hi @TobiasSQL and @brettwooldridge

Thank you for bringing up the concern with us. We re-evaluated he risks involved and agree with your recommendations. The current implementation would degrade performance to a lot extent for many customers executing large queries. We agree with that.

Since we are nearing stable release of the driver, we agreed to make below changes to the driver right away and proceed with the following:

• Remove above implementation of "checkIfContextChange" method
• Reintroduce Retry-Logic for recreating lost handle.
• Disable Caching of Prepared Statements by default so it does not impact customers if they upgrade driver directly in their applications. (retain connection property 'disableStatementPooling' = 'true')
• We would also list down the limitation of this feature in release notes as we will address known issue with retry logic in future releases i.e. double execution of queries in special case where PreparedStatement query comprises of multiple statements in single query, and one of them raises error codes used to trigger retry attempts.

We also think that splitting sp_prepexec calls to sp_prepare and sp_execute while executing Prepared Statement Queries would resolve above issue and exception can be captured in sp_prepare stage without executing the queries. Please correct me if I misinterpreted the issue or solution.

It would be really helpful if you can provide us your views on the listed approach for upcoming release.

[TobiasSQL]

Thanks @cheenamalhotra, most of this makes good sense to me :-) A few comments:

1. For the re-try logic I would suggest evaluating if the problems found were mainly in the "batch"-execute path or in both. If this is batch-related I suggest disabling re-try in the batch case.
2. Also, need to make sure only to re-try from an execution where the driver decided to call sp_execute, not for sp_executesql or any other case (i.e. based on context).
3. I think it makes sense to default the feature to off per your suggestion.
4. On your sp_prepare question I don't think this makes sense. The error would never occur when calling sp_prepexec, only on calls to sp_execute. So the only thing this would do is add another (unnecessary) round trip.

Please cc me and @brettwooldridge on the PR.

[brettwooldridge]

@TobiasSQL The hacker in me couldn't let it go...

While the tweak of the existing code (below) looks hairy, it:

• Virtually eliminates false positives like "AddUser".
• Performs in a single pass over the string.
• Does not convert the string to upper case.
• Is extremely narrow about when the checks are applied.

When I measured this on a couple of large SQL statements, several kilobytes long, it benchmarked at a couple of hundred nanoseconds.

The logic is:

• Scan looking for a space.
• When a space is found, look back three characters to see if that character is a "U" or "S".
• If not, continue scanning.
• If so, we have a likely candidate for "USE " or "SET ".
• Perform a deeper comparison.

Just thought I'd throw this out there, as sometimes the simplest solutions end up being the most stable...

void checkIfContextChanged(final String sql) {
    if (connection.contextIsAlreadyChanged) {
        connection.contextChanged = true;
        return;
    }

    for (int i = 3; i < sql.length() - 3; i++) {
        if (sql.charAt(i) == ' ') {
            char ch = Character.toUpperCase(sql.charAt(i - 3));
            if (ch != 'U' && ch != 'S') {  // not candidate "USE " or "SET "
                continue;
            }

            // look backwards ... was this "USE "?
            if (sql.regionMatches(true /*ignoreCase*/, i - 3, "USE", 0, 3)) {
                connection.contextIsAlreadyChanged = true;
                connection.contextChanged = true;
                return;                    
            }
            // else likely "SET "

            // find next non-whitespace
            for (i++; i < sql.length() - 4 && Character.isWhitespace(sql.charAt(i)); i++);

            if (sql.regionMatches(true /*ignoreCase*/, i, "ANSI_NULLS", 0, "ANSI_NULLS".length()) ||
                sql.regionMatches(true /*ignoreCase*/, i, "DEFAULT_SCHEMA", 0, "DEFAULT_SCHEMA".length()) ||
                sql.regionMatches(true /*ignoreCase*/, i, "QUOTED_IDENTIFIER", 0, "QUOTED_IDENTIFIER".length())) {

                connection.contextIsAlreadyChanged = true;
                connection.contextChanged = true;
                return;
            }
        }
    }
}

Still has the false positive exposure ... for example, the text "recuse yourself" would trigger due to a space preceded by "use". But maybe acceptable?

(Expressions like "ANSI_NULLS".length() above are completely optimized away by the compiler into a constant like 10, their inclusion merely helps readability).

UPDATE:

char ch = sql.charAt(i - 3);
if (ch != 'U' && ch != 'u' && ch != 'S' && ch != 's') {  // not candidate "USE " or "SET "

ends up being more efficient than:

char ch = Character.toUpperCase(sql.charAt(i - 3));
if (ch != 'U' && ch != 'S') {  // not candidate "USE " or "SET "

slightly less readable, but understandable with the comment.

[TobiasSQL]

I love the passion @brettwooldridge! :-)

My sense would still be to avoid this since we are trying to read the "mind" of the server. I.e. if things change on the server and handle does not need to be discarded the driver is still "optimizing". My $.02.

What is your take?

[brettwooldridge]

Understood.

[AfsanehR-zz]

Hi @TobiasSQL , @brettwooldridge
We are in the process of reverting back the retry logic implementation.
Attached you can find the issues we found with the retry logic initially which led us to remove it and switch to context change detection.
Would appreciate to get your feedback and thoughts here:
https://gist.github.com/AfsanehR/53f3d6bcab785694162a2016e157682d#file-gistfile1-txt

The current dev branch does not have the retry logic, you can use version V6.3.4 for testing.

[TobiasSQL]

Thanks @v-afrafi , let me know when the dev-branch is in the right place and I can take a look. The duplicate insert issue is what I eluded to above, that is easily fixed based on context for retry.

[TobiasSQL]

@v-afrafi, I did an implementation on v6.3.4. It is very simple: for the retry logic you basically never retry if a prepare needs to happen.

In SQLServerPreparedStatement do the following:

1. In the two places where doPrepExec is called you save the return value (needsPrepare)
2. Modify retryBasedOnFailedReuseOfCachedHandle to take needsPrepare as a 3rd argument and always return false if needsPrepare is true.

[AfsanehR-zz]

@TobiasSQL Thanks Tobias! That fixed the issue 🎉
Please let us know your feedback on the newly created pr #618 which also includes the fix you mentioned!

[TobiasSQL]

Thanks @v-afrafi , submitted one comment. Thanks for the quick turn around!

[AfsanehR-zz]

@TobiasSQL I found another issue similar to the one I posted above, here is a sample:
https://gist.github.com/AfsanehR/2e90f325008423cdc0eb92e30eea100c

There is a fix that if we add these at the following code lines will fix the issue:
https://github.com/Microsoft/mssql-jdbc/pull/618/files#diff-a51517a0e4bf7e029ed04f9476a37f89R551
https://github.com/Microsoft/mssql-jdbc/pull/618/files#diff-a51517a0e4bf7e029ed04f9476a37f89R2700
https://github.com/Microsoft/mssql-jdbc/pull/618/files#diff-a51517a0e4bf7e029ed04f9476a37f89R2731

 PreparedStatementHandle cachedHandle = connection
                        .getCachedPreparedStatementHandle(new Sha1HashKey(preparedSQL, preparedTypeDefinitions, dbName));
                if (retryBasedOnFailedReuseOfCachedHandle(e, attempt, needsPrepare) && null == cachedHandle)
                    continue;

The reason is that needsPrepared at the fifth iteration returns false, but we are doing retry again after raiseError happens.
The fix will look up in the cache for that query, and if exists won't retry. I am still investigating how much it will affect the retryLogic in total. It looks like it again disables it this way?

[cheenamalhotra]

Hello Tobias,

Would like to know your thoughts on the above issue before we take a call to implement the fix as per our analysis.

Appreciate your response soon, since we are nearing release schedule.

[TobiasSQL]

Hi, I get a 404 error when trying to go to https://gist.github.com/v-afrafi/2e90f325008423cdc0eb92e30eea100c

[cheenamalhotra]

Hi Tobias,
Please try: https://gist.github.com/AfsanehR/2e90f325008423cdc0eb92e30eea100c

[TobiasSQL]

Thanks, I see. The problem here is that the way we (I) test is busted given we now retry. We need to change things a bit, I suggest the following:

1. Remove the special-casing of error number 99586 (in retryBasedOnFailedReuseOfCachedHandle and the test-cases)
2. In the test do the following:
3. Prepare a statement with a simple INSERT
4. Execute once and check that one row is now in the table
5. Get the handle number (need to be exposed through a getter if not already) and separately do an executeStatement with sp_unprepare of that handle. This will cause the next execution of the prepared statement to fail and it should now retry correctly.
6. Check that only two rows were inserted in the table.

Something along these lines :-)

[cheenamalhotra]

Thanks @TobiasSQL . understood. Looks like a test scenario issue, we could not reproduce it with context change that we thought originally. We will be merging the pr soon.

[cheenamalhotra]

Closing issue as requested changes merged to driver.