Repository navigation
Measure tempdb space against its ceiling, not its current allocation (#2515) - #2521
Conversation
…2515) TempDbSpaceInfo.UsedPercent divided by total_reserved + unallocated, and both halves come from dm_db_file_space_usage, which reports the data files as CURRENTLY ALLOCATED. That is distance to the next autogrow. It reads as real headroom on a pre-sized on-prem box only because such a tempdb has already grown to its cap. On GP_S_Gen5_2: four 16 MB data files, 62.44 MB allocated, max_size 2,097,152 pages each for a 65,536 MB ROWS ceiling. One ~57 MB #temp table read 95.7% full against the allocation and 0.09% against the cap, so at the shipped 80% default enabling Azure tempdb collection (#2512) would have armed an alert that fires on the first busy minute of every Azure target. The ceiling is discoverable, so this is a denominator correction rather than a policy call. tempdb_stats now collects max_size_mb = SUM(max_size) over tempdb's ROWS files only - dm_db_file_space_usage reports DATA allocation, so folding the log's cap in would understate usage everywhere - and both SKUs divide by it. Where any data file is unlimited (-1) there is no ceiling to measure against and the allocation stays the denominator, which is today's behaviour, so unlimited-growth on-prem and RDS targets do not move. Deliberately not a size floor: a floor suppresses the alert on a genuinely full large tempdb at the moment it starts growing, still fires at the autogrow boundary once cleared, and any value silently redefines 'small' for every on-prem and RDS target already relying on today's behaviour. The analyze_server TEMPDB_USAGE fact carried its own copy of the same denominator and is fixed with it, so the pager and the analysis pass cannot describe one server two ways. The alert detail gained a Max Size field naming which denominator produced the percentage. Darling schema v81, Lite schema v56. The column is nullable with no backfill; a historical row's NULL reads as 'no ceiling measured' and reports the percentage it always did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <summary> | ||
| /// Seeds tempdb_stats across 16 collection points. | ||
| /// </summary> | ||
| /// <summary> | ||
| /// <paramref name="maxSizeMb"/> is the #2515 growth ceiling: null seeds NULL, which is what every row | ||
| /// collected before the v56 migration looks like and the state the callers here want by default. | ||
| /// </summary> |
There was a problem hiding this comment.
Minor doc nit: this is two separate /// <summary>...</summary> blocks stacked back-to-back rather than one merged block. C# only recognizes a single <summary> element per member for IntelliSense/doc generation, so the second block (the maxSizeMb explanation) won't render where intended. Suggest merging into one summary, e.g.:
| /// <summary> | |
| /// Seeds tempdb_stats across 16 collection points. | |
| /// </summary> | |
| /// <summary> | |
| /// <paramref name="maxSizeMb"/> is the #2515 growth ceiling: null seeds NULL, which is what every row | |
| /// collected before the v56 migration looks like and the state the callers here want by default. | |
| /// </summary> | |
| /// <summary> | |
| /// Seeds tempdb_stats across 16 collection points. <paramref name="maxSizeMb"/> is the #2515 growth | |
| /// ceiling: null seeds NULL, which is what every row collected before the v56 migration looks like | |
| /// and the state the callers here want by default. | |
| /// </summary> |
There was a problem hiding this comment.
Fixed in 038d0bd — merged into one summary, same shape as your suggestion. DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks caught the same site in the Darling PostgreSQL job, so this was a red build rather than a nit; the two of you found it independently.
|
Reviewed the denominator fix end-to-end (collector T-SQL, both stores' migrations, both alert-read adapters, the shared Verified correct:
One nit left as an inline comment: Nothing else stood out — no correctness bugs, no Lite/Darling drift, no security concerns (no dynamic SQL/string concatenation introduced), and no missing-index-DMV suggestions to make. |
… Server check DocCommentHygieneTests caught a doc block I appended above an existing one rather than into it: SeedTempDbAsync carried two <summary> elements, so XML docs would take the LAST and a human reading top-down gets the first. Merged into one. The review bot found the same site independently. Also, having run the collector's shipped query text against a real SQL Server 2022 (8 tempdb data files) rather than only against fixtures, the doc comment now records what it returned: unlimited files give -1; capping the eight data files at 100 MB and the LOG at 2,048 MB gives 800.00, so the log's cap really is excluded rather than merely intended to be; returning one data file to UNLIMITED takes the whole answer back to -1. Two accuracy fixes while here: the V81 rung's view-refresh comment said the frozen list was eleven columns (tempdb_stats has twelve), and the live test's -1 note claimed a mishandled sign would report 80,000% used, which the allocation clamp would in fact hide - the sign matters because it is what separates Unlimited from Unknown in the rendered detail, not because the percentage moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewWent through the full diff (production code + both Lite and Darling test suites) against Correctness of the denominator fix — T-SQL style — Lite/Darling parity — checked column-for-column: both Minor, non-blocking observation: the No missing-index-DMV suggestions, no correctness bugs, no parity drift found. Test coverage (unit + live-store E2E on both SKUs) is thorough and specifically pins the three-state ( |
The arm's note claimed the service reads the ceiling column through the viewer, so a viewer on a V80 store 'would be reading a store whose service half cannot supply it'. No viewer query names max_size_mb — the service's alert adapter is what reads it, and a store at 80 is being served by a service at 80, which is behaving correctly for its own version. The honest reason is the standing invariant every top rung has: RequiredStoreSchemaVersion is StorageVersion.SchemaVersion, so a fully migrated store has to map to exactly this. What the banner is worth on the way is real and worth stating: a store still at 80 has no ceiling anywhere in its history, so every tempdb percentage in it is the old distance-to-next-autogrow measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewWent through the full diff (production code, migrations, and the new/changed tests across both SKUs). This is a solid, well-scoped fix. Correctness
Lite/Darling parity
Style
Security / perf
No blocking issues found. Nice attention to backward compatibility (NULL/ |
Closes #2515.
The mechanism
TempDbSpaceInfo.UsedPercentwasTotalReservedMb / (TotalReservedMb + UnallocatedMb) * 100, and both halves come fromtempdb.sys.dm_db_file_space_usage, which reports the data files as CURRENTLY ALLOCATED. That is distance to the next autogrow. It reads like real headroom on a pre-sized on-prem box only because such a tempdb has already grown to its cap.The comment on #2515 settles what kind of defect that is: the ceiling is discoverable, so this is a denominator correction, not a policy call. On
GP_S_Gen5_2, tempdb is four ROWS files of 16 MB — 62.44 MB allocated — and every one of them hasmax_size= 2,097,152 pages, a 65,536 MB ROWS ceiling. One ordinary ~57 MB#temptable reads 95.7% full against the allocation and 0.09% against the cap. At the shipped 80% default that is a page on the first busy minute of every Azure target.The fix
max_sizeis a real value, the denominator isSUM(max_size)over ROWS files, converted to MB.-1), there is no ceiling to measure against and the current allocation stays the denominator — today's behaviour, so unlimited-growth on-prem and RDS targets do not move at all.UsedPercentnow means the same thing on every engine: distance to the point where tempdb cannot grow any further.Not a size floor, per the issue: a floor suppresses the alert on a genuinely full large tempdb at the moment it starts growing, still fires at the autogrow boundary once cleared, and any value silently redefines "small" for every on-prem and RDS target already relying on today's behaviour.
What moves on existing targets
max_size(typical on-prem)max_size_mbNULL → 0)max_sizeit has not grown intomax_sizeit is approachingRow three is the one to be loud about: it is a correction, not a regression, but it will move live numbers on servers people are already watching. It is called out explicitly in the CHANGELOG and pinned by
TheCappedOnPremShape_ReportsLowerThanItUsedTo.Things the issue asked to get right
LOG files are excluded from the denominator, verified against the collector's columns. Every other column in
tempdb_statscomes fromdm_db_file_space_usage, which reports DATA allocation — folding the log's cap into the same ratio would understate usage on every server, not just Azure. The subquery isWHERE df.type = 0 /*ROWS*/, pinned byQuery_ReadsTheCeilingFromTheRowsFilesOnly_AndSumsItWideEnough.AlertEngineTests' 1000 MB fixture does not imply a cap, and should not start to. It carries noMaxSizeMb, so it describes a tempdb whose ceiling was never measured — exactly the case where the denominator stays the allocation. The 80% pin is unchanged, and that it is unchanged is the guarantee that no unlimited or not-yet-recollected target moves. There is a comment on the fixture saying so, and the capped case got its own test rather than being folded in.FactScorer.ScoreTempDbVersionStore's 1/2/5 GB bars are unchanged and need no change. They scoremax_version_store_mbin absolute MB; the ceiling is a denominator, so it cannot move their reachability. They stay unreachable on an Azure tempdb still at ~62 MB for the reason #2516 gave — a version store cannot exceed 1 GB inside a 62 MB tempdb — and become reachable there exactly when tempdb autogrows past a gigabyte, which is the same rule as everywhere else. The arm is self-consistent; the assessment is recorded in its doc comment so the next reader does not have to re-derive it.A migration WAS needed, and the derivation-instead alternative does not work. The obvious way to avoid new storage is to read the cap out of
database_size_stats, which already carriesmax_size_mbper file. On Azure SQL Database that collector runs a database-scoped query against the connected database'ssys.database_files(deliberately — #1631), so tempdb never appears in it. The one platform this fix exists for is the one where the derivation has nothing to read. SoTempDbStatsCollectorcollects the cap itself, which is a payload/schema change:PgMigrations.ScriptsV81tempdb-max-size+V81Sql,StorageVersion.SchemaVersion→ 81reader.GetBoolean(56)+hasTempDbMaxSizeparam + newest-first arm returning 81CollectionLogFanoutRollupStoreTestsgives up "80 is the maximum" and pins its own ordinal instead ofmapParameters - 1DuckDbInitializer.CurrentSchemaVersion→ 56 with an idempotentADD COLUMN IF NOT EXISTS, and the column appended toGoldenCollectorSchema(the one legal edit to that oracle)The rung also refreshes
collect.v_tempdb_stats. Postgres freezes a view'sSELECT *column list at CREATE, and the analysis fact collector reads tempdb through that V4 passthrough — without the refresh an upgraded store would keep serving the old column list andanalyze_serverwould go on scoring against the allocation while the alert had already moved. Lite needs no view work because it rebuilds everyv_passthrough on start.Scope note: the analysis fact carried a second copy of the bug
PgFactCollector/DuckDbFactCollectorcompute theTEMPDB_USAGEfraction themselves rather than going throughTempDbSpaceInfo, with the samereserved + unallocateddenominator. Leaving it would have meantanalyze_serverscoring the same Azure target at 96% full while the pager stayed quiet — one server, two answers. Both are fixed with the same rule, and the fact gained amax_size_mbmetadata key (added, never redefined — the existing keys are a consumer surface).Also
The alert detail gained a Max Size field, because "95% used" is unreadable without knowing which denominator produced it. Three states, three words: a cap in MB,
Unlimited, orUnknownfor a snapshot taken before the ceiling was collected. Shared builder, so both SKUs render byte-identically.get_tempdb_trendand the viewer's tempdb reads report raw MB and compute no percentage, so they are consistent as-is and were deliberately left alone rather than having their payload shape changed.Testing
Darling.Tests/TempDbCeilingStoreTests— the rung, the DDL, the view refresh, the probe/reader/map agreement, and the arithmetic with the issue's real numbers: 62.44 MB allocated against a 65,536 MB cap with 59.75 MB used does not clear an 80% threshold, while the identical snapshot without a ceiling reads 95.69% and does. Plus the gated live E2E through the real adapter for all three states (cap,-1, NULL), cleanup viaLiveStoreCleanup.RunAsync, INSERT column list copied fromDarlingAlertReadAdapterTests.Lite.Tests/TempDbCeilingStoreTests— the same three shapes through real DuckDB and the real reads, plus the analysis fact.AlertEngineTests/LiteAlertForwardingTests— the same decision through the alert ENGINE on both SKUs, which is what actually pages someone.TempDbStatsCollectorDefinitionTests— payload append, the ROWS-only filter, the bigint widen before theSUM, and NULL-vs--1surviving the read as different values.Ran locally on macOS: full solution builds clean, and a throwaway
net10.0harness against the realPerformanceMonitor.Alerting/.Collectors/.Darling.Storageassemblies exercised every arithmetic and rendering assertion above (all PASS), including the "without the fix" reading of 95.69% that proves these tests fail without the change. The Windows suites cannot execute on macOS; CI is the arbiter for those.And the T-SQL was run against a real engine, not just against fixtures — the one thing no CI job here covers.
BuildQuery(...).Textwas dumped from the built assembly (the shipped string, not a retyped copy) and executed on SQL Server 2022 with 8 tempdb data files:max_size_mb-1.00800.00UNLIMITED-1.00Row two is the LOG exclusion proven rather than intended: 800, not 2,848. Row three is
MINdoing its job — one unlimited data file means tempdb as a whole can grow without limit, so the whole answer goes back to-1.Note that until #2516 lands,
AppliesTois still!target.IsAzureSqlDb, so this new subquery cannot reach an Azure target fromdevat all; it runs only wheretempdb.sys.database_filesis unremarkable. The Azure read itself is the measurement recorded on #2515.Coordination with #2516
#2516 (the Azure gate removal) can now ship — but it should not ship before this, and the two must not be merged blind. Until #2516 lands,
AppliesTois still!target.IsAzureSqlDbondev, so nothing here can affect an Azure target yet; the new subquery only runs against on-prem, RDS and Managed Instance, wheretempdb.sys.database_filesis unremarkable.I based on
devrather than stacking, so CI here is real. A trial merge offix/2512-azure-tempdb-gateinto this branch produced one small conflict and one thing git merges cleanly but wrongly — whoever merges second should expect:TempDbStatsCollector.csclass summary — a genuine conflict, both sides rewrote it. Keep Collect tempdb on Azure SQL Database: the gate's stated reason was false (#2512) #2516's "applies to every SQL Server target" sentence and this branch's ceiling paragraph.TempDbStatsCollector.AppliesTo's doc comment merges cleanly and is then out of date: Collect tempdb on Azure SQL Database: the gate's stated reason was false (#2512) #2516's paragraph says the ratio "needs a size floor before it can safely carry thetempdb Spacealert — filed as The tempdb Space alert's percentage has no size floor, and on Azure SQL Database that makes it fire on one temp table #2515". It got a denominator instead. That paragraph needs a sentence saying so.Lite.Tests/TempDbStatsCollectorDefinitionTests.AzureSqlDb_MeasuredValues_ComposeThroughToThePayload(added by Collect tempdb on Azure SQL Database: the gate's stated reason was false (#2512) #2516) merges cleanly and then does not compile: it builds a five-column first result set and an eight-argumentRow. It needs a sixth column and a ninthRow/writer value.[#2512]:and[#2515]:. Keep one copy of each.