I have worked for a product sustenance team for the better part of my 7+ years of experience and hence, I can closely relate to the problem at hand today. The challenges with sustenance teams are many, and one of them is understanding and building upon code written by other teams – developers of whom may not even be around in the organization. In addition to this the code that needs to be extended may be legacy code which was written years ago and may not confirm to the newer coding standards. To reduce the impact, most sustenance teams therefore do not spend the time to re-engineer or re-write a piece of code just because it is not up to standards, but instead work on extending the functionality. They leave it to the new development teams to re-write the code as part of the system refresh cycles.

The Scenario

This took the better of one of the engineers recently. An existing procedure (written in the old-fashioned way to use CURSORs) had to be extended. It was upto this poor developer to extend the stored procedure to incorporate some additional logic and release it out to QA within a considerably tight time-frame.

This stored procedure used the slower approach of using CURSORs to iterate through the list of employees. This cursor was to be re-used after another set of business operators to achieve the new requirement. The engineer therefore decided to add the additional piece of code after the old code ended – which is where he made a fatal mistake that cost him a couple of hours to notice.

The Example

To establish a parallel with the requirement, let us assume that a procedure exists to increment the VacationHours of all employees of a certain grade with AdventureWorks Cycles by 1 hour. We will extend this stored procedure by attempting to revert back the change to VacationHours, leaving a small error in doing so.

USE AdventureWorks2012
GO
IF OBJECT_ID('HumanResources.proc_DemoDellocateCursorIssues') IS NOT NULL
BEGIN
    DROP PROCEDURE HumanResources.proc_DemoDellocateCursorIssues
END
GO
CREATE PROCEDURE HumanResources.proc_DemoDellocateCursorIssues
    @orgLevel INT
AS
BEGIN
    SET NOCOUNT ON
    DECLARE @businessEntityId INT 

    DECLARE employeeCursor INSENSITIVE CURSOR
    FOR (SELECT e.BusinessEntityID
         FROM HumanResources.Employee AS e
         WHERE e.OrganizationLevel = @orgLevel
        )

    OPEN employeeCursor 

    BEGIN TRANSACTION outerTran
        BEGIN TRY
            FETCH NEXT FROM employeeTran INTO @businessEntityId

            WHILE (@@FETCH_STATUS <> -1)
            BEGIN
                WHILE (@@FETCH_STATUS <> -2)
                BEGIN
                    UPDATE e
                    SET VacationHours += 1
                    FROM HumanResources.Employee AS e
                    WHERE BusinessEntityID = @businessEntityId
                END
                FETCH NEXT FROM employeeTran INTO @businessEntityId
            END

            --IF @@TRANCOUNT > 0
            --    COMMIT TRANSACTION outerTran
        END TRY
        BEGIN CATCH
            IF @@TRANCOUNT > 0
                ROLLBACK TRANSACTION outerTran
        END CATCH

        CLOSE employeeCursor
        DEALLOCATE employeeCursor

        --OLD CODE ENDS HERE!!! ADDITIONAL CODE BEGINS

        --This part did not exist before
        --Some more business logic added here

        --Re-open the cursor and begin processing
        OPEN employeeCursor
        BEGIN TRY
            FETCH NEXT FROM employeeTran INTO @businessEntityId

            WHILE (@@FETCH_STATUS <> -1)
            BEGIN
                WHILE (@@FETCH_STATUS <> -2)
                BEGIN
                    UPDATE e
                    SET VacationHours -= 1
                    FROM HumanResources.Employee AS e
                    WHERE BusinessEntityID = @businessEntityId
                END
                FETCH NEXT FROM employeeTran INTO @businessEntityId
            END
            IF @@TRANCOUNT > 0
                COMMIT TRANSACTION outerTran
        END TRY
        BEGIN CATCH
            IF @@TRANCOUNT > 0
                ROLLBACK TRANSACTION outerTran
        END CATCH

        CLOSE employeeCursor
        DEALLOCATE employeeCursor
END
GO

The Error

All those who have noticed the error, no further reading of this post is necessary. All those who have not found the error yet, please read on…

Executing the procedure raises the following error:

USE AdventureWorks2012
GO
EXEC HumanResources.proc_DemoDellocateCursorIssues @orgLevel = 4
GO

Msg 16916, Level 16, State 1, Procedure proc_DemoDellocateCursorIssues, Line 40
A cursor with the name ’employeeCursor’ does not exist.
Msg 16916, Level 16, State 1, Procedure proc_DemoDellocateCursorIssues, Line 67
A cursor with the name ’employeeCursor’ does not exist.
Msg 16916, Level 16, State 1, Procedure proc_DemoDellocateCursorIssues, Line 68
A cursor with the name ’employeeCursor’ does not exist.

The error comes simply because the CURSOR – employeeCursor has been DEALLOCATEd before being used again. Let’s focus on the part where the old code ends and new extension was added:

        CLOSE employeeCursor
        --Notice the DEALLOCATE from the older implementation here. The solution is to comment this DEALLOCATE statement
        --DEALLOCATE employeeCursor

        --OLD CODE ENDS HERE!!! ADDITIONAL CODE BEGINS

        --This part did not exist before
        --Some more business logic added here

        --Re-open the cursor and begin processing
        OPEN employeeCursor

It was a simple typographical error on the part of the developer, but resulted in his spending an hour or two in debugging the issue and ultimately approaching me for help. A 10-minute study of the code from a different pair of eyes (mine) helped him resolve realize the miss and resolve the issue.

Moral of the story

The moral of the story is not whether we should use Cursors or not, but there is a more important behavioural pattern that I would like to highlight.

After being sure that we have done our level best in writing a piece of code, most of us tend to keep troubleshooting the code for hours at an end if it happens to develop an error. All I urge is that if you are sure that you have done due diligence from your end, do not be afraid to ask for peer reviews – they help because they involve a different pair of eyes on the same problem. The difficult part is not making it a habit by having others resolve all your problems for you.

Until we meet next time,

Be courteous. Drive responsibly.