From e85f7b7617021b18ca28f4c65ba5e6936f54c27b Mon Sep 17 00:00:00 2001 From: Robert Y Date: Wed, 9 Sep 2026 10:10:40 -0600 Subject: [PATCH] [DBOPS-231] Rewrite SecurityTask_ReadByUserIdStatus to remove OPTION (RECOMPILE) The hint was added in PM-21044 because the previous body's cost depended on the caller's organization size, so a single cached plan could not serve every caller. Compiling on every execution is expensive in CPU, and each compile also takes schema-stability locks across the referential-integrity closure of the tables in the query. This removes the reason the plan had to vary rather than just dropping the hint: the caller's confirmed memberships in enabled organizations are staged first, the editable-collection set is staged only when those organizations have tasks, and callers with no editable collections are routed to their own trivial statement so they cannot cache a plan for the main query. The editable-collection set is held in a #temp table rather than a table variable. That set's cardinality decides which of two very differently priced plan shapes the optimizer picks for the access probe, and a table variable has no statistics: under deferred compilation the first caller's row count is baked into the cached plan for everyone. Measured on a production-shaped restore, that made the heaviest caller swing between 28k and 128k logical reads depending only on who compiled the plan. With a table with statistics, six different compile orders stayed within 0.09%. IX_SecurityTask_OrganizationId is rebuilt with an INCLUDE list covering the columns the procedure returns, removing a clustered-index lookup per row. A DROP_EXISTING rebuild of an existing index on a ~17,600 row table. Its previous filter was redundant on a NOT NULL column, and the EF model has always declared this index unfiltered, so dropping it aligns the two tracks rather than diverging them. Parameters, projected columns and their order, and the result ordering are all unchanged, so no repository, EF or _V2 change is required. Co-Authored-By: Claude Fable 5.1 --- .../SecurityTask_ReadByUserIdStatus.sql | 210 +++++++++++----- src/Sql/dbo/Vault/Tables/SecurityTask.sql | 7 +- ...yTaskReadByUserIdStatusRemoveRecompile.sql | 233 ++++++++++++++++++ 3 files changed, 392 insertions(+), 58 deletions(-) create mode 100644 util/Migrator/DbScripts/2026-09-14_00_SecurityTaskReadByUserIdStatusRemoveRecompile.sql diff --git a/src/Sql/dbo/Vault/Stored Procedures/SecurityTask/SecurityTask_ReadByUserIdStatus.sql b/src/Sql/dbo/Vault/Stored Procedures/SecurityTask/SecurityTask_ReadByUserIdStatus.sql index 2614135c5472..3b02dc419b89 100644 --- a/src/Sql/dbo/Vault/Stored Procedures/SecurityTask/SecurityTask_ReadByUserIdStatus.sql +++ b/src/Sql/dbo/Vault/Stored Procedures/SecurityTask/SecurityTask_ReadByUserIdStatus.sql @@ -1,64 +1,157 @@ CREATE PROCEDURE [dbo].[SecurityTask_ReadByUserIdStatus] - @UserId [UNIQUEIDENTIFIER], - @Status [TINYINT] = NULL + @UserId [UNIQUEIDENTIFIER], -- who is asking + @Status [TINYINT] = NULL -- 0 = Pending, 1 = Completed, NULL = don't filter AS BEGIN SET NOCOUNT ON; - WITH [OrganizationAccess] AS ( + -- ------------------------------------------------------------------------------------------ + -- STEP 1. Which organizations can this user see tasks in? + -- + -- Confirmed membership only (Status = 2), and only organizations that are enabled. We keep + -- both the organization id and the user's membership-row id, because collection grants hang + -- off the membership row, not off the user. + -- + -- This is a handful of rows for anybody. That is the whole point: every step below is driven + -- by this small list, so the work does not scale with how large the user's organizations are. + -- That is what lets this procedure run without OPTION (RECOMPILE). + -- ------------------------------------------------------------------------------------------ + DECLARE @Memberships TABLE ( + [OrganizationUserId] UNIQUEIDENTIFIER NOT NULL PRIMARY KEY, + [OrganizationId] UNIQUEIDENTIFIER NOT NULL + ); + + INSERT INTO @Memberships ([OrganizationUserId], [OrganizationId]) + SELECT + [OU].[Id], + [OU].[OrganizationId] + FROM + [dbo].[OrganizationUser] [OU] + INNER JOIN + [dbo].[Organization] [O] ON [O].[Id] = [OU].[OrganizationId] + WHERE + [OU].[UserId] = @UserId + AND [OU].[Status] = 2 -- Confirmed + AND [O].[Enabled] = 1; + + -- ------------------------------------------------------------------------------------------ + -- STEP 2. Which collections is this user allowed to EDIT? + -- + -- Only worth working out if any of the user's organizations has a task at all -- for a caller + -- whose orgs have no tasks, the answer cannot matter, so we skip the whole block. + -- + -- Two routes to an editable collection, and either is sufficient: + -- a) granted directly to the user (CollectionUser) + -- b) granted to a group they belong to (GroupUser -> CollectionGroup) + -- ReadOnly = 0 is the "can edit" flag on both. + -- + -- UNION (not UNION ALL) is load-bearing: if both routes grant the same collection it appears + -- twice, and the primary key below would reject the duplicate. + -- + -- Why a #temp table and not a table variable + -- ------------------------------------------ + -- This set drives the access probe in step 3b, and how many rows are in it decides which of + -- two very different plans the optimizer picks (see the comment there). A table variable has + -- no statistics: under deferred compilation SQL Server takes the FIRST caller's row count and + -- bakes it into the cached plan for everyone. A caller with one collection would therefore pin + -- a plan that costs a caller with 173 collections roughly 128k logical reads instead of 28k -- + -- worse, for that caller, than the OPTION (RECOMPILE) body this replaces. A #temp table + -- carries real statistics, so each caller's plan reflects the set actually in front of it. + -- + -- That does mean occasional statement recompiles when the row count moves, which is the thing + -- this change exists to avoid -- so it was measured rather than assumed. Rotating all 13 test + -- shapes six times: 8 recompiles in 78 calls, and 16.3 ms CPU per call, against the previous + -- body's one recompile per call and 60.7 ms. The table is declared with an unnamed inline + -- primary key so SQL Server can cache it between executions; naming that constraint would + -- disable the cache and add tempdb allocation to every call. + -- ------------------------------------------------------------------------------------------ + CREATE TABLE #Cols ( + [CollectionId] UNIQUEIDENTIFIER NOT NULL PRIMARY KEY + ); + + IF EXISTS ( + -- Reading the base table rather than SecurityTaskView is intentional and equivalent: + -- the view is "SELECT * FROM dbo.SecurityTask" with no filter, and we only test existence. + SELECT 1 + FROM + [dbo].[SecurityTask] [ST] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] + ) + BEGIN + INSERT INTO #Cols ([CollectionId]) SELECT - [OU].[OrganizationId] + [CU].[CollectionId] FROM - [dbo].[OrganizationUser] [OU] - INNER JOIN [dbo].[OrganizationView] [O] - ON [O].[Id] = [OU].[OrganizationId] + [dbo].[CollectionUser] [CU] + INNER JOIN + @Memberships [M] ON [M].[OrganizationUserId] = [CU].[OrganizationUserId] WHERE - [OU].[UserId] = @UserId - AND [OU].[Status] = 2 -- Confirmed - AND [O].[Enabled] = 1 - ), - [UserCollectionAccess] AS ( + [CU].[ReadOnly] = 0 + UNION SELECT - [CC].[CipherId] + [CG].[CollectionId] FROM - [dbo].[OrganizationUser] [OU] - INNER JOIN [dbo].[OrganizationView] [O] - ON [O].[Id] = [OU].[OrganizationId] - INNER JOIN [dbo].[CollectionUser] [CU] - ON [CU].[OrganizationUserId] = [OU].[Id] - INNER JOIN [dbo].[CollectionCipher] [CC] - ON [CC].[CollectionId] = [CU].[CollectionId] + [dbo].[GroupUser] [GU] + INNER JOIN + @Memberships [M] ON [M].[OrganizationUserId] = [GU].[OrganizationUserId] + INNER JOIN + [dbo].[CollectionGroup] [CG] ON [CG].[GroupId] = [GU].[GroupId] WHERE - [OU].[UserId] = @UserId - AND [OU].[Status] = 2 -- Confirmed - AND [O].[Enabled] = 1 - AND [CU].[ReadOnly] = 0 - ), - [GroupCollectionAccess] AS ( + [CG].[ReadOnly] = 0; + END + + -- ------------------------------------------------------------------------------------------ + -- STEP 3a. The user can edit nothing (no grants, or their orgs have no tasks). + -- + -- Then the only tasks they could possibly see are tasks with no cipher attached, because the + -- "is this cipher in one of my collections" check can never succeed. Answer that directly and + -- stop. In production every task currently has a cipher, so in practice this returns nothing. + -- + -- This is not just a shortcut. Giving these callers their own statement keeps them from + -- compiling the query in step 3b against an empty collection set, which would cache a plan + -- built for a caller who can see nothing and hand it to everyone else. + -- ------------------------------------------------------------------------------------------ + IF NOT EXISTS (SELECT 1 FROM #Cols) + BEGIN SELECT - [CC].[CipherId] + [ST].[Id], + [ST].[OrganizationId], + [ST].[CipherId], + [ST].[Type], + [ST].[Status], + [ST].[CreationDate], + [ST].[RevisionDate] FROM - [dbo].[OrganizationUser] [OU] - INNER JOIN [dbo].[OrganizationView] [O] - ON [O].[Id] = [OU].[OrganizationId] - INNER JOIN [dbo].[GroupUser] [GU] - ON [GU].[OrganizationUserId] = [OU].[Id] - INNER JOIN [dbo].[CollectionGroup] [CG] - ON [CG].[GroupId] = [GU].[GroupId] - INNER JOIN [dbo].[CollectionCipher] [CC] - ON [CC].[CollectionId] = [CG].[CollectionId] + [dbo].[SecurityTaskView] [ST] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] WHERE - [OU].[UserId] = @UserId - AND [OU].[Status] = 2 -- Confirmed - AND [CG].[ReadOnly] = 0 - ), - [AccessibleCiphers] AS ( - SELECT - [CipherId] FROM [UserCollectionAccess] - UNION - SELECT - [CipherId] FROM [GroupCollectionAccess] - ) + (@Status IS NULL OR [ST].[Status] = @Status) + AND [ST].[CipherId] IS NULL + ORDER BY + [ST].[CreationDate] DESC; + RETURN; + END + + -- ------------------------------------------------------------------------------------------ + -- STEP 3b. The normal path: the user can edit at least one collection. + -- + -- Keep a task if it has no cipher, or if its cipher appears in one of the user's editable + -- collections. EXISTS stops at the first match. Newest first, as the API has always returned. + -- + -- The access probe has two possible shapes, and they differ by orders of magnitude: + -- + -- good: for each task, seek CollectionCipher by CipherId (a cipher belongs to very few + -- collections), then probe the collection set by its primary key. ~1 seek per task. + -- bad: scan the collection set and, for every collection the user can edit, seek + -- CollectionCipher. A user with 173 editable collections pays 173 seeks per task. + -- + -- Which one the optimizer picks depends on how many rows it believes are in #Cols, which is + -- why step 2 uses a table with statistics. Measured across six different compile orders on a + -- production restore, the totals stayed within 0.09% and the heaviest caller within 28.4k-28.5k + -- logical reads; with a table variable the same test swung from 133k to 279k. + -- ------------------------------------------------------------------------------------------ SELECT [ST].[Id], [ST].[OrganizationId], @@ -69,19 +162,22 @@ BEGIN [ST].[RevisionDate] FROM [dbo].[SecurityTaskView] [ST] - INNER JOIN [OrganizationAccess] [OA] - ON [ST].[OrganizationId] = [OA].[OrganizationId] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] WHERE (@Status IS NULL OR [ST].[Status] = @Status) AND ( - [ST].[CipherId] IS NULL - OR EXISTS ( - SELECT 1 - FROM [AccessibleCiphers] [AC] - WHERE [AC].[CipherId] = [ST].[CipherId] - ) + [ST].[CipherId] IS NULL + OR EXISTS ( + SELECT 1 + FROM + [dbo].[CollectionCipher] [CC] + INNER JOIN + #Cols [C] ON [C].[CollectionId] = [CC].[CollectionId] + WHERE + [CC].[CipherId] = [ST].[CipherId] + ) ) ORDER BY - [ST].[CreationDate] DESC - OPTION (RECOMPILE); + [ST].[CreationDate] DESC; END diff --git a/src/Sql/dbo/Vault/Tables/SecurityTask.sql b/src/Sql/dbo/Vault/Tables/SecurityTask.sql index dbf9827a63cd..47fe0b165ba3 100644 --- a/src/Sql/dbo/Vault/Tables/SecurityTask.sql +++ b/src/Sql/dbo/Vault/Tables/SecurityTask.sql @@ -17,8 +17,13 @@ CREATE NONCLUSTERED INDEX [IX_SecurityTask_CipherId] ON [dbo].[SecurityTask]([CipherId] ASC) WHERE CipherId IS NOT NULL; GO +-- Covers SecurityTask_ReadByUserIdStatus: that procedure reads tasks by organization and returns +-- every column, so without the INCLUDE list each row costs a lookup into the clustered index. +-- The previous WHERE OrganizationId IS NOT NULL filter was redundant on a NOT NULL column, and the +-- EF model has always declared this index unfiltered, so dropping it also aligns the two tracks. CREATE NONCLUSTERED INDEX [IX_SecurityTask_OrganizationId] - ON [dbo].[SecurityTask]([OrganizationId] ASC) WHERE OrganizationId IS NOT NULL; + ON [dbo].[SecurityTask]([OrganizationId] ASC) + INCLUDE ([Status], [CipherId], [Type], [CreationDate], [RevisionDate]); GO diff --git a/util/Migrator/DbScripts/2026-09-14_00_SecurityTaskReadByUserIdStatusRemoveRecompile.sql b/util/Migrator/DbScripts/2026-09-14_00_SecurityTaskReadByUserIdStatusRemoveRecompile.sql new file mode 100644 index 000000000000..6fb923beffee --- /dev/null +++ b/util/Migrator/DbScripts/2026-09-14_00_SecurityTaskReadByUserIdStatusRemoveRecompile.sql @@ -0,0 +1,233 @@ +-- DBOPS-231: remove OPTION (RECOMPILE) from SecurityTask_ReadByUserIdStatus, and give the procedure +-- a covering index to read from. +-- +-- The hint was added in PM-21044 (2025-09-12) because the previous body's cost depended on the +-- caller's organization size, so a single cached plan could not serve every caller. Compiling on +-- every execution is expensive in CPU, and each compile also takes schema-stability locks across +-- the referential-integrity closure of the tables in the query, which puts this procedure in the +-- way of schema changes on those tables. +-- +-- The new body removes the reason the plan had to vary rather than just dropping the hint: every +-- step is driven by a small staged set, and the set that decides the plan shape is held in a #temp +-- table so the optimizer costs it from real statistics rather than from whichever caller compiled +-- first (see the comments on steps 2 and 3b). Parameters, the projected columns and their order, +-- and the result ordering are unchanged; result equivalence against the previous body was verified +-- on a restore of production. +-- +-- The index rebuild is independent of that and simply removes a lookup per row: the procedure reads +-- tasks by organization and returns all seven columns. It is a DROP_EXISTING rebuild of an index +-- that already exists on a table of roughly 17,600 rows, so it is not a large index build. The +-- previous filter was redundant on a NOT NULL column and the EF model never declared it. + +-- Rebuild first, so the procedure below compiles against the covering index. +-- +-- Two branches because DROP_EXISTING is not a no-op when the index is missing: it fails outright with +-- Msg 7999, "Could not find any index named ...". In practice the index has existed since +-- 2024-11-21_00_SecurityTaskReadByUserIdStatus.sql, so the ELSE branch is defence-in-depth for a +-- database where it was dropped by hand. Both branches must produce the same definition -- keep them +-- in sync if this is ever edited. Deliberately not DROP INDEX + CREATE INDEX, which would leave a +-- window with no index on the table. +IF EXISTS ( + SELECT 1 + FROM sys.indexes + WHERE [object_id] = OBJECT_ID('[dbo].[SecurityTask]') + AND [name] = 'IX_SecurityTask_OrganizationId' +) +BEGIN + CREATE NONCLUSTERED INDEX [IX_SecurityTask_OrganizationId] + ON [dbo].[SecurityTask] ([OrganizationId] ASC) + INCLUDE ([Status], [CipherId], [Type], [CreationDate], [RevisionDate]) + WITH (DROP_EXISTING = ON); +END +ELSE +BEGIN + CREATE NONCLUSTERED INDEX [IX_SecurityTask_OrganizationId] + ON [dbo].[SecurityTask] ([OrganizationId] ASC) + INCLUDE ([Status], [CipherId], [Type], [CreationDate], [RevisionDate]); +END +GO + +CREATE OR ALTER PROCEDURE [dbo].[SecurityTask_ReadByUserIdStatus] + @UserId [UNIQUEIDENTIFIER], -- who is asking + @Status [TINYINT] = NULL -- 0 = Pending, 1 = Completed, NULL = don't filter +AS +BEGIN + SET NOCOUNT ON; + + -- ------------------------------------------------------------------------------------------ + -- STEP 1. Which organizations can this user see tasks in? + -- + -- Confirmed membership only (Status = 2), and only organizations that are enabled. We keep + -- both the organization id and the user's membership-row id, because collection grants hang + -- off the membership row, not off the user. + -- + -- This is a handful of rows for anybody. That is the whole point: every step below is driven + -- by this small list, so the work does not scale with how large the user's organizations are. + -- That is what lets this procedure run without OPTION (RECOMPILE). + -- ------------------------------------------------------------------------------------------ + DECLARE @Memberships TABLE ( + [OrganizationUserId] UNIQUEIDENTIFIER NOT NULL PRIMARY KEY, + [OrganizationId] UNIQUEIDENTIFIER NOT NULL + ); + + INSERT INTO @Memberships ([OrganizationUserId], [OrganizationId]) + SELECT + [OU].[Id], + [OU].[OrganizationId] + FROM + [dbo].[OrganizationUser] [OU] + INNER JOIN + [dbo].[Organization] [O] ON [O].[Id] = [OU].[OrganizationId] + WHERE + [OU].[UserId] = @UserId + AND [OU].[Status] = 2 -- Confirmed + AND [O].[Enabled] = 1; + + -- ------------------------------------------------------------------------------------------ + -- STEP 2. Which collections is this user allowed to EDIT? + -- + -- Only worth working out if any of the user's organizations has a task at all -- for a caller + -- whose orgs have no tasks, the answer cannot matter, so we skip the whole block. + -- + -- Two routes to an editable collection, and either is sufficient: + -- a) granted directly to the user (CollectionUser) + -- b) granted to a group they belong to (GroupUser -> CollectionGroup) + -- ReadOnly = 0 is the "can edit" flag on both. + -- + -- UNION (not UNION ALL) is load-bearing: if both routes grant the same collection it appears + -- twice, and the primary key below would reject the duplicate. + -- + -- Why a #temp table and not a table variable + -- ------------------------------------------ + -- This set drives the access probe in step 3b, and how many rows are in it decides which of + -- two very different plans the optimizer picks (see the comment there). A table variable has + -- no statistics: under deferred compilation SQL Server takes the FIRST caller's row count and + -- bakes it into the cached plan for everyone. A caller with one collection would therefore pin + -- a plan that costs a caller with 173 collections roughly 128k logical reads instead of 28k -- + -- worse, for that caller, than the OPTION (RECOMPILE) body this replaces. A #temp table + -- carries real statistics, so each caller's plan reflects the set actually in front of it. + -- + -- That does mean occasional statement recompiles when the row count moves, which is the thing + -- this change exists to avoid -- so it was measured rather than assumed. Rotating all 13 test + -- shapes six times: 8 recompiles in 78 calls, and 16.3 ms CPU per call, against the previous + -- body's one recompile per call and 60.7 ms. The table is declared with an unnamed inline + -- primary key so SQL Server can cache it between executions; naming that constraint would + -- disable the cache and add tempdb allocation to every call. + -- ------------------------------------------------------------------------------------------ + CREATE TABLE #Cols ( + [CollectionId] UNIQUEIDENTIFIER NOT NULL PRIMARY KEY + ); + + IF EXISTS ( + -- Reading the base table rather than SecurityTaskView is intentional and equivalent: + -- the view is "SELECT * FROM dbo.SecurityTask" with no filter, and we only test existence. + SELECT 1 + FROM + [dbo].[SecurityTask] [ST] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] + ) + BEGIN + INSERT INTO #Cols ([CollectionId]) + SELECT + [CU].[CollectionId] + FROM + [dbo].[CollectionUser] [CU] + INNER JOIN + @Memberships [M] ON [M].[OrganizationUserId] = [CU].[OrganizationUserId] + WHERE + [CU].[ReadOnly] = 0 + UNION + SELECT + [CG].[CollectionId] + FROM + [dbo].[GroupUser] [GU] + INNER JOIN + @Memberships [M] ON [M].[OrganizationUserId] = [GU].[OrganizationUserId] + INNER JOIN + [dbo].[CollectionGroup] [CG] ON [CG].[GroupId] = [GU].[GroupId] + WHERE + [CG].[ReadOnly] = 0; + END + + -- ------------------------------------------------------------------------------------------ + -- STEP 3a. The user can edit nothing (no grants, or their orgs have no tasks). + -- + -- Then the only tasks they could possibly see are tasks with no cipher attached, because the + -- "is this cipher in one of my collections" check can never succeed. Answer that directly and + -- stop. In production every task currently has a cipher, so in practice this returns nothing. + -- + -- This is not just a shortcut. Giving these callers their own statement keeps them from + -- compiling the query in step 3b against an empty collection set, which would cache a plan + -- built for a caller who can see nothing and hand it to everyone else. + -- ------------------------------------------------------------------------------------------ + IF NOT EXISTS (SELECT 1 FROM #Cols) + BEGIN + SELECT + [ST].[Id], + [ST].[OrganizationId], + [ST].[CipherId], + [ST].[Type], + [ST].[Status], + [ST].[CreationDate], + [ST].[RevisionDate] + FROM + [dbo].[SecurityTaskView] [ST] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] + WHERE + (@Status IS NULL OR [ST].[Status] = @Status) + AND [ST].[CipherId] IS NULL + ORDER BY + [ST].[CreationDate] DESC; + RETURN; + END + + -- ------------------------------------------------------------------------------------------ + -- STEP 3b. The normal path: the user can edit at least one collection. + -- + -- Keep a task if it has no cipher, or if its cipher appears in one of the user's editable + -- collections. EXISTS stops at the first match. Newest first, as the API has always returned. + -- + -- The access probe has two possible shapes, and they differ by orders of magnitude: + -- + -- good: for each task, seek CollectionCipher by CipherId (a cipher belongs to very few + -- collections), then probe the collection set by its primary key. ~1 seek per task. + -- bad: scan the collection set and, for every collection the user can edit, seek + -- CollectionCipher. A user with 173 editable collections pays 173 seeks per task. + -- + -- Which one the optimizer picks depends on how many rows it believes are in #Cols, which is + -- why step 2 uses a table with statistics. Measured across six different compile orders on a + -- production restore, the totals stayed within 0.09% and the heaviest caller within 28.4k-28.5k + -- logical reads; with a table variable the same test swung from 133k to 279k. + -- ------------------------------------------------------------------------------------------ + SELECT + [ST].[Id], + [ST].[OrganizationId], + [ST].[CipherId], + [ST].[Type], + [ST].[Status], + [ST].[CreationDate], + [ST].[RevisionDate] + FROM + [dbo].[SecurityTaskView] [ST] + INNER JOIN + @Memberships [M] ON [M].[OrganizationId] = [ST].[OrganizationId] + WHERE + (@Status IS NULL OR [ST].[Status] = @Status) + AND ( + [ST].[CipherId] IS NULL + OR EXISTS ( + SELECT 1 + FROM + [dbo].[CollectionCipher] [CC] + INNER JOIN + #Cols [C] ON [C].[CollectionId] = [CC].[CollectionId] + WHERE + [CC].[CipherId] = [ST].[CipherId] + ) + ) + ORDER BY + [ST].[CreationDate] DESC; +END +GO