SQL Server - Delete from table variables not instantaneous?

Question

I have inherited maintaining a stored procedure that is executed nightly via a SQL Agent job. It has been running well for months, but all of sudden last nights run duplicated some work and missed some work.

The job runs in the middle of the night, and there are no users at this time. I restored a backup of the database from just before the problematic run to a test server, re-ran the procedure, and everything worked just fine. It's small data too, maybe 100-200 rows a night.

Here is a representation of one of the loops in the procedure where the issue was encountered:

DECLARE @uniqueId int
DECLARE @examId int

DECLARE @TempSingleContactTable TABLE 
(
    uniqueId int IDENTITY(1,1) PRIMARY KEY, 
    examId int not null,
    contactEmail nvarchar(max) null,
)

[data inserted into @TempSingleContactTable]

WHILE EXISTS (SELECT * FROM @TempSingleContactTable)
  BEGIN
    Select top 1 @uniqueId = uniqueId, 
        @examId = examID,  from @TempSingleContactTable

    [*****PROBLEM HERE- this line with same value for @examId ran multiple times, but eventually continued]


    DELETE FROM @TempSingleContactTable WHERE examID = @examId 
  END

The only thing I can see that could cause the issue above is if the DELETE call didn't work. Is it possible that a DELETE call against a table variable is not instantaneous?

---

EDIT: Any info on what could be causing the Delete from @TempSingleContactTable to fail sporadically is much appreciated.

---

EDIT 2: Additional investigation has shown that this automated once-nightly procedure has failed the same way twice over two months. Interestingly, each time it failed, the previous night's run didn't alter any data and it always should. Unfortunately, there is no logging information to determine what might have caused the previous nights issues. It seems like they have to be related, although it could be a red herring. I have added logging with the hope of getting at the actual underlying cause.

Answer 1

From what it looks like you've inherited a "poor man's cursor". Somehow people heard that cursors are 'evil' and they then come up with this =( I'm not going to start a debate on how set-based is preferred over cursor-based (read: line by line) operations. In some situations you simply have no choice; maybe this is one too.

Converting your loop to a decent cursor would probably already 'stabilize' that part of the loop; but it also immediately shows there is a bit a of 'problem' with your loop.

On first sight, the equivalent cursor would be this:

DECLARE @uniqueId int
DECLARE @examId int

DECLARE @TempSingleContactTable TABLE 
(
    uniqueId int IDENTITY(1,1) PRIMARY KEY, 
    examId int not null,
    contactEmail nvarchar(max) null
)

-- [data inserted into @TempSingleContactTable]

DECLARE exams_loop CURSOR LOCAL FAST_FORWARD 
    FOR SELECT uniqueId, examID
          FROM @TempSingleContactTable
OPEN exams_loop 
FETCH NEXT FROM exams_loop INTO @uniqueId, @examId 
WHILE @@FETCH_STATUS = 0
    BEGIN

        -- internals...

        FETCH NEXT FROM exams_loop INTO @uniqueId, @examId 
    END
CLOSE exams_loop 
DEALLOCATE exams_loop 

But when looking more closely there is a catch: the end of your loop deletes all records for a given examID. So if there are multiple records with the same examID, this means that some uniqueID values would be skipped. (remark: it's not even certain which ones, never ever be tempted to rely on them being in natural order because there is a PK on the field!)

As such the following code is a better replacement:

DECLARE exams_loop CURSOR LOCAL FAST_FORWARD 
    FOR SELECT MIN(uniqueId), examID
          FROM @TempSingleContactTable
         GROUP BY examID
OPEN exams_loop 
FETCH NEXT FROM exams_loop INTO @uniqueId, @examId 
WHILE @@FETCH_STATUS = 0
    BEGIN

        -- internals...

        FETCH NEXT FROM exams_loop INTO @uniqueId, @examId 
    END
CLOSE exams_loop 
DEALLOCATE exams_loop 

This time it will indeed be the lowest uniqueID that wins out instead of a random one, but in all fairness I think repeatability (which is what we're talking about here really) is to be preferred over randomness.

Anyway, in summary :

• rather use a real cursor instead of a poor-man's replacement because it's a bad replacement to start with
• if you really want to keep the loop as it is right now, change the table-definition to this:

=>

DECLARE @TempSingleContactTable TABLE 
(
    uniqueId int IDENTITY(1,1) PRIMARY KEY, 
    examId int not null UNIQUE (examId, uniqueId),
    contactEmail nvarchar(max) null
)

this way you'll at least have an index on the field when you're deleting. (even though I'd highly discourage intensive operations on @table-variables, they tend to go south once you put 'medium' amounts of data in there, let alone start doing operations on it... #temp-tables are much more robust in that respect!)