Cursor inside Trigger not working

Question

USE [ddb]
GO
SET ANSI_NULLS OFF
GO
SET QUOTED_IDENTIFIER ON
GO

CREATE TRIGGER [dbo].[requeststrigger]
ON [dbo].[requests]
AFTER INSERT,UPDATE
AS
BEGIN
DECLARE @email VARCHAR(400);
DECLARE @firstname VARCHAR(400);
DECLARE @requestno VARCHAR(400);
DECLARE @lastname VARCHAR(400);
DECLARE @statusid INT;
DECLARE thecursor CURSOR FOR 
SELECT inserted.requestno,contacts.firstname,contacts.lastname,contacts.email 
FROM request_contacts,contacts,inserted 
WHERE request_contacts.requestid=inserted.requestid 
AND contacts.contactid=request_contacts.contactid 
AND request_contacts.notification=1 
AND contacts.notification=1;

SET @statusid = (SELECT statusid FROM inserted);

IF @statusid = 4 AND @statusid <> (SELECT statusid FROM deleted)
BEGIN

SET NOCOUNT ON
SET ARITHABORT ON
OPEN thecursor;

    FETCH NEXT FROM thecursor
    INTO @requestno,@firstname,@lastname,@email

    WHILE @@FETCH_STATUS = 0
    BEGIN    
       EXEC MAIL_SEND @email,@firstname,@requestno,@lastname;           
    FETCH NEXT FROM thecursor
    INTO @requestno,@firstname,@lastname,@email

    END
CLOSE thecursor;

DEALLOCATE thecursor

SET NOCOUNT OFF
END

END

This simply makes the whole UPDATE/INSERT not work. When I remove the cursor declaration, it works. The cursor is just selecting a field from a table that is existing in the same database called "contacts". What is wrong?

Answer 1

A trigger should ideally do something very fast, so that it can run "incognito". If what you want is to loop through a child table and send a mail for each row, I agree with the suggestion to change the trigger to instead just update a table with the information that should be e-mailed, and have a job run every n seconds/minutes/hours/whateverintervalyouwant.

Here's how I would change your current trigger.

First a table to store the information to send:

CREATE TABLE NotificationsToSend(ID int IDENTITY(1,1) PRIMARY KEY, email varchar(400), firstname varchar(400), lastname varchar(400), requestno varchar(400), date_sent datetime DEFAULT null)
GO
CREATE INDEX ix_date_sent ON NotificationsToSend(date_sent)
GO

And then some changes to the trigger:

CREATE TRIGGER [dbo].[requeststrigger]
ON [dbo].[requests]
AFTER INSERT,UPDATE
AS
BEGIN
insert into NotificationsToSend(requestno, firstname, lastname, email)
SELECT inserted.requestno,contacts.firstname,contacts.lastname,contacts.email 
FROM inserted inner join request_contacts
on inserted.requestid=request_contacts.requestid
inner join contacts on request_contacts.contactid=contacts.contactid
WHERE request_contacts.notification=1 AND contacts.notification=1 and inserted.statusid=4 and inserted.statusid not in (select statusid from deleted)
END

Finally I would create a stored procedure which actually sends the emails:

CREATE PROC SendNotificationEmails
AS
DECLARE @email varchar(400), @firstname varchar(400), @lastname varchar(400), @requestno varchar(400), @id int
DECLARE TRIGGER tr FOR select email, firstname, lastname, requestno, id from NotificationsToSend where date_sent is null
OPEN tr
FETCH NEXT FROM tr INTO @email, @firstname, @lastname, @requestno, @id
WHILE @@FETCH_STATUS=0
BEGIN
    EXEC MAIL_SEND @email,@firstname,@requestno,@lastname
    UPDATE NotificationsToSend SET date_sent=CURRENT_TIMESTAMP where ID=@ID
    FETCH NEXT FROM tr INTO @email, @firstname, @lastname, @requestno, @id
END

An alternative to this would be to include the logic to send e-mails into the original stored procedure which performs the updates. But that would mean the application has to wait for an e-mail to be sent for each child-row, and that's not very efficient either.

My suggestion is written totally without even the simplest compilation tests, so it probably needs a few tweaks...

Answer 2

0

I'm not sure, entirely, but I have a feeling it's SQL Server's way of telling you that a cursor inside a trigger is not a good way to go.

Triggers, ok (some people will try and avoid them like the plague, but I accept that they have their place) - but a cursor within a trigger is going to be a performance issue.

Try and do what you wanted to do in a set-based manner, rather than using the cursor.

answered Jul 29 '10 at 12:16 AM

Matt Whitfield ♦♦
29.2k ● 56 ● 63 ● 87

Any suggestion? What I am trying to do is whenever a trigger of TABLE1 is fired (when a row is inserted/updated), I want to jump to TABLE2 another table and get all the records that are associated with that record in TABLE1 (through foreign key) and then loop through those results to call a procedure to do something with each row.

Jul 29 '10 at 12:26 AM johnshaddad

Any ideas?

Jul 29 '10 at 12:44 AM johnshaddad

Really wouldn't do this in the trigger, move the logic out to the place where the insert/update is performed - hopefully a stored procedure.

Jul 29 '10 at 12:56 AM Kev Riley ♦♦

using cursor is not efficient at all. You should do whatever you need to do in that stored procedure inside the trigger code

Jul 29 '10 at 12:57 AM Squirrel

Ouch - sending mail from a cursor inside a trigger. Honestly, your best bet is to re-think this process. Inside the trigger simply insert the data that you need to send the email into another table, then have another process that runs off-line to send the email.

What do you mean, exactly, when you say that it makes the UPDATE/INSERT 'not work'?

Jul 29 '10 at 01:52 AM Matt Whitfield ♦♦

(comments are locked)

Cursor inside Trigger not working

Follow this question

Related Questions