SQL Trigger - Audting Config Table - Send Email with update details

Question

Keep in mind i'm not an expert or developer, working in IT operations and improving my coding skills.

Goal: Trigger to send email when a specific configuration table is updated or deleted. Should never happen unless done for a release

CREATE TRIGGER [Cfg].[t_tablename_TrackConfigChanges] ON [Cfg].[tablename]
AFTER INSERT, UPDATE, DELETE
AS
 /* All purpose trigger for auditing of config tables for Insertion, Update, and Delete */
 --If no rows were updated, then end this trigger (caused by an update or insert affects no rows)
 IF @@ROWCOUNT = 0
 BEGIN
 RETURN
 END
 SET NOCOUNT ON;
 DECLARE @isInsert BIT
 DECLARE @isdelete BIT
 --If something was inserted flag our variable
 IF EXISTS
 (
 SELECT TOP 1 1
 FROM inserted
 )
 BEGIN
 SET @isInsert = 1
 END
 --If something was deleted flag our variable
 IF EXISTS
 (
 SELECT TOP 1 1
 FROM deleted
 )
 BEGIN
 SET @isdelete = 1
 END
 /* If something was inserted, or deleted send an email about it with the specified action and the data that was changed, updated, or deleted */
 IF @isInsert = 1
 OR @isdelete = 1
 BEGIN
 BEGIN TRY
 DECLARE @table_name VARCHAR(30)
 DECLARE @user VARCHAR(100)
 DECLARE @email_subject VARCHAR(1000)
 DECLARE @body NVARCHAR(MAX)= N'';
 DECLARE @operation VARCHAR(10)
 /* Get the table that this trigger is executing for */
 SELECT @table_name = OBJECT_SCHEMA_NAME(parent_id)+'.'+OBJECT_NAME(parent_id)
 FROM sys.triggers
 WHERE object_id = @@PROCID
 SET @user = SYSTEM_USER
 /* Since an email cannot access the inserted or deleted tables, or any local variables 
 as it executes in a different session, I decided to append the results of the query 
 into one big string separated by carriage returns for each row */
 IF @isInsert = 1
 AND @isdelete = 1
 BEGIN
 SET @operation = 'UPDATE'
 SELECT @body+=CHAR(13)+CHAR(10)+'columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+'| INSERTED'
 FROM inserted;
 SELECT @body+=CHAR(13)+CHAR(10)+'columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+'| DELETED'
 FROM deleted;
 END
 ELSE
 BEGIN
 IF @isInsert = 1
 BEGIN
 SET @operation = 'INSERT'
 SELECT @body+=CHAR(13)+CHAR(10)+'columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+'| INSERTED'
 FROM inserted;
 END
 ELSE
 BEGIN
 IF @isdelete = 1
 BEGIN
 SET @operation = 'DELETE'
 SELECT @body+=CHAR(13)+CHAR(10)+'columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+' columnname'+RTRIM(columnname)+'| DELETED'
 FROM deleted;
 END
 END
 END
 /* Send the email to the appropriate team */
 SET @email_subject = '[Database Configuration Monitor] '+@@ServerName+' - '
 SET @email_subject+=@operation+' operation has been performed on '+@table_name+' by '+@user
 EXEC msdb.dbo.sp_send_dbmail 
 @profile_name = 'Profile', 
 @recipients = '[email protected]', 
 --@copy_recipients = 'Whoever I want to copy'
 @body = @body, 
 @subject = @email_subject
 /* If an email fails, it sets @error to a non zero value */
 IF @@error <> 0
 BEGIN
 /* Declare a dummy varaible as we dont want the transaction to fail if the email fails */
 DECLARE @dummy BIT= 1
 END
 END TRY
 BEGIN CATCH
 /* If anything fails, its ok as we don't want to cause a transaction to rollback due to a failure */
 END CATCH
 END 

• DForck42: I'm more curious about the NEED to do this as opposed to the technical implementation. why not just buckle down the security on this particular table?

Answer 1

If you're using SELECT TOP 1 1 in an effort to improve performance, know that this isn't always what will happen. In particular, the TOP operator can constrain the ability of the optimizer to pick the best plan. SELECT 1 inside of a EXISTS will do just fine in 99.9% of cases.

Additionally, I find it easier to read this:

SELECT @isInsert = COUNT( * )
 WHERE EXISTS( SELECT 1 FROM inserted )

This will always set @isInsert to either 1 or 0, and reduces the amount of necessary code.

---

In some cases, using functions like OBJECT_SCHEMA_NAME can actually impair performance by causing blocking (can't find a reference, but at least observationally it has been found to be true before). Also, you don't even need it - you know the name of your table already.

---

Instead of your big IF ELSE block, you could do this:

SET @operation = CASE WHEN @isInsert = 1 AND @isDelete = 1 THEN 'UPDATE'
 WHEN @isInsert = 1 THEN 'INSERT'
 ELSE 'DELETE' END
SET @body = STUFF( ( SELECT CHAR(13) + CHAR(10) + FormattedColumnResults + '| ' + OperationType
 FROM ( SELECT 'columnname' + RTRIM( columnname ) FormattedColumnResults,
 'INSERTED' OperationType
 FROM inserted
 UNION ALL
 SELECT 'columnname' + RTRIM( columnname ) FormattedColumnResults,
 'DELETED' OperationType
 FROM deleted ) ModifiedRows
 FOR XML PATH(''), TYPE ).value('(./text())[1]', 'varchar(MAX)' ), 1, 2, '' )

Here I'm relying on the fact that setting @operation is very simple, and then using STUFF to safely concatenate values. In particular, your previous result was using unsupported syntax, and could have ended up giving incorrect results. This also doesn't need to be worried about whether or not the value is actually inserted/deleted, because those tables would just be empty.

---

Lastly, if the email fails you should probably log that to a table (and include the message you would have sent) so the information can be recovered later (and someone knows to troubleshoot your trigger)