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

  • DesNorton

    SSC-Insane

    Points: 22933

    NOLOCK and READUNCOMMITTED only affect reads.  This proc is doing inserts and updates, so it MAY have an impact if all other procs use NOLOCK/READ UNCOMMITTED.

     

    My gut says that changing the insert the update with a GOTO into a single insert will give you the most bang for your buck.

  • Jonathan AC Roberts

    SSCoach

    Points: 17161

    You need to speed up the statements that run after this stored procedure is called that are contained in the same transaction. Or, if you can, rearrange the calls to this stored procedure so it is called just before the transaction is committed instead of in the middle or beginning of the processing.

  • scdecade

    SSChasing Mays

    Points: 654

    The whole retry loop could be removed in my opinion.  This is a basic upsert.  The variables used for flow of control are deterministic so the retry is not really necessary.  In general the network is where unexplained nonsense happens.  If a retry policy (of the entire procedure) is an operational requirement then it ought to be handled outside the proc imo.    For .NET a really, really great way to handle retry, circuit breakers, etc. is using Polly.

    Is there a unique constraint on object_name in the GID_Values table?  Is it possible to add other constraints to this table?  Because the error checking being done in this proc could (some would argue 'should') happen in DDL.  The NULL error checks on input variables are unnecessary because neither of the variables (@chrObjectName and/or @QtyNeeded) has been assigned a NULL default.  The proc would throw an exception if a null value were passed to either variable.  If custom error messages are required it could be handled in DDL too (I'm pretty sure (I'd have to review it)).

    The input variable types are not consistent with the table described in the 2nd docx document.  Object_name is listed as char(30) also gid_value and gid_values_sid are listed as integer.  The proc accepts varchar(30) and decimal(12,0)'s.  Why?  Implicit type conversions are risky imo.  There was just a thread on SSC, not sure where, about how different decimal types don't equal each other.

    Here's a rough attempt at new code.  I just carried over your error handling but it could be looked at too (I think you want to rethrow before the rollback).  Because you're running in production at volume and because there are lots of unknowns this is meant only as suggestions for further investigation.

    ALTER PROCEDURE [dbo].[prGetGid]  
    @chrObjectName char(30),
    @relGidValue int OUTPUT,
    @QtyNeeded int
    AS
    set nocount on
    begin try
    declare
    @l_today datetime=getdate(),
    @rowcount int=0;

    if len(rtrim(@chrObjectName))=0
    throw 16, 'Object Name is blank. Contact product support. ', 1;

    IF @QtyNeeded < 1
    throw 16, 'Quantity needed is less than 1. Contact product support. ', 1;

    /* first try to insert */
    insert GID_Values ([object_name], gid_value, last_touch)
    values (rtrim(@chrObjectName), 0, @l_today);
    select @rowcount = @@rowcount;

    if @rowcount=0
    begin
    declare
    @updcount int=0;

    /* second, if 0 rows were inserted then update */
    update
    GID_Values /*WITH(ROWLOCK)*/
    set
    gid_value = gid_value + @QtyNeeded,
    last_touch = @l_today,
    @relGidValue = gid_value + 1
    where
    [object_name] = rtrim(@chrObjectName);
    select @updcount = @@rowcount;

    if @updcount=0
    throw 16, 'Error Creating a Unique Identifier. Contact product support. ', 1;
    end
    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;
    end
    go

    • This reply was modified 1 month ago by  scdecade.

    Aus dem Paradies, das Cantor uns geschaffen, soll uns niemand vertreiben können

  • frederico_fonseca

    SSChampion

    Points: 14391

    scdecade wrote:

    The whole retry loop could be removed in my opinion.  This is a basic upsert.  The variables used for flow of control are deterministic so the retry is not really necessary.  In general the network is where unexplained nonsense happens.  If a retry policy (of the entire procedure) is an operational requirement then it ought to be handled outside the proc imo.    For .NET a really, really great way to handle retry, circuit breakers, etc. is using Polly.

    Is there a unique constraint on object_name in the GID_Values table?  Is it possible to add other constraints to this table?  Because the error checking being done in this proc could (some would argue 'should') happen in DDL.  The NULL error checks on input variables are unnecessary because neither of the variables (@chrObjectName and/or @QtyNeeded) has been assigned a NULL default.  The proc would throw an exception if a null value were passed to either variable.  If custom error messages are required it could be handled in DDL too (I'm pretty sure (I'd have to review it)).

    The input variable types are not consistent with the table described in the 2nd docx document.  Object_name is listed as char(30) also gid_value and gid_values_sid are listed as integer.  The proc accepts varchar(30) and decimal(12,0)'s.  Why?  Implicit type conversions are risky imo.  There was just a thread on SSC, not sure where, about how different decimal types don't equal each other.

    Here's a rough attempt at new code.  I just carried over your error handling but it could be looked at too (I think you want to rethrow before the rollback).  Because you're running in production at volume and because there are lots of unknowns this is meant only as suggestions for further investigation.

    ALTER PROCEDURE [dbo].[prGetGid]  
    @chrObjectName char(30),
    @relGidValue int OUTPUT,
    @QtyNeeded int
    AS
    set nocount on
    begin try
    declare
    @l_today datetime=getdate(),
    @rowcount int=0;

    if len(rtrim(@chrObjectName))=0
    throw 16, 'Object Name is blank. Contact product support. ', 1;

    IF @QtyNeeded < 1
    throw 16, 'Quantity needed is less than 1. Contact product support. ', 1;

    /* first try to insert */
    insert GID_Values ([object_name], gid_value, last_touch)
    values (rtrim(@chrObjectName), 0, @l_today);
    select @rowcount = @@rowcount;

    if @rowcount=0
    begin
    declare
    @updcount int=0;

    /* second, if 0 rows were inserted then update */
    update
    GID_Values /*WITH(ROWLOCK)*/
    set
    gid_value = gid_value + @QtyNeeded,
    last_touch = @l_today,
    @relGidValue = gid_value + 1
    where
    [object_name] = rtrim(@chrObjectName);
    select @updcount = @@rowcount;

    if @updcount=0
    throw 16, 'Error Creating a Unique Identifier. Contact product support. ', 1;
    end
    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;
    end
    go

    Apart from the logical error the code has (not doing what the original code does) I would not blindly change this to be a insert followed by an update - we should consider what is the most common operation.

    Doing an insert on a table that has a unique key as this one (object_name) would cause it to fail  if we also don't add a "not exists" to that insert.

    small change on the original code below - removed the retry and set the output variable to 1 if it is an insert  and newly inserted record is created with the "requested quantity" - this as per original code

    /* 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;

    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 that means the record does not exist so insert a new one
    -- in this case set the gid_value to the requested Qty when creating the new record
    -- and we set the return variable to 1
    IF @rowcount = 0
    BEGIN
    BEGIN TRY
    INSERT INTO GID_Values ([object_name], gid_value, last_touch)
    VALUES (RTRIM(@chrObjectName), @QtyNeeded, @l_today);

    -- if it is a new record then we start the count at 1
    SET @relGidValue = 1;
    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>*/

    -- if at this point the rowcount is still zero that means that both the update and the insert failed
    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;

     

  • x

    SSC-Insane

    Points: 23536

    I have a question about the code being posted:

    So you run an update, and if no rows were updated (reading @@rowcount), then you do an insert. What happens if between the attempted update and the attempted insert, another concurrent process successfully inserts because it was in progress slightly earlier in the timeframe?

    This looks like a classic race, obviously it would work if it were inside a transaction with a suitable isolation level, I'm thinking at least repeatable read but nothing less than that, but nobody seems to be touching on this (which is actually pretty common, very few people seem to deal with this in my opinion).

     

     

     

  • frederico_fonseca

    SSChampion

    Points: 14391

    x wrote:

    I have a question about the code being posted:

    So you run an update, and if no rows were updated (reading @@rowcount), then you do an insert. What happens if between the attempted update and the attempted insert, another concurrent process successfully inserts because it was in progress slightly earlier in the timeframe?

    This looks like a classic race, obviously it would work if it were inside a transaction with a suitable isolation level, I'm thinking at least repeatable read but nothing less than that, but nobody seems to be touching on this (which is actually pretty common, very few people seem to deal with this in my opinion).

    yes that can happen - and on that case the transaction will fail and will be rolled back as the caller does have a explicit begin transaction/commit/rollback alongside try/catch blocks

  • x

    SSC-Insane

    Points: 23536

    frederico_fonseca wrote:

    x wrote:

    I have a question about the code being posted:

    So you run an update, and if no rows were updated (reading @@rowcount), then you do an insert. What happens if between the attempted update and the attempted insert, another concurrent process successfully inserts because it was in progress slightly earlier in the timeframe?

    This looks like a classic race, obviously it would work if it were inside a transaction with a suitable isolation level, I'm thinking at least repeatable read but nothing less than that, but nobody seems to be touching on this (which is actually pretty common, very few people seem to deal with this in my opinion).

    yes that can happen - and on that case the transaction will fail and will be rolled back as the caller does have a explicit begin transaction/commit/rollback alongside try/catch blocks

    gotcha! I did look for a "begin tran" but I didn't check the word doc (I don't like downloading office docs). Thanks!

     

Viewing 7 posts - 16 through 22 (of 22 total)

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