Skip to content

Fix/querystore grid performance#146

Open
rferraton wants to merge 2 commits intoerikdarlingdata:devfrom
rferraton:fix/querystore-grid-performance
Open

Fix/querystore grid performance#146
rferraton wants to merge 2 commits intoerikdarlingdata:devfrom
rferraton:fix/querystore-grid-performance

Conversation

@rferraton
Copy link
Contributor

What does this PR do?

  • Try to improve performance of QS grid query

    • remove filter IS NULL on Plan
    • use a temp table for elected plan top + filter
    • remove plan_text and plan_string from temp table
    • use left join with query anq query text to allow filter if any but join pruning if no filter on sql text or plan hash
    • for wait stats push plan_id elected in a temp table and filter with this temp table
  • add 2 progress bars for waits stats gloabl and grid

  • fix some issue with timerange selector handles

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

2026-03-25_17h51_25

Remarks

I found the queries slower than on SSMS. should check the config. Fetch size maybe ?

Describe the testing you've done. Include:

  • Plan files tested : Query Store Only
  • Platforms tested : Windows

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

Copy link
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Thanks for the work here, @rferraton — there are several genuinely good improvements in this PR. However, the query changes in QueryStoreService.cs conflict with PR #145 (already merged to dev), which rewrote the main grid query using sp_QuickieStore patterns. Merging this as-is would regress that work.

Query changes — conflicts with #145

PR #145 already shipped a 4-phase materialized temp table approach:

  • #intervals with clustered PK + EXISTS semi-join for time filtering
  • #plan_stats with clustered PK for plan-level aggregation
  • OPTION(RECOMPILE) on aggregation phases to prevent parameter sniffing
  • TRY_CONVERT for safe plan XML handling
  • Single join to text/plan tables for the final TOP N winners

This PR's query changes would overwrite all of that with:

  • CTE-based plan_agg (not materialized, no clustered PK)
  • No interval pre-filtering temp table
  • No RECOMPILE hints
  • Double-joins to query/query_text/plan — once into #top_plans for filters, again in the final SELECT for text/plan
  • LEFT OUTER JOINs that aren't needed (Query Store has referential integrity, and the optimizer eliminates unused INNER JOINs)
  • Final SELECT is missing ORDER BY — results won't be in expected sort order

Good parts we'd like to keep

These improvements are valuable and should be applied on top of current dev:

1. Wait stats split into global vs per-planFetchGlobalWaitStatsOnlyAsync + FetchPerPlanWaitStatsAsync is smart architecture. Starting global wait stats early (before the grid query completes) is a genuine parallelization win.

2. Plan ID temp table for per-plan wait stats — passing visible plan IDs via #plan_ids so we only fetch wait stats for the TOP N plans is a good optimization.

3. Progress bars — grid loading overlay and wait stats loading indicator are nice UX additions.

4. Handle hit zones — transparent rectangles at left/right slicer handles to fix pointer event interception.

Suggested path forward

Could you rebase onto current dev (which has #145's query) and drop the QueryStoreService.cs query rewrite? Keep the wait stats split, plan ID filtering, progress bars, and handle hit zones. That way we get all your UI improvements without regressing the query performance work.

Alternatively, we can extract the good parts on our side if that's easier for you. Let us know which you'd prefer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants