From 5731ae90b096e02ce51fe92cb15301ad71687a98 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 22 Apr 2026 12:37:04 -0400 Subject: [PATCH 1/2] Loosen memory grant color thresholds + show compile time in Runtime panel (#215 C1, C4) (#261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1: memory grant utilization color was too harsh. Old thresholds said anything under 80% used was warn/bad, so a 61% grant showed orange. Joe's point: operators spill near their max grant, so moderate utilization is fine and even preferable; what we actually want to flag is significant over-granting (very low utilization, reserved memory wasted). New thresholds: >= 40% utilized: good (neutral / green) 20-39%: warn (orange) < 20%: bad (red) Applied in three places: - PlanViewerControl.EfficiencyColor (used for memory grant, DOP efficiency, and thread utilization — loosens all three consistently) - HtmlExporter memory card eff class - Index.razor memory card eff class C4: compile time is now shown as a plan-level property in the Runtime panel of all three surfaces regardless of whether Rule 19 fires, per Joe's point that compile time belongs in the category-B "plan-level" grouping. Previously only visible when compile CPU >= 1000ms via the warning. Small `Compile: Nms` row next to Elapsed/CPU/DOP. Version bump 1.7.4 -> 1.7.5. Co-authored-by: Claude Opus 4.7 (1M context) --- .../Controls/PlanViewerControl.axaml.cs | 14 +++++++++++--- src/PlanViewer.App/PlanViewer.App.csproj | 2 +- src/PlanViewer.Core/Output/HtmlExporter.cs | 4 +++- src/PlanViewer.Web/Pages/Index.razor | 9 ++++++++- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index 836a377..65b5b87 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -2800,9 +2800,12 @@ void AddRow(string label, string value, string? color = null) rowIndex++; } - // Efficiency thresholds: white >= 80%, yellow >= 60%, orange >= 40%, red < 40% - static string EfficiencyColor(double pct) => pct >= 80 ? "#E4E6EB" - : pct >= 60 ? "#FFD700" : pct >= 40 ? "#FFB347" : "#E57373"; + // Efficiency thresholds: white >= 40%, orange >= 20%, red < 20%. + // Loosened per Joe's feedback (#215 C1): for memory grants, moderate + // utilization (e.g. 60%) is fine — operators can spill near their max, + // so we shouldn't flag anything above a real over-grant threshold. + static string EfficiencyColor(double pct) => pct >= 40 ? "#E4E6EB" + : pct >= 20 ? "#FFB347" : "#E57373"; // Runtime stats (actual plans) if (statement.QueryTimeStats != null) @@ -2815,6 +2818,11 @@ static string EfficiencyColor(double pct) => pct >= 80 ? "#E4E6EB" AddRow("UDF elapsed", $"{statement.QueryUdfElapsedTimeMs:N0}ms"); } + // Compile time — plan-level property (category B). Show regardless of + // threshold so it's always visible, not just when Rule 19 fires. + if (statement.CompileTimeMs > 0) + AddRow("Compile", $"{statement.CompileTimeMs:N0}ms"); + // Memory grant — color by utilization percentage if (statement.MemoryGrant != null) { diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 815d970..d4b5e1d 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -6,7 +6,7 @@ app.manifest EDD.ico true - 1.7.4 + 1.7.5 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.Core/Output/HtmlExporter.cs b/src/PlanViewer.Core/Output/HtmlExporter.cs index fc9f54e..b38f131 100644 --- a/src/PlanViewer.Core/Output/HtmlExporter.cs +++ b/src/PlanViewer.Core/Output/HtmlExporter.cs @@ -308,6 +308,8 @@ private static void WriteRuntimeCard(StringBuilder sb, StatementResult stmt) WriteRow(sb, "CPU:Elapsed", ratio.ToString("N2")); } } + if (stmt.CompileTimeMs > 0) + WriteRow(sb, "Compile", $"{stmt.CompileTimeMs:N0} ms"); if (stmt.DegreeOfParallelism > 0) WriteRow(sb, "DOP", stmt.DegreeOfParallelism.ToString()); if (stmt.NonParallelReason != null) @@ -315,7 +317,7 @@ private static void WriteRuntimeCard(StringBuilder sb, StatementResult stmt) if (stmt.MemoryGrant != null && stmt.MemoryGrant.GrantedKB > 0) { var pctUsed = (double)stmt.MemoryGrant.MaxUsedKB / stmt.MemoryGrant.GrantedKB * 100; - var effClass = pctUsed >= 80 ? "eff-good" : pctUsed >= 40 ? "eff-warn" : "eff-bad"; + var effClass = pctUsed >= 40 ? "eff-good" : pctUsed >= 20 ? "eff-warn" : "eff-bad"; WriteRow(sb, "Memory", FormatKB(stmt.MemoryGrant.GrantedKB) + " granted"); sb.AppendLine($"
Used{FormatKB(stmt.MemoryGrant.MaxUsedKB)} ({pctUsed:N0}%)
"); } diff --git a/src/PlanViewer.Web/Pages/Index.razor b/src/PlanViewer.Web/Pages/Index.razor index 6d885d8..5b5b243 100644 --- a/src/PlanViewer.Web/Pages/Index.razor +++ b/src/PlanViewer.Web/Pages/Index.razor @@ -171,6 +171,13 @@ else } } + @if (ActiveStmt!.CompileTimeMs > 0) + { +
+ Compile + @ActiveStmt!.CompileTimeMs.ToString("N0") ms +
+ } @if (ActiveStmt!.DegreeOfParallelism > 0) {
@@ -188,7 +195,7 @@ else @if (ActiveStmt!.MemoryGrant != null && ActiveStmt!.MemoryGrant.GrantedKB > 0) { var pctUsed = (double)ActiveStmt!.MemoryGrant.MaxUsedKB / ActiveStmt!.MemoryGrant.GrantedKB * 100; - var effClass = pctUsed >= 80 ? "eff-good" : pctUsed >= 60 ? "eff-ok" : pctUsed >= 40 ? "eff-warn" : "eff-bad"; + var effClass = pctUsed >= 40 ? "eff-good" : pctUsed >= 20 ? "eff-warn" : "eff-bad";
Memory @FormatKB(ActiveStmt!.MemoryGrant.GrantedKB) granted From e05bc69f1484f64732e930b35c0fb7df33041ebc Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 22 Apr 2026 13:17:14 -0400 Subject: [PATCH 2/2] C8 expensive-operator carve-out + C9 columnstore on wide bare scans (#215) (#263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C9: Rule 34 (Bare Scan) previously only fired when the scan output was ≤3 columns, since rowstore nonclustered indexes get expensive with more columns. Joe's point: columnstore (CCI/NCCI) doesn't care about column count, so a wide bare scan with nothing to suggest is a missed recommendation. Rule 34 now also fires on wide outputs (>3 columns) with columnstore-focused advice. Narrow-output path keeps the NC-index advice as before. C8: new Rule 35 "Expensive Operator" — emits when an operator's self- time is ≥20% of statement elapsed AND no other warning has already fired on the node. Ensures large work doesn't disappear just because the tool has nothing specific to suggest (Joe's scan-with-nothing-to-say case). Benefit % = self-time share; severity Critical at ≥50%, Warning otherwise. Self-guarded against existing warnings so it never stacks on top of a spill/lookup/etc. Neither rule fires on Joe's private plan (every significant operator already has a warning) or on 20260415_1.sqlplan (parallel work split). That's the expected "no false positives" behavior. Version bump 1.7.5 -> 1.7.6. Co-authored-by: Claude Opus 4.7 (1M context) --- src/PlanViewer.App/PlanViewer.App.csproj | 2 +- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 58 +++++++++++++++++--- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index d4b5e1d..cacf6da 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -6,7 +6,7 @@ app.manifest EDD.ico true - 1.7.5 + 1.7.6 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 8969af5..653b224 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -921,21 +921,38 @@ _ when nonSargableReason.StartsWith("Function call") => ? GetOperatorOwnElapsedMs(node) > 0 : node.CostPercent >= 20; - if (colCount <= 3 && isSignificant) + if (isSignificant) { var scanKind = node.PhysicalOp == "Clustered Index Scan" ? "Clustered index scan" : "Heap table scan"; - var indexAdvice = node.PhysicalOp == "Clustered Index Scan" - ? "Consider a nonclustered index on the output columns (as key or INCLUDE) so SQL Server can read a narrower structure." - : "Consider a clustered or nonclustered index on the output columns so SQL Server can read a narrower structure."; - node.Warnings.Add(new PlanWarning + if (colCount <= 3) { - WarningType = "Bare Scan", - Message = $"{scanKind} reads the full table with no predicate, outputting {colCount} column(s): {Truncate(node.OutputColumns, 200)}. {indexAdvice} For analytical workloads, a columnstore index may be a better fit.", - Severity = PlanWarningSeverity.Warning - }); + // Narrow output: a nonclustered rowstore index can cover this cheaply. + var indexAdvice = node.PhysicalOp == "Clustered Index Scan" + ? "Consider a nonclustered index on the output columns (as key or INCLUDE) so SQL Server can read a narrower structure." + : "Consider a clustered or nonclustered index on the output columns so SQL Server can read a narrower structure."; + + node.Warnings.Add(new PlanWarning + { + WarningType = "Bare Scan", + Message = $"{scanKind} reads the full table with no predicate, outputting {colCount} column(s): {Truncate(node.OutputColumns, 200)}. {indexAdvice} For analytical workloads, a columnstore index may be a better fit.", + Severity = PlanWarningSeverity.Warning + }); + } + else + { + // Wider output: rowstore NC index isn't a great fit (would have to + // carry too many columns), but columnstore doesn't care about column + // count. Suggest it for analytical / aggregate-style workloads. + node.Warnings.Add(new PlanWarning + { + WarningType = "Bare Scan", + Message = $"{scanKind} reads the full table with no predicate, outputting {colCount} columns. A nonclustered rowstore index isn't a great fit for wide outputs, but if this is an analytical or aggregate-style query, a columnstore index (CCI or NCCI) can scan the same data far more cheaply — column count doesn't penalize columnstore the way it does rowstore indexes.", + Severity = PlanWarningSeverity.Warning + }); + } } } @@ -1229,6 +1246,29 @@ _ when nonSargableReason.StartsWith("Function call") => w.Message = $"Implicit conversion prevented an index seek, forcing a scan instead. Fix the data type mismatch: ensure the parameter or variable type matches the column type exactly. {w.Message}"; } } + + // Rule 35: Expensive Operator — always show operators that take a significant + // share of statement time even when no other rule has something to say. Joe + // (#215 C8) wanted expensive scans that the tool had nothing to suggest on + // to still surface as top items. Threshold: self-time >= 20% of statement + // elapsed. Only emits if no other warning is already on the node to avoid + // doubling up. The benefit % is just the self-time share. + if (!cfg.IsRuleDisabled(35) && node.HasActualStats && node.Warnings.Count == 0 + && stmt.QueryTimeStats != null && stmt.QueryTimeStats.ElapsedTimeMs > 0) + { + var selfMs = GetOperatorOwnElapsedMs(node); + var pct = (double)selfMs / stmt.QueryTimeStats.ElapsedTimeMs * 100; + if (pct >= 20.0) + { + node.Warnings.Add(new PlanWarning + { + WarningType = "Expensive Operator", + Message = $"{node.PhysicalOp} took {selfMs:N0}ms ({pct:N1}% of statement elapsed) but no specific rule identified a fix. Worth investigating: is the row volume necessary? Are upstream estimates driving this operator harder than it should be?", + Severity = pct >= 50 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning, + MaxBenefitPercent = Math.Round(Math.Min(100.0, pct), 1) + }); + } + } } ///