question

johnshaddad avatar image
johnshaddad asked

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?
sql-servertriggercursor
2 comments
10 |1200 characters needed characters left characters exceeded

Up to 2 attachments (including images) can be used with a maximum of 512.0 KiB each and 1.0 MiB total.

is the posted trigger code complete ? or is it just a section of it ? Can you post the full trigger code ?
0 Likes 0 ·
There you go, I modified the question and posted the full code for the trigger.
0 Likes 0 ·
Magnus Ahlkvist avatar image
Magnus Ahlkvist answered
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...
3 comments
10 |1200 characters needed characters left characters exceeded

Up to 2 attachments (including images) can be used with a maximum of 512.0 KiB each and 1.0 MiB total.

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.
0 Likes 0 ·
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,.."
0 Likes 0 ·
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.
0 Likes 0 ·
Matt Whitfield avatar image
Matt Whitfield answered
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.
6 comments
10 |1200 characters needed characters left characters exceeded

Up to 2 attachments (including images) can be used with a maximum of 512.0 KiB each and 1.0 MiB total.

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'?
2 Likes 2 ·
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.
0 Likes 0 ·
Any ideas?
0 Likes 0 ·
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.
0 Likes 0 ·
using cursor is not efficient at all. You should do whatever you need to do in that stored procedure inside the trigger code
0 Likes 0 ·
Show more comments

Write an Answer

Hint: Notify or tag a user in this post by typing @username.

Up to 2 attachments (including images) can be used with a maximum of 512.0 KiB each and 1.0 MiB total.