January 20, 2020 at 4:24 am
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.
January 20, 2020 at 9:35 am
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.
January 20, 2020 at 3:13 pm
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
Aus dem Paradies, das Cantor uns geschaffen, soll uns niemand vertreiben können
January 20, 2020 at 6:02 pm
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;
January 20, 2020 at 7:42 pm
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).
January 20, 2020 at 8:04 pm
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
January 20, 2020 at 8:13 pm
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 21 (of 21 total)
You must be logged in to reply to this topic. Login to reply