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

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

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)

StackExchange Code Review Q#142852, answer score: 2