SQL Stored Procedure with Input parameters with While loop

Question

I have a code below that should insert records into the table but unfortunately this code foes not work in case multiple records are inserted or updated or deleted. How should I rewrite the code for procedure to loop through all the inserted / deleted records? And I do need to use that stored procedure with Input parameters (not just simple insert into ... select ... from ...)

IF EXISTS (SELECT * FROM MyDB.sys.triggers WHERE object_id = OBJECT_ID(N'[dbo].[MyTable_DEL_UPD_INS]'))
    DROP TRIGGER [dbo].[MyTable_DEL_UPD_INS]
GO

CREATE TRIGGER [dbo].[MyTable_DEL_UPD_INS]
ON [MyDB].[dbo].[MyTable] 
AFTER DELETE, UPDATE, INSERT
NOT FOR REPLICATION
AS 
BEGIN
    DECLARE @PKId INT, 
            @Code VARCHAR(5), 
            @AuditType VARCHAR(10)

    SET @Code = 'TEST'

    IF EXISTS (SELECT * FROM deleted d)
       AND NOT EXISTS (SELECT * FROM inserted i) 
    BEGIN 
        SELECT TOP 1 
            @PKId = d.[MyTable_PK],
            @AuditType = 'DELETE' 
        FROM 
            deleted d WITH (NOLOCK)

        IF @PKId IS NOT NULL 
           AND @Code IS NOT NULL 
            EXEC MyDB.[dbo].[SP_Audit] @PKId, @Code, @AuditType 
    END

    IF EXISTS (SELECT * FROM deleted d)
        AND EXISTS (SELECT * FROM inserted i) 
    BEGIN 
        SELECT TOP 1 
            @PKId = d.[MyTable_PK],
            @AuditType = 'UPDATE' 
        FROM 
            deleted d WITH (NOLOCK)

        IF @PKId IS NOT NULL 
           AND @Code IS NOT NULL 
            EXEC MyDB.[dbo].[SP_Audit] @PKId, @Code, @AuditType 
    END

    IF NOT EXISTS (SELECT * FROM deleted d)
        AND EXISTS (SELECT * FROM inserted i) 
    BEGIN 
        SELECT TOP 1 
            @PKId = d.[MyTable_PK],
            @AuditType = 'INSERT' 
        FROM 
            deleted d WITH (NOLOCK)

        IF @PKId IS NOT NULL 
           AND @Code IS NOT NULL 
            EXEC MyDB.[dbo].[SP_Audit] @PKId, @Code, @AuditType 
    END
END 
GO

ALTER TABLE [MyDB].[dbo].[MyTable] ENABLE TRIGGER [MyTable_DEL_UPD_INS]

Having a loop in a trigger is just like a really bad idea. it's going to affect performance awfully. Add that to the fact that you have an SP that, as it currently stands, is going to need a further loop to be able to use (so that for multiple rows, you loop through each one), you end up with a simple `INSERT` containing 100 or so rows requiring a huge amount of resource. What is the SP_Audit doing? Can you provide the SQL for it? If you have to use it, it may mean you're going to need to change it; which means you might need to change everything that references it... — Thom A, Jan 26 '18 at 16:14
Also, `WITH (NOLOCK)` isn't going to achieve anything in your trigger. In fact, I suggest you remove it; as it's more likely to cause problems than solve anything. — Thom A, Jan 26 '18 at 16:17
I do agree with you! Performance would be awfully. Th SP is simply does Select Into... Insert ... From ... and nothing else :). I would probably try to convince me teammate to give up on using the SP. He was so much eager to use that as in his opinion it's elegant... — Data Engineer, Jan 26 '18 at 16:36
Good comment on WITH (NOLOCK) usage. In my company they put that everywhere even on CTEs and Temp tables. Do not see any point of it... I would arguably agree that on a physical object it would make more sense. — Data Engineer, Jan 26 '18 at 16:41
Side note: you should **not** use the `sp_` prefix for your stored procedures. Microsoft has [reserved that prefix for its own use (see *Naming Stored Procedures*)](http://msdn.microsoft.com/en-us/library/ms190669%28v=sql.105%29.aspx), and you do run the risk of a name clash sometime in the future. [It's also bad for your stored procedure performance](http://www.sqlperformance.com/2012/10/t-sql-queries/sp_prefix). It's best to just simply avoid `sp_` and use something else as a prefix - or no prefix at all! — marc_s, Jan 26 '18 at 17:02

score 0 · Answer 1 · answered Jan 28 '18 at 09:08

You should avoid using loops in triggers.
Triggers should be as quick to run as possible, since SQL Server will not return control to whatever statement that fired the trigger until the trigger is completed.

So instead of a loop, you should modify your SP_Audit procedure to work with multiple records instead of a single one.
usually, this is easily be done using a table valued parameter.
If you could post the SP_Audit as well, we could give you a complete solution.
Since you didn't post it, you can use these guidelines as a start:

First, you create a user defined table type:

CREATE TYPE dbo.Ids AS TABLE
(
    Id int NOT NULL PRIMARY KEY
)

GO

Then, you create the procedure to use it:

CREATE PROCEDURE [dbo].[STP_Audit_MultipleRecords]
(
    @IDs dbo.Ids readonly, 
    @Code CHAR(4), 
    @AuditType CHAR(6)
)
AS
-- Implementation here
GO

Last, your write your trigger like this:

CREATE TRIGGER [dbo].[MyTable_DEL_UPD_INS]
ON [MyDB].[dbo].[MyTable] 
AFTER DELETE, UPDATE, INSERT
NOT FOR REPLICATION
AS 
BEGIN

    DECLARE @HasDeleted bit = 0, 
            @HasInserted bit = 0,
            @AuditType CHAR(6),
            @Code CHAR(4)

    SET @Code = 'TEST'

    DECLARE @IDs as dbo.Ids

    IF EXISTS (SELECT * FROM deleted d)
        SET @HasDeleted = 1

    IF EXISTS (SELECT * FROM inserted i) 
        SET @HasInserted = 1


    IF @HasDeleted = 1 
    BEGIN
        IF @HasInserted = 1
        BEGIN
             SET @AuditType = 'UPDATE' 
        END
        ELSE
        BEGIN
            SET @AuditType = 'DELETE' 
        END
    END
    ELSE
    IF @HasInserted = 1
        BEGIN
            SET @AuditType = 'INSERT' 
        END

    INSERT INTO @IDs (Id)
    SELECT [MyTable_PK]
    FROM inserted
    UNION 
    SELECT [MyTable_PK]
    FROM deleted 

    EXEC [dbo].[STP_Audit_MultipleRecords] @IDs, @Code, @AuditType 

END 
GO

Notes:

The @HasDeleted and @HasInserted variables are to allow you to only execute the EXISTS query once for every procedure.
Getting the primary key values from the deleted and inserted table is done using a single union query. Since union eliminates duplicate values, you can write this query just once. If you want to, you can write a different query for each audit type, but then you will have to repeat the same query 3 times (with different tables)
I've changed the data types of your @code and @AuditType variables to char, since they have a fixed length.

SQL Stored Procedure with Input parameters with While loop

1 Answers1