Today I came across a self-created problem that could have been avoided if I had only followed my own advice. It wasn't directly and explicitly mentioned in this context, but the concept is the same. Let's start with the source of the problem: one stored procedure that called another. The first ("inner") procedure was something like this: take a database_id as a parameter, and show all the table names with indexes that have more updates than seeks/scans/lookups:
CREATE PROCEDURE dbo.bar
@database_id INT,
@object_id INT = NULL
AS
BEGIN
SET NOCOUNT ON;
PRINT '';
PRINT DB_NAME(@database_id);
PRINT '';
DECLARE c CURSOR
FORWARD_ONLY STATIC READ_ONLY
FOR
(
SELECT DISTINCT [object_id]
FROM sys.dm_db_index_usage_stats
WHERE [database_id] = @database_id
AND [object_id] = COALESCE(@object_id, [object_id])
AND user_updates > user_seeks + user_scans + user_lookups
);
OPEN c;
FETCH NEXT FROM c INTO @object_id;
WHILE @@FETCH_STATUS = 0
BEGIN
PRINT OBJECT_SCHEMA_NAME(@object_id, @database_id)
+ '.' + OBJECT_NAME(@object_id, @database_id);
FETCH NEXT FROM c INTO @object_id;
END
DEALLOCATE c;
END
GO
|
Now that is great, but what if I wanted to do this for multiple (or all databases), without modifying dbo.bar? Well that's simple, right? We'll just create a new ("outer") stored procedure, dbo.foo, which acts as a wrapper for dbo.bar. In reality this could probably take a comma-separated list of database names or IDs, but for brevity let's just keep it simple and assume we want a report for all databases. This just means another cursor, like so:
ALTER PROCEDURE dbo.foo
AS
BEGIN
SET NOCOUNT ON;
DECLARE @database_id INT;
DECLARE c CURSOR
FORWARD_ONLY STATIC READ_ONLY
FOR
(
SELECT [database_id]
FROM sys.databases
);
OPEN c;
FETCH NEXT FROM c INTO @database_id;
WHILE @@FETCH_STATUS = 0
BEGIN
EXEC dbo.bar @database_id = @database_id;
FETCH NEXT FROM c INTO @database_id;
END
DEALLOCATE c;
END
GO |
Do you spot the problem yet?
In the inner procedure, I've declared a cursor with a pretty short and meaningless name, c. Which I tend to do a lot (I did say "bad habit" right?), since I've never come across this issue before - I don't tend to architect solutions that require nested cursors, in fact I avoid cursors as much as I can. The problem is that the outer procedure is coded using the same construct and, both predictably and unfortunately, the same cursor name, c. So when the inner procedure starts, it fails to create the cursor because it already exists and is open. And once the inner procedure is finished, it deallocates c, but then the outer procedure also tries to reference it, leading to these errors:
Msg 16915, Level 16, State 1, Procedure bar, Line 14
A cursor with the name 'c' already exists.
Msg 16905, Level 16, State 1, Procedure bar, Line 19
The cursor is already open.
...
Msg 16916, Level 16, State 1, Procedure foo, Line 21
A cursor with the name 'c' does not exist.
Msg 16916, Level 16, State 1, Procedure foo, Line 26
A cursor with the name 'c' does not exist.
|
There are two solutions here; I fully intend to implement both of them, both to solve this problem, and going forward in general:
- Add LOCAL to the declaration of the cursors, so that they are not global (which is the default). In fact, I usually do this but for some reason it was left out in this case.
- Use a more meaningful and unique name for each cursor, such as "objects" on the inner, and "databases" on the outer.
[NOTE: These procedures are completely contrived and do not represent what the
real code actually does, except in structure alone. I'm just trying to prevent the business logic
from interfering with my reason for posting.]
Now through this exercise I also came across another interesting problem with the use of nested cursors, but I will talk about that in a more general upcoming blog post about some of the worst error messages that come out of SQL Server.