x

Cursor inside Trigger not working

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?
more ▼

asked Jul 29, 2010 at 12:10 AM in Default

johnshaddad gravatar image

johnshaddad
43 8 8 10

is the posted trigger code complete ? or is it just a section of it ? Can you post the full trigger code ?
Jul 29, 2010 at 12:38 AM Squirrel
There you go, I modified the question and posted the full code for the trigger.
Jul 29, 2010 at 12:59 AM johnshaddad
(comments are locked)
10|1200 characters needed characters left

2 answers: sort voted first

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...
more ▼

answered Jul 29, 2010 at 03:40 AM

Magnus Ahlkvist gravatar image

Magnus Ahlkvist
16.6k 17 20 33

Oh, and by the way - I changed the query to use ansi joins, and I included the statusid-check in the query instead of doing it afterwards.
Jul 29, 2010 at 03:42 AM Magnus Ahlkvist

Thanks a lot for the help! There is just a type in the last code:

"DECLARE TRIGGER tr FOR select email,..." should be "DECLARE CURSOR tr FOR select email,.."
Jul 29, 2010 at 07:13 AM johnshaddad
I wish I could say "It's because I never declare cursors", but unfortunately most of our databases are flooded with cursor definitions, so I guess I'll have to blame the typo on something else.. I haven't worked for three weeks, but instead enjoyed sunny weather and beers. That must be it.
Jul 29, 2010 at 01:32 PM Magnus Ahlkvist
(comments are locked)
10|1200 characters needed characters left

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.
more ▼

answered Jul 29, 2010 at 12:16 AM

Matt Whitfield gravatar image

Matt Whitfield ♦♦
29.5k 61 65 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, 2010 at 12:26 AM johnshaddad
Any ideas?
Jul 29, 2010 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, 2010 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, 2010 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, 2010 at 01:52 AM Matt Whitfield ♦♦
(comments are locked)
10|1200 characters needed characters left
Your answer
toggle preview:

Up to 2 attachments (including images) can be used with a maximum of 524.3 kB each and 1.0 MB total.

New code box

There's a new way to format code on the site - the red speech bubble logo will automatically format T-SQL for you. The original code box is still there for XML, etc. More details here.

Follow this question

By Email:

Once you sign in you will be able to subscribe for any updates here

By RSS:

Answers

Answers and Comments

SQL Server Central

Need long-form SQL discussion? SQLserverCentral.com is the place.

Topics:

x346
x121
x59

asked: Jul 29, 2010 at 12:10 AM

Seen: 2881 times

Last Updated: Jul 29, 2010 at 06:32 AM