Batch SQL Server per-database queries - #24950
Conversation
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f900b05f1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
evalya-impact-summaryevalya impact analysis |
0444867 to
310afc5
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0440bdf to
bc3cb25
Compare
|
I ran a temporary end-to-end test against a real Azure SQL Database. It asserted that Azure returned |
Review from sethsamuel is dismissed. Related teams and files:
- database-monitoring-agent
- sqlserver/tests/test_integration.py
Validation ReportAll 21 validations passed. Show details
|
|
I fixed a flaky test and it dismissed your review :-| |
What does this PR do?
Collapses two SQL Server operations that previously scaled with the number of autodiscovered databases:
sys.database_filesquery through[database].sys.sp_executesql, preserving each database's execution context, and the client drains the streamed result sets withnextset().The database file query still uses
FILEPROPERTY(name, 'SpaceUsed')inside each database, so it preserves currenttempdbsizes and does not requireVIEW ANY DEFINITION. Database names are supplied as an XML parameter and quoted withQUOTENAME. A database that becomes unavailable after autodiscovery is skipped so later databases are still collected.Azure SQL Database retains the direct database-scoped query because it does not support cross-database execution. Azure SQL Managed Instance uses the batched path.
Motivation
The previous implementation issued two SQL requests per autodiscovered database for each affected path. Both paths were benchmarked through the real SSH tunnel against the same SQL Server 2022 instance with 3,945 databases, with one workload running at a time:
This removes 15,779 requests (99.987%) and saves 644.283 seconds, or 10 minutes 44.3 seconds, per collection at this scale. The combined result is the arithmetic sum of separately measured components under the same tunnel and database population, not a separate end-to-end run.
The status benchmark ran the final path four times around one full legacy sweep. Final timings ranged from 0.334 to 0.425 seconds; the median was 0.353 seconds and the first row arrived in 0.134 to 0.186 seconds. A server-local control measured 2.086 seconds for the legacy path and 0.243 to 0.288 seconds for the final path, confirming that the large tunneled improvement comes primarily from eliminating round trips.
Correctness was compared against the original behavior across all 3,945 databases: every connection status matched, and database file coverage and every stable value matched across 7,897 rows. Restricted-permission probes also matched for accessible, inaccessible, and nonexistent databases. Database file collection was verified with
CONNECT ANY DATABASEbut notVIEW ANY DEFINITION, including rows from regular, partitioned, andtempdbdatabases. One-page changes observed in logspace_usedvalues followed whichever query ran later and were confirmed as live checkpoint activity rather than a code-path difference.Focused tests cover bounded query count, multi-result draining, and the Azure SQL Database fallback. Integration tests compare database file size, used space, and state with database-local
sys.database_filesresults and verify collection under restricted permissions. The full Linux ODBC/FreeTDS and Windows ODBC/OLE DB CI matrix passes.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged