Ensure that batchParamValues is cleared in all cases when using bulk update mode#1768
Ensure that batchParamValues is cleared in all cases when using bulk update mode#1768ianroberts wants to merge 1 commit intomicrosoft:mainfrom
Conversation
…atch via bulk update. Closes microsoft#1767 Signed-off-by: Ian Roberts <[email protected]>
|
|
|
Hi @ianroberts, Thank you for the report and for the potential fix in the pull request. We'll take a look at both and get back to you as soon as we can. |
Jeffery-Wasty
requested review from
Jeffery-Wasty,
VeryVerySpicy,
lilgreenbird and
tkyc
Jeffery-Wasty
left a comment
Jeffery-Wasty
left a comment
There was a problem hiding this comment.
@ianroberts We'll be taking a closer look at this at a later date.
| for (int i = 0; i < batchParamValues.size(); ++i) { | ||
| updateCounts[i] = 1; | ||
| } | ||
| try { |
There was a problem hiding this comment.
Simply adding batchParamValues = null to a finally clause should be enough, no need for an additional try block.
| return updateCounts; | ||
| loggerExternal.exiting(getClassNameLogging(), "executeBatch", updateCounts); | ||
| return updateCounts; | ||
| } finally { |
There was a problem hiding this comment.
This can be moved to 2133, batchParamValues = null should be executed in all cases so it makes sense for it to be in a finally block belonging to the initial try block.
There was a problem hiding this comment.
No, there are a number of checks inside the outer try block that can throw IllegalArgumentException in order to abort the bulk copy and fall back to the regular batch insert method. I introduced the inner try in order to avoid clearing out batchParamValues until we know for sure that we're committed to the bulk copy code path and won't be backing out to try again with a batch insert.
There was a problem hiding this comment.
Alternative approach could be to remove both the additional inner try I've added here and the one at line 2296 in the "old style batch insert" branch, and instead wrap the whole method body in an outer try/finally that ensures batchParamValues is cleared in all cases immediately before the method returns. That's a bigger diff with all the indentation changes but might be clearer overall.
There was a problem hiding this comment.
I've made a separate PR #1869 for the alternative approach, which is also rebased against the latest main.
| for (int i = 0; i < batchParamValues.size(); ++i) { | ||
| updateCounts[i] = 1; | ||
| } | ||
| try { |
There was a problem hiding this comment.
Same comments as above.
| return updateCounts; | ||
| loggerExternal.exiting(getClassNameLogging(), "executeLargeBatch", updateCounts); | ||
| return updateCounts; | ||
| } finally { |
|
Let's just keep all changes in one PR, #1869 , even if they're 'alternate scenarios' we can just choose which commit we'd like to merge. |
Jeffery-Wasty
removed
the
Under Investigation
2 participants
Closes #1767