Fix multiple executeBatch issue when AE is enabled#1334
Fix multiple executeBatch issue when AE is enabled#1334lilgreenbird wants to merge 20 commits intomicrosoft:devfrom
Conversation
…ed by default (microsoft#1254) * skip AKV test properly * removed enclave properties string to failed errors as enclave tests could be skipped
|
|
||
| static String numericPrecisionTable[][] = {{"Float", "float"}, {"Decimal", "decimal"}, {"Numeric", "numeric"}}; | ||
|
|
||
| static String intTable[][] = {{"int", "int"}}; |
There was a problem hiding this comment.
This variable seems like it's never used?
| ++numBatchesPrepared; | ||
| needsPrepare = doPrepExec(tdsWriter, batchParam, hasNewTypeDefinitions, hasExistingTypeDefinitions); | ||
| if (needsPrepare || numBatchesPrepared == numBatches) { | ||
| if (reqStarted || needsPrepare || numBatchesPrepared == numBatches) { |
There was a problem hiding this comment.
I think this solution is a roundabout solution to the problem (which might also cause other regressions). The reason why we are getting an assertion error is because in AE enabled cases, we're retrieving the parameter encryption metadata twice in the second iteration of execute batch. Not only is this second retrieval of parameter encryption metadata hurting performance, it had the side effect of making the driver think that the request had been completed (getting the parameter encryption metadata counts as a command, which sets the requestComplete (and other flags) to true, which causes assertion error when we get to the actual execution of the prepared statement). You can check this behavior in the image below from the SQL profiler:
If you were to compare this profiler result to the non-AE path or the solution I've provided below, you'll see that the driver will no longer retrieve the paramter metadata twice.
My suggested solution is to instead call encryptionMetadataIsRetrieved = true; in line 2744 of this file, so that we only retrieve the param metadata once, which I believe is the correct way to solve this problem.
Let me know once you've confirmed if the fix will work (and tested it internally as well), so I can make a jar out of this PR and see if it solves the NPE that the user is experiencing from issue #1360.
|
Continued in PR #1378 |
3 participants
fixes issue 1301