Why does my cursor stop in the middle of a loop?

Question

The code posted here is 'example' code, it's not production code. I've done this to make the problem I'm explaining readable / concise.

---

Using code similar to that below, we're coming across a strange bug. After every INSERT the WHILE loop is stopped.

table containst 100 rows, when the insert is done after 50 rows then the cursor stops, having only touched the first 50 rows. When the insert is done after 55 it stops after 55, and so on.

-- This code is an hypothetical example written to express
-- an problem seen in production

DECLARE @v1 int
DECLARE @v2 int

DECLARE MyCursor CURSOR FAST_FORWARD FOR
SELECT Col1, Col2
FROM table

OPEN MyCursor

FETCH NEXT FROM MyCursor INTO @v1, @v2

WHILE(@@FETCH_STATUS=0)
BEGIN

  IF(@v1>10)
  BEGIN
    INSERT INTO table2(col1) VALUES (@v2)
  END

  FETCH NEXT FROM MyCursor INTO @v1, @v2

END

CLOSE MyCursor
DEALLOCATE MyCursor

There is an AFTER INSERT trigger on table2 which is used to log mutaties on table2 into an third table, aptly named mutations. This contains an cursor which inserts to handle the insert (mutations are logged per-column in an very specific manner, which requires the cursor).

A bit of background: this exists on an set of small support tables. It is an requirement for the project that every change made to the source data is logged, for auditing purposes. The tables with the logging contain things such as bank account numbers, into which vast sums of money will be deposited. There are maximum a few thousand records, and they should only be modified very rarely. The auditing functionality is there to discourage fraud: as we log 'what changed' with 'who did it'.

The obvious, fast and logical way to implement this would be to store the entire row each time an update is made. Then we wouldn't need the cursor, and it would perform an factor better. However the politics of the situation means my hands are tied.

Phew. Now back to the question.

Simplified version of the trigger (real version does an insert per column, and it also inserts the old value):

--This cursor is an hypothetical cursor written to express
--an problem seen in production.

--On UPDATE a new record must be added to table Mutaties for
--every row in every column in the database.  This is required
--for auditing purposes.

--An set-based approach which stores the previous state of the row
--is expressly forbidden by the customer


DECLARE @col1 int
DECLARE @col2 int
DECLARE @col1_old int
DECLARE @col2_old int

--Loop through old values next to new values
DECLARE MyTriggerCursor CURSOR FAST_FORWARD FOR
SELECT i.col1, i.col2, d.col1 as col1_old, d.col2 as col2_old
FROM Inserted i
  INNER JOIN Deleted d ON i.id=d.id

OPEN MyTriggerCursor 

FETCH NEXT FROM MyTriggerCursor INTO @col1, @col2, @col1_old, @col2_old

--Loop through all rows which were updated
WHILE(@@FETCH_STATUS=0)
BEGIN

    --In production code a few more details are logged, such as userid, times etc etc

    --First column
    INSERT Mutaties (tablename, columnname, newvalue, oldvalue)
    VALUES ('table2', 'col1', @col1, @col1_old)

    --Second column
    INSERT Mutaties (tablename, columnname, newvalue, oldvalue)
    VALUES ('table2', 'col2', @col2, @col1_old)

    FETCH NEXT FROM MyTriggerCursor INTO @col1, @col2, @col1_old, @col2_old

END

CLOSE MyTriggerCursor
DEALLOCATE MyTriggerCursor

Why is the code exiting in the middle of the loop?

Answer 1

Post your trigger too. then your problem can be understood properly

Answer 2

This code does not fetch any further values from the cursor, nor does it increment any values. As it is, there is no reason to implement a cursor here.

Your entire code could be rewritten as:

DECLARE @v1 int
DECLARE @v2 int

SELECT @v1 = Col1, @v2 = Col2
FROM table

IF(@v1>10)
    INSERT INTO table2(col1) VALUES (@v2)

---

Edit: Post has been edited to fix the problem I was referring to.

Answer 3

Ryan, your problem is that @@FETCH_STATUS is global to all cursors in an connection.

So the cursor within the trigger ends with an @@FETCH_STATUS of -1. When control returns to the code above, the last @@FETCH_STATUS was -1 so the cursor ends.

That's explained in the documentation, which can be found on MSDN [here].

What you can do is use an local variable to store the @@FETCH_STATUS, and put that local variable in the loop. So you get something like this:

DECLARE @v1 int
DECLARE @v2 int
DECLARE @FetchStatus int

DECLARE MyCursor CURSOR FAST_FORWARD FOR
SELECT Col1, Col2
FROM table

OPEN MyCursor

FETCH NEXT FROM MyCursor INTO @v1, @v2

SET @FetchStatus = @@FETCH_STATUS

WHILE(@FetchStatus=0)
BEGIN

  IF(@v1>10)
  BEGIN
    INSERT INTO table2(col1) VALUES (@v2)
  END

  FETCH NEXT FROM MyCursor INTO @v1, @v2

  SET @FetchStatus = @@FETCH_STATUS

END

CLOSE MyCursor
DEALLOCATE MyCursor

It's worth noting that this behaviour does not apply to nested cursors. I've made an quick example, which on SqlServer 2008 returns the expected result (50).

USE AdventureWorks
GO

DECLARE @LocationId smallint
DECLARE @ProductId smallint

DECLARE @Counter int
SET @Counter=0

DECLARE MyFirstCursor CURSOR FOR 
SELECT TOP 10 LocationId
FROM Production.Location

OPEN MyFirstCursor

FETCH NEXT FROM MyFirstCursor INTO @LocationId

WHILE (@@FETCH_STATUS=0)
BEGIN

    DECLARE MySecondCursor CURSOR FOR
    SELECT TOP 5 ProductID
    FROM Production.Product

    OPEN MySecondCursor

    FETCH NEXT FROM MySecondCursor INTO @ProductId

    WHILE(@@FETCH_STATUS=0)
    BEGIN

     SET @Counter=@Counter+1

     FETCH NEXT FROM MySecondCursor INTO @ProductId 

    END

    CLOSE MySecondCursor
    DEALLOCATE MySecondCursor

    FETCH NEXT FROM MyFirstCursor INTO @LocationId

END

CLOSE MyFirstCursor
DEALLOCATE MyFirstCursor

--
--Against the initial version of AdventureWorks, counter should be 50.
--
IF(@Counter=50)
    PRINT 'All is good with the world'
ELSE
    PRINT 'Something''s wrong with the world today'

Answer 4

As ck mentioned, you are not fetching any further values. The @@FETCH_STATUS thus get's its value from your cursor contained in your AFTER INSERT trigger.

You should change your code to

DECLARE @v1 int
DECLARE @v2 int
DECLARE MyCursor CURSOR FAST_FORWARD FOR
SELECT Col1, Col2
FROM table

OPEN MyCursor

FETCH NEXT FROM MyCursor INTO @v1, @v2

WHILE(@@FETCH_STATUS=0)
BEGIN
  IF(@v1>10)
  BEGIN
    INSERT INTO table2(col1) VALUES (@v2)
  END
  FETCH NEXT FROM MyCursor INTO @v1, @v2
END

Answer 5

Your problem is that you should NOT be using a cursor for this at all! This is the code for the example given above.

INSERT INTO table2(col1)
SELECT Col1 FROM table
where col1>10

You also should never ever use a cursor in a trigger, that will kill performance. If someone added 100,000 rows in an insert this could take minutes (or even hours) instead of millseconds or seconds. We replaced one here (that predated my coming to this job) and reduced an import to that table from 40 minites to 45 seconds.

Any production code that uses a cursor should be examined to replace it with correct set-based code. in my experience 90+% of all cursors can be reqwritten in a set-based fashion.

Answer 6

this is a simple misunderstanding of triggers... you don't need a cursor at all for this

if UPDATE(Col1)
begin

    insert into mutaties
    (
     tablename, 
     columnname, 
     newvalue
    )
    select
    'table2',
    coalesce(d.Col1,''),
    coalesce(i.Col1,''),
    getdate()
    from inserted i
     join deleted d on i.ID=d.ID
      and coalesce(d.Col1,-666)<>coalesce(i.Col1,-666)

end

basically what this code does is it checks to see if that column's data was updated. if it was, it compares the new and old data, and if it's different it inserts into your log table.

you're first code example could easily be replaced with something like this

insert into table2 (col1)
select Col2
from table
where Col1>10

Answer 7

You do not have to use a cursor to insert each column as a separate row.

Here is an example:

INSERT LOG.DataChanges
SELECT
   SchemaName = 'Schemaname',
   TableName = 'TableName',
   ColumnName = CASE ColumnID WHEN 1 THEN 'Column1' WHEN 2 THEN 'Column2' WHEN 3 THEN 'Column3' WHEN 4 THEN 'Column4' END
   ID = Key1,
   ID2 = Key2,
   ID3 = Key3,
   DataBefore = CASE ColumnID WHEN 1 THEN I.Column1 WHEN 2 THEN I.Column2 WHEN 3 THEN I.Column3 WHEN 4 THEN I.Column4 END,
   DataAfter = CASE ColumnID WHEN 1 THEN D.Column1 WHEN 2 THEN D.Column2 WHEN 3 THEN D.Column3 WHEN 4 THEN D.Column4 END,
   DateChange = GETDATE(),
   USER = WhateverFunctionYouAreUsingForThis
FROM
   Inserted I
   FULL JOIN Deleted D ON I.Key1 = D.Key1 AND I.Key2 = D.Key2
   CROSS JOIN (
      SELECT 1 UNION ALL SELECT 2 UNION ALL SELECT 3 UNION ALL SELECT 4
   ) X (ColumnID)

In the X table, you could code additional behavior with a second column that specially describes how to handle just that column (let's say you wanted some to post all the time, but others only when the value changes). What's important is that this is an example of the cross join technique of splitting rows into each column, but there is a lot more that can be done. Note that the full join allows this to work on inserts and deletes as well as updates.

I also fully agree that storing each row is FAR superior. See this forum for more about this.