June 8, 2020 at 5:06 pm
How to make the below code faster, does it help by changing updlock to nolock help, or please share if you see anything which can be improved syntax wise, etc?
-------------------------------------------------------------------------------------------------
CREATE PROCEDURE [dbo].spTest(@DateToProcess DateTime, @ForceTotalRecalc bit = 0) AS
SET XACT_ABORT ON
SET NOCOUNT ON
DECLARE EffectiveDate DATE
DECLARE @LastDateProcessed DATE
DECLARE @ThisDate DATE
DECLARE @EventLogID INT
DECLARE @status INT
BEGIN TRAN
DECLARE @BalAcc_Identification INT
SELECT TOP 1 @BalAcc_Identification = BalAcc_Identification , @EventLogID =Event_ID
FROM dbo.[tbTEST_Bal] WITH(updlock)
WHERE [Started] IS NULL
UPDATE dbo.[tbTEST_Bal] WITH(updlock)
SET [Started] = 1
WHERE BalAcc_Identification = @BalAcc_Identification
AND Event_ID =@EventLogID
COMMIT
WHILE @BalAcc_Identification is not null
BEGIN
SET EffectiveDate = NULL
SELECT EffectiveDate = dbo.fnGetDateOnly(AdvanceEffectiveDate),
@status=[AdvanceStatus_ID]
FROM dbo.vwIndividualHSAAdvanceInfo_LatestVersion
WHERE BalAcc_Identification = @BalAcc_Identification AND Event_ID =@EventLogID
SET @LastDateProcessed = NULL
IF @ForceTotalRecalc = 0
BEGIN
SELECT TOP 1 @LastDateProcessed = dbo.fnGetDateOnly(AsOfDate)
FROM dbo.tbHSAAdvanceBalanceTrans
WHERE Trans_BalAcc_Identification = @BalAcc_Identification
AND Event_ID=@EventLogID
ORDER BY AsOfDate DESC
END
ELSE
BEGIN
SET @LastDateProcessed = NULL
END
SET @ThisDate = NULL
SET @ThisDate = ISNULL(DATEADD(DAY,1,@LastDateProcessed), EffectiveDate)
WHILE @ThisDate <= @DateToProcess
BEGIN
BEGIN TRY
EXEC [dbo].[spTrans_InsertHSAAdvanceDailyBalance] @BalAcc_Identification=@BalAcc_Identification, @EventLogID=@EventLogID ,@Date=@ThisDate ,@Status=@Status
SET @ThisDate = DATEADD(DAY,1,@ThisDate)
END TRY
BEGIN CATCH
DECLARE @EODError_Message NVARCHAR(4000) = ERROR_MESSAGE()
INSERT INTO dbo.tbEOD_ErrorLog (EODError_Step, EODError_AccountID, EODError_Message, EODERROR_Date)
VALUES ('Account_Calc', @BalAcc_Identification, @EODError_Message, GETDATE())
END CATCH
END
UPDATE dbo.[tbTEST_Bal]
SET Completed = GETDATE()
WHERE BalAcc_Identification = @BalAcc_Identification
AND Event_ID =@EventLogID
SELECT @BalAcc_Identification = NULL
SELECT @EventLogID =NULL
BEGIN TRAN
SELECT TOP 1 @BalAcc_Identification = BalAcc_Identification , @EventLogID =Event_ID
FROM dbo.[tbTEST_Bal] WITH(updlock)
WHERE [Started] IS NULL
If (@BalAcc_Identification is not null AND @EventLogID is not null)
BEGIN
UPDATE dbo.[tbTEST_Bal] WITH(updlock)
SET [Started] = 1
WHERE BalAcc_Identification = @BalAcc_Identification
AND Event_ID =@EventLogID
END
COMMIT
END
June 8, 2020 at 6:15 pm
That is a long stored procedure, but I will offer some input.
First, my advice is unless you NEED a query hint, don't use them. The SQL optimizer generally will take out the locks that it needs and your hints may actually slow things down. This isn't always the case (sometimes hints are helpful to the engine), but generally I try not to use them.
Second, changing it to nolock won't help. SQL will ignore it on the UPDATES because you can't do that (you need a lock on the table to do the update). The only case where it would make a difference is on your SELECT, but you likely don't need an update lock on a SELECT statement. And there are risks by using NOLOCK, so my advice is to avoid it whenever possible.
Next, potential bugs. Your first SELECT is a TOP 1 without an ORDER BY. That is bad practice and may result in incorrect results. You should ALWAYS include an ORDER BY with a TOP statement UNLESS you don't care what value you get as long as it matches the requirements in the WHERE.
As for performance tweaks, could you post the execution plan for that query? We cannot see your data or your table structure (indexes, keys, rows), so trying to give advice on what to improve on is impossible. We may see something that is incredibly slow, but you are working with 2 rows of data so the improvements we suggest are pointless.
If you want guesses based entirely on the query, I am thinking it isn't going to work as you wrote it, but that is a simple typo - EffectiveDate should be @EffectiveDate, right?
Secondly, that WHILE loop is going to be a performance bottleneck as you are working in row-based operations rather than set based operations. But since you are calling another stored procedure inside that loop, I am thinking row based may be what you need to do? Although, depending on what that other stored procedure is doing, you may be able to rewrite that as well to get better performance.
The above is all just my opinion on what you should do.
As with all advice you find on a random internet forum - you shouldn't blindly follow it. Always test on a test server to see if there is negative side effects before making changes to live!
I recommend you NEVER run "random code" you found online on any system you care about UNLESS you understand and can verify the code OR you don't care if the code trashes your system.
June 8, 2020 at 6:16 pm
Can you paste the execution plan?
June 8, 2020 at 6:50 pm
June 9, 2020 at 3:37 pm
Quick look at your execution plan, it looks like your slow operations are in your loop. The insert is the highest cost operation. That being said, you should get a performance boost by creating an index on the table tbTEST_Bal on the Started column. How much I am not certain as it depends on how many rows are in that table. The way it is now, the worst case scenario is there are no NULL values for Started in that table and in that case, you would need to look at every row in the table. Now, if that table has 10 rows, and your insert is into a table with 100 million rows and 100 indexes, the insert is going to be a much better beast to tackle.
But going off the execution plan, there isn't a "heavy hitter" portion of the query, everything seems to run at approximately the same cost. The worst offender for the cost % are the INSERTS, worst offender ofr compile time is a toss up between your SELECT TOP and one of the UPDATES. And your worst offenders for CPU are your SELECT TOP's, so my guess is that you should work on optimizing those as your first step, and the index I proposed should help.
If you are unable to add indexes and are only able to work with the query, then things get more tricky. In that case, you need to work on those loops.
Looking at the execution plan, that stored procedure in the middle is doing a SELECT and an INSERT, no calculations. I imagine that can be pulled out of that stored proc and put into this one and you can likely get rid of your loops completely then. But if you cannot change that stored procedure, then your next best bet is going to be reducing the number of loops. If you can get it down to 1 loop, you should get better performance.
Just so I understand what this does, first you are finding a case where started is NULL in the table tbTest_Bal. Next, you changed Started from NULL to 1. Then you go into a loop (lets call this loop1). In loop1, you are getting the @EffectiveDate and @status vales from dbo.vwIndividualHSAAdvanceInfo_LatestVersion. Next if @ForceTotalRecalc = 0, you get the @LastDateProcessed from tbHSAAdvanceBalanceTrans. Otherwise @LastDateProcessed is NULL. Next, you set @ThisDate to @LastDateProcessed plus 1 day OR EffeciveDate if @LastDateProcessed is NULL. Then a second loop (lets call this loop2) happens. We will skip over that loop because all you are doing is calling a second stored procedure. If you are willing to re-write that stored procedure too, then both loops may be possible to remove. Next you update tbTEST_Bal to set the Completed value to the current datetime for this one item, then you grab thenext value from tbTest_Bal where started is NULL and change Started from NULL to 1 and repeat loop1.
To summarize the above - you get two identifiers from tbTest_Bal, mark that specific item as started, go into a loop to get all associated data for that item and then go into a second loop to actually update the data. It looks like the tables tbTEST_Bal, vwIndividualHSAAdvanceInfo_LatestVersion, tbHSAAdvanceBalanceTrans (the 3 tables used to get the data for the stored procedure) are all related. So, since the tables are related, if you can bring those 3 queries down to a single query with the results required for the loops as well, you can toss that into a cursor (still a loop unfortunately, but you can probably get by with a single loop instead of 2 loops) and you can call the stored procedure from that one. I think the only tricky bit here is going to be the date parts, but that can be done in the cursor as well. A quick little query to get a range of dates (that can be modified to be used with your query) is something like this:
DECLARE @start DATETIME = DATEADD( DAY
, -10
, GETDATE()
);
DECLARE @end DATETIME = GETDATE();
WITH [cte1]
AS
(
SELECT
@start AS [Start]
UNION ALL
SELECT
DATEADD( DAY
, 1
, [cte1].[Start]
)
FROM[cte1]
WHERE[cte1].[Start] < @end
)
SELECT
[cte1].[Start]
FROM[cte1];
Does the above help and make sense? If not, I can give you a better idea of what I am thinking and provide full code where you only have a single loop and a few common table expressions (cte... hence my cte1 in the above).
To summarize, I see 2 options:
1 - remove the WHILE loops, replace them with a single Cursor (still a loop unfortunately, but we are down to 1 loop and you can add options to the cursor that you know to be valid like LOCAL and FAST_FORWARD) and build an index.
2 - modify the code to remove the stored procedure call and do it all in a single stored procedure potentially without any loops.
I say "Potentially" as I don't know for certain what that second stored procedure does.
The above is all just my opinion on what you should do.
As with all advice you find on a random internet forum - you shouldn't blindly follow it. Always test on a test server to see if there is negative side effects before making changes to live!
I recommend you NEVER run "random code" you found online on any system you care about UNLESS you understand and can verify the code OR you don't care if the code trashes your system.
Viewing 5 posts - 1 through 4 (of 4 total)
You must be logged in to reply to this topic. Login to reply