4

I have a table (Database2.dbo.OrganizerDataDependencyChange) that contains information about when a row in certain other tables was last changed. On each of these tables I have a trigger that calls a stored procedure (Database2.dbo.SaveOrganizerDataDependencyChange) that simply updates the Database2.dbo.OrganizerDataDependencyChange with the time that the trigger fired. These triggers are fired from either Database1 or Database2, so they often are cross-database calls.

I'm getting read-write deadlocks on the Database1.dbo.OrganizerDataDependencyChange table when two different rows on the table are being updated and I don't understand why. There's only one index on the table, and it's the clustered index that completely covers the query so no lookup deadlock is possible, and though there are two statements in the proc, I specifically re-wrote it to what I understand is the best way to avoid concurrency issues, with a WHERE NOT EXISTS instead of using IF/ELSE logic, so it shouldn't be coming at this index from different angles, right? Since it's fired in triggers and cross-database I'm having some difficulty reproducing the issue, I can of course reproduce blocking but that should be fine and is what I expect.

Can someone help me understand what's going on here? I could probably fix it with a NOLOCK hint or maybe an applock but I still wouldn't understand why it was happening.

Here's the table:

CREATE TABLE [dbo].[OrganizerDataDependencyChange](
[TableID] [INT] NOT NULL,
[Database] [VARCHAR](150) NOT NULL,
[Updated] [DATETIME] NULL
)
CREATE CLUSTERED INDEX [CX_OrganizerDataDependencyChange_TableID_DB] ON [dbo].[OrganizerDataDependencyChange]
(
[TableID] ASC,
[Database] ASC
)

And here's the stored procedure:

CREATE PROCEDURE [dbo].[SaveOrganizerDataDependencyChange]
(
@TableID int,
@Database varchar(150)
)
AS
BEGIN

SET NOCOUNT ON;

    DECLARE @rowcount INT;

INSERT INTO dbo.OrganizerDataDependencyChange
        ( TableID, [Database], Updated )
SELECT TOP 1
    TableID = @TableID,
    [Database] = ISNULL(@Database, ''),
    Updated = GETDATE()
FROM dbo.OrganizerDataDependencyChange
WHERE NOT EXISTS
(
    SELECT
        1
    FROM dbo.OrganizerDataDependencyChange
    WHERE TableID = @TableID
    AND [Database] = @Database
)

SET @rowcount = @@ROWCOUNT  --How many rows were inserted?

UPDATE dbo.OrganizerDataDependencyChange
SET Updated = GETDATE()
WHERE @rowcount = 0 --Only run this if no rows were inserted
AND TableID = @TableID
AND [Database] = ISNULL(@Database, '')

END

And finally a couple of the deadlock graphs I'm getting out of XE that shows the ridiculous nested triggers:

Deadlock 1

Deadlock 2

MSSQL2014 Standard, if that makes a difference. Isolation level is the default Read Committed. If there's anything else I can share to shed more light please let me know!

6

I think you will find this pattern better:

BEGIN TRANSACTION;

UPDATE dbo.OrganizerDataDependencyChange WITH (HOLDLOCK)
  SET Updated = GETDATE()
  WHERE TableID = @TableID
  AND [Database] = ISNULL(@Database, '');

IF @@ROWCOUNT = 0
BEGIN
  INSERT INTO dbo.OrganizerDataDependencyChange
      ( TableID, [Database], Updated )
  VALUES(@TableID, ISNULL(@Database, ''), GETDATE());
END

COMMIT TRANSACTION;

Stop thinking about having to "check" if there is a row to update, and then updating it, which can lead to two full scans. Just try to update it. If no rows are updated, no harm, no foul - you can simply perform an insert (I talk about this a bit here).

  • Isn't UPDLOCK redundant here ? It seems to work the same with and without it – a1ex07 Apr 3 '17 at 21:44
  • @a1ex07 Possibly, I prefer to be safe and explicit since those things could be copied into larger transactions and I will get blamed when that doesn't go well... – Aaron Bertrand Apr 3 '17 at 22:20
  • @AaronBertrand I think this will be what I'll go with, does this also address the deadlocking? I looked at this blog post from Sam Saffron and he says that it needs to be SERIALIZABLE to avoid deadlocks, which makes sense to me. Is it necessary? – Zaphodb2002 Apr 4 '17 at 16:24
  • @Zaphodb2002 I've changed it to holdlock (serializable's little cousin). – Aaron Bertrand Apr 4 '17 at 17:27
1

I believe the following links will help out here. The first link will likely illustrate how to solve your problem. The second link will help explain what is occurring and why you are having issues. The last link is some additional research that may help you further understand what is happening.

Working 'upsert' examples.

A breakout of locking, blocking, and concurrency when using update and insert.

Stack on concurrency update/insert.

  • So is this article just completely off? That was what I had initially found in my research and what had prompted the rewrite (which did fix a number of deadlocks but seems to have caused just as many different ones). Also TIL the word 'upsert', lol. – Zaphodb2002 Apr 3 '17 at 19:13
  • 1
    I honestly think the solution to this has to do with locking hints instead of ways to write this. I read through his post and I cannot refute or prove one way or another. What I can do is provide anecdotal evidence that by using the upsert merge with locking hints as indicated in my articles, I was able to run the upsert proc in 8 different windows with go 1000, while having several other sessions running selects on the same table. I did not cause any deadlocks, but I did crash my CPU / Laptop. – Shaulinator Apr 3 '17 at 19:40
1

I prefer to be a bit more defensive when performing an upsert, so I use sp_getapplock and sp_releaseapplock to ensure exclusive access to the upsert'd table.

The example code below first creates the table that will be the target of the upsert:

USE tempdb;
GO
IF OBJECT_ID(N'dbo.OrganizerDataDependencyChange', N'U') IS NOT NULL
DROP TABLE dbo.OrganizerDataDependencyChange;
CREATE TABLE dbo.OrganizerDataDependencyChange
(
    PadOneRowPerPage char(5000) NOT NULL
    , TableID int NOT NULL
    , DatabaseName sysname NOT NULL
    , Updated datetime NULL
    , CONSTRAINT PK_OrganizerDataDependencyChange
        PRIMARY KEY CLUSTERED (TableID, DatabaseName)
        WITH (
            ALLOW_ROW_LOCKS = ON
            , ALLOW_PAGE_LOCKS = ON
            , PAD_INDEX = OFF
            , FILLFACTOR = 100
            )
) ON [PRIMARY]
WITH (
    DATA_COMPRESSION = NONE
    );
GO

Since this table is quite likely to be very active if you have update history for multiple tables stored in it, I've added a column PadOneRowPerPage that is a char(5000). This column ensures only one row is created per page, which will reduce allocation and write contention on this table. You will also note I have DATA_COMPRESSION set to NONE; since we are intentionally filling each page with only a single row, data compression is not required and may in fact be detrimental.

Here is the stored procedure to do the actual upsert. It includes a parameter @CallLevel which is used to control nesting into the procedure to a maximum of 5 levels deep. This allows the procedure to call itself in a retry mechanism if it could not obtain an app lock:

IF OBJECT_ID(N'dbo.SaveOrganizerDataDependencyChange', N'P') IS NOT NULL
DROP PROCEDURE dbo.SaveOrganizerDataDependencyChange;
GO
CREATE PROCEDURE dbo.SaveOrganizerDataDependencyChange
(
    @TableID int
    , @DatabaseName varchar(150)
    , @CallLevel int = 1
)
AS
BEGIN
    SET NOCOUNT ON;

    DECLARE @msg nvarchar(1000);
    DECLARE @rowcount int;
    DECLARE @Updated TABLE (
        Updated datetime NOT NULL
    );
    DECLARE @AppLockResult int;
    DECLARE @Result int;

    IF @CallLevel > 5
    BEGIN
        SET @msg = N'Maximum nesting level reached for SaveOrganizerDataDependencyChange';
        THROW 50002, @msg, 1;
        RETURN 0;
    END

    EXEC @AppLockResult = sys.sp_getapplock 
        @Resource = N'SaveOrganizerDataDependencyChange_Resource'
        , @LockMode = 'Exclusive' /*  Shared, Update, IntentShared, 
                                      IntentExclusive, or Exclusive */
        , @LockOwner = 'Transaction'
        , @LockTimeout = NULL --use @@LOCK_TIMEOUT
        , @DbPrincipal = N'public'; /* only run if the caller is a member 
                                       of this role */

    IF @AppLockResult >= 0
    BEGIN
        UPDATE dbo.OrganizerDataDependencyChange
        SET Updated = GETDATE()
        OUTPUT 1 INTO @Updated (Updated)
        WHERE TableID = @TableID
            AND DatabaseName = @DatabaseName;

        IF NOT EXISTS (SELECT 1 FROM @Updated)
        BEGIN
            INSERT INTO dbo.OrganizerDataDependencyChange (
                TableID, DatabaseName, Updated)
            VALUES (@TableID, @DatabaseName, GETDATE());
        END

        EXEC sys.sp_releaseapplock 
            @Resource = N'SaveOrganizerDataDependencyChange_Resource'
            , @LockOwner = 'Transaction'
            , @DbPrincipal = N'public'; /* only run if the caller is a 
                                           member of this role */

        RETURN CONVERT(int, 1); --success
    END
    ELSE 
    BEGIN
        SET @CallLevel += 1;
        EXEC @Result = dbo.SaveOrganizerDataDependencyChange 
                          @TableID
                          , @DatabaseName
                          , @CallLevel;
        IF @Result = 0 
        BEGIN
            SET @msg = N'SaveOrganizerDataDependencyChange Could not obtain app lock.';
            THROW 50001, @msg, 1;
            RETURN 0;
        END
    END
END
GO

You may notice that I'm not using @@ROWCOUNT to determine if the update was successful, as Aaron used in his answer. You could decide to use @@ROWCOUNT since it will be slightly quicker for SQL Server to run the stored procedure; however I chose to use a table variable since it makes the code more robust if you decide in the future to add code between the update and the insert code that might affect the @@ROWCOUNT value.

Here I'm creating a table that will have the trigger that calls the upsert procedure created above:

IF OBJECT_ID(N'dbo.SomeTable', N'U') IS NOT NULL
DROP TABLE dbo.SomeTable;
CREATE TABLE dbo.SomeTable
(
    ID int NOT NULL IDENTITY(1,1)
        CONSTRAINT PK_SomeTable
        PRIMARY KEY CLUSTERED
) ON [PRIMARY] 
WITH (DATA_COMPRESSION = PAGE);
GO

CREATE TRIGGER dbo.upd_SomeTable
ON dbo.SomeTable
AFTER UPDATE, INSERT
AS
BEGIN
    SET NOCOUNT ON;
    DECLARE @Ret int;
    DECLARE @TableID int = OBJECT_ID(N'dbo.SomeTable', N'U');
    DECLARE @DatabaseName sysname = DB_NAME();
    BEGIN TRY
        BEGIN TRANSACTION
        EXEC @Ret = dbo.SaveOrganizerDataDependencyChange 
            @TableID = @TableID
            , @DatabaseName = @DatabaseName;
        IF @Ret = 1
        BEGIN
            COMMIT TRANSACTION;
        END
        ELSE
        BEGIN
            ROLLBACK TRANSACTION;
        END
    END TRY
    BEGIN CATCH
        DECLARE @msg nvarchar(2048);
        DECLARE @err int;
        DECLARE @state int;
        SET @msg = 50000 + ERROR_MESSAGE();
        SET @err = ERROR_NUMBER();
        SET @state = ERROR_STATE();
        IF @@TRANCOUNT > 0 
        BEGIN
            ROLLBACK TRANSACTION;
        END
        ;THROW @err, @msg, @state;
    END CATCH
END
GO

Here, we run this code in as many query windows as possible, in order to test the upsert procedure for deadlocks and to get a sense of performance. The GO 10000 will execute the INSERT statement 10,000 times, simulating quite heavy activity on the table. The more concurrent executions of this statement the better. I was able to get at least 5 sets of this code running concurrently, and experience no deadlocks, no errors, and the 50,000 executions completed within 8 seconds on my development machine:

SET NOCOUNT ON;
GO
INSERT INTO dbo.SomeTable DEFAULT VALUES;
GO 10000 --run the above insert 10,000 times

Show the contents of the upserted table:

SELECT *
FROM dbo.OrganizerDataDependencyChange;
+------------+--------------+-------------------------+
|  TableID   | DatabaseName |         Updated         |
+------------+--------------+-------------------------+
| 1861581670 | tempdb       | 2017-04-04 08:07:59.497 |
+------------+--------------+-------------------------+
0

I like the Aaron's upsert examples suggestion for total cost of ownership if you are going to keep this pattern around for a while like in all your processes.

But I also see a simple change could be the where not exists Change that to a left outer join where pk is null. and Add With (NoLock) on those reads if you are batch processing and dirty reads are not an issue.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.