update and insert queries creating a deadlock

Question

I will try to explain my problem as detailed as possible, and i would appreciate any help/suggestion. My problem is regarding a deadlock being caused by two queries (one insert and one update). I'm using MS-SQL server 2008

I have two applications using the same database:

1. Web app (on every request multiple records are inserted in the Impressions table by calling a stored procedure)
2. Windows service (calculates all the Impressions done in one minute, every minute, for the previous minute and sets a flag on each of the Impressions calculated via a stored procedure as well)

The web app inserts the impressions records without using a transaction, while the windows service application calculates the impressions while using a IsolationLevel.ReadUncommitted transaction. The stored procedure in the windows service app does something like this:

Windows Service stored procedure:

Loops trough all the impressions that have the isCalculated flag set to false and date < @now , increments a counter and other data in another table connected to the impressions table, and sets the isCalculated flag to true on impressions that have date < @now. Because this stored procedure is pretty big, no point in pasting it, here is a shortened code snippet of what the proc does:

DECLARE @nowTime datetime = convert(datetime, @now, 21) 
DECLARE dailyCursor CURSOR FOR

SELECT  Daily.dailyId, 
        Daily.spentDaily, 
        Daily.impressionsCountCache ,
        SUM(Impressions.amountCharged) as sumCharged, 
        COUNT(Impressions.impressionId) as countImpressions
FROM    Daily INNER JOIN Impressions on Impressions.dailyId = Daily.dailyId
WHERE   Impressions.isCharged=0 AND Impressions.showTime < @nowTime AND Daily.isActive = 1
GROUP BY Daily.dailyId, Daily.spentDaily, Daily.impressionsCountCache

OPEN dailyCursor

DECLARE @dailyId int, 
        @spentDaily decimal(18,6), 
        @impressionsCountCache int, 
        @sumCharged decimal(18,6), 
        @countImpressions int

FETCH NEXT FROM dailyCursor INTO @dailyId,@spentDaily, @impressionsCountCache, @sumCharged, @countImpressions

WHILE @@FETCH_STATUS = 0
    BEGIN   

        UPDATE Daily 
        SET spentDaily= @spentDaily + @sumCharged, 
            impressionsCountCache = @impressionsCountCache + @countImpressions
        WHERE dailyId = @dailyId

        FETCH NEXT FROM dailyCursor INTO @dailyId,@spentDaily, @impressionsCountCache, @sumCharged, @countImpressions
    END
CLOSE dailyCursor
DEALLOCATE dailyCursor

UPDATE Impressions 
SET isCharged=1 
WHERE showTime < @nowTime AND isCharged=0

Web App Stored Procedure:

This procedure is pretty simple it just inserts the record in the table. Here is a shortened code snippet:

INSERT INTO Impressions 
(dailyId, date, pageUrl,isCalculated) VALUES 
(@dailyId, @date, @pageUrl, 0)

The Code

The code that calls these stored procedures is pretty simple it just creates the SQL commands passing the needed parameters and executes them

//i send the date like this
string date = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss.fff", 
CultureInfo.InvariantCulture);

SqlCommand comm = sql.StoredProcedureCommand("storedProcName", 
parameters, values);

I'm experiencing deadlocks very often (the exceptions occur in the web app, not the windows service), and after using the SQL-Profiler, I found out that the deadlocks are probably happening because of these two queries (I don't have much experience in analyzing profiler data).

The latest trace data collected from the SQL server profiler can be found on the bottom of this question

In theory these two stored procedures should be able to work together because the first one inserts the records one by one with date=DateTime.Now, and the second one calculates the Impressions that have date < DateTime.Now.

Edit:

Here is the code run in the windows service app:

SQL sql = new SQL();
DateTime endTime = DateTime.Now;
//our custom DAL class that opens a connection
sql.StartTransaction(IsolationLevel.ReadUncommitted);
try
{
    List<string> properties = new List<string>() { "now" };
    List<string> values = new List<string>() { endTime.ToString("yyyy-MM-dd HH:mm:ss.fff", CultureInfo.InvariantCulture) };
    SqlCommand comm = sql.StoredProcedureCommannd("ChargeImpressions", properties, values);
    comm.Transaction = sql.Transaction;
    ok = sql.CheckExecute(comm);
}
catch (Exception up)
{
    ok = false;
    throw up;
}
finally
{
    if (ok)
      sql.CommitTransaction();
    else
      sql.RollbackTransactions();
    CloseConn();
}

EDIT:

I added the indexes on both of the tables as suggested by Martin Smith like this:

CREATE NONCLUSTERED INDEX [IDX_Daily_DailyId] ON [dbo].[Daily] 
(
    [daily] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, SORT_IN_TEMPDB = OFF, IGNORE_DUP_KEY = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
GO

and

CREATE NONCLUSTERED INDEX [IDX_Impressions_isCharged_showTime] ON [dbo].[Impressions] 
(
    [isCharged] ASC,
    [showTime] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, SORT_IN_TEMPDB = OFF, IGNORE_DUP_KEY = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
GO

For now no exceptions, will report back later

Edit:

Unfortunately this did not solve the deadlock issue. I will start a deadlock trace in profiler to see if the deadlocks are the same as before.

Edit:

Pasted the new trace (to me it looks the same as the previous one), couldn't capture a screen of the execution plan (its too big) but here is the xml from the execution plan.And here is a screenshot of the execution plan of the insert query:

 <deadlock victim="process14e29e748">
  <process-list>
   <process id="process14e29e748" taskpriority="0" logused="952" waitresource="KEY: 6:72057594045071360 (f473d6a70892)" waittime="4549" ownerId="2507482845" transactionname="INSERT" lasttranstarted="2011-09-05T11:59:16.587" XDES="0x15bef83b0" lockMode="S" schedulerid="1" kpid="2116" status="suspended" spid="65" sbid="0" ecid="0" priority="0" trancount="2" lastbatchstarted="2011-09-05T11:59:16.587" lastbatchcompleted="2011-09-05T11:59:16.587" clientapp=".Net SqlClient Data Provider"  hostpid="2200"  isolationlevel="snapshot (5)" xactid="2507482845" currentdb="6" lockTimeout="4294967295" clientoption1="671088672" clientoption2="128056">
    <executionStack>
     <frame procname="dbo.InsertImpression" line="27" stmtstart="2002" stmtend="2560" sqlhandle="0x03000600550e30512609e200529f00000100000000000000">
INSERT INTO Impressions 
    (dailyId, languageId, showTime, pageUrl, amountCharged, age, ipAddress, userAgent, portalId, isCharged,isCalculated) VALUES 
    (@dailyId, @languageId, @showTime, @pageUrl, @amountCharged, @age, @ip, @userAgent, @portalId, 0, 0)     </frame>
    </executionStack>
    <inputbuf>
Proc [Database Id = 6 Object Id = 1362103893]    </inputbuf>
   </process>
   <process id="process6c9dc8" taskpriority="0" logused="335684" waitresource="KEY: 6:72057594045464576 (5fcc21780b69)" waittime="4475" ownerId="2507482712" transactionname="transaction_name" lasttranstarted="2011-09-05T11:59:15.737" XDES="0x1772119b0" lockMode="U" schedulerid="2" kpid="3364" status="suspended" spid="88" sbid="0" ecid="0" priority="0" trancount="2" lastbatchstarted="2011-09-05T11:59:15.737" lastbatchcompleted="2011-09-05T11:59:15.737" clientapp=".Net SqlClient Data Provider"  hostpid="1436" isolationlevel="read uncommitted (1)" xactid="2507482712" currentdb="6" lockTimeout="4294967295" clientoption1="671088672" clientoption2="128056">
    <executionStack>
     <frame procname="dbo.ChargeImpressions" line="60" stmtstart="4906" stmtend="5178" sqlhandle="0x03000600e3c5474f0609e200529f00000100000000000000">
UPDATE Impressions 
    SET isCharged=1 
    WHERE showTime &amp;lt; @nowTime AND isCharged=0

    </frame>
    </executionStack>
    <inputbuf>
Proc [Database Id = 6 Object Id = 1330103779]    </inputbuf>
   </process>
  </process-list>
  <resource-list>
   <keylock hobtid="72057594045071360" dbid="6" objectname="dbo.Daily" indexname="PK_Daily" id="lock14c6aab00" mode="X" associatedObjectId="72057594045071360">
    <owner-list>
     <owner id="process6c9dc8" mode="X"/>
    </owner-list>
    <waiter-list>
     <waiter id="process14e29e748" mode="S" requestType="wait"/>
    </waiter-list>
   </keylock>
   <keylock hobtid="72057594045464576" dbid="6" objectname="dbo.Impressions" indexname="IDX_Impressions_isCharged_showTime" id="lock14c901200" mode="X" associatedObjectId="72057594045464576">
    <owner-list>
     <owner id="process14e29e748" mode="X"/>
    </owner-list>
    <waiter-list>
     <waiter id="process6c9dc8" mode="U" requestType="wait"/>
    </waiter-list>
   </keylock>
  </resource-list>
 </deadlock>

Edit:

After suggestions from Jonathan Dickinson:

1. I changed the stored procedure (removed the cursor),
2. I changed the IDX_Impressions_isCharged_showTime to not allow PAGE_LOCKS and
3. I added -1 second to the @now property in the windows service application to avoid borderline deadlock cases.

Update:

The query execution time was decreased after the last changes, but the number of exceptions hasn't.

Hopefully last update:

The changes proposed by Martin Smith are now live, the insert query now uses the non-clustered index and in theory this should fix the issue. For now no exceptions have been reported (Keeping my fingers crossed)

Answer 1

Avoid cursors, that query had no need for them. SQL is not an imperative language (which is why it gets a bad name because everyone uses it as one) - it's a set language.

First thing you can do is speed up the basic execution of your SQL, less time parsing/executing the query means less chance of a deadlock:

• Prefix all your tables with [dbo] - this cuts up to 30% off the parse stage.
• Alias your tables - it cuts off a small amount off the planning stage.
• Quoting identifiers may speed things up.
• These are tips from an ex-SQL-PM before anyone decides to dispute it.

You can use a CTE to get the data to update and then use a UPDATE ... FROM ... SELECT statement to do the actual updates. This will be faster than a cursor, because cursors are dog slow when compared to clean set operations (even the fastest 'fire hose' cursor like yours). Less time spent updating means less of a chance of a deadlock. Note: I don't have your original tables, I can't validate this - so check it against a development DB.

DECLARE @nowTime datetime = convert(datetime, @now, 21);

WITH [DailyAggregates] AS
(
    SELECT  
        [D].[dailyId] AS [dailyId],
        [D].[spentDaily] AS [spentDaily],
        [D].[impressionsCountCache] AS [impressionsCountCache],
        SUM([I].[amountCharged]) as [sumCharged],
        COUNT([I].[impressionId]) as [countImpressions]
        FROM [dbo].[Daily] AS [D]
            INNER JOIN [dbo].[Impressions] AS [I]
               ON [I].[dailyId] = [D].[dailyId]
        WHERE [I].[isCharged] = 0
          AND [I].[showTime] < @nowTime 
          AND [D].[isActive] = 1
    GROUP BY [D].[dailyId], [D].[spentDaily], [D].[impressionsCountCache]
)
UPDATE [dbo].[Daily]
    SET [spentDaily] = [A].[spentDaily] + [A].[sumCharged],
        [impressionsCountCache] = [A].[impressonsCountCache] + [A].[countImpressions]
    FROM [Daily] AS [D]
    INNER JOIN [DailyAggregates] AS [A]
       ON [D].[dailyId] = [A].[dailyId];

UPDATE [dbo].[Impressions]
SET [isCharged] = 1 
WHERE [showTime] < @nowTime 
  AND [isCharged] = 0;

Furthermore you could disallow PAGE locks on your index, this will decrease the chances of a few rows locking a whole page (because of locking escalation, only a certain percentage of rows need to be locked before the entire page is just locked).

CREATE NONCLUSTERED INDEX [IDX_Impressions_isCharged_showTime] ON [dbo].[Impressions]              
(
    [showTime] ASC, -- I have a hunch that switching these around might have an effect.
    [isCharged] ASC  
)
WITH (ALLOW_PAGE_LOCKS = OFF)
ON [PRIMARY] 
GO

This will just mitigate the chances of a deadlock. You might try restricting @now a date in the past (i.e. today - 1 day) to make sure that the inserted row does not fall into the update predicate; chances are it will prevent the deadlock entirely.

Answer 2

Your windows service cursor updates various rows in Daily for which it takes X locks. These won't be released until the transaction ends.

Your web app then does an Insert into Impressions and keeps an X lock on the newly inserted row whilst it waits for an S lock on one of the rows in Daily that are locked by the other process. It needs to read this to validate the FK constraint.

Your windows service then does the Update on Impressions taking U locks on the rows it scans along the way. There is no index that allows it to seek into the rows so this scan includes the row added by the web app.

So

(1) You could add a composite index to Impressions on showTime, isCharged or vice-versa (check the execution plans) to allow the rows that the windows service will update to be found by an index seek rather than a full scan.

-Or

(2) You could add a redundant non clustered index on Daily(DailyId). This will be a lot narrower than the clustered one so the FK validation will likely use that in preference to needing an S lock on the clustered index row.

Edit

Disclaimer: The following is based on supposition and observation rather than anything I have found documented!

It seems that idea (2) does not work "as is". The execution plan shows that the FK validation still continues to happen against the clustered index regardless of the fact that a narrower index is now available. sys.foreign_keys has columns referenced_object_id, key_index_id and I speculate that the validation will currently always happen on the index listed there and the Query Optimiser doesn't currently consider alternatives but haven't found anything documenting this.

I found that the relevant values in sys.foreign_keys and the query plan changed to start using the narrower index after I dropped and re-added the Foreign Key constraint.

CREATE TABLE Daily(
    DailyId INT IDENTITY(1,1) PRIMARY KEY CLUSTERED  NOT NULL,
    Filler CHAR(4000) NULL,
) 

INSERT INTO Daily VALUES ('');


CREATE TABLE Impressions(
    ImpressionId INT IDENTITY(1,1) PRIMARY KEY NOT NULL,
    DailyId INT NOT NULL CONSTRAINT FK REFERENCES Daily (DailyId), 
    Filler CHAR(4000) NULL,
)

/*Execution Plan uses clustered index - There is no NCI*/ 
INSERT INTO Impressions VALUES (1,1) 

ALTER TABLE Daily ADD CONSTRAINT
    UQ_Daily UNIQUE NONCLUSTERED(DailyId) 

/*Execution Plan still use clustered index even after NCI created*/    
INSERT INTO Impressions VALUES (1,1) 

ALTER TABLE Impressions DROP CONSTRAINT FK
ALTER TABLE Impressions  WITH CHECK ADD  CONSTRAINT FK FOREIGN KEY(DailyId)
REFERENCES Daily (DailyId)    

/*Now Execution Plan now uses non clustered index*/    
INSERT INTO Impressions VALUES (1,1)    

Answer 3

I am sure that the changes the other answers suggest are called for since for example the use of a cursor is not necessary in your case... from the code you supplied there is even no need for the WHILE too...

I am no SQL Server guy... If I needed to do what your Stored Procedure is doing I would make sure that @nowTime = DateTime.Now.AddSeconds(-1) and code it similar to the following:

BEGIN

UPDATE Daily D SET 
D.spentDaily= D.spentDaily + (SELECT SUM(I.amountCharged) FROM Impressions I WHERE I.isCharged=0 AND I.showTime < @nowTime AND I.DailyId = D.DailyId), 
D.impressionsCountCache = D.impressionsCountCache + (SELECT COUNT(I.impressionId) FROM Impressions I WHERE I.isCharged=0 AND I.showTime < @nowTime AND I.DailyId = D.DailyId)
WHERE D.DailyId IN (SELECT I.DailyId FROM Impressions I WHERE I.isCharged=0 AND I.showTime < @nowTime AND I.DailyId = D.DailyId) AND D.isActive = 1;

UPDATE Impressions I SET
I.isCharged=1 
WHERE I.showTime < @nowTime AND I.isCharged=0;

COMMIT;

END

Even with high load never had any deadlock trouble with any parallel INSERT/UPDATE/DELETE on Impressions this way (although that was Oracle)... HTH