Repository navigation
The store accepts a known plan digest for a row and keeps that plan alive (inert, #5158) - #5174
Merged
Merged
Conversation
… it alive (inert) Adds ICollectorRowWriter.PayloadOrDigest(content, knownDigest), defaulting to Value(content). PgCollectorRowWriter overrides it: a known digest with no content writes the digest alone to the row's digest column, with no plan text and no dim insert, and the digest joins the batch's touch set. PayloadDimensionWriter.FlushAsync runs one statement per dimension table for the touch set in the batch's own transaction. It refreshes last_seen only past the same 6-hour guard as the insert path, so the dimension GC's liveness invariant is unchanged, and it returns the digests that have no dim row. No collector passes a digest yet, so nothing changes at runtime.
…cannot deadlock - TouchSql locks with FOR UPDATE SKIP LOCKED: the upsert's conflict arm and the touch have no shared lock order, so a waiting touch could close a cycle and roll back a whole batch. A live fact crosses two connections' upserts and touches under a 2 s lock timeout. - A known digest at a position with no diverted payload throws instead of writing a NULL plan. - FlushAsync documents that its absent list is upper-case hex; a fact round-trips a lower-case digest. - The test wrapper forwards the collector's own digest ahead of its stored one. - TouchSql's doc notes that a digest another uncommitted batch is inserting can be reported absent.
…writer # Conflicts: # PerformanceMonitor.Collectors/ICollectorRowWriter.cs
erikdarlingdata
marked this pull request as ready for review
October 5, 2026 05:25
This was referenced Oct 5, 2026
This was referenced Oct 5, 2026
erikdarlingdata
added a commit
that referenced
this pull request
Oct 5, 2026
…behind queryStatsDeferredPlanFetch (on) (#5158) (#5209) query_stats fetches plan XML only for plans this host hasn't stored, behind queryStatsDeferredPlanFetch (on) (#5158) Fixes #5158 for query_stats; procedure_stats follows separately. The third of three PRs: #5173 added the collector's deferred fetch and #5174 the store's digest-only write; this wires them together and turns them on. - Every query_stats run rendered sys.dm_exec_text_query_plan for its ~200 rows and shipped the XML, though almost all were already stored: 53.75 GB/h rendered and 28.83 GB/h shipped on one large store against 1.14 GB/h distinct, with the busiest server's slow runs more than 90% result drain. - PlanDigestCache<TKey>, one per server for the service's lifetime, keyed for query_stats on QueryStatsPlanKey (server, database, plan_handle, statement offsets, creation_time, plan_generation_num). An entry is confirmed only after the store commit succeeds, including after a store-write reattempt; digests the store reports missing are evicted ignoring case; entries unseen for an hour are pruned. - For query_stats with plan capture on, the main query runs without plan columns. On the same target connection and within the same per-item budget, a cache hit carries the stored digest and size (an over-cap hit the size with a NULL plan), and misses go to one BuildPlanFetchQuery call, split above 1,000 keys. A failed second query ships the rows without plans, caches nothing and logs one Warning. No store probe runs first; the host's confirmed commits are the authority. Azure SQL Database keeps the inline capture. - queryStatsDeferredPlanFetch in darling.json, default true, read at service start; false gives today's inline SQL exactly. Documented in the README. Each run records plans_rendered, plans_rendered_bytes and plan_fetch_ms in its collection log note. - Accepted differences: a statement recompile between the two queries can store the new plan under the old identity for that one row; a plan the engine updates in place without a recompile keeps its first-rendered XML until the identity changes; after a restart each server renders once in full. - Tests: PlanDigestCacheTests (identity sensitivity, confirm-after-commit, over-cap, case-insensitive eviction, the split, plan_fetch_ms) and QueryStatsDeferredPlanFetchLiveTests against SQL Server 2022 and the store (run 2 renders no cached plan and resolves through v_query_stats; sp_recompile re-fetches; a plan freed between the queries; an over-cap plan kept by size).
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
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.
Part of #5158. This is the second of three PRs, and it changes no behavior: no collector passes a digest yet.
Why
#5158 stops
query_statsfrom re-rendering plans the store already holds. A row whose plan the host has already stored will carry only the plan's content digest. The store has to write that digest straight into the row, with no content, and keep the plan's dimension row from aging out while rows still reference it.What changes (
PerformanceMonitor.Darling.Storage)PgCollectorRowWriter.PayloadOrDigest(content, knownDigest):Value(content)path;PayloadDimensions.DigestLengthBytes(32) bytes. So does a known digest at a payload position with no dimension diversion, which would otherwise write a row with no plan, silently.PayloadDimensions.Digest's own output, held by the host. Nothing re-derives it.PayloadDimensionBatchkeeps the touch set per dimension table, deduplicated and in digest order.IsEmptycounts it.PayloadDimensionWriter.FlushAsyncruns one more statement per dimension table, in the batch's transaction (PayloadDimensions.TouchSql):last_seenfor the touched digests older than the existing 6-hour guard. It usesFOR UPDATE SKIP LOCKED: the upsert and the touch are two locking statements, so no single lock order covers both, and two batches that each upsert one stale digest and touch the other's would otherwise deadlock. Skipping a row is safe, because whoever holds it is refreshing it (an upsert, another touch, or query_store's touch). A holder that rolls back is retouched next cycle. The dimension sweep can't be the holder, because it only reaches rows at least the guard plus a day old;FlushAsyncreturns those as a list of upper-case hex digests (compare case-insensitively) and logs their count at Debug. This PR doesn't act on the list; the next PR uses it to forget those digests, so the plan is fetched again. A digest still being inserted by another, uncommitted batch can be reported absent, which costs one extra render.last_seenrefreshed at least every 6 hours, which is the same guard the insert path uses. That keeps it inside the dimension sweep's liveness window, as it is today.Tests (
PayloadDigestPassThroughLiveTests, live store, 8 facts)v_query_statsthe same way a row written with content does: one dimension row, two fact rows on one digest.last_seenstays put; at 6 h 01 m it moves to the batch time.lock_timeoutat 2 s and no sleeps.SKIP LOCKEDfails the two-batch fact with a lock timeout (55P03).SECTION: None (inert; the user-facing entry ships with the PR that turns it on)