diff --git a/CHANGELOG.md b/CHANGELOG.md index 47e8e800..f63991b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A cross-database lock now names the database the contended object actually lives in, from both blocking collectors** ([#1893]) - [#1876] made the two collectors' `contentious_object` labels agree at the incident identity, and corrected the `blocked_process_report` sentinel to name the lock RESOURCE's database rather than the blocked session's. The DMV snapshot side could not follow: its normalization runs in C# against a stored row whose only database name is `database_name`, which the collector writes as the blocked SESSION's database. A cross-database lock is held in one database by a session running in another, so for exactly that case the two sides still disagreed - the report row said `database: StackOverflow2013` where the snapshot row said `database: master` - and one contended object still raised two alerts. + + **The resource's database id comes from `sys.dm_os_waiting_tasks.resource_description`, not from the wait resource.** MS Learn documents every lock resource type's description as carrying a `dbid=` token - `keylock`, `pagelock`, `ridlock`, `objectlock`, `databaselock`, `filelock`, `extentlock`, `applicationlock`, `metadatalock`, `hobtlock` and `allocunitlock` - so a single parse covers every lock shape, where splitting `wait_resource` positionally would need a branch per resource type and would still miss the ones whose layout differs. `DB_NAME()` then runs in the same query, which is the whole reason this had to move server-side: the id was always sitting in the row, but nothing downstream could turn it into a name. Verified live on SQL Server 2022 against a genuine cross-database KEY lock, whose description reads `keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...`; the same blocking pair produced `Unresolved: key lock, database: pm1893_res` from both collectors, byte for byte, where before the fix the snapshot row carried the raw `KEY: 11:72057594045726720 (8194443284a0)` and could only ever have been named with the session's database. + + **The snapshot sweep deliberately still does NOT resolve the object behind a KEY, PAGE or RID lock.** That is the per-database lookup the report side needs a server-side cursor and [#1865]'s permission screen to do safely, and this collector runs on a far tighter cadence - naming the database is a string parse, resolving the object is a cross-database metadata read per row. Only the naming changed. The parse is restricted to `LCK_%` waits that have a wait resource, so latch and `RESOURCE_SEMAPHORE` rows - whose `resource_description` is a bare `::` or a latch class, with no `dbid=` to read - keep byte-identically what they had; and where no `dbid=` is found the row keeps its old raw value rather than being relabelled with a database nobody verified. + + **Query text only - no schema moved**, so no upgrade-folder work and no store migration in any of the three stores; the collector writes the same 19 columns with a better value in one of them, pinned by a test. The deprecated Dashboard's hand-maintained twin (`install/56_collect_dmv_blocking_snapshot.sql`) takes the identical change, guarded by a test that reads the file, so the SQL Server store cannot drift from Lite's and Darling's. This is the **third and final** blocking-fingerprint transition in this release, after [#1865] and [#1876]: DMV-sourced incidents for cross-database locks re-fire once against their new key. Consolidating all three into one upgrade was deliberate. One narrower difference remains and is filed rather than folded in ([#1898]): [#1865]'s trailing reason (`(no metadata access)`, `(page reallocated)`) exists only on the report side, so when it is present the two labels still differ by that suffix - dropping it from the identity while keeping it on the row is a product decision, not a tweak. + - **The same blocked object no longer raises two different alerts depending on which collector saw it** ([#1876]) - two collectors write `contentious_object` and they never agreed on how. `blocked_process_report` writes a plain `schema.object`, or an `Unresolved: key lock, database: Foo` sentinel when the lookup failed; `dmv_blocking_snapshots` writes a `QUOTENAME`'d `[schema].[object]` and, for the KEY, PAGE and RID locks it does not resolve at all, the **raw wait resource**. The alert engine merges both into one list and hashes that label into the incident's dedup key, so `dbo.Users` and `[dbo].[Users]` were two incidents for one table - and the DMV fallback, which exists to stand in for the XE session when it captured nothing (AWS RDS, or an unset blocked-process threshold), could never dedup against the reports it is standing in for. **The raw wait resource was the worse half**: `KEY: 6:72057594041991168 (8194443284a0)` carries a hobt id and a per-VALUE lock hash, so it was not merely a second identity for the object, it was a NEW identity on almost every sample - one recurring unresolvable lock rendered as an unbounded stream of one-occurrence incidents, each with its own cooldown. Those rows now collapse to the report side's sentinel and group by (lock type, database) exactly as report rows do. **Normalized at the incident identity - not at collection, and deliberately not where the two collectors' rows are merged.** Doing it on the read side at all is what fixes rows **already in the store**; a collector-side fix would only have helped rows gathered after the upgrade, leaving every historical DMV row still fingerprinting by lock hash. Doing it at the grouper specifically buys two things the obvious placement does not. **Coverage:** two independent producers raise blocking incidents - the live alert builders, which go through the merge, and the `top_blocking_chains` drill-down behind the analysis alerts, whose SQL `UNION`s the two collectors before anything downstream can tell them apart - and the grouper is the one point neither can bypass, which also makes a future third producer correct by default. **Not destroying evidence:** `dmv_blocking_snapshots` stores no `wait_resource` column at all, so for a lock it could not resolve, `contentious_object` IS the raw resource, hobt id and lock hash included - and that merge feeds the blocking grids and both `get_blocking` MCP tools as well as the alert path, so rewriting rows there would have fixed the fingerprint by deleting the only copy of the evidence an operator needs to chase the lock. **Grids, MCP payloads and the store keep exactly what the collector wrote; only what the ALERT groups on and names as the contended object is normalized.** @@ -2145,3 +2153,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1893]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1893 [#1873]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1873 [#1896]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1896 +[#1898]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1898 diff --git a/Darling/Darling.Tests/DarlingDmvResourceDatabaseSentinelTests.cs b/Darling/Darling.Tests/DarlingDmvResourceDatabaseSentinelTests.cs new file mode 100644 index 00000000..356de7b2 --- /dev/null +++ b/Darling/Darling.Tests/DarlingDmvResourceDatabaseSentinelTests.cs @@ -0,0 +1,280 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Notifications; +using Xunit; + +namespace Darling.Tests; + +/// +/// #1893, Darling half: the DMV blocking snapshot's sentinel now names the database the LOCK RESOURCE +/// lives in, so a cross-database lock fingerprints the same from either collector. +/// +/// +/// #1876 made the two collectors' labels agree at the incident identity and corrected the report side's +/// sentinel to name resource_database_id. The DMV side could not follow, because the normalizer runs +/// in C# against a stored row whose only database name is database_name — which the collector writes +/// as the BLOCKED SESSION's database. For a cross-database lock the session runs in one database while the +/// contended object lives in another, so the two sides still disagreed. +/// +/// +/// +/// The fix is server-side and cheap: the resource's database id is read from +/// sys.dm_os_waiting_tasks.resource_description, whose documented format carries a dbid= +/// token for EVERY lock resource type, and DB_NAME() runs in the same query. The DMV side still does +/// NOT resolve the OBJECT behind a KEY, PAGE or RID lock — that is the expensive per-database lookup the +/// report side needs a cursor and #1865's permission screen for, and this sweep runs on a much tighter +/// cadence. +/// +/// +/// Lite.Tests' DmvResourceDatabaseSentinelTests pins the identical expectations, so editing one app's copy alone fails a build. +/// +public sealed class DarlingDmvResourceDatabaseSentinelTests +{ + /* ── the fingerprint, which is the whole point ── */ + + [Fact] + public void A_Cross_Database_Lock_Fingerprints_The_Same_From_Either_Collector() + { + /* The exact strings both collectors produced for ONE live cross-database KEY lock on SQL 2022 + (session in pm1893_ctx, resource in pm1893_res). Red before #1893: the DMV row carried the raw + resource, which #1876's normalizer could only name with the SESSION's database. */ + var report = Group(Event(ReportSentinel, SessionDatabase)); + var snapshot = Group(Event(DmvSentinelAfter, SessionDatabase)); + + Assert.Equal(ReportSentinel, DmvSentinelAfter); + Assert.Equal(report[0].Incident.DedupKey, snapshot[0].Incident.DedupKey); + } + + [Fact] + public void The_Old_Raw_Resource_Could_Only_Ever_Have_Named_The_Session_Database() + { + /* Why the fix had to be server-side, demonstrated rather than asserted: hand #1876's normalizer the + value the DMV collector used to store, and the best it can do is the session's database — the + resource's id is right there in the string, but C# cannot turn 11 into a name. */ + var normalized = ContentiousObjectLabel.Normalize(DmvValueBefore, SessionDatabase); + + Assert.Equal("Unresolved: key lock, database: pm1893_ctx", normalized); + Assert.NotEqual(ReportSentinel, normalized); + Assert.NotEqual(Group(Event(ReportSentinel, SessionDatabase))[0].Incident.DedupKey, + Group(Event(DmvValueBefore, SessionDatabase))[0].Incident.DedupKey); + } + + [Fact] + public void The_New_Sentinel_Passes_Through_The_Normalizer_Untouched() + { + /* The collector now emits report form, so #1876's read-side transform must be a no-op on it — if it + ever started re-deriving the database from the row, it would put the session's name back and undo + this fix silently. Checked with a DIFFERENT database name passed in, so a transform that rewrote + the value would visibly change it. */ + Assert.Equal(DmvSentinelAfter, ContentiousObjectLabel.Normalize(DmvSentinelAfter, SessionDatabase)); + Assert.Equal(DmvSentinelAfter, ContentiousObjectLabel.Normalize(DmvSentinelAfter, "some-other-database")); + } + + /* ── the parse, and its documented basis ── */ + + [Fact] + public void The_Resource_Database_Comes_From_Resource_Descriptions_Documented_Dbid_Token() + { + /* MS Learn documents every lock resource_description as carrying dbid= — keylock, pagelock, + ridlock, objectlock, databaselock, filelock, extentlock, applicationlock, metadatalock, hobtlock + and allocunitlock — so ONE parse covers every lock shape, where splitting wait_resource + positionally would need a per-type branch. Verified live on SQL 2022: + 'keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...'. */ + var sql = Sql(); + + Assert.Contains("CHARINDEX(N'dbid=', wt.resource_description)", sql, StringComparison.Ordinal); + Assert.Contains("wt.resource_description", sql, StringComparison.Ordinal); + /* Digits only, up to the first non-digit: 'dbid=11 id=lock...' must yield 11, not 11 plus whatever + follows. The appended '.' guarantees PATINDEX finds a terminator even at end of string. */ + Assert.Contains("PATINDEX(N'%[^0-9]%', d.tail + N'.')", sql, StringComparison.Ordinal); + Assert.Contains("DB_NAME(resparse.resource_database_id)", sql, StringComparison.Ordinal); + } + + [Fact] + public void The_Parse_Is_Restricted_To_Lock_Waits_That_Have_A_Resource() + { + /* Latch and RESOURCE_SEMAPHORE rows have no 'TYPE: ' resource shape and no dbid= to read — their + resource_description is a bare '::' or a latch class. Gating on the wait + TYPE rather than sniffing the string keeps every one of those rows byte-identical to before, and + is honest about why: this is a lock-resource parse. */ + var sql = Sql(); + + Assert.Contains("WHERE wt.wait_type LIKE N'LCK[_]%'", sql, StringComparison.Ordinal); + Assert.Contains("AND der_b.wait_resource IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("AND der_b.wait_resource <> N''", sql, StringComparison.Ordinal); + } + + [Fact] + public void Without_A_Dbid_The_Old_Value_Is_Kept_Exactly() + { + /* The fallback is unchanged on purpose: a row this cannot name is better left carrying the raw + resource than relabelled with a database nobody verified. */ + var sql = Sql(); + + Assert.Contains("ELSE der_b.wait_resource", sql, StringComparison.Ordinal); + var sentinel = sql.IndexOf("WHEN resparse.resource_database_id IS NOT NULL", StringComparison.Ordinal); + var fallback = sql.IndexOf("ELSE der_b.wait_resource", StringComparison.Ordinal); + Assert.True(sentinel > 0 && fallback > sentinel, "the sentinel must be tried before the raw fallback"); + } + + [Fact] + public void The_Object_Resolution_Branch_Is_Untouched() + { + /* #1893 deliberately does NOT give this collector KEY/PAGE/RID object resolution: that is the + per-database lookup the report side needs a cursor and a permission screen (#1865) for, and this + sweep runs on a far tighter cadence. Only the naming changed. */ + var sql = Sql(); + + Assert.Contains("WHEN objparse.object_id IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("QUOTENAME(OBJECT_SCHEMA_NAME(objparse.object_id, objparse.database_id))", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sys.partitions", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sys.dm_db_page_info", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sp_executesql", sql, StringComparison.Ordinal); + Assert.DoesNotContain("CURSOR", sql, StringComparison.Ordinal); + } + + /* ── the shape must be the report collector's, not merely similar ── */ + + [Fact] + public void Both_Collectors_Classify_Lock_Types_With_The_Same_Ordered_Rules() + { + /* The sentinel's lock word has to match or the labels differ by a token and fingerprint apart. Both + sides test KEY, OBJECT, RID then PAGE in that order and fall back to the leading token upper-cased + and capped at 32 — the order matters because a compound resource can embed more than one. */ + foreach (var sql in new[] { Sql(), ReportSql() }) + { + var order = Regex.Matches(sql, @"LIKE N'%(KEY|OBJECT|RID|PAGE): %'") + .Select(m => m.Groups[1].Value) + .ToArray(); + + Assert.Equal(new[] { "KEY", "OBJECT", "RID", "PAGE" }, order); + Assert.Contains("CHARINDEX(N':', ", sql, StringComparison.Ordinal); + Assert.Contains("), 32)", sql, StringComparison.Ordinal); + } + } + + [Fact] + public void Both_Collectors_Build_The_Sentinel_From_The_Same_Literal_Pieces() + { + /* Three literals, spelled the same on both sides. A stray capital or a missing comma here is a + silent fingerprint split, which is exactly the class of defect #1876 and #1893 exist to close. */ + foreach (var sql in new[] { Sql(), ReportSql() }) + { + Assert.Contains("N'Unresolved: '", sql, StringComparison.Ordinal); + Assert.Contains("N' lock, '", sql, StringComparison.Ordinal); + Assert.Contains("N'database: '", sql, StringComparison.Ordinal); + Assert.Contains("N'unknown'", sql, StringComparison.Ordinal); + } + + /* And both fall back to the event/session database before giving up, in the same order. */ + Assert.Contains("COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown')", Sql(), StringComparison.Ordinal); + Assert.Contains("COALESCE(DB_NAME(b.resource_database_id), DB_NAME(b.database_id), N'unknown')", ReportSql(), StringComparison.Ordinal); + + /* Both LOWER the lock word. The fingerprint would survive a case split (it lower-cases before + hashing), so this is about the label a human reads: one collector writing 'KEY lock' beside + another's 'key lock' for the same contention is the drift this pair of issues is about. */ + Assert.Contains("LOWER(resparse.lock_type)", Sql(), StringComparison.Ordinal); + Assert.Contains("LOWER(b.lock_type)", ReportSql(), StringComparison.Ordinal); + } + + /* ── the deprecated Dashboard's SQL twin ── */ + + [Fact] + public void The_Dashboard_Install_Script_Carries_The_Same_Change() + { + /* install/56 is the Dashboard's hand-maintained copy of this collector. It has drifted before, and a + store that labels the same lock differently from Lite's and Darling's is the divergence this + repo's parity rule exists to stop. */ + var install = File.ReadAllText(FindRepoFile(Path.Combine("install", "56_collect_dmv_blocking_snapshot.sql"))); + + Assert.Contains("WHEN resparse.resource_database_id IS NOT NULL", install, StringComparison.Ordinal); + Assert.Contains("N'Unresolved: '", install, StringComparison.Ordinal); + Assert.Contains("COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown')", install, StringComparison.Ordinal); + Assert.Contains("CHARINDEX(N'dbid=', wt.resource_description)", install, StringComparison.Ordinal); + Assert.Contains("PATINDEX(N'%[^0-9]%', d.tail + N'.')", install, StringComparison.Ordinal); + Assert.Contains("WHERE wt.wait_type LIKE N'LCK[_]%'", install, StringComparison.Ordinal); + } + + [Fact] + public void The_Change_Is_Query_Text_Only_And_Stores_No_New_Column() + { + /* No schema moved, so no upgrade-folder work and no store migration: the collector writes the same + 19 columns it always did, with a better value in one of them. */ + var names = DmvBlockingSnapshotCollector.Instance.PayloadColumns.Select(c => c.Name).ToArray(); + + Assert.Contains("contentious_object", names); + Assert.DoesNotContain("wait_resource", names); + Assert.DoesNotContain("resource_database_id", names); + } + + /* ── fixtures: the literal strings the two collectors produced for ONE live lock ── */ + + /// What the DMV collector stored for that lock BEFORE #1893 — the raw wait resource. + private const string DmvValueBefore = "KEY: 11:72057594045726720 (8194443284a0)"; + + /// What it stores now, read off the live run. + private const string DmvSentinelAfter = "Unresolved: key lock, database: pm1893_res"; + + /// What the report collector produced for the same lock when it could not resolve the object. + private const string ReportSentinel = "Unresolved: key lock, database: pm1893_res"; + + /// The database the BLOCKED SESSION was running in — deliberately not the resource's. + private const string SessionDatabase = "pm1893_ctx"; + + /* ── helpers ── */ + + private static string Sql() => + DmvBlockingSnapshotCollector.Instance.BuildQuery(Context()).Text.ReplaceLineEndings("\n"); + + private static string ReportSql() => + BlockedProcessReportCollector.Instance.BuildQuery(Context()).Text.ReplaceLineEndings("\n"); + + private static CollectorContext Context() => new() + { + ServerId = 42, + ServerName = "test-server", + CollectionTime = new DateTime(2026, 7, 30, 12, 0, 0, DateTimeKind.Utc), + Deltas = null!, + Target = new CollectorTargetInfo(), + }; + + private static BlockingIncidentGrouper.BlockedEvent[] Event(string contentiousObject, string database) => + new[] + { + new BlockingIncidentGrouper.BlockedEvent( + database, contentiousObject, + "UPDATE pm1893_res.dbo.locktarget SET v = v + 100 WHERE id = 1;", + "UPDATE pm1893_res.dbo.locktarget SET v = v + 1 WHERE id = 1;", + 13882, "X"), + }; + + private static List Group(BlockingIncidentGrouper.BlockedEvent[] events) => + BlockingIncidentGrouper.Group("SQL2022", events); + + private static string FindRepoFile(string relativePath) + { + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 8 && dir is not null; i++) + { + var candidate = Path.Combine(dir, relativePath); + if (File.Exists(candidate)) + { + return candidate; + } + dir = Path.GetDirectoryName(dir); + } + throw new FileNotFoundException($"Could not locate {relativePath} walking up from {AppContext.BaseDirectory}"); + } +} diff --git a/Lite.Tests/DmvResourceDatabaseSentinelTests.cs b/Lite.Tests/DmvResourceDatabaseSentinelTests.cs new file mode 100644 index 00000000..f33fc14e --- /dev/null +++ b/Lite.Tests/DmvResourceDatabaseSentinelTests.cs @@ -0,0 +1,280 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Notifications; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #1893, Lite half: the DMV blocking snapshot's sentinel now names the database the LOCK RESOURCE +/// lives in, so a cross-database lock fingerprints the same from either collector. +/// +/// +/// #1876 made the two collectors' labels agree at the incident identity and corrected the report side's +/// sentinel to name resource_database_id. The DMV side could not follow, because the normalizer runs +/// in C# against a stored row whose only database name is database_name — which the collector writes +/// as the BLOCKED SESSION's database. For a cross-database lock the session runs in one database while the +/// contended object lives in another, so the two sides still disagreed. +/// +/// +/// +/// The fix is server-side and cheap: the resource's database id is read from +/// sys.dm_os_waiting_tasks.resource_description, whose documented format carries a dbid= +/// token for EVERY lock resource type, and DB_NAME() runs in the same query. The DMV side still does +/// NOT resolve the OBJECT behind a KEY, PAGE or RID lock — that is the expensive per-database lookup the +/// report side needs a cursor and #1865's permission screen for, and this sweep runs on a much tighter +/// cadence. +/// +/// +/// Darling.Tests' DarlingDmvResourceDatabaseSentinelTests pins the identical expectations, so editing one app's copy alone fails a build. +/// +public sealed class DmvResourceDatabaseSentinelTests +{ + /* ── the fingerprint, which is the whole point ── */ + + [Fact] + public void A_Cross_Database_Lock_Fingerprints_The_Same_From_Either_Collector() + { + /* The exact strings both collectors produced for ONE live cross-database KEY lock on SQL 2022 + (session in pm1893_ctx, resource in pm1893_res). Red before #1893: the DMV row carried the raw + resource, which #1876's normalizer could only name with the SESSION's database. */ + var report = Group(Event(ReportSentinel, SessionDatabase)); + var snapshot = Group(Event(DmvSentinelAfter, SessionDatabase)); + + Assert.Equal(ReportSentinel, DmvSentinelAfter); + Assert.Equal(report[0].Incident.DedupKey, snapshot[0].Incident.DedupKey); + } + + [Fact] + public void The_Old_Raw_Resource_Could_Only_Ever_Have_Named_The_Session_Database() + { + /* Why the fix had to be server-side, demonstrated rather than asserted: hand #1876's normalizer the + value the DMV collector used to store, and the best it can do is the session's database — the + resource's id is right there in the string, but C# cannot turn 11 into a name. */ + var normalized = ContentiousObjectLabel.Normalize(DmvValueBefore, SessionDatabase); + + Assert.Equal("Unresolved: key lock, database: pm1893_ctx", normalized); + Assert.NotEqual(ReportSentinel, normalized); + Assert.NotEqual(Group(Event(ReportSentinel, SessionDatabase))[0].Incident.DedupKey, + Group(Event(DmvValueBefore, SessionDatabase))[0].Incident.DedupKey); + } + + [Fact] + public void The_New_Sentinel_Passes_Through_The_Normalizer_Untouched() + { + /* The collector now emits report form, so #1876's read-side transform must be a no-op on it — if it + ever started re-deriving the database from the row, it would put the session's name back and undo + this fix silently. Checked with a DIFFERENT database name passed in, so a transform that rewrote + the value would visibly change it. */ + Assert.Equal(DmvSentinelAfter, ContentiousObjectLabel.Normalize(DmvSentinelAfter, SessionDatabase)); + Assert.Equal(DmvSentinelAfter, ContentiousObjectLabel.Normalize(DmvSentinelAfter, "some-other-database")); + } + + /* ── the parse, and its documented basis ── */ + + [Fact] + public void The_Resource_Database_Comes_From_Resource_Descriptions_Documented_Dbid_Token() + { + /* MS Learn documents every lock resource_description as carrying dbid= — keylock, pagelock, + ridlock, objectlock, databaselock, filelock, extentlock, applicationlock, metadatalock, hobtlock + and allocunitlock — so ONE parse covers every lock shape, where splitting wait_resource + positionally would need a per-type branch. Verified live on SQL 2022: + 'keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...'. */ + var sql = Sql(); + + Assert.Contains("CHARINDEX(N'dbid=', wt.resource_description)", sql, StringComparison.Ordinal); + Assert.Contains("wt.resource_description", sql, StringComparison.Ordinal); + /* Digits only, up to the first non-digit: 'dbid=11 id=lock...' must yield 11, not 11 plus whatever + follows. The appended '.' guarantees PATINDEX finds a terminator even at end of string. */ + Assert.Contains("PATINDEX(N'%[^0-9]%', d.tail + N'.')", sql, StringComparison.Ordinal); + Assert.Contains("DB_NAME(resparse.resource_database_id)", sql, StringComparison.Ordinal); + } + + [Fact] + public void The_Parse_Is_Restricted_To_Lock_Waits_That_Have_A_Resource() + { + /* Latch and RESOURCE_SEMAPHORE rows have no 'TYPE: ' resource shape and no dbid= to read — their + resource_description is a bare '::' or a latch class. Gating on the wait + TYPE rather than sniffing the string keeps every one of those rows byte-identical to before, and + is honest about why: this is a lock-resource parse. */ + var sql = Sql(); + + Assert.Contains("WHERE wt.wait_type LIKE N'LCK[_]%'", sql, StringComparison.Ordinal); + Assert.Contains("AND der_b.wait_resource IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("AND der_b.wait_resource <> N''", sql, StringComparison.Ordinal); + } + + [Fact] + public void Without_A_Dbid_The_Old_Value_Is_Kept_Exactly() + { + /* The fallback is unchanged on purpose: a row this cannot name is better left carrying the raw + resource than relabelled with a database nobody verified. */ + var sql = Sql(); + + Assert.Contains("ELSE der_b.wait_resource", sql, StringComparison.Ordinal); + var sentinel = sql.IndexOf("WHEN resparse.resource_database_id IS NOT NULL", StringComparison.Ordinal); + var fallback = sql.IndexOf("ELSE der_b.wait_resource", StringComparison.Ordinal); + Assert.True(sentinel > 0 && fallback > sentinel, "the sentinel must be tried before the raw fallback"); + } + + [Fact] + public void The_Object_Resolution_Branch_Is_Untouched() + { + /* #1893 deliberately does NOT give this collector KEY/PAGE/RID object resolution: that is the + per-database lookup the report side needs a cursor and a permission screen (#1865) for, and this + sweep runs on a far tighter cadence. Only the naming changed. */ + var sql = Sql(); + + Assert.Contains("WHEN objparse.object_id IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("QUOTENAME(OBJECT_SCHEMA_NAME(objparse.object_id, objparse.database_id))", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sys.partitions", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sys.dm_db_page_info", sql, StringComparison.Ordinal); + Assert.DoesNotContain("sp_executesql", sql, StringComparison.Ordinal); + Assert.DoesNotContain("CURSOR", sql, StringComparison.Ordinal); + } + + /* ── the shape must be the report collector's, not merely similar ── */ + + [Fact] + public void Both_Collectors_Classify_Lock_Types_With_The_Same_Ordered_Rules() + { + /* The sentinel's lock word has to match or the labels differ by a token and fingerprint apart. Both + sides test KEY, OBJECT, RID then PAGE in that order and fall back to the leading token upper-cased + and capped at 32 — the order matters because a compound resource can embed more than one. */ + foreach (var sql in new[] { Sql(), ReportSql() }) + { + var order = Regex.Matches(sql, @"LIKE N'%(KEY|OBJECT|RID|PAGE): %'") + .Select(m => m.Groups[1].Value) + .ToArray(); + + Assert.Equal(new[] { "KEY", "OBJECT", "RID", "PAGE" }, order); + Assert.Contains("CHARINDEX(N':', ", sql, StringComparison.Ordinal); + Assert.Contains("), 32)", sql, StringComparison.Ordinal); + } + } + + [Fact] + public void Both_Collectors_Build_The_Sentinel_From_The_Same_Literal_Pieces() + { + /* Three literals, spelled the same on both sides. A stray capital or a missing comma here is a + silent fingerprint split, which is exactly the class of defect #1876 and #1893 exist to close. */ + foreach (var sql in new[] { Sql(), ReportSql() }) + { + Assert.Contains("N'Unresolved: '", sql, StringComparison.Ordinal); + Assert.Contains("N' lock, '", sql, StringComparison.Ordinal); + Assert.Contains("N'database: '", sql, StringComparison.Ordinal); + Assert.Contains("N'unknown'", sql, StringComparison.Ordinal); + } + + /* And both fall back to the event/session database before giving up, in the same order. */ + Assert.Contains("COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown')", Sql(), StringComparison.Ordinal); + Assert.Contains("COALESCE(DB_NAME(b.resource_database_id), DB_NAME(b.database_id), N'unknown')", ReportSql(), StringComparison.Ordinal); + + /* Both LOWER the lock word. The fingerprint would survive a case split (it lower-cases before + hashing), so this is about the label a human reads: one collector writing 'KEY lock' beside + another's 'key lock' for the same contention is the drift this pair of issues is about. */ + Assert.Contains("LOWER(resparse.lock_type)", Sql(), StringComparison.Ordinal); + Assert.Contains("LOWER(b.lock_type)", ReportSql(), StringComparison.Ordinal); + } + + /* ── the deprecated Dashboard's SQL twin ── */ + + [Fact] + public void The_Dashboard_Install_Script_Carries_The_Same_Change() + { + /* install/56 is the Dashboard's hand-maintained copy of this collector. It has drifted before, and a + store that labels the same lock differently from Lite's and Darling's is the divergence this + repo's parity rule exists to stop. */ + var install = File.ReadAllText(FindRepoFile(Path.Combine("install", "56_collect_dmv_blocking_snapshot.sql"))); + + Assert.Contains("WHEN resparse.resource_database_id IS NOT NULL", install, StringComparison.Ordinal); + Assert.Contains("N'Unresolved: '", install, StringComparison.Ordinal); + Assert.Contains("COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown')", install, StringComparison.Ordinal); + Assert.Contains("CHARINDEX(N'dbid=', wt.resource_description)", install, StringComparison.Ordinal); + Assert.Contains("PATINDEX(N'%[^0-9]%', d.tail + N'.')", install, StringComparison.Ordinal); + Assert.Contains("WHERE wt.wait_type LIKE N'LCK[_]%'", install, StringComparison.Ordinal); + } + + [Fact] + public void The_Change_Is_Query_Text_Only_And_Stores_No_New_Column() + { + /* No schema moved, so no upgrade-folder work and no store migration: the collector writes the same + 19 columns it always did, with a better value in one of them. */ + var names = DmvBlockingSnapshotCollector.Instance.PayloadColumns.Select(c => c.Name).ToArray(); + + Assert.Contains("contentious_object", names); + Assert.DoesNotContain("wait_resource", names); + Assert.DoesNotContain("resource_database_id", names); + } + + /* ── fixtures: the literal strings the two collectors produced for ONE live lock ── */ + + /// What the DMV collector stored for that lock BEFORE #1893 — the raw wait resource. + private const string DmvValueBefore = "KEY: 11:72057594045726720 (8194443284a0)"; + + /// What it stores now, read off the live run. + private const string DmvSentinelAfter = "Unresolved: key lock, database: pm1893_res"; + + /// What the report collector produced for the same lock when it could not resolve the object. + private const string ReportSentinel = "Unresolved: key lock, database: pm1893_res"; + + /// The database the BLOCKED SESSION was running in — deliberately not the resource's. + private const string SessionDatabase = "pm1893_ctx"; + + /* ── helpers ── */ + + private static string Sql() => + DmvBlockingSnapshotCollector.Instance.BuildQuery(Context()).Text.ReplaceLineEndings("\n"); + + private static string ReportSql() => + BlockedProcessReportCollector.Instance.BuildQuery(Context()).Text.ReplaceLineEndings("\n"); + + private static CollectorContext Context() => new() + { + ServerId = 42, + ServerName = "test-server", + CollectionTime = new DateTime(2026, 7, 30, 12, 0, 0, DateTimeKind.Utc), + Deltas = null!, + Target = new CollectorTargetInfo(), + }; + + private static BlockingIncidentGrouper.BlockedEvent[] Event(string contentiousObject, string database) => + new[] + { + new BlockingIncidentGrouper.BlockedEvent( + database, contentiousObject, + "UPDATE pm1893_res.dbo.locktarget SET v = v + 100 WHERE id = 1;", + "UPDATE pm1893_res.dbo.locktarget SET v = v + 1 WHERE id = 1;", + 13882, "X"), + }; + + private static List Group(BlockingIncidentGrouper.BlockedEvent[] events) => + BlockingIncidentGrouper.Group("SQL2022", events); + + private static string FindRepoFile(string relativePath) + { + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 8 && dir is not null; i++) + { + var candidate = Path.Combine(dir, relativePath); + if (File.Exists(candidate)) + { + return candidate; + } + dir = Path.GetDirectoryName(dir); + } + throw new FileNotFoundException($"Could not locate {relativePath} walking up from {AppContext.BaseDirectory}"); + } +} diff --git a/PerformanceMonitor.Collectors/DmvBlockingSnapshotCollector.cs b/PerformanceMonitor.Collectors/DmvBlockingSnapshotCollector.cs index 751ba90b..41382de0 100644 --- a/PerformanceMonitor.Collectors/DmvBlockingSnapshotCollector.cs +++ b/PerformanceMonitor.Collectors/DmvBlockingSnapshotCollector.cs @@ -79,6 +79,19 @@ public static int SyntheticMonitorLoop(DateTime collectionTime) CASE WHEN objparse.object_id IS NOT NULL THEN ISNULL(QUOTENAME(OBJECT_SCHEMA_NAME(objparse.object_id, objparse.database_id)) + N'.' + QUOTENAME(OBJECT_NAME(objparse.object_id, objparse.database_id)), der_b.wait_resource) + /* #1893: a lock this collector does not resolve now says WHICH DATABASE the contended object + lives in, in the blocked_process_report collector's exact sentinel shape, so the two + collectors' rows for one object fingerprint identically (#1876). The database has to come + from the RESOURCE, not the session: a cross-database lock is held in one database by a + session running in another, and database_name above is the session's. */ + WHEN resparse.resource_database_id IS NOT NULL + THEN N'Unresolved: ' + + CASE + WHEN resparse.lock_type IS NULL + THEN N'' + ELSE LOWER(resparse.lock_type) + N' lock, ' + END + + N'database: ' + COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown') ELSE der_b.wait_resource END, blocked_sql_text = dest_b.text, @@ -127,6 +140,45 @@ OUTER APPLY object_id = TRY_CONVERT(integer, PARSENAME(REPLACE(SUBSTRING(der_b.wait_resource, 9, 200), N':', N'.'), 2)) WHERE der_b.wait_resource LIKE N'OBJECT: %' ) AS objparse +/* #1893: the resource's database id, and the lock type to name it with. + The id comes from sys.dm_os_waiting_tasks.resource_description rather than from wait_resource, + because MS Learn documents EVERY lock resource type's description as carrying a dbid= token -- + keylock, pagelock, ridlock, objectlock, databaselock, filelock, extentlock, applicationlock, + metadatalock, hobtlock and allocunitlock all end with (or contain) dbid= -- so one parse + covers every lock shape instead of a per-type positional split of wait_resource. Verified live on + SQL 2022: a cross-database KEY lock reports + 'keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...'. + The lock TYPE still comes from wait_resource, using the report collector's classifier verbatim, so + the two sides produce the same token for the same lock. + Restricted to LCK_% waits with a wait_resource: latch and RESOURCE_SEMAPHORE rows have no + 'TYPE: ' resource shape and no dbid= to read, and they keep exactly the value they had. */ +OUTER APPLY +( + SELECT + resource_database_id = + TRY_CONVERT + ( + integer, + NULLIF(LEFT(d.tail, PATINDEX(N'%[^0-9]%', d.tail + N'.') - 1), N'') + ), + lock_type = + CASE + WHEN der_b.wait_resource LIKE N'%KEY: %' THEN N'KEY' + WHEN der_b.wait_resource LIKE N'%OBJECT: %' THEN N'OBJECT' + WHEN der_b.wait_resource LIKE N'%RID: %' THEN N'RID' + WHEN der_b.wait_resource LIKE N'%PAGE: %' THEN N'PAGE' + ELSE LEFT(UPPER(LEFT(der_b.wait_resource, CHARINDEX(N':', der_b.wait_resource + N':') - 1)), 32) + END + FROM + ( + SELECT + tail = SUBSTRING(wt.resource_description, CHARINDEX(N'dbid=', wt.resource_description) + 5, 10) + ) AS d + WHERE wt.wait_type LIKE N'LCK[_]%' + AND der_b.wait_resource IS NOT NULL + AND der_b.wait_resource <> N'' + AND CHARINDEX(N'dbid=', wt.resource_description) > 0 +) AS resparse WHERE wt.blocking_session_id IS NOT NULL AND wt.blocking_session_id <> 0 AND wt.blocking_session_id <> wt.session_id diff --git a/install/56_collect_dmv_blocking_snapshot.sql b/install/56_collect_dmv_blocking_snapshot.sql index 148cc739..30771b4f 100644 --- a/install/56_collect_dmv_blocking_snapshot.sql +++ b/install/56_collect_dmv_blocking_snapshot.sql @@ -155,11 +155,22 @@ BEGIN /* Blocker status from its SESSION (a sleeping/idle blocker has no request row). */ blocking_status = ISNULL(der_k.status, ses_k.status), /* Resolve "OBJECT: dbid:objectid:indexid" only (cheap, cross-db via OBJECT_NAME). KEY/PAGE/RID - stay raw -- resolving a hobt cross-database needs dynamic SQL, deliberately skipped. */ + are not resolved -- resolving a hobt cross-database needs dynamic SQL, deliberately skipped + (this sweep runs on a much tighter cadence than the blocked-process reader). Since #1893 they + are NAMED instead: the report collector's exact sentinel shape, carrying the database the + lock RESOURCE lives in, so one contended object fingerprints once across both collectors. */ contentious_object = CASE WHEN objparse.object_id IS NOT NULL THEN ISNULL(QUOTENAME(OBJECT_SCHEMA_NAME(objparse.object_id, objparse.database_id)) + N'.' + QUOTENAME(OBJECT_NAME(objparse.object_id, objparse.database_id)), der_b.wait_resource) + WHEN resparse.resource_database_id IS NOT NULL + THEN N'Unresolved: ' + + CASE + WHEN resparse.lock_type IS NULL + THEN N'' + ELSE LOWER(resparse.lock_type) + N' lock, ' + END + + N'database: ' + COALESCE(DB_NAME(resparse.resource_database_id), DB_NAME(der_b.database_id), N'unknown') ELSE der_b.wait_resource END, blocked_sql_text = dest_b.text, @@ -212,6 +223,44 @@ BEGIN object_id = TRY_CONVERT(integer, PARSENAME(REPLACE(SUBSTRING(der_b.wait_resource, 9, 200), N':', N'.'), 2)) WHERE der_b.wait_resource LIKE N'OBJECT: %' ) AS objparse + /* #1893: the resource's database id, and the lock type to name it with. Kept in lockstep with + PerformanceMonitor.Collectors/DmvBlockingSnapshotCollector.cs. + The id comes from sys.dm_os_waiting_tasks.resource_description rather than from wait_resource, + because MS Learn documents EVERY lock resource type's description as carrying a dbid= token -- + keylock, pagelock, ridlock, objectlock, databaselock, filelock, extentlock, applicationlock, + metadatalock, hobtlock and allocunitlock -- so one parse covers every lock shape instead of a + per-type positional split of wait_resource. Verified live on SQL 2022: a cross-database KEY lock + reports 'keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...'. + The lock TYPE still comes from wait_resource, using the report collector's classifier verbatim. + Restricted to LCK_% waits with a wait_resource: latch and RESOURCE_SEMAPHORE rows have no + 'TYPE: ' resource shape and no dbid= to read, and they keep exactly the value they had. */ + OUTER APPLY + ( + SELECT + resource_database_id = + TRY_CONVERT + ( + integer, + NULLIF(LEFT(d.tail, PATINDEX(N'%[^0-9]%', d.tail + N'.') - 1), N'') + ), + lock_type = + CASE + WHEN der_b.wait_resource LIKE N'%KEY: %' THEN N'KEY' + WHEN der_b.wait_resource LIKE N'%OBJECT: %' THEN N'OBJECT' + WHEN der_b.wait_resource LIKE N'%RID: %' THEN N'RID' + WHEN der_b.wait_resource LIKE N'%PAGE: %' THEN N'PAGE' + ELSE LEFT(UPPER(LEFT(der_b.wait_resource, CHARINDEX(N':', der_b.wait_resource + N':') - 1)), 32) + END + FROM + ( + SELECT + tail = SUBSTRING(wt.resource_description, CHARINDEX(N'dbid=', wt.resource_description) + 5, 10) + ) AS d + WHERE wt.wait_type LIKE N'LCK[_]%' + AND der_b.wait_resource IS NOT NULL + AND der_b.wait_resource <> N'' + AND CHARINDEX(N'dbid=', wt.resource_description) > 0 + ) AS resparse WHERE wt.blocking_session_id IS NOT NULL AND wt.blocking_session_id <> 0 AND wt.blocking_session_id <> wt.session_id /*ignore intra-query (CXPACKET) self-waits*/