January 14, 2010 | SQL Server

When you don't follow your own "bad habits" advice…

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:

  1. 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.
       
  2. 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.

4 comments on this post

    • Peter - January 19, 2010, 8:29 PM

      Try a cursor variable instead. As the name implies, they have variable-style scoping:
      DECLARE @cursor CURSOR
      SET @cursor = CURSOR STATIC FOR
       SELECT database_id FROM sys.databases
      OPEN @cursor
      — etc.
      GO
      — will raise an error, since it is already out of scope.
      DEALLOCATE @cursor

    • Marcin - February 8, 2010, 5:09 PM

      Isn't the CURSOR_DEFAULT database option solution for this? I believe a need for global cursors is quite rare so I assume it can be safely use on most of the real world databases? Especially as one can still declare cursors with global scope by adding word GLOBAL to the definition.

    • AaronBertrand - February 8, 2010, 5:16 PM

      Marcin, yes that is true, but I challenge you to find many databases out in the real world where the default (GLOBAL) has been changed.

    • Marcin - February 8, 2010, 6:08 PM

      You are absolutely right I really don't know why Microsoft did not change the default settings to LOCAL. Great blog by the way!

Comments are closed.