SQL Server trigger's cursor is not working

Question

I created a cursor inside a trigger and it is not working properly. Please help me fix it

Create trigger Posts_Raw_To_Queue_Trigger  ON SendNotificationPostsRaw FOR INSERT
AS
BEGIN

DECLARE @PostID uniqueidentifier  
DECLARE @UserID uniqueidentifier
DECLARE @ProfID int
DECLARE @Email nvarchar(100)
DECLARE @CreationTime  datetime
DECLARE @SpecialityID int

SELECT @ProfID= ProfessionalID,@Email= Email from Professionals where UserID=@UserID
SELECT @PostID = I.PostID,@UserID = I.UserID ,@CreationTime =I.CreationTime  FROM INSERTED I

DECLARE post_relation_cursor CURSOR FOR select CategoryId  from PostCategoryRelations  where PostId=@PostID;

OPEN post_relation_cursor;
FETCH NEXT FROM post_relation_cursor INTO @SpecialityID 
WHILE @@FETCH_STATUS = 0  
BEGIN  
       INSERT INTO SendNotificationPostsQueue (UserID,PostID,SpecialityID,TemplateID,CreationTime,SendTime,JSONParameters) Values(@UserID,@PostID,1,1,'04/11/2013','04/11/2013','')

       FETCH NEXT FROM post_relation_cursor INTO @SpecialityID;
END;
CLOSE post_relation_cursor;
DEALLOCATE post_relation_cursor;

END

If I remove cursor and insert dummy values into SendNotificationPostsQueue, it works. So there is problem with my cursor... Please tell me why cursor is not working?

Answer 1

It doesn't appear that you need to use a cursor at all and would be better off not using one in almost all cases. Simply replace the body of your trigger (the part between begin and end) with a standard insert:

INSERT INTO SendNotificationPostsQueue (UserID,PostID,SpecialityID,TemplateID,CreationTime,SendTime,JSONParameters)
SELECT
    i.UserID,
    i.PostID,
    1,
    1,
    '04/11/2013', -- Might want i.CreationTime or current_timestamp
    '04/11/2013',
    ''
FROM INSERTED i
    -- possibly want "LEFT JOIN Professionals p on i.UserID = p.UserID" here to grab other info

Note how I am not using the values clause of an insert which can only insert one row. I'm putting a select statement as part of the insert, thus inserting as many rows as the select returns. This means that we don't need to use a cursor, and don't need a bunch of variables to power the cursor.

One issue with your current code, as @automatic has mentioned, is that you're assuming INSERTED only holds one row. If it has more than one, then you'll throw an error when you try to assign a column to a variable.

For reasons of elegance, maintainability, and performance, I strongly urge you to abandon this cursor and run a simple insert (since that's all that your cursor is doing anyway).