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,
SQLTwins by Nakul Vachhrajani 