Fix high-impact SQL collector defects from Fable code review (chunk 5)#1106
Merged
Conversation
- 09 Query Store: plan_type and plan_forcing_type were stored in each other's columns. The dynamic SELECT emits plan_forcing_type (2017+ block) before plan_type (2022+ block), but the INSERT...EXEC column list had plan_type first; INSERT...EXEC binds by ordinal, so every Query Store row had the two swapped. Reordered the INSERT list to match the SELECT. - 23/25/28 (blocked-process / deadlock / system-health wrappers): when an explicit @procedure_database was passed, the proc stored QUOTENAME(name) (e.g. "[master]") in the *_database variable and then QUOTENAME'd it again at the EXECUTE site, producing "[[master]]]" — so the explicit-database path was always broken. The default (NULL) path stores the raw name. Now the explicit path also stores the raw name (quote-escaped via REPLACE) so the single call-site QUOTENAME brackets it once, consistent with the default path. - 06 ensure_collection_table self-heal created collect.server_properties without lock_pages_in_memory / instant_file_initialization_enabled / memory_dump_count (present in 02 and inserted by 53's collector), so a self-healed table failed every collection with "invalid column name". Added the three columns. Validation: all five scripts CREATE OR ALTER cleanly on SQL2016 (2.12.0.0), exit 0. 2017-2025 still need your version testing (those instances were down). Remaining collector findings flagged for follow-up: 29/31 dedup window shorter than the sweep (duplicate rows); 05 gauge metrics run through counter-delta math; 23 marks unparsed blocked-process rows processed past the cap; 38 trace-flag removal detection; 10 procedure_stats tempdb/function-branch filters; 08 first-run self-defeat; 11 raw @procedure_database literal; 27 memory_pressure self-heal unreachable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High-impact SQL collector fixes from the Fable code review (chunk 5 of 7). Validated on SQL2016 (the minimum supported version was the only instance reachable); 2017–2025 still need your version testing.
Fixes
[HIGH] Query Store
plan_type↔plan_forcing_typeswapped —install/09The dynamic SELECT emits
plan_forcing_type(2017+ block) beforeplan_type(2022+ block), but theINSERT...EXECcolumn list hadplan_typefirst.INSERT...EXECbinds by ordinal, so every Query Store row stored each value in the other's column. Reordered the INSERT list to match the SELECT.[HIGH] Double-
QUOTENAMEbroke the explicit-database path —install/23, 25, 28With an explicit
@procedure_database, the proc storedQUOTENAME(name)([master]) in the*_databasevariable and thenQUOTENAME'd it again at theEXECUTEsite →[[master]]]. The default (NULL) path stores the raw name. Now the explicit path also stores the raw name (quote-escaped viaREPLACE) so the single call-siteQUOTENAMEbrackets once — consistent with the default path. (TheOBJECT_IDexistence check keeps its ownQUOTENAME.)[MEDIUM]
server_propertiesself-heal drift —install/06ensure_collection_tablerecreatedcollect.server_propertieswithoutlock_pages_in_memory/instant_file_initialization_enabled/memory_dump_count(present in 02, inserted by 53's collector), so a self-healed table failed every collection with "invalid column name". Added the three columns.Validation
All five changed scripts
CREATE OR ALTERcleanly on SQL2016 (2.12.0.0), exit 0 (e.g. "Query Store collector created successfully").Remaining collector findings — flagged for follow-up (your version testing)
29/31 dedup window shorter than the collection sweep (recurring duplicate rows); 05 gauge metrics (memory_clerks, perfmon, blocking-max) run through monotonic-counter delta math → phantom spikes when values drop (needs
cntr_typegating for perfmon); 23 marks unparsed blocked-process rowsis_processed=1past the 1000-event cap (silent loss); 38 trace-flag removal detection compares to the last change-batch not last-known-state; 10 procedure_stats omits tempdb (dbid 2) and the FUNCTION branch ignores DB exclusions; 08 first-run check defeated by the TABLE_MISSING log row; 11 raw@procedure_databaseliteral (injection/quote); 27 memory_pressure self-heal unreachable (read in DECLARE before the TRY).🤖 Generated with Claude Code