diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c2e4da2d1..b8312f6e48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Lite's portable ZIP is self-contained, which HALVED it** ([#2501]) - `Publish Lite` is now `-r win-x64 --self-contained` in both `build.yml` and `nightly.yml`, so neither Lite artifact has a .NET prerequisite any more and the failure [#2489] documented stops existing: a tester who unzips onto a stock Windows Server no longer meets the .NET host's bare `You must install .NET to run this application` before a line of our code runs. **The size went the opposite way from what bundling a runtime suggests.** The old publish was RID-agnostic, so it copied every platform its packages ship - **537 MB of `runtimes\` on a 565 MB tree** (osx 130, linux-x64 116, linux-arm64 70, win-arm64 56, then win-x86, musl, loongarch64 and riscv64), of which only the **52 MB `win-x64`** folder could ever load on Windows. `DuckDB.NET.Bindings.Full` is most of it, SkiaSharp and SqlClient behind it. Dropping ~485 MB of unloadable native payload beats the cost of bundling .NET, WPF and ASP.NET Core by roughly two to one: measured on one commit and one SDK, **565 MB tree / 212.7 MB zipped becomes 277 MB / 114.2 MB**. It matters most for the **nightly** ZIP, which is the UAT download and is not offered as a `Setup.exe` at all. **A RID-specific publish needed two more files than the flag.** `Lite/packages.lock.json` had only a `net10.0-windows7.0` target, and a RID restore adds `net10.0-windows7.0/win-x64` to it - after which the `dotnet restore --locked-mode` that BOTH workflows run before the publish fails `NU1004: the project's runtime identifiers have changed`, because locked mode compares the PROJECT's RID set (empty) against the lock file's (win-x64). Reproduced locally; that is a red CI run on every PR, not the future `--no-restore` trap it was filed as. The fix is `win-x64` in `PerformanceMonitorLite.csproj`, so the project itself asks for that graph and one committed lock file satisfies the RID-less locked-mode restore and the RID publish alike; `RuntimeIdentifiers` (plural) sets no RID on the build, so a plain `dotnet build` stays RID-agnostic and `Lite.Tests` is untouched. **SignPath needed nothing** - the `Lite` artifact-configuration slug already receives both shapes today, and the signed re-zip reads `signed/Lite/*`, inheriting whatever shape `publish/Lite` has. Auto-update is unaffected; the ZIP is not a Velopack channel. `LiteRuntimePrerequisiteDocsTests` went red on the flag alone (3 of its 7 facts) and was rewritten to state every claim BOTH ways round: [#2499]'s version asserted only that the docs DID name the runtimes, so two of its facts stayed green while the prose went stale. It now also derives the lock file's RID coverage from the `-r` flags in the workflows, and every new assertion was proven red with its fix reverted. ### Fixed +- **query_store plan and text fetch: force HASH JOIN so the in-memory Query Store TVF is read once** ([#2791]) - `sys.query_store_plan` unions the on-disk table with the TVF `QUERY_STORE_PLAN_IN_MEM`, and the optimizer has no statistics for it: on AYR it estimated 1,000 rows against 14,633 actual (1,463% off). That guess put the TVF on the inner side of a Nested Loops join, re-executed once per candidate plan_id up to the 512 `MaxCandidatePlans` cap, each execution scanning the whole in-memory Query Store before any plan was decompressed. sp_QuickieStore put these statements at **55,000-61,000ms CPU each**, top of the whole instance, while the `TOP (50000)` runtime-stats payload this was long assumed to be averaged ~2s. `OPTION(RECOMPILE, HASH JOIN)` reads the TVF once: **~60,000ms CPU to 0.508s** measured on AYR. The same shape and the same fix apply to the text fetch, which joins two TVF-backed views directly. Rowsets verified byte-identical with and without the hint at 16/128/512 candidates under the collector's own ARITHABORT OFF. - **Web dashboard: the composer's partial-window notice read "1 days" on a one-day store** ([#2785]) - the day count in BuildRetentionNotice was interpolated with a hardcoded "days" plural, so a store reaching back a single day read "reaches back about 1 days, but the requested window starts 1 days back". It now pluralises on the rendered number - "1" singular, "1.5"/"30" plural. - **Web dashboard: an offline server no longer shows a green "Collectors OK"** ([#2779]) - the summary Collectors chip keyed its verdict on the FAILING count alone, and a stale collector counts as neither healthy nor failing, so a server that had stopped collecting kept a green "Collectors OK - N healthy - 0 failing" even while its own header read "no recent collection" and its Collection Health tab showed every collector STALE. The chip now reuses the reachability signal the card already carries (is_online, the same one that bands the card Offline): an offline server reads "Stale - no recent collection" in the neutral tone instead. One shared metricBands builder, so the fleet cards and the per-server detail header are both fixed at once - and no new stale-count threshold to over-fire on normally-infrequent collectors. - **Web dashboard: an over-long time range showed a raw API validation error instead of a range hint** ([#2780]) - the Range dropdown offers "last 30 days", but the per-server CPU/query reads cap at 168 hours and return an error for anything wider, which the web dashboard mounted verbatim ("hours_back value '720' exceeds maximum of 168 hours..."). A shared read-error renderer (used by every panel, inline or composite) now recognises that specific over-range error and renders a notice naming the window the view keeps ("This view keeps up to 168 hours (7 days) of history - pick a shorter range") rather than the API's developer wording. @@ -3103,6 +3104,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2764]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2764 [#2766]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2766 [#2785]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2785 +[#2791]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2791 [#2779]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2779 [#2776]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2776 [#2772]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2772 diff --git a/Darling/Darling.Tests/QueryStorePlanFetchTests.cs b/Darling/Darling.Tests/QueryStorePlanFetchTests.cs index 3a613186b0..cafb47a524 100644 --- a/Darling/Darling.Tests/QueryStorePlanFetchTests.cs +++ b/Darling/Darling.Tests/QueryStorePlanFetchTests.cs @@ -436,6 +436,52 @@ private static int CountOf(string haystack, string needle) private static string LiveSql(CollectorContext context) => QueryStoreCollector.Instance.BuildPerItemQuery(Db, context).Text; + /* --------------------------------------------------------------------------------------------- + #2791: the fetch statements read TVF-backed Query Store views, for which the optimizer has no + statistics and uses a fixed guess (1,000 estimated against 14,633 actual on AYR, 1,463% off). + That guess put QUERY_STORE_PLAN_IN_MEM on the INNER side of a Nested Loops join, re-executed + once per candidate id up to MaxCandidatePlans = 512, at 55,000-61,000ms CPU per fetch. The + query-level hint is the only lever - the joins live inside the view definition. + --------------------------------------------------------------------------------------------- */ + + [Fact] + public void PlanFetch_ForcesHashJoin_OnTheStatementThatReadsTheCatalogView() + { + var text = QueryStoreCollector.Instance + .BuildPlanFetchByIdsQuery("db", Context(capturePlanXml: true), new long[] { 1, 2, 3 }, 12_582_912).Text; + + Assert.Contains("OPTION(RECOMPILE, HASH JOIN)", text, StringComparison.Ordinal); + } + + [Fact] + public void TextFetch_ForcesHashJoin_ForTheSameReason() + { + var text = QueryStoreCollector.Instance + .BuildTextFetchByIdsQuery("db", Context(capturePlanXml: true, fetchTextSeparately: true), new long[] { 1, 2, 3 }, 12_582_912).Text; + + Assert.Contains("OPTION(RECOMPILE, HASH JOIN)", text, StringComparison.Ordinal); + } + + /// + /// The hint belongs ONLY on the statement that joins. The second statement of each builder reads the + /// #temp and joins nothing, so forcing a strategy there would be cargo-cult - and a query-level hint on + /// a joinless statement is the kind of thing that gets copied forward and later defended as load-bearing. + /// Exactly one hinted statement, exactly one plain RECOMPILE, in both builders. + /// + [Fact] + public void TheBudgetStatementsKeepPlainRecompile_BecauseTheyJoinNothing() + { + var plan = QueryStoreCollector.Instance + .BuildPlanFetchByIdsQuery("db", Context(capturePlanXml: true), new long[] { 1 }, 12_582_912).Text; + var text = QueryStoreCollector.Instance + .BuildTextFetchByIdsQuery("db", Context(capturePlanXml: true, fetchTextSeparately: true), new long[] { 1 }, 12_582_912).Text; + + Assert.Equal(1, plan.Split("HASH JOIN").Length - 1); + Assert.Equal(1, plan.Split("OPTION(RECOMPILE);").Length - 1); + Assert.Equal(1, text.Split("HASH JOIN").Length - 1); + Assert.Equal(1, text.Split("OPTION(RECOMPILE);").Length - 1); + } + private static CollectorContext Context(bool capturePlanXml, bool fetchTextSeparately = false) { var context = new CollectorContext diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index 1efbeab802..2d0b495b0f 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -1194,6 +1194,16 @@ EXECUTE [{escapedDbName}].sys.sp_executesql /// catalog, K=114 and a 12 MB budget, both shapes returned the same 114 rows and 1.7 MB, and the join-back /// form took 274ms cold / 262ms warm against 133ms for this one. Plan-id-only with no XML touched was 114ms, /// so this shape sits 19ms above the floor while the join-back form pays for the decompression twice. + /// + /// The obvious next idea — evaluate the byte budget against the COMPRESSED length first, so plans the + /// budget will discard are never decompressed at all — is not available through this view, and that was + /// measured rather than assumed (#2791). Over 512 candidate ids: selecting plan_id alone is 14ms, + /// DATALENGTH(qsp.query_plan) is 321ms, and a full CONVERT(nvarchar(max), ...) is 331ms. The + /// DATALENGTH form costs what the full decompression costs because the VIEW decompresses on any access to + /// the column, so there is no cheap size to filter on; the compressed blob lives in the undocumented + /// sys.plan_persist_plan, which is not a surface to ship against. The budget therefore bounds what + /// is SHIPPED, not what is decompressed, and that is a property of the catalog rather than a shortcoming + /// of this query. /// public CollectorQuery BuildPlanFetchByIdsQuery(string item, CollectorContext context, IReadOnlyList planIds, long budgetBytes) { @@ -1255,7 +1265,44 @@ forces a spool. The frame is per-row precisely because the cut has to fall betwe SET NOCOUNT ON so the SELECT INTO emits no result set: the caller's reader takes the first result set as the shipped rows, and a stray done-count row would derail it. The #temp is created inside this sp_executesql scope and dropped at the end - explicit DROP for clarity; it would auto-drop on - scope exit regardless. */ + scope exit regardless. + + HASH JOIN on the fetch statement (#2791), and it is the whole fix rather than a tuning knob. + sys.query_store_plan's view definition unions the on-disk table with the in-memory TVF + QUERY_STORE_PLAN_IN_MEM, and the optimizer has NO statistics for that TVF - it uses a fixed guess. + Measured on AYR the guess is 1,000 rows against 14,633 actual, 1,463% off, which is what makes + Nested Loops look cheap: the TVF lands on the INNER side and is re-executed once per candidate + plan_id, up to MaxCandidatePlans = 512 times, each execution scanning the whole in-memory Query + Store before a single plan is decompressed. sp_QuickieStore put this statement at 55,000-61,000ms + CPU, top of the entire instance. Under the hint it is a Hash Match reading the TVF ONCE: 0.508s. + + Query-level rather than per-join because the joins live inside the view definition and cannot be + hinted individually. That means it also applies to the Clustered Index Seek into plan_persist_plan + (91% of estimated cost), and the seek->scan trade this risks DOES happen: under the hint that + access becomes a Clustered Index Scan. It is a non-issue, and that is measured rather than argued + - the scan costs 0.032s, against 0.373s for the TVF and 0.436s for the Hash Match above it, and + the whole statement finishes in 0.508s on an 85%-full Query Store. Recorded in this direction on + purpose: the estimate says the seek is 91% of the cost and the actual plan says it is 6% of a + half-second, so the operator the estimate points at is not the one that matters. + + The cost of forcing it: a Hash Match needs a workspace memory grant where the Nested Loops it + replaces needs none, and that applies on EVERY invocation, including small ones the optimizer + would have served grant-free (review catch). Measured rather than waved through - grants on a + 40,882-plan Query Store, no spill in any case: + + 4 candidate ids : unhinted 0KB (pure loops) -> hinted 1,264KB + 512 candidate ids: unhinted 1,760KB (already a hash anyway) -> hinted 3,624KB + + So the hint does introduce a grant on small candidate sets, and it is ~1.2MB; at the other end + MaxCandidatePlans caps the input at 512 and the grant at ~3.6MB, where the optimizer was already + choosing a hash and paying 1.8MB of it regardless. The cap is what makes this bounded rather than + a function of Query Store size. Sweeps are concurrent across servers but sequential within one, so + the fleet-wide worst case is a few concurrent sweeps' worth, single-digit MB - against a statement + that was burning 55,000-61,000ms of CPU. If this were ever wrong it would show as RESOURCE_SEMAPHORE + waits or a hash spill on the monitored instance, neither of which appears here. + + The SECOND statement below keeps plain OPTION(RECOMPILE): it reads only #plan_fetch and joins + nothing, so there is no join strategy to force. Not an oversight - checked. */ var body = $@"SET NOCOUNT ON; SELECT @@ -1264,7 +1311,8 @@ scope exit regardless. */ query_plan_text = CONVERT(nvarchar(max), qsp.query_plan) INTO #plan_fetch FROM sys.query_store_plan AS qsp -WHERE qsp.plan_id IN ({idList}); +WHERE qsp.plan_id IN ({idList}) +OPTION(RECOMPILE, HASH JOIN); SELECT plan_id = b.plan_id, @@ -1362,7 +1410,19 @@ ids stay missing forever — a stall that looks like a quiet database. */ byte-budget cut is unchanged. SET NOCOUNT ON so the SELECT INTO emits no result set. #temp is scoped to this sp_executesql and - dropped at the end. ROWS UNBOUNDED PRECEDING for the same per-row-cut reason as the plan fetch. */ + dropped at the end. ROWS UNBOUNDED PRECEDING for the same per-row-cut reason as the plan fetch. + + HASH JOIN for the same reason as the plan fetch (#2791), and this one is a two-view join written + out in the open: sys.query_store_query and sys.query_store_query_text are BOTH TVF-backed unions + over their in-memory halves, driven by an IN list, which is exactly the shape that put the plan + fetch on the inner side of a loop 512 times over. The plan fetch is the variant that carries the + AYR measurement; this one is the same defect treated the same way, and that distinction is stated + rather than blurred - the join-strategy change and the identical-rowset property are verified + here, the 60s->0.5s number is not this statement's and is not claimed for it. + + Same memory-grant trade as the plan fetch above, and the same bound: the id list is capped by the + caller, so the hash input does not scale with Query Store size. Measured on the same 40,882-plan + catalog with no spill. */ var body = $@"SET NOCOUNT ON; SELECT @@ -1373,7 +1433,8 @@ SET NOCOUNT ON so the SELECT INTO emits no result set. #temp is scoped to this s FROM sys.query_store_query AS qsq JOIN sys.query_store_query_text AS qst ON qst.query_text_id = qsq.query_text_id -WHERE qsq.query_id IN ({idList}); +WHERE qsq.query_id IN ({idList}) +OPTION(RECOMPILE, HASH JOIN); SELECT query_id = b.query_id,