SQL Server A trigger to work on multiple row inserts

Question

I am maintaining some code that has a trigger on a table to increment a column. That column is then used by a 3rd party application A. Lets say that the table is called test with two columns num1 and num2. The trigger runs on each insert of num1 in test. Following is the trigger:

USE [db1]
GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER TRIGGER [dbo].[TEST_MYTRIG] ON [dbo].[test]
FOR INSERT AS
begin
SET NOCOUNT ON
DECLARE @PROC_NEWNUM1 VARCHAR (10)
DECLARE @NEWNUM2 numeric(20)

SELECT @PROC_NEWNUM1 = num1 FROM INSERTED
select @NEWNUM2 = MAX(num2) from TEST
if @NEWNUM2 is null
Begin
set  @NEWNUM2  = 0
end
set @NEWNUM2 = @NEWNUM2 + 1
UPDATE TEST SET num2 = @NEWNUM2 WHERE num1 = @PROC_NEWNUM1
SET NOCOUNT OFF
End

This works fine in simple row based inserts, but there is another 3rd party app B (sigh) that sometimes does multiple inserts on this table something like this but not exactly:

INSERT INTO [db1].[dbo].[test]
           ([num1])

   Select db1.dbo.test.num1 from [db1].[dbo].[test]
GO

This causes the trigger to behave erratically...

Now I don't have access to the source of app A or B and only control the database and the trigger. Is there anything that can be done with the trigger so that the updates done to num2 are correct in case of multiple inserts?

Solution:

Following is the solution based on affan's code:

 DECLARE @PROC_NEWNUM1 VARCHAR (10)
 DECLARE @NEWNUM2 numeric(20)
 DECLARE my_Cursor CURSOR FAST_FORWARD FOR SELECT num1 FROM INSERTED;

 OPEN my_Cursor 
 FETCH NEXT FROM my_Cursor into @PROC_NEWNUM1

 WHILE @@FETCH_STATUS = 0 
 BEGIN 

 select @NEWNUM2 = MAX(num2) from TEST
 if @NEWNUM2 is null
 Begin
    set  @NEWNUM2  = 0
 End
 set @NEWNUM2 = @NEWNUM2 + 1
 UPDATE TEST SET num2 = @NEWNUM2  WHERE num1 = @PROC_NEWNUM1
 FETCH NEXT FROM my_Cursor into @PROC_NEWNUM1  
 END

CLOSE my_Cursor
DEALLOCATE my_Cursor

Check here for a set based approach: SQL Server - Rewrite trigger to avoid cursor based approach

A **cursor** inside a **trigger** is a **horribly bad idea**; cursors are notoriously slow and memory hogs, and anything in a trigger ought to be as lean and fast as ever humanly possible. I would **strongly** advise to use a different approach (from my own, personal and agonizing experience with such constructs) — marc_s, Apr 24 '13 at 18:09
Couldn't `if @NEWNUM2 is null` check be replaced with `COALESCE(MAX(num2),0)` ? — JustAMartin, Aug 11 '16 at 14:39

score 6 · Accepted Answer · answered Feb 02 '10 at 06:01

6

You just have to open a cursor on INSERTED and iterate it for @PROC_NEWNUM1 and put your rest of code that loop. e.g

 DECLARE @PROC_NEWNUM1 VARCHAR (10)
 DECLARE @NEWNUM2 numeric(20)
 DECLARE my_Cursor CURSOR FOR SELECT num1 FROM INSERTED; 
 OPEN my_Cursor; 

 FETCH NEXT FROM @PROC_NEWNUM1; 


 WHILE @@FETCH_STATUS = 0 
 BEGIN FETCH NEXT FROM my_Cursor 
 select @NEWNUM2 = MAX(num2) from TEST
 if @NEWNUM2 is null
 Begin
  set  @NEWNUM2  = 0
 end
 set @NEWNUM2 = @NEWNUM2 + 1
 UPDATE TEST SET num2 = @NEWNUM2 WHERE num1 = @PROC_NEWNUM1

 END; 

CLOSE my_Cursor; DEALLOCATE my_Cursor;

answered Feb 02 '10 at 06:01

particle

3,280
4
27
39

Though the above is not entirely correct and has some errors, it did help me by giving me a foundation to work with. I have added the updated version of the above in my question. – Ali Feb 02 '10 at 11:45
4

I'm sorry, but a cursor is not the best way to do this. – ErikE Feb 07 '10 at 07:56
9

@Cocowalla You do a `JOIN` between the INSERTED table and the base data table. To get incrementing values for the update, use `Row_Number()` (SQL 2005 and up) or an insert to a temp table with an identity column, or a Numbers table with a million rows or so (adds only 13 megabytes to the database). In fact, better than all this is to replace the column with an identity column that gives incrementing values already! – ErikE Oct 31 '11 at 18:08

score 3 · Answer 2 · edited May 23 '17 at 10:29

3

Take a look at inserted pseudo table in your trigger as it will contain multiple rows during these operations. You should always handle multiple rows in your triggers anyway.

See here for more info:

How to test for multiple row actions in a SQL Server trigger?

edited May 23 '17 at 10:29

Community

1
1

answered Feb 01 '10 at 18:08

Keith Adler

20,880
28
119
189

HLGEM · Answer 3 · 2013-04-24T13:31:07.057

2

Trigger needs to be rewriteen to handle multiple row inserts. Never write a trigger like that using variables. All triggers must alawys consider that someday someone is going to do a multi-row insert/update/delete.

You shouldn't be incrementing columns that way in a trigger either, if you need incremented column numbers why aren't you using an identity column?

edited Apr 24 '13 at 13:31

answered Feb 01 '10 at 18:08

HLGEM

94,695
15
113
186

I didn't like the way the columns were being incremented either and I have even changed the trigger in Oracle to use a sequence. As for identity, my first thought was to use alter table to change it to an identity but unfortunately that is not possible and other ways to do it require more changes than I am brave enough to hazard. – Ali Feb 02 '10 at 05:41

score 0 · Answer 4 · answered May 28 '21 at 21:42

As already pointed out, cursors can be problematic and it's best to use joins between the triggered table and the inserted and deleted tables.

Here's an example of how to do that:

ALTER TRIGGER [dbo].[TR_assign_uuid_to_some_varchar_column]
ON [dbo].[myTable]
AFTER INSERT, UPDATE
AS
BEGIN
/********************************************
 APPROACH
  * we only care about update and insert in this case
  * for every row in the "inserted" table, assign a new uuid for blanks
*********************************************/

       update t 
              set uuid_as_varchar = lower(newid()) 
       from myTable t 

        -- inserted table is populated for row updates and new row inserts
       inner join inserted i on i.myPrimaryKey = t.myPrimarykey

        -- deleted table is populated for row updates and row deletes
       left join deleted d on d.myPrimaryKey = i.myPrimaryKey

        -- only update the triggered table for rows applicable to the trigger and
        -- the condition of currently having a blank or null stored for the id
       where 
           coalesce(i.uuid_as_varchar,'') = '' 

           -- you can also check the row being replaced as use that as part of the conditions, e.g. 
           or ( coalesce(i.uuid_as_varchar,'') <> coalesce(d.uuid_as_varchar,'') );
       
END

SQL Server A trigger to work on multiple row inserts

4 Answers4

Linked