Scope the de-skew's accuracy claim where the column outlives a DST transition - #3229
Scope the de-skew's accuracy claim where the column outlives a DST transition#3229erikdarlingdata wants to merge 2 commits into
Conversation
…ansition server_properties.utc_offset_minutes is DATEDIFF(MINUTE, GETUTCDATE(), GETDATE()) -- the offset in force at COLLECTION time, one current value. Subtracting it from a stored timestamp is exact only while both sit on the same side of a DST transition, and #3212 shipped sixteen de-skewed fields without saying so on the two reads where that can be false. Most of the sixteen describe current state -- a running job, an open transaction, a cleaner that ran seconds ago -- so the timestamp and the offset are always in the same period. get_index_usage is the exception and the worst case: sys.dm_db_index_usage_stats persists since the instance restarted, which on a stable production box is routinely months, so a large share of last_user_access values predate the most recent transition and come back 60 minutes early. Silently, and in the plausible direction. The fleet is exposed rather than theoretically exposed -- its targets report utc_offset_minutes of -240, which is EDT, a DST-observing zone on summer time -- and sqlserver_start_time on the same row is the bound on how far back it reaches. get_pvs_stats is a tier less severe and gets the same note: the off-row cleaner runs continuously, but aborted_version_cleaner_* can reach back weeks on a quiet database (a live read showed 2026-08-26 against a 2026-09-09 snapshot), so it can cross a transition. Documented rather than fixed, deliberately. The proper fix is a ZONE instead of an offset -- CURRENT_TIMEZONE_ID() collected alongside utc_offset_minutes, then AT TIME ZONE at the read boundary, which handles transitions -- and that is a collected-column addition plus a migration rung, which is its own change. What this carries is the #2993 shape: the scope of the claim, stated where the de-skew happens, so the next reader knows the frame is exact inside the current DST period and within an hour outside it. Same family as #2993 by a different mechanism: there a zone was captured and discarded, here an offset is captured and over-applied. Both end with a naive timestamp confidently placed in the wrong frame.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// keeps it NULL. This read returns NO other timestamp, which is why converting rather than labelling | ||
| /// matters more here than elsewhere: there is nothing else in the payload for a reader to notice a | ||
| /// disagreement against.</para> | ||
| /// <para><b>The de-skew is exact only inside the current DST period, and this is the read where that |
There was a problem hiding this comment.
Lite/Parity: this same DST-exactness caveat applies to Lite, and Lite's description isn't updated.
Lite/Mcp/McpObjectStatsTools.cs computes last_user_access with the identical pattern: GetServerUtcOffsetMinutesAsync reads the latest utc_offset_minutes (itself DATEDIFF(MINUTE, GETUTCDATE(), GETDATE()) at collection time — same single-current-value offset as Darling's server_properties.utc_offset_minutes), then applies it via r.LastUserAccess?.AddMinutes(-utcOffsetMinutes) against a DMV column that persists since the last instance restart. That's the exact scenario this PR documents as being 60 minutes off across a DST transition — but Lite's tool description (McpObjectStatsTools.cs:57) just says "this read de-skews them" with no scope caveat.
Since this PR is intentionally scoped to documenting rather than fixing, should the same caveat be added to Lite's description (or a follow-up issue opened) so the two apps don't drift on a documented-vs-undocumented known limitation?
| /// phantom four-hour stall is the worst place for an unmarked frame. The trend read needs nothing: it | ||
| /// returns only <c>collection_time</c>.</para> | ||
| /// | ||
| /// <para><b>Exact only inside the current DST period.</b> The collected offset is the one in force at |
There was a problem hiding this comment.
Same parity gap as the DarlingObjectStatsReader note: Lite/Mcp/McpPvsTools.cs de-skews aborted_version_cleaner_start_time/_end_time the same way (AddMinutes(-utcOffsetMinutes) off a single collected offset, McpPvsTools.cs:77-78), off a value that "can reach back weeks on a quiet database" per this PR's own reasoning — so the same DST-crossing exactness limitation applies there too, but Lite's tool description (McpPvsTools.cs:26) doesn't carry the scope note this PR adds to Darling.
ReviewThis is a doc-only PR (XML doc comments, no SQL or logic changes) — verified by diff: only Technical claims checked against the code and hold up:
Lite/Darling parity gap (posted as inline comments): the exact same de-skew mechanism and exact same exactness limitation exist in Lite —
Both are the same "single collected offset applied to a DMV value that can predate the last DST transition" shape this PR documents for Darling, but Lite's tool descriptions weren't updated with the same scope caveat. Since the PR is explicitly about documenting rather than fixing, worth deciding whether Lite gets the same doc note now or via a tracked follow-up — otherwise the two apps end up with one documented limitation and one silent one for the identical bug. No correctness, security, or performance concerns — this is comment-only. |
…tate's composition The note justified itself with a deployment fact -- the fleet's reported offset and its zone. A shipped product's source should not reason from one estate's composition: the exposure is that ANY target in a DST-observing zone has it, and on RDS a non-UTC zone is an ordinary creation-time parameter rather than an exotic setting, which also kills the plausible-sounding assumption that an RDS instance is probably UTC. #2932 already carries the measurement, so the general argument loses nothing and is checkable without a private fact. Same for the version gate: the offset path stays because the product supports servers older than SQL Server 2019, not because any particular fleet does or does not contain one.
|
Superseded by #3231 — same content, opened fresh as a single commit. Closing rather than pushing more commits: this branch's first commit carried a sentence that justified the exposure by a deployment fact, and the follow-up commit removed it from the source without removing it from that commit. #3231 carries only the generalised form, in one commit, with the same file content — the staged blobs are byte-identical to this branch's tip. To be accurate about what this achieves: it limits what a future reader encounters and keeps the sentence out of |
|
Reviewed. This is a genuine doc-only change: no SQL, no C# logic, only XML-doc prose added to DarlingObjectStatsReader.cs and DarlingPvsReader.cs. Verified independently:
One gap worth flagging: Lite/Darling parity of the documentation itself. The identical bug exists in Lite, undocumented:
A search for DST/daylight mentions across Lite returns nothing, so there is no equivalent note anywhere in Lite calling out that these two tools' timestamps are only exact inside the current DST period. Since this PR's whole point is documenting rather than fixing the limitation, and the described fix (collected zone plus AT TIME ZONE) would need to land in both stores' migration ladders per the Two-Store Parity contract anyway, it would be worth adding the same caveat to the two Lite call sites (even as a short inline comment, matching Lite's terser style) so a future Lite-side fix is not scoped by someone who never saw this analysis. Not a blocker for this PR as scoped, but leaving Lite's docs silent on a defect this PR just spent several paragraphs proving is real reads as an oversight rather than a deliberate choice. |
1 participant
server_properties.utc_offset_minutesisDATEDIFF(MINUTE, GETUTCDATE(), GETDATE())— the offset in force at collection time, one current value. Subtracting it from a stored timestamp is exact only while the timestamp and the offset sit on the same side of a DST transition. #3212 shipped sixteen de-skewed fields without saying so on the two reads where that can be false.Raised by another session reading the merged code, and confirmed here against
DarlingServerConnector.cs:94and the fleet's reported offset.Why
get_index_usageis the case that mattersThirteen of the sixteen describe current state — a running job, an open transaction, a cleaner that ran seconds ago — so their timestamps and the collected offset are always in the same DST period, and the de-skew is exact.
last_user_accessis not current state. It is theGREATESTof foursys.dm_db_index_usage_statscolumns that persist since the instance restarted, which on a stable production box is routinely months. So a large share of those values predate the most recent transition and come back 60 minutes early — silently, and in the plausible direction, which is the bad one.sqlserver_start_timeon the same row is the bound on how far back it can reach.The fleet is exposed rather than theoretically exposed: any target in a DST-observing zone has it — the offset differs by an hour between summer and winter, so every pre-transition timestamp is de-skewed by an offset that was not in force when it was written. A target configured to UTC is unaffected, and on AWS RDS the instance takes its time zone from a creation-time parameter, so a non-UTC zone is an ordinary configuration rather than an exotic one. #2932 records the measurement behind the four-hour figure above.
get_pvs_statsgets the same note one tier down: the off-row cleaner runs continuously and is unaffected in practice, butaborted_version_cleaner_*can reach back weeks on a quiet database — a live read showed2026-08-26against a2026-09-09snapshot — so it can cross a transition.Documented rather than fixed, deliberately
The proper fix is a zone instead of an offset:
CURRENT_TIMEZONE_ID()(SQL Server 2019+) collected alongsideutc_offset_minutes, thenAT TIME ZONEat the read boundary, which handles transitions correctly for a timestamp of any age. That is a collected-column addition, a migration rung, and swapping the- make_interval(...)expression for anAT TIME ZONEconversion in five readers, with the offset kept as the fallback whileserver_timezone_idis null — which it will be for every server until its nextserver_propertiescollection.There is no backfill, and an earlier revision of this description wrongly said there was. Nothing stored needs rewriting: the label is
DeSkewedAtReadprecisely because the collector shipsus.last_user_seekverbatim and the offset is subtracted in the projection, so no stored column is wrong today. And a single current zone ID is legitimately valid retroactively where a single current offset is not — that asymmetry is the whole defect.utc_offset_minuteschanges twice a year, so today's value is wrong for half of history;CURRENT_TIMEZONE_ID()returns a zone identity that is stable for the life of the server, so collecting it going forward and applying it to already-stored rows is sound. It is the one field whose present value describes the past.Two things that stay genuinely open, so this does not read as free:
CURRENT_TIMEZONE_ID()is SQL Server 2019+, so the offset path has to be kept rather than replaced — the product supports servers older than that and will for someone regardless of any particular deployment — and a server whose Windows time-zone data disagrees with the historical rule for a given date is still wrong for that date. Much narrower than the current failure, not zero.What this carries is the #2993 shape: the scope of the claim, stated where the de-skew happens, so the next reader knows the frame is exact inside the current DST period and within an hour outside it. Same family as #2993 by a different mechanism — there a zone was captured and discarded, here an offset is captured and over-applied, and both end with a naive timestamp confidently placed in the wrong frame.
Verification
Doc-only; no shipped SQL or behaviour changes. Both affected projects build with 0 errors and both files keep CRLF with no bare LF.
One check worth naming because it is a live trap in this area:
ConsumedTimestampFrameDisciplineTests.ProjectionAliasisIgnoreCaseover" as <word>"and reads string literals and comments, so ordinary prose next to a census column name registers as a SQL projection alias — it cost #3212 two CI rounds. I ran that exact regex over every added line and no added prose produces a reportable match.