feat: Enhance user group management and transaction handling in stored procedures
- Improved user group deletion logic to accurately report success or failure. - Updated user group renaming checks to prevent unintended inserts or updates. - Added documentation on transaction blocking risks during full resets in stored procedures. - Removed unnecessary login scripts from database project.
This commit is contained in:
Stefan
@@ -0,0 +1,65 @@
|
||||
---
|
||||
status: Accepted
|
||||
date: 2026-09-16
|
||||
applyTo:
|
||||
- "Fuchs_Database/dbo/Stored Procedures/mfr__updt__*.sql"
|
||||
- "Fuchs_DataService/FdsMfrClient.cs"
|
||||
supersededBy: ""
|
||||
---
|
||||
|
||||
# 0022 — MFR sync transaction blocking during full resets is an accepted risk
|
||||
|
||||
## Context
|
||||
Every `mfr__updt__*` stored procedure (25 of them, one per synced mfr entity table) wraps its
|
||||
DELETE-stale-references + MERGE-upsert logic in a single explicit `BEGIN TRANSACTION` /
|
||||
`COMMIT TRANSACTION`, with `SET XACT_ABORT ON` and a `TRY/CATCH` that rolls back and logs to
|
||||
`fds__admin_dberrors` on failure. This gives all-or-nothing atomicity for one entity's sync
|
||||
batch: either every stale side-table row is cleaned up and the target table is upserted, or
|
||||
none of it is, so a mid-batch failure can never leave orphaned side-table rows alongside a
|
||||
stale/missing target row.
|
||||
|
||||
`FdsMfrClient.cs`'s `CommandAfter` (which invokes the `mfr__updt__*` proc) and `CommandBefore`/
|
||||
staging `SqlBulkCopy` both use a 600s command timeout (`MergeCommandTimeoutSeconds`), raised
|
||||
from `SqlCommand`'s 30s default specifically so a **full reset** of a large entity table (not
|
||||
the normal incremental sync, which only carries the rows that changed since the last run) has
|
||||
room to complete. Before that timeout was raised, a slow full-reset batch would self-abort at
|
||||
30s — the client cancellation would cancel the batch and roll back the open transaction —
|
||||
capping how long the transaction (and the locks it holds on the target table and the affected
|
||||
side tables) could stay open. With the timeout at 600s, a genuinely large full-reset batch can
|
||||
now hold that same transaction, and therefore those same locks, for up to ten minutes, during
|
||||
which other readers of the live `mfr__*` tables (e.g. intranet pages querying companies,
|
||||
contacts, invoices, …) can block waiting for the write transaction to release its locks —
|
||||
instead of blocking, at most, on one DELETE or one MERGE statement individually.
|
||||
|
||||
## Decision
|
||||
Accept this as a known, documented risk rather than changing behavior. Full resets are rare
|
||||
(a manual/administrative operation, not the routine incremental sync path) and are expected to
|
||||
run outside of business hours. The atomicity the transaction provides — no orphaned side-table
|
||||
rows left behind by a failed sync — is worth more than shortening the worst-case blocking
|
||||
window for an operation this infrequent.
|
||||
|
||||
## Consequences
|
||||
- A full reset of a large `mfr__*` entity table can block ordinary intranet reads of that
|
||||
table (and the side tables `mfr__*PartnerSet`, `mfr__#customvalues`, `mfr__#locations`,
|
||||
`mfr__comments`) for as long as that reset's DELETE+MERGE takes to execute, up to the 600s
|
||||
command timeout.
|
||||
- Anyone scheduling or manually triggering a full reset of a large entity table should do so
|
||||
during low-traffic hours, same as any other maintenance operation with table-level locking.
|
||||
- If full resets become frequent enough, or large enough, for this blocking window to matter in
|
||||
practice, revisit this decision — options considered and rejected here (see below) remain
|
||||
available.
|
||||
|
||||
## Alternatives considered
|
||||
- **Drop the explicit transaction**, letting each DELETE/MERGE statement auto-commit
|
||||
individually (the behavior before the transaction wrapping was added). Shortens the lock
|
||||
window back to per-statement, but reintroduces the exact orphaned-data risk on mid-batch
|
||||
failure that the transaction wrapping was added to prevent. Rejected — full-batch atomicity
|
||||
was a deliberate improvement and giving it up trades a rare, low-traffic-hours blocking issue
|
||||
for a correctness regression on every sync.
|
||||
- **Enable Read Committed Snapshot Isolation (RCSI)** (`ALTER DATABASE ... SET
|
||||
READ_COMMITTED_SNAPSHOT ON`) so readers use row versioning instead of blocking on the writer's
|
||||
locks, keeping the transaction's atomicity intact while removing reader blocking entirely.
|
||||
This is the architecturally cleanest fix, but it is a database-wide isolation behavior change
|
||||
(affects every table, every query, tempdb version-store load) that needs its own testing and
|
||||
sign-off rather than being folded into this change. Left as the preferred follow-up if the
|
||||
accepted risk above turns out not to be acceptable in practice.
|
||||
@@ -49,14 +49,18 @@ let $usr = {
|
||||
},
|
||||
/* Re-fetches a single user and swaps its grid row in place, keeping selection + ribbon
|
||||
(used after changes that only affect that one row: email, group membership). Falls back
|
||||
to a full reload when the row is no longer in the grid. */
|
||||
to a full reload when the row is no longer in the grid. Looks up the current row node by
|
||||
id inside the response handler (not before the request) so two overlapping calls — e.g.
|
||||
two quick checkbox toggles in groupsDlg — each replace whatever row is live at the time
|
||||
their own response arrives, instead of the second call operating on a node the first
|
||||
already detached via replaceWith. */
|
||||
refreshRow: function (rw) {
|
||||
let old = $('#usr' + rw.useraccount_id);
|
||||
if (old.length === 0) { $usr.renderLst(); return; }
|
||||
if ($('#usr' + rw.useraccount_id).length === 0) { $usr.renderLst(); return; }
|
||||
$ocms.postXT({
|
||||
url: $ocms.url('usr/list'), data: { useraccount_id: rw.useraccount_id }, success: function (r) {
|
||||
let fresh = (r.users || [])[0];
|
||||
if (!fresh) { $usr.renderLst(); return; }
|
||||
let old = $('#usr' + rw.useraccount_id);
|
||||
if (!fresh || old.length === 0) { $usr.renderLst(); return; }
|
||||
let selected = old.is('.selected');
|
||||
let row = $usr.buildRow(fresh).toggleClass('selected', selected);
|
||||
old.replaceWith(row);
|
||||
|
||||
@@ -16,7 +16,10 @@ public class FdsMfrClient : IDisposable
|
||||
private readonly MFRClient _mfrClient;
|
||||
private readonly ILogger<FdsMfrClient> _logger;
|
||||
|
||||
// Full-reset merges of large entity tables exceed SqlCommand's 30 s default.
|
||||
// Full-reset merges of large entity tables exceed SqlCommand's 30 s default. Raising this
|
||||
// also raises how long a full-reset's mfr__updt__* transaction can hold locks and block
|
||||
// readers of the live mfr__* tables — an accepted, documented risk, see
|
||||
// Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md.
|
||||
private const int MergeCommandTimeoutSeconds = 600;
|
||||
|
||||
private static MFRClientCredentials DefaultCredentials() =>
|
||||
@@ -259,7 +262,13 @@ public class FdsMfrClient : IDisposable
|
||||
try
|
||||
{
|
||||
tbl.AcceptChanges();
|
||||
var dtwa = new DatatableWriterAsync(tbl, FdsShared.FDSConnectionString());
|
||||
var dtwa = new DatatableWriterAsync(tbl, FdsShared.FDSConnectionString())
|
||||
{
|
||||
// A large full-reset batch can exceed SqlBulkCopy's 30s default just as
|
||||
// easily as the merge proc it feeds into — match CommandAfter's timeout
|
||||
// so staging itself doesn't fail before the merge/fds__setStatus ever runs.
|
||||
BulkCopyTimeoutSeconds = MergeCommandTimeoutSeconds
|
||||
};
|
||||
// One fixed debug copy per entity table (last failed batch) instead of a new t_<setid>_ table per attempt.
|
||||
string fallbackTable = $"t_err_{tbl.TableName}";
|
||||
dtwa.CommandAfterError = new SqlCommand(
|
||||
|
||||
@@ -146,8 +146,6 @@
|
||||
<Build Include="Security\fds_rwe.sql" />
|
||||
<Build Include="Security\fuchs_web.sql" />
|
||||
<Build Include="Security\fuchs_fds.sql" />
|
||||
<Build Include="Security\fuchs_web.login.sql" />
|
||||
<Build Include="Security\fuchs_fds.login.sql" />
|
||||
<Build Include="dbo\Functions\vat_val.sql" />
|
||||
<Build Include="dbo\Functions\strings_replaceSpecialCharsInHTML.sql" />
|
||||
<Build Include="dbo\Functions\strings_removeLeading0_255.sql" />
|
||||
|
||||
@@ -1,6 +0,0 @@
|
||||
-- Server-level login referenced by the [fuchs_fds] database user.
|
||||
-- Exists only so the project model resolves (SQL71501); the real password
|
||||
-- is managed on the server and never taken from this script.
|
||||
CREATE LOGIN [fuchs_fds]
|
||||
WITH PASSWORD = N'placeholder-not-deployed';
|
||||
|
||||
@@ -1,6 +0,0 @@
|
||||
-- Server-level login referenced by the [fuchs_web] database user.
|
||||
-- Exists only so the project model resolves (SQL71501); the real password
|
||||
-- is managed on the server and never taken from this script.
|
||||
CREATE LOGIN [fuchs_web]
|
||||
WITH PASSWORD = N'placeholder-not-deployed';
|
||||
|
||||
@@ -13,6 +13,11 @@ BEGIN
|
||||
SET NOCOUNT ON;
|
||||
DECLARE @utcnow datetime = GETUTCDATE();
|
||||
DECLARE @auth smallint = [dbo].[fis_admin_getUserAuth](@authaccount);
|
||||
-- Tracks whether THIS call actually soft-deleted an active group, so a never-existing or
|
||||
-- already-deleted @usergroup_id reports failure instead of a false "ok:true" (the previous
|
||||
-- version derived success from "not currently active", which is also true when nothing here
|
||||
-- ever existed).
|
||||
DECLARE @deleted bit = 0;
|
||||
|
||||
IF @auth > 1 AND EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id and [DateDeleted] is null)
|
||||
BEGIN
|
||||
@@ -21,10 +26,22 @@ BEGIN
|
||||
SET [UserDeleted] = @authaccount
|
||||
,[DateDeleted] = @utcnow
|
||||
WHERE
|
||||
[usergroup_id] = @usergroup_id;
|
||||
[usergroup_id] = @usergroup_id
|
||||
AND [DateDeleted] is null;
|
||||
|
||||
IF @@ROWCOUNT > 0 SET @deleted = 1;
|
||||
|
||||
-- Soft-delete the group's active memberships too, matching the confirm text
|
||||
-- ("Die Mitgliedschaft aller Benutzer in dieser Gruppe geht dabei verloren.").
|
||||
UPDATE [dbo].[fis_usergroups_members]
|
||||
SET [UserDeleted] = @authaccount
|
||||
,[DateDeleted] = @utcnow
|
||||
WHERE
|
||||
[usergroup_id] = @usergroup_id
|
||||
AND [DateDeleted] is null;
|
||||
|
||||
END
|
||||
|
||||
SELECT CAST((CASE WHEN NOT EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id and [DateDeleted] is null) THEN 1 ELSE 0 END) as bit);
|
||||
SELECT @deleted;
|
||||
|
||||
END
|
||||
@@ -17,9 +17,14 @@ BEGIN
|
||||
|
||||
DECLARE @createdID as TABLE ([id] varchar(5));
|
||||
|
||||
IF @auth > 1
|
||||
-- Renaming an existing group (@usergroup_id supplied) requires that id to currently be an
|
||||
-- active group; otherwise the MERGE below would silently INSERT it as a brand-new group
|
||||
-- (rename-of-unknown-id becoming create) or resurrect/UPDATE a soft-deleted row without
|
||||
-- clearing its DateDeleted (leaving it deleted yet renamed). @usergroup_id = NULL (create)
|
||||
-- always passes this check.
|
||||
IF @auth > 1 AND (@usergroup_id IS NULL OR EXISTS (SELECT * FROM [dbo].[fis_usergroups] WHERE [usergroup_id] = @usergroup_id AND [DateDeleted] IS NULL))
|
||||
BEGIN
|
||||
|
||||
|
||||
MERGE [dbo].[fis_usergroups] as TARGET
|
||||
USING (VALUES( ISNULL(@usergroup_id, [dbo].[fis_fn_id] ('fis_usergroups'))
|
||||
,ENCRYPTBYKEY(@enc_key, @name)
|
||||
|
||||
@@ -20,6 +20,11 @@ BEGIN
|
||||
DECLARE @tmp_cmd nvarchar(1000) = N'SELECT DISTINCT * FROM ' + @tblname
|
||||
INSERT INTO @tmp EXECUTE [sp_executesql] @tmp_cmd;
|
||||
|
||||
-- Wrapping the DELETEs + MERGE below in one transaction gives all-or-nothing atomicity for
|
||||
-- this entity's sync batch (same pattern in every mfr__updt__* proc). During a large full
|
||||
-- reset this can hold locks on the affected tables for as long as the batch takes to run —
|
||||
-- an accepted, documented risk, not an oversight. See
|
||||
-- Docs/Decisions/0022-mfr-sync-transaction-blocking-accepted-risk.md before changing this.
|
||||
BEGIN TRANSACTION;
|
||||
|
||||
IF @referencetable = 'mfr__companies'
|
||||
|
||||
Reference in New Issue
Block a user