is WITH (ROWLOCK) hint a culprit in this code?

  • VoldemarG

    Hall of Fame

    Points: 3615

    In the attached Stored Procedure, the yellow-highlighted code causes major blocking, especially when the SP gets executed 50 times at the same time or even more times.

    When low contention/concurrency times of the day happen, everything works fine. Exec this SP takes a second.

    But during highest concurrency periods happen, the entire .aspx web page from which this SP is called is HANGING and timing out after 3 minutes of freeze.

    Database Transaction Isolation level = Read Committed.

    My analysis suggests that the WITH (ROWLOCK) hint is the culprit, and if I just remove it, the operations will go smoothly and blocking will at least be reduced.  The table being updated in the identified culprit Update statement (highlighted)  has only a few thousand rows, no more than 10 000.

    This is a Prod issue, and I greatly appreciate your input.

     

     

    Voldemar likes to play CHESS (and IS good at it!)

  • VoldemarG

    Hall of Fame

    Points: 3615

    Sorry. PDF file not allowed.

    Attaching as WORD.

    Attachments:
    You must be logged in to view attached files.

    Voldemar likes to play CHESS (and IS good at it!)

  • ScottPletcher

    SSC Guru

    Points: 98401

    Shouldn't matter, unless it's part of a (much) larger transaction.  A rowlock (at least) will be required to do the UPDATE anyway.  But you could remove it, just in case.

    Much more likely to resolve the issue would be to make sure the WHERE clause can be satisfied by an index and that it doesn't end up resulting in a table/index scan.

    SQL DBA,SQL Server MVP(07, 08, 09) Prosecutor James Blackburn, in closing argument in the Fatal Vision murders trial: "If in the future, you should cry a tear, cry one for them [the murder victims]. If in the future, you should say a prayer, say one for them. And if in the future, you should light a candle, light one for them."

  • frederico_fonseca

    SSChampion

    Points: 14478

    code here instead of word doc - easier for most to see

    one thing i would change is the fact that the code, if the record does not exist already, does an insert followed by an update of the record just created - record should be created with the required values without the need for the update to happen

     

     
    /* DECLARE @oGID int

    EXEC [prGetGidv2]
    @chrObjectName = 'DAN_TESTV2'
    ,@relGidValue = @oGID OUTPUT
    ,@QtyNeeded = 100

    SELECT @oGID
    */
    ALTER PROCEDURE [dbo].[prGetGid]
    (@chrObjectName VARCHAR(30),
    @relGidValue DECIMAL(12,0) OUTPUT,
    @QtyNeeded DECIMAL(12,0) = 1)
    AS
    DECLARE
    @l_today DATETIME,
    @rowcount INT,
    @retry BIT

    BEGIN

    SET NOCOUNT ON

    SET @l_today = GETDATE()
    SET @relGidValue = -1
    SET @retry = 0

    BEGIN TRY
    /*<DocGen_Description>Ensure to validate the object name. If object name is blank or null then raise the error
    message: "Object Name is blank. Contact product support." </DocGen_Description>*/
    IF (RTRIM(@chrObjectName) = '' OR @chrObjectName = NULL)
    BEGIN
    RAISERROR ('Object Name is blank. Contact product support.', 16, 1);
    END;
    /*<DocGen_Description>Ensure to validate the quantity needed. If quantity needed is less than 1 or null then raise the error
    message: "Quantity needed is less than 1. Contact product support."</DocGen_Description>*/
    IF (@QtyNeeded < 1 OR @QtyNeeded = NULL)
    BEGIN
    RAISERROR ('Quantity needed is less than 1. Contact product support.', 16, 1);
    END

    Retry:

    UPDATE
    GID_Values WITH(ROWLOCK)
    SET
    gid_value = gid_value + @QtyNeeded,
    last_touch = @l_today,
    @relGidValue = gid_value + 1
    WHERE
    [object_name] = RTRIM(@chrObjectName)

    SET @rowcount = @@ROWCOUNT

    IF @rowcount = 0 AND @retry = 0
    BEGIN
    BEGIN TRY
    INSERT INTO GID_Values ([object_name], gid_value, last_touch)
    VALUES (RTRIM(@chrObjectName), 0, @l_today)

    SET @retry = 1

    GOTO Retry
    END TRY
    BEGIN CATCH
    /*<DocGen_Description>In case error in creating unique identifier for entity then raise the error message: "Error Creating an Unique Identifier For Entity."
    </DocGen_Description>*/
    RAISERROR ('Error Creating an Unique Identifier For Entity. ', 16, 1)
    END CATCH;
    END
    /*<DocGen_Description>If no records added then raise the error message: "Error Getting Unique Identifier For Entity After Retry."
    </DocGen_Description>*/
    ELSE IF @rowcount = 0
    RAISERROR ('Error Getting Unique Identifier For Entity After Retry. ', 16, 1)

    END TRY
    BEGIN CATCH
    IF (XACT_STATE()) <> 0
    BEGIN
    ROLLBACK TRANSACTION;
    END;
    DECLARE @ErrorMessage NVARCHAR(4000);
    DECLARE @ErrorSeverity INT;
    DECLARE @ErrorState INT;

    SELECT
    @ErrorMessage = ERROR_MESSAGE(),
    @ErrorSeverity = ERROR_SEVERITY(),
    @ErrorState = ERROR_STATE();

    RAISERROR (@ErrorMessage, @ErrorSeverity, @ErrorState);

    END CATCH;

    /*</DocGen_Tool>*/
    RETURN
    END

  • Grant Fritchey

    SSC Guru

    Points: 396384

    Query hints should absolutely be the exception, not the rule. So, I'm always inclined to remove them. However, using Chesterton's Fence as a model, we need to ask, why is it there in the first place? I suspect it's an attempt to deal with deadlocks. I'd pull it, but, let's talk about that.

    In testing, you'll pull it. Performance will either, stay the same, or increase some. Then, you'll hit production. Whatever inspired this to be put in place is due to a production level load, lots of transactions all at once. Deadlocks were occurring, probably because of classic coding issues, two different modification paths to the data. A rowlock ensure that no one could accidently take out locks, but now, acts as a blocker. So, you need to plan to identify and fix the core deadlock issue when you put this change in place.

    That's just a guess though.

    ----------------------------------------------------
    The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood...
    Theodore Roosevelt

    The Scary DBA
    Author of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd Edition
    Product Evangelist for Red Gate Software

  • VoldemarG

    Hall of Fame

    Points: 3615

    Thanks.

    But how exactly do you suggest it can be re-written to do that?

    Voldemar likes to play CHESS (and IS good at it!)

  • Grant Fritchey

    SSC Guru

    Points: 396384

    I can't say without knowing it's actually hitting deadlocks. Then, it's about the deadlocks encountered. Sorry, no way to know what that is from where I'm sitting.

    ----------------------------------------------------
    The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood...
    Theodore Roosevelt

    The Scary DBA
    Author of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd Edition
    Product Evangelist for Red Gate Software

  • VoldemarG

    Hall of Fame

    Points: 3615

    Sorry I should have mentioned it. yes, it occasionally hits deadlocks.

    Many times it is just blocking timeouts, a few times here and there there are deadlocks.

    Voldemar likes to play CHESS (and IS good at it!)

  • VoldemarG

    Hall of Fame

    Points: 3615

    when the proc is called 100 times a minute or more, that's when deadlocks start. at something like 50 hits a minute or less, no deadlocks, only occasional blocking chains, caused by same SP against same SP calls.

    index is working well.

     

    Voldemar likes to play CHESS (and IS good at it!)

  • Jeff Moden

    SSC Guru

    Points: 996076

    From what I can see, the current code tries an update.  If there's nothing to update, it does an insert and then returns to try the same update again.  That makes no sense to me and should probably be fixed.

    Also, instead of WITH(ROWLOCK), try WITH(UPDLOCK) instead.  I have no scientific explanation for why I make that suggestion.  I can only tell you that I saw it resolve a deadlock issue nearly a decade ago.

    --Jeff Moden


    RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
    First step towards the paradigm shift of writing Set Based code:
    ________Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column.
    "If you think its expensive to hire a professional to do the job, wait until you hire an amateur."--Red Adair
    "Change is inevitable... change for the better is not."

    Helpful Links:
    How to post code problems
    How to Post Performance Problems
    Create a Tally Function (fnTally)

  • Grant Fritchey

    SSC Guru

    Points: 396384

    And, deadlocks are fundamentally a performance tuning issue, so look at the exec plan to be sure there are no tuning opportunities. Then, determine what the deadlocks are exactly from and see if it's due to the classic deadly embrace or something else. Possibly rearranging the order of operations between two different queries could help with the deadlocks. Eliminating or mitigating the deadlocks w/o the UPDLOCK or ROWLOCK will reduce blocking and timeouts. However, it's all going to require a lot of documentation and testing to arrive at a happy place.

    ----------------------------------------------------
    The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood...
    Theodore Roosevelt

    The Scary DBA
    Author of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd Edition
    Product Evangelist for Red Gate Software

  • VoldemarG

    Hall of Fame

    Points: 3615

    Should I just replace  Update-->Insert-->Update (retry) with IF EXISTS   [update] ELSE [insert] ?

    Voldemar likes to play CHESS (and IS good at it!)

  • VoldemarG

    Hall of Fame

    Points: 3615

    Also, attached is the doc. that shows table structure, and a few typical calls to this SP (some of them yes, do happen within large(r) transactions), fired 100 times a minute, called from hundreds of other SPs.

    Attachments:
    You must be logged in to view attached files.

    Voldemar likes to play CHESS (and IS good at it!)

  • Jonathan AC Roberts

    SSCoach

    Points: 17212

    Could it be that the SP is called by long running processes (transactions) that are updating the same row (with the same [object_name]) and one of the transactions hasn't finished while another process is calling the SP to update the same object_name?

  • VoldemarG

    Hall of Fame

    Points: 3615

    yes, I believe it can be, since at certain high contention times this SP is called 100 times a minute.

    But once your statement is confirmed, what can possibly be done to remediate such timeouts/blocks/deadlocks ?

    This SP is the most called SP among 10000 SPs that we have in this database. It is in the core of each business transacion, and any db operatoin.

    Should removing (ROWLOCK) hint and changing Isolation Level from Read Committed inside this SP to  Read Uncommitted be tried? But in such case, wouldn't it greately violate the integrity of business operations and rules?

     

    Voldemar likes to play CHESS (and IS good at it!)

Viewing 15 posts - 1 through 15 (of 22 total)

You must be logged in to reply to this topic. Login to reply