c# - 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.