T-SQL: A proper way to CLOSE/DEALLOCATE cursor in the update trigger

Question

Let's say I've got a trigger like this:

CREATE TRIGGER trigger1
   ON [dbo].[table1] 
   AFTER UPDATE
AS 
BEGIN               
    --declare some vars
    DECLARE @Col1 SMALLINT 
    DECLARE @Col1 TINYINT 

    --declare cursor        
    DECLARE Cursor1 CURSOR FOR 
    SELECT Col1, Col2 FROM INSERTED             

    --do the job
    OPEN Cursor1
    FETCH NEXT FROM Cursor1 INTO @Col1, @Col2

    WHILE @@FETCH_STATUS = 0
    BEGIN
        IF ...something...
        BEGIN           
            EXEC myProc1 @param1 = @Col1, @Param2 = @Col2
        END             
        ELSE
        IF ...something else...
        BEGIN           
            EXEC myProc2 @param1 = @Col1, @Param2 = @Col2
        END     

        FETCH NEXT FROM Cursor1 INTO @Col1, @Col2               
    END

    --clean it up       
    CLOSE Cursor1
    DEALLOCATE Cursor1                  
END

I want to be sure that Cursor1 is always closed and deallocated. Even myProc1 or myProc2 fails.

Shall I use try/catch block?

Solution 1

Yes, use TRY/CATCH but make sure you deallocate etc after. Unfortunately, there is no finally in SQL Server.

However, I suggest wrapping this in another try/catch

CREATE TRIGGER trigger1 ON [dbo].[table1] AFTER UPDATE
AS 
BEGIN                           
    --declare some vars
    DECLARE @Col1 SMALLINT, @Col1 TINYINT 

    BEGIN TRY
        --declare cursor            
        DECLARE Cursor1 CURSOR FOR 
        SELECT Col1, Col2 FROM INSERTED                     

        --do the job
        OPEN Cursor1
        FETCH NEXT FROM Cursor1 INTO @Col1, @Col2

        WHILE @@FETCH_STATUS = 0
        BEGIN
            IF ...something...
                    EXEC myProc1 @param1 = @Col1, @Param2 = @Col2
            ELSE
            IF ...something else...
                    EXEC myProc2 @param1 = @Col1, @Param2 = @Col2

            FETCH NEXT FROM Cursor1 INTO @Col1, @Col2                               
        END
    END TRY
    BEGIN CATCH
        --do what you have to
    END CATCH

    BEGIN TRY
        --clean it up               
        CLOSE Cursor1
        DEALLOCATE Cursor1                                  
    END TRY
    BEGIN CATCH
        --do nothing
    END CATCH
END

Whether a cursor in a trigger is a good idea is a different matter...

Solution 2

Ten years later, I figure I should add some information to this particular question.

There are two primary solutions to your problem. First, use a LOCAL cursor declaration:

DECLARE --Operation
    Cursor1 -- Name
CURSOR -- Type
    LOCAL READ_ONLY FORWARD_ONLY -- Modifiers
FOR -- Specify Iterations
SELECT Col1, Col2 FROM INSERTED;

This limits your particular cursor to only your active session, rather than the global context of the server, assuming no other action is calling into this cursor. Similar in principle is to use a Cursor Variable, which would look like this:

DECLARE @Cursor1 CURSOR;
SET @Cursor1 = CURSOR LOCAL READ_ONLY FORWARD_ONLY FOR SELECT Col1, Col2 FROM INSERTED;

In using a cursor variable, you can always overwrite it at anytime using the SET syntax, in addition to managing the scope to being within your particular session like the previous example. By overwriting the cursor context, you effectively deallocate any past reference it had. That said, both of these approaches accomplish your original intention by linking the status of the cursor to the activity of the current connection.

This might leave a lingering lock if your app context is using Connection Pooling, in which case you should use the Try-Catch pattern as follows:

CREATE TRIGGER trigger1
   ON [dbo].[table1] 
   AFTER UPDATE
AS 
BEGIN               
    --declare some vars
    DECLARE @Col1 SMALLINT;
    DECLARE @Col2 TINYINT;

    --declare cursor        
    DECLARE 
        Cursor1 
    CURSOR 
        LOCAL READ_ONLY FORWARD_ONLY 
    FOR 
        SELECT 
            Col1, 
            Col2 
        FROM 
            INSERTED;

    --do the job
    OPEN Cursor1;

    BEGIN TRY

        FETCH 
            NEXT 
        FROM 
            Cursor1 
        INTO 
            @Col1, 
            @Col2;

        WHILE @@FETCH_STATUS = 0
            BEGIN
                IF -- my condition
                    EXEC myProc1 @param1 = @Col1, @Param2 = @Col2;
                ELSE IF -- additional condition
                    EXEC myProc2 @param1 = @Col1, @Param2 = @Col2;

                FETCH 
                    NEXT 
                FROM 
                    Cursor1 
                INTO 
                    @Col1, 
                    @Col2;
            END;
    END TRY

    BEGIN CATCH
        -- Error Handling
    END CATCH

    --clean it up       
    CLOSE Cursor1;
    DEALLOCATE Cursor1;
END;

Using the pattern in this way reduces the code duplication, or need to check the status of the cursor. Basically, the Cursor-initialization should be safe, as is the open statement. Once the cursor is open, you will want to always close-deallocate it from the session, and that should always be a safe action assuming the cursor has been opened (which we just established should always be a safe operation). As such, leaving those outside the confines of the Try-Catch means that everything can be neatly closed at the end, after the Catch block.

It's worth mentioning that I specified the READ_ONLY attribute of the cursor, as well as FORWARD_ONLY, since your sample code didn't scroll back-and-forth between records in the set. If you are modifying the underlying rows in those procedures, you are probably better off using a STATIC cursor to ensure you don't accidentally cause an infinite loop. That shouldn't be a problem since you're using the INSERTED table to manage your cursor context, but still worth mentioning for other potential use cases.

If you want to learn more about cursors in SQL Server, I highly recommend reading this blog post on the subject, as he goes into great detail explaining what the various modifiers of a cursor are, and the effects they have within the Database Engine.

Solution 3

What you should do is never ever use a cursor in a trigger. Write correct set-based code instead. If someone did an import of data into your table of 100,000 new records you would lock up the table for hours and bring your database to a screaming halt. It is a very poor practice to use a cursor in a trigger.