Plan Viewer "Key Lookup" not showing right icon/text#413
Merged
erikdarlingdata merged 1 commit intoerikdarlingdata:devfrom Mar 4, 2026
Merged
Conversation
ClaudioESSilva
changed the title
erikdarlingdata
approved these changes
Mar 4, 2026
Owner
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Good fix — matches SSMS behavior for Key Lookup / RID Lookup display.
One minor note: the Contains check to prevent redundant logical op display (e.g., "Key Lookup (Clustered) (Key Lookup)") is a bit broad — it'll suppress any logical op that's a substring of the physical op. Probably fine in practice since SQL Server operator names don't typically overlap that way.
LGTM.
erikdarlingdata
added a commit
that referenced
this pull request
Mar 4, 2026
The Key Lookup parser fix (PR #413) changes PhysicalOp from "RID Lookup" to "RID Lookup (Heap)". The analyzer rule used exact equality which no longer matched. Changed to StartsWith. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6 tasks
erikdarlingdata
added a commit
that referenced
this pull request
Mar 4, 2026
The Key Lookup parser fix (PR #413) changes PhysicalOp from "RID Lookup" to "RID Lookup (Heap)". The analyzer rule used exact equality which no longer matched. Changed to StartsWith. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Mar 6, 2026
* Fix #410 (#411) * Fix #412 (#413) * GUI installer: log installation history to config.installation_history (#414) The GUI installer read from installation_history for version detection but never wrote to it, causing subsequent upgrades to fail to detect the prior install. Adds LogInstallationHistoryAsync mirroring the CLI installer's existing LogInstallationHistory method. Fixes #409 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Feature/long running queries config settings (#415) * Added exclusions in GetLongRunningQueriesAsync() method for SP_SERVER_DIAGNOSTIC wait types, and WAITFOR wait types. * Added TOP parameter for query in GetLongRunningQueriesAsync() method to allow future configurability on the number of long-running queries returned. Added optional parameter to control display of WAITFOR types in future. * Replaced waitForFilter string constructor with C# string interpolation instead of janky string addition. * Added exclusion for backup-related waits to GetLongRunningQueriesAsync() method. Removed System.Collections.Generic using statement as it is unnecessary. * Reverted Controls/LandingPage.xaml.cs * Added BROKER_RECEIVE_WAITFOR wait type to waitforFilter exclusions. Added miscWaitsFilter to exclude XE_LIVE_TARGET_TVF waits. Removed unused parameters. Corrected minimum value for maxLongRunningQueryCount (minimum 1 instead of 5). * Added filtering for SP_SERVER_DIAGNOSTICS, WAITFOR, BROKER_RECEIVE_WAITFOR, BACKUPTHREAD, BACKUPIO, and XE_LIVE_TARGET_TVF wait types in Lite/Services/LocalDataService.WaitStats.cs * Add configurable max results setting for long-running queries Adds LongRunningQueryMaxResults to UserPreferences (default 5) and exposes it in the Settings UI alongside the existing duration threshold. Threads the value through GetAlertHealthAsync and GetLongRunningQueriesAsync to replace the hardcoded TOP(5) in the DMV query. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Apply max results validation and Lite parity for long-running queries Adds Math.Clamp(1, int.MaxValue) guard to GetLongRunningQueriesAsync in both Dashboard and Lite, and updates the Dashboard settings validation to use an explicit range check with a descriptive error message. Mirrors the LongRunningQueryMaxResults setting across the Lite project: adds the App property, loads/saves it to settings.json, exposes it in the Lite Settings UI, and passes it through to GetLongRunningQueriesAsync to replace the hardcoded LIMIT 5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Use GetInt64() when loading long-running query max results from JSON Prevents an OverflowException if the value in settings.json is outside the int32 range. The value is read as long, clamped to [1, int.MaxValue], then cast back to int. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add configurable long-running query filter toggles Replaces hardcoded wait type exclusions in GetLongRunningQueriesAsync with user-configurable booleans for SP_SERVER_DIAGNOSTICS, WAITFOR / BROKER_RECEIVE_WAITFOR, backup waits, and miscellaneous waits. All four filters default to true (existing behavior preserved). Settings are exposed in the Notifications section of both Dashboard and Lite Settings UIs and persisted to UserPreferences / settings.json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * merged with incoming dev branch * Parameterized TOP/LIMIT value in Dashboard/Services/DatabaseService.NocHealth.cs and Lite/Services/LocalDataService.WaitStats.cs, and clamped the upper bound of the value to 1000 to avoid foot shooting. Removed blank lines as per Erik's request. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Sync plan viewer fixes from plan-b: spool labels, unmatched index detail (#416) - Spool operators now show Eager/Lazy prefix (e.g., "Eager Index Spool" instead of just "Index Spool") by prepending from LogicalOp - PlanIconMapper entries added for all Eager/Lazy spool variants - UnmatchedIndexes warning now parses child Parameterization elements to show specific database.schema.table.index names Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fixes many warnings, and pre-calculates the RegEx patterns at compile time instead of at runtime: warning CA1847: Use 'string.Contains(char)' instead of 'string.Contains(string)' when searching for a single character (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1847) warning CA1875: Use 'Regex.Count' instead of 'Regex.Matches(...).Count' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1875) warning CA1863: Cache a 'CompositeFormat' for repeated use in this formatting operation (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1863) (#346) Co-authored-by: Orestes Zoupanos <orestes.zoupanos@tpr.gov.uk> * Complete GeneratedRegex conversion and remove Compiled flags (#420) PR #346 left Lite PlanAnalyzer with 3 old-style regex fields and inline Regex.IsMatch calls, and added unnecessary RegexOptions.Compiled to all GeneratedRegex attributes (source generator always compiles, flag is ignored). - Convert remaining Lite regex fields to use GeneratedRegex source generator - Convert all inline Regex.IsMatch calls to GeneratedRegex in both apps - Remove RegexOptions.Compiled from all [GeneratedRegex] attributes - Keep both PlanAnalyzer copies in sync with identical regex methods Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Add permissions section to README with least-privilege setup (#421) Documents required permissions for all platforms: on-prem Full/Lite, Azure SQL Database (contained user), Azure SQL MI, and AWS RDS. Includes copy-paste SQL scripts. Updates comparison table and troubleshooting to link to the new section. Prompted by user question in issue #418. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Replace custom TrayToolTip with plain ToolTipText to fix crash The custom visual TrayToolTip (Border + TextBlock) triggers a known race condition in Hardcodet.NotifyIcon.Wpf where Popup.CreateWindow throws "The root Visual of a VisualTarget cannot have a parent." This crash poisons the ReaderWriterLockSlim on the UI thread, breaking Overview queries permanently until restart. Plain string ToolTipText avoids the WPF Popup infrastructure entirely. Fixes #422 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add resilience to DuckDB read lock acquisition If an unhandled exception leaks a read lock on the UI thread, subsequent EnterReadLock() calls throw LockRecursionException permanently. Catch this and return a no-op disposable instead, since the thread already has read protection in place. This makes the lock self-healing: even if a crash leaks the lock, subsequent operations recover gracefully rather than failing forever. Fixes #423 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Restore custom TrayToolTip and silently handle Hardcodet crash (issue #422) Reverts the ToolTipText approach which caused context menu positioning and theme regressions. Instead, keeps the original custom TrayToolTip for proper dark theme styling and popup anchoring, and adds IsTrayToolTipCrash() detection in both apps' DispatcherUnhandledException handlers to silently swallow the rare Hardcodet VisualTarget race condition without showing error dialogs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix incorrect table name in Data Retention section README referenced config.retention_settings which doesn't exist. Retention is configured via the retention_days column in config.collection_schedule. Same bug reported in #223 but the fix never actually landed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix RID Lookup analyzer rule to match new PhysicalOp label (#429) The Key Lookup parser fix (PR #413) changes PhysicalOp from "RID Lookup" to "RID Lookup (Heap)". The analyzer rule used exact equality which no longer matched. Changed to StartsWith. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Add uninstall option to CLI and GUI installers (#431) Adds complete uninstall capability that removes all server-level objects: - 3 SQL Agent jobs (Collection, Data Retention, Hung Job Monitor) - 2 Extended Events sessions (BlockedProcess, Deadlock) - Server-side traces - PerformanceMonitor database Changes: - New install/00_uninstall.sql standalone script for SSMS users - CLI: --uninstall flag with interactive confirmation - GUI: Uninstall button (red, enabled when installed version detected) - Fix: existing clean install now removes all 3 jobs + XE sessions (previously missed Hung Job Monitor and both XE sessions) blocked process threshold (s) is intentionally NOT reset during uninstall as other monitoring tools may depend on it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * LOB compression + deduplication for query stats tables (#419) Databases were growing to 150-200 GB in under a week on busy servers. LOB columns (query_text, query_plan_text, query_sql_text, compilation_metrics) were 92-94% of storage. COMPRESS()/DECOMPRESS() achieves 90-91% reduction. Schema changes: - query_stats, query_store_data, procedure_stats: LOB columns changed from nvarchar(max)/xml to varbinary(max) with COMPRESS() on write - Dropped unused query_plan xml columns - Added row_hash binary(32) for deduplication - Added tracking tables (query_stats_latest_hash, procedure_stats_latest_hash, query_store_data_latest_hash) for efficient hash lookups Collector changes (08, 09, 10): - COMPRESS() on all text/plan INSERT expressions - collect_query/collect_plan flag support added to query_stats and procedure_stats - HASHBYTES('SHA2_256', binary_concat) dedup: only INSERT rows where metrics changed - MERGE source deduped with ROW_NUMBER() to prevent duplicate key errors Read-side changes: - All reporting views (46, 47) wrap compressed columns in DECOMPRESS() - Dashboard C# queries updated with DECOMPRESS() in SQL strings Upgrade path: - Batched DELETE WITH OUTPUT migration compresses existing data in place - IDENTITY reseed preserves ID continuity - Old tables renamed to _old for safety (manual DROP later) Version bump: 2.1.0 → 2.2.0 across all four projects. Tested: clean install (sql2016), CLI upgrade (sql2017, sql2019, sql2022), GUI upgrade (sql2025). All collectors healthy, dedup validated, all views and MCP tools return readable decompressed data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add RESTORING database filter to waiting_tasks collector (#430) The waiting_tasks collector joins sys.databases without filtering d.state = 0 (ONLINE), which means sessions running in the context of a RESTORING database (mirroring passive/AG secondary) can pass through to sys.dm_exec_sql_text and sys.dm_exec_text_query_plan. While the dumps in #430 turned out to be an internal SQL Server 2016 background thread crash (not our collector), this hardens the waiting_tasks collector to match the pattern already used in query_stats, procedure_stats, query_store, and file_io_stats collectors (PR #385). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add CI check to require version bump on PRs to main Ensures the Dashboard.csproj version has been bumped before merging dev into main. Only runs on PRs from dev to main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Restore commercial support tiers to README The procurement/compliance support tiers were accidentally removed during the README restructure in PR #377. Restores the Supported ($500/yr) and Priority ($2,500/yr) tiers alongside the existing sponsorship and consulting sections. Reported in #436. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add wait stats query drill-down (#372) Right-click any wait type in the chart legend to see queries causing it. Classifies waits into categories (correlated, chain, uncapturable, filtered) and shows the appropriate data: correlated metrics for brief waits like SOS_SCHEDULER_YIELD, head blockers for lock waits (LCK_M_*), and direct wait-type filtering for everything else. Chain mode shows the same full column set with blocking path prepended — no jarring layout changes. Both Dashboard and Lite. No new collectors needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix poison wait false positives and alert log parsing (#445) The Lite poison wait query had no time filter, so stale data from days/weeks ago with high avg waits kept triggering alerts indefinitely. Added a 10-minute window matching Dashboard's existing filter. Also fixed alert history logging: non-numeric display strings (poison wait, LRQ, TempDB, job alerts) failed double.TryParse and logged as 0/0. Added optional numeric parameters to TrySendAlertEmailAsync so call sites can pass actual values for the DuckDB alert log while keeping display strings for emails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Cláudio Silva <claudiosil100@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Hannah Vernon <hannah@mvct.com> Co-authored-by: Orestes <MisterZeus@users.noreply.github.com> Co-authored-by: Orestes Zoupanos <orestes.zoupanos@tpr.gov.uk>
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
2 participants
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.
What does this PR do?
Fix #412
Which component(s) does this affect?
How was this tested?
Check #412
Checklist
dotnet build -c Debug)