Sql-server – Is it good practice to only delete through a trigger to enforce ‘deleted by’ auditing

auditsql servert-sqltrigger

We've decided to design our database such that we actually delete rows when we want to delete some data, rather than just marking them as deleted, because the latter approach means that you always have to worry about filtering out the "marked as deleted" rows, which seems like an unnecessary headache.

However, we are auditing deleted rows in separate audit tables so as to keep the deleted data separately. We have a generic audit trigger that inserts all deleted columns (and their values) to the audit table on delete. We wish, though, to also audit the user ID of who deleted the row anytime a row is deleted. My idea is to have a deleted_by_user_id column on each table, and a trigger that blocks a delete if this column is not set. The transaction should fail and rollback if the "deleted by" user is not specified.

It would also automatically delete the row when it was set during an INSERT or UPDATE, meaning the row could only be deleted by setting this column. This would mean that any delete would necessarily include the user ID that deleted it as part of the audit. The account deleting the row will always be the service account for the web app in the database. It's our internal user IDs that we're interested in as to "who" is deleting the row, and that's what we want to capture.

Would this be good practice? What would be the pros and cons of using this method, and would it impact performance much compared to just using DELETE statements for row deletion?

Best Answer

I am not sure that using an IsDeleted column would create unnecessary headache. Locking down the table, and allowing non-administrative action via a view which filters on that column, while not displaying it, isn't very difficult. Add an Instead Of Delete trigger to the view, and you're all set.

Given that it's essentially boiler plate once you have the initial pattern, you could very easily write a procedure locks down the table and (re)generates an associated view, adding the filtering and trigger when it finds it has IsDeleted. It could add user logging requirements if there was a deleted_by_user_id field as well. I'd recommend parameter that allow rebuilding the world, or a pattern that is matched against table names (and/or schema).

In addition, versus forcing a deleted_by_user_id to be directly populated, I would look at leveraging SESSION_CONTEXT. The trigger would check against an agreed upon session context variable (e.g. 'AppUser'), throw if not set, then use that for populating deleted_by_user_id from that.

On the application side, you should be able to create sub-class SqlConnection and override Open. After calling the base Open, you should be able to run a EXEC sp_set_session_context N'AppUser', '{username}';, there by setting the App user for the session.

The advantage of using IsDeleted is that you don't have to write a whole row of data when doing a delete. If you do bulk deletes, or a lot of deletes in general, this could be advantageous. Though I would couple with Partial Index, cover just undeleted records. The advantage removing and writing to another table is that after reorganization you should have much more contiguous data, which in many cases generally makes things faster.

Related Solutions

Sql-server – Is it possible to raiserror within a For Delete Trigger that rolls back the deletes

In order for that to work as you've coded it above, you need a rollback after your RAISERROR or, as the comments correctly state, wrap in a TRY/CATCH.

By itself, it's not going to be treated the same as an actual statement or batch terminating error. For example, this will work the way you expect:

create table T1 ( i1 int  );
create table t2 (i2 int primary key);
go

create trigger T1_ForDelete on T1
for delete
as
    insert into t2 (i2)
    SELECT i1 
    FROM DELETED
go

insert into T1 (i1) values (1);
insert into t2 (i2) values (1);

delete from T1;
go

SELECT * from t1

In the above code the trigger will error, and cause the delete (outer) transaction to rollback. By itself, RAISERROR is only really transferring control and/or sending messages up the stack (so to speak)

BOL describes this behavior:

When RAISERROR is run with a severity of 11 or higher in a TRY block, it transfers control to the associated CATCH block. The error is returned to the caller if RAISERROR is run:

Outside the scope of any TRY block.

With a severity of 10 or lower in a TRY block.

With a severity of 20 or higher that terminates the database connection.

So RAISERROR outside the scope of a TRY block simply returns the error to the caller and is not treated as a statement-terminating error regardless of the severity you define.

Sql-server – Passing info on who deleted record onto a Delete trigger

Is there a way to pass information onto the Delete trigger such that it could know who deleted the record?

Yes: by using a very cool (and under utilized feature) called CONTEXT_INFO. It is essentially session memory that exists in all scopes and is not bound by transactions. It can be used to pass info (any info--well, any that fits into the limited space) to triggers as well as back and forth between sub-proc / EXEC calls. And I have used it before for this exact same situation.

Context info is a VARBINARY(128)
Set via: SET CONTEXT_INFO
Get via: CONTEXT_INFO()

Test with the following to see how it works. Notice that I am converting to CHAR(128) before the CONVERT(VARBINARY(128), ... This is to force blank-padding to make it easier to convert back to VARCHAR when getting it out of CONTEXT_INFO() since VARBINARY(128) is right-padded with 0x00s.

SELECT CONTEXT_INFO();
-- Initially = NULL

DECLARE @EncodedUser VARBINARY(128);
SET @EncodedUser = CONVERT(VARBINARY(128),
                            CONVERT(CHAR(128), 'I deleted ALL your records! HA HA!')
                          );
SET CONTEXT_INFO @EncodedUser;

SELECT CONTEXT_INFO() AS [RawContextInfo],
       RTRIM(CONVERT(VARCHAR(128), CONTEXT_INFO())) AS [DecodedUser];

Results:

0x492064656C6574656420414C4C20796F7572207265636F7264732120484120484121202020202020...
I deleted ALL your records! HA HA!

PUTTING IT ALL TOGETHER:

The app should call a "Delete" stored procedure that passes in the UserName (or whatever) that is deleting the record. I assume this is already the model being used since it sounds like you are already tracking Insert and Update operations.

The "Delete" stored procedure does:

DECLARE @EncodedUser VARBINARY(128);
SET @EncodedUser = CONVERT(VARBINARY(128),
                            CONVERT(CHAR(128), @UserName)
                          );
SET CONTEXT_INFO @EncodedUser;

-- DELETE STUFF HERE

The audit trigger does:

-- Set the INT value in LEFT (currently 50) to the max size of [UserWhoMadeChanges]
INSERT INTO AuditTable (IdOfRecordedAffected, UserWhoMadeChanges) 
   SELECT del.ID, COALESCE(
                     LEFT(RTRIM(CONVERT(VARCHAR(128), CONTEXT_INFO())), 50),
                     '<unknown>')
   FROM DELETED del;

Please note that, as @SeanGallardy pointed out in a comment, due to other procedures and/or ad hoc queries deleting records from this table, it is possible that either:
- CONTEXT_INFO has not been set and is still NULL:
  
  For this reason I have updated the above INSERT INTO AuditTable to use a COALESCE to default the value. Or, if you don't want a default and require a name, then you could do something similar to:
```
DECLARE @UserName VARCHAR(50); -- set to the size of AuditTable.[UserWhoMadeChanges]
SET @UserName = LEFT(RTRIM(CONVERT(VARCHAR(128), CONTEXT_INFO())), 50);

IF (@UserName IS NULL)
BEGIN
   ROLLBACK TRAN; -- cancel the DELETE operation
   RAISERROR('Please set UserName via "SET CONTEXT_INFO.." and try again.', 16 ,1);
END;

-- use @UserName in the INSERT...SELECT
```
- CONTEXT_INFO has been set to a value that is not a valid UserName, and hence might exceed the size of the AuditTable.[UserWhoMadeChanges] field:
  
  For this reason I added a LEFT function to ensure that whatever is grabbed out of CONTEXT_INFO will not break the INSERT. As noted in the code, you just need to set the 50 to the actual size of the UserWhoMadeChanges field.

UPDATE FOR SQL SERVER 2016 AND NEWER

SQL Server 2016 added an improved version of this per-session memory: Session Context. The new Session Context is essentially a hash table of Key-Value pairs with the "Key" being of type sysname (i.e. NVARCHAR(128) ) and the "Value" being SQL_VARIANT. Meaning:

There is now a separation of values so less likely to conflict with other uses
You can store various types, no longer needing to worry about the odd behavior when getting the value back out via CONTEXT_INFO() (for details, please see my post: Why Doesn’t CONTEXT_INFO() Return the Exact Value Set by SET CONTEXT_INFO?)
You get a lot more space: 8000 bytes max per "Value", up to 256kb total across all keys (compared to the 128 bytes max of CONTEXT_INFO)

For details, please see the following documentation pages:

Best Answer

Related Solutions

Sql-server – Is it possible to raiserror within a For Delete Trigger that rolls back the deletes

Sql-server – Passing info on who deleted record onto a Delete trigger

Related Question