Need a way to speed up this SP. Causing occasional deadlocks Help
-
The situations is this, a table is pre-populated with special numbers (not always sequential). A salespoint will will request a number from this table. When it does, the ckd_out_to column is changed from the default -1 to the salespoint's number. Only one salespoint can have this number.
Under extreme load, deadlocks begin to happen, and the call itself takes too long. Below is the table structure and the SP we are using (The reason num_assign is char is complicated but we are stuck with it)
We are open to Any ideas however radical!
Many Thanks in advance
Tim
CREATE TABLE [assignno] (
[num_assign] [char] (17) COLLATE SQL_Latin1_General_CP1_CI_AS NOT NULL ,
[ckd_out_to] [int] NOT NULL ,
[assign_gst] [numeric](17, 0) NOT NULL ,
[assign_pas] [numeric](17, 0) NOT NULL ,
[num_source] [int] NOT NULL ,
CONSTRAINT [assignno_num_assign] PRIMARY KEY CLUSTERED
(
[num_assign]
 
ON [PRIMARY] ,CONSTRAINT [CK_assignno_notempty] CHECK (len([num_assign]) > 0)
) ON [PRIMARY]
GO
/*
** Retrieve a new assign_no from the assignno table
*/
CREATE PROCEDURE [siriussp_get_assignno]
@tnSalesPoint int,
@tnNum_Source int,
@NewAssign char(17) output
AS
set nocount on
if @tnNum_Source is NULL
begin
begin transaction
set @NewAssign=(select cast(min(cast(num_assign as numeric(17,0))) as char(17)) from assignno (serializable) where ckd_out_to=-1)
update assignno set ckd_out_to=@tnSalesPoint where num_assign=@NewAssign
commit transaction
end
else
begin
begin transaction
set @NewAssign=(select cast(min(cast(num_assign as numeric(17,0))) as char(17)) from assignno (serializable) where ckd_out_to=-1 and num_source = @tnNum_Source)
update assignno set ckd_out_to=@tnSalesPoint where num_assign=@NewAssign
commit transaction
end
GO
-
Could you please specify what other indexes are on this table?
-
There is an index on ckd_out_to. Funny that that didn't come over from QA's Script Object to Clipboard command ...
-
I see from your script that you are using 'num_assign' assign field for both min() and where in update. Could you possibly create an unique index on that column? Depending on how often the data is inserted into this table - you might make 'num_assign' your clustered index field. Hope this helps
-
The problem is that your selects are using different access paths than the update uses and this will cause deadlocks in a system with significant usage. If you do as listed below you should minimize the number of deadlocks to only those that may occur due to two simultaneous calls using different paths. If most of the calls to the proc have @tnNum_Source as NULL or as real value then there should be very few deadlocks. There are other options as well to eliminate deadlocks but they would involve table structure changes. One final thing I might put forward is that if the num_source column contains a sequential series of values and the range is known, you could set a null @tnNum_Source to a random value in that range and then only have the single access path for updates.
Try the following:
1. Make the PK nonclustered
2. Drop the index on ckd_out_to
3. create idx1 on (ckd_out_to asc, num_assign asc)
4. create idx2 on (ckd_out_to asc,num_source asc, num_assign asc)
5. Rewrite the proc as follows:
CREATE PROCEDURE siriussp_get_assignno
@tnSalesPoint int,
@tnNum_Source int,
@NewAssign char(17) output
AS
SET NOCOUNT ON
IF @tnNum_Source IS NULL
BEGIN
BEGIN TRANSACTION
UPDATE assignno with (rowlock)
SET ckd_out_to = @tnSalesPoint
, @NewAssign = num_assign
WHERE ckd_out_to = -1
AND num_assign = (SELECT TOP 1 num_assign
FROM assignno WITH (ROWLOCK, SERIALIZABLE, INDEX(idx1))
WHERE ckd_out_to = -1
ORDER BY ckd_out_to, num_assign)
COMMIT TRANSACTION
END
ELSE
BEGIN
BEGIN TRANSACTION
UPDATE assignno WITH (ROWLOCK)
SET ckd_out_to = @tnSalesPoint
, @NewAssign = num_assign
WHERE ckd_out_to = -1
AND num_source = @tnNum_Source
AND num_assign = (SELECT TOP 1 num_assign
FROM assignno WITH (ROWLOCK, SERIALIZABLE, INDEX(idx2))
WHERE ckd_out_to = -1
AND num_source = @tnNum_Source
ORDER BY ckd_out_to, num_source, num_assign)
COMMIT TRANSACTION
END
GO
Ian Dundas
Senior IT Analyst - Database
Manitoba Public Insurance Corp. -
Would the following be a simpler/faster way for the 1st update. The style of solution could be repeated for the 2nd update also.
Basically you only want 1 row to be updated per transaction. You don't seem to care/mind/require that it is the lowest sequence number available...so therefore why sort to find the minimum?
set rowcount 1
UPDATE a with (rowlock)
SET a.ckd_out_to = @tnSalesPoint, @NewAssign = num_assign
from assignno a
WHERE a.ckd_out_to = -1
COMMIT TRANSACTION
HTH
-
Also keep in mind his variable for Num_Source which can be null run one query, not null do the other.
set rowcount 1
UPDATE a with (rowlock)
SET a.ckd_out_to = @tnSalesPoint, @NewAssign = num_assign
from assignno a
WHERE a.ckd_out_to = -1 AND
a.num_source = IsNull(@tnNum_Source,a.num_source)
COMMIT TRANSACTION
That is as long as the minimum doesn't matter.
-
What about this?
1. Make the PK nonclustered
2. Drop the index on ckd_out_to
3. create idx1 on (ckd_out_to asc,num_source asc, num_assign asc)
UPDATE a with (rowlock)
SET a.ckd_out_to = @tnSalesPoint, @NewAssign = num_assign
from assignno a
WHERE a.ckd_out_to = -1 AND
a.num_source = IsNull(@tnNum_Source,a.num_source)
AND num_assign = (SELECT TOP 1 num_assign
FROM assignno WITH (READPAST, INDEX(idx1))
WHERE ckd_out_to = -1
AND num_source = @tnNum_Source
ORDER BY ckd_out_to, num_source, num_assign)
That way it skips past locked records?
-
We have decided to go with below. The real change from your suggestions is the repeatableread and readpast hints in the subquery. Incredible speed improvement!
We are about to implement this in a high load site on a server with multi-processors.
Does any one see any danger flags with this approach?
Thank you all for your help!
** Retrieve a new assign_no from the assignno table */ CREATE PROCEDURE [siriussp_get_assignno] @tnSalesPoint int, @tnNum_Source int, @NewAssign char(17) output WITH RECOMPILE AS set nocount on declare @nfound int , @cAssign char(17) set @nfound = 0 set @cAssign = null set @NewAssign = null if @tnNum_Source is NULL begin begin transaction UPDATE assignno with (rowlock, holdlock) SET ckd_out_to = @tnSalesPoint , @NewAssign = num_assign WHERE ckd_out_to = -1 AND num_assign = (SELECT TOP 1 num_assign FROM assignno WITH (repeatableread readpast) WHERE ckd_out_to = -1 ORDER BY num_assign) commit transaction end else begin begin transaction UPDATE assignno WITH (rowlock, holdlock) SET ckd_out_to = @tnSalesPoint , @NewAssign = num_assign WHERE ckd_out_to = -1 AND num_source = @tnNum_Source AND num_assign = (SELECT TOP 1 num_assign FROM assignno WITH (repeatableread readpast) WHERE ckd_out_to = -1 AND num_source = @tnNum_Source ORDER BY num_assign) commit transaction end GO
-
My only concern would be the use of the WITH RECOMPILE. If this is really necessary I would suggest putting each of the the UPDATE statements into separate procedures and then have the siriussp_get_assignno procedure call the appropriate proc depending upon whether @tnNum_Source is NULL or not. Forcing a recompile on every execution means that its plan is not cached and this will increase the CPU quite a bit under a high load. Splitting the proc will introduce a bit more over head but it should be less than that introduces by recompiling the proc every time it is executed. One thing I am not sure of is if the proc will also be locked during each recompile forcing serialization on its usage. It is my understanding that this will happen when a proc without the RECOMPILE clause is recompiled by SQL Server for various reasons. If this is the same when the WITH RECOMPILE is used, you could be hampering throughput by forcing lock waits on the proc.
What index chnages if any did you decide on?
Ian Dundas
Senior IT Analyst - Database
Manitoba Public Insurance Corp. -
Thank you for your help Ian.
How about this?
** Retrieve a new assign_no from the assignno table */ CREATE PROCEDURE [siriussp_get_assignno] @tnSalesPoint int, @tnNum_Source int, @NewAssign char(17) output WITH RECOMPILE AS set nocount on declare @nfound int , @cAssign char(17) set @nfound = 0 set @cAssign = null set @NewAssign = null begin transaction UPDATE assignno WITH (rowlock, holdlock) SET ckd_out_to = @tnSalesPoint , @NewAssign = num_assign WHERE ckd_out_to = -1 AND num_source = @tnNum_Source AND num_assign = (SELECT TOP 1 num_assign FROM assignno WITH (repeatableread readpast) WHERE ckd_out_to = -1 AND num_source = IsNull(@tnNum_Source,a.num_source) ORDER BY num_assign) commit transaction GO
So far, no index changes were decided.
Thanks,
Tim
-
Sorry, without the Recompile of course.
-
Your original requirement stated "A salespoint will request a number from this table."....However you seem to be implementing this as "A salespoint will request THE LOWEST AVAILABLE number from this table."
Is this necessary, because it is forcing you down the road of putting in a "sort" in your subquery (ie the ORDER BY)...which is a performance hit?
You say, that a key boost to performance was the introduction of the repeatable read, readpast hints, etc.....would putting that extra structure on the suggestion I made be of benefit, or is it no better/worse than what you have?....I'm interested as a self-education exercise to know how close/far off the mark I am/was on this.
set rowcount 1
UPDATE a with (rowlock)
SET a.ckd_out_to = @tnSalesPoint, @NewAssign = num_assign
from assignno a WITH (READPAST, INDEX(idx1))
WHERE a.ckd_out_to = -1
-
Hi Andrew,
I was hoping that the "lowest available number" was a req we could drop but alas, we needed to keep it. It turns out that gaps in this "check out" table were not acceptable.
So with that constraint, we had to do an ordered subquery. We are also trying this without any index changes. What we arrived at was a combination of Ian's Update select and Antares686's "a.num_source = IsNull(@tnNum_Source,a.num_source)" trick to get rid of the two seperate paths.
Then we opted for the readpast hint to simply skip over records that are currently locked.
If we didn't have the min num constraint, we would have gone with your approach. Simple and elegant.
My current fear is if there is any unforseeable downside to using readpast in this scenario. As mentioned before, this is a heavy use scenario running on multiprocessors where the chances of real concurancy is high. I do not have experience with this kind of environment and feel very much like a hack at this point.
If anyone has any insight to contribute, it would greatly be appreciated.
Thanks in advance!
Tim
-
Thanks for the reply Tim.
Only issue I can see is if the 'readpast' ends up skipping 'all records' with nothing getting updated. You may wish to put in a check to see that at least (only) 1 record got updated....and that if this unusual situation arises, that it is reported back to your application...for a possible 'try again please' scenario....(once you hit this issue, the skip-deadlocks might escalate exponentially...just as regular dead-locks cascade upwards in effect...and reporting it back to your application might give you a handle on it's effects on performance)
Viewing 15 posts - 1 through 14 (of 14 total)
You must be logged in to reply to this topic. Login to reply