Cache sp_columns_170 availability per connection in getColumns() - #3019
Cache sp_columns_170 availability per connection in getColumns()#3019Muskan Gupta (muskan124947) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves SQLServerDatabaseMetaData#getColumns() by avoiding repeated failed attempts to call sp_columns_170 on servers where it does not exist (SQL Server < 2025), caching the procedure’s availability as a per-connection tri-state on SQLServerConnection, and cleaning up the Azure DW fallback flow.
Changes:
- Cache
sp_columns_170availability per physical connection (tri-state) to avoid repeated probe failures and reduce server load. - Refactor duplicated prepare/bind/execute logic into helpers for both non-Azure DW and Azure DW code paths.
- Add tests intended to validate the “probe at most once per connection” behavior and connection scoping of the cache.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java | Implements per-connection caching and refactors getColumns() execution/fallback logic (including Azure DW handling). |
| src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java | Adds a volatile tri-state flag with accessor methods to cache sp_columns_170 support per connection. |
| src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java | Adds tests to validate probing/caching behavior and connection scoping for sp_columns_170. |
Suppressed comments (1)
src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java:2103
- This assertion assumes the sp_columns_170 support flag is always decided after the first getColumns() call. However, recordSpColumns170Failure intentionally leaves the state undetermined (null) for non-2812 failures (timeouts/transient errors), so this can be flaky. Consider asserting the cached value only when a "procedure not found" fallback was observed.
// Once the probe has run, the connection must have a decided state rather than remaining undetermined.
assertNotNull(getSpColumns170SupportedFlag(sqlServerConnection),
"sp_columns_170 support should be cached on the connection after the first getColumns() call");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3019 +/- ##
============================================
- Coverage 60.74% 60.29% -0.46%
+ Complexity 5332 5183 -149
============================================
Files 153 153
Lines 36679 36709 +30
Branches 6733 6738 +5
============================================
- Hits 22282 22132 -150
- Misses 10738 10759 +21
- Partials 3659 3818 +159 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sp_columns_170 only exists on SQL Server 2025 and later. getColumns() probed it on every call, so on older servers each call produced a failed server request before falling back to sp_columns_100. Importing a catalog of N tables therefore issued N failed requests. Availability is a property of the server, so the outcome is now cached on SQLServerConnection as a tri-state flag (null = undetermined, TRUE = present, FALSE = absent). sp_columns_170 is probed at most once per connection and later calls on a server without it go straight to sp_columns_100. The try/catch around the probe is kept so the driver still recovers if the procedure is unavailable unexpectedly. The flag is only cached as FALSE when the server explicitly reports error 2812 (Could not find stored procedure). Any other failure leaves the state undetermined so a transient error cannot permanently downgrade the connection to sp_columns_100 and silently drop metadata for types that only sp_columns_170 reports. Also stop wrapping buildAzureDWResultSet() in the fallback try block on Azure DW, where a failure while building the result set incorrectly triggered a fallback, and factor the duplicated prepare/bind/execute blocks into helpers. Fixes #3013 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d6a904a-1e9d-44f7-90bf-5a251e71dc30
Keep the cached sp_columns_170 state unchanged for failures other than error 2812, so a transient error no longer discards support that has already been proven. Count only the procedure-not-found fallback in the probe test, and read the cached state through the package private accessor instead of the private field. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d6a904a-1e9d-44f7-90bf-5a251e71dc30
Adds two tests that seed the connection-scoped sp_columns_170 state to FALSE and verify getColumns() goes straight to sp_columns_100, on both the regular SQL Server path and the Azure DW path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6d6a904a-1e9d-44f7-90bf-5a251e71dc30
53ca356 to
ee5383c
Compare
sp_columns_170 only exists on SQL Server 2025 and later. getColumns() probed it on every call, so on older servers each call produced a failed server request before falling back to sp_columns_100. Importing a catalog of N tables therefore issued N failed requests.
Availability is a property of the server, so the outcome is now cached on SQLServerConnection as a tri-state flag (null = undetermined, TRUE = present, FALSE = absent). sp_columns_170 is probed at most once per connection and later calls on a server without it go straight to sp_columns_100.
The try/catch around the probe is kept so the driver still recovers if the procedure is unavailable unexpectedly. The flag is only cached as FALSE when the server explicitly reports error 2812 (Could not find stored procedure). Any other failure leaves the state undetermined so a transient error cannot permanently downgrade the connection to sp_columns_100 and silently drop metadata for types that only sp_columns_170 reports.
Also stop wrapping buildAzureDWResultSet() in the fallback try block on Azure DW, where a failure while building the result set incorrectly triggered a fallback, and factor the duplicated prepare/bind/execute blocks into helpers.
Fixes #3013