May 16, 2017 at 7:14 am
Hello
I am writing a stored procedure which will be called from a website (.aspx, c#) to update the stock quantity based on the ProductID of the item. The stored procedure needs to perform various checks so that it can return a meaningful message to the user (e.g. instead of just "Cannot update stock", it will say something like "Cannot update stock because the product could not be found".
I have approached this by calling a series of nested IF/ELSE statements, but I am not sure if this is a good way to do this - especially as when I start to add more tests (there will be 7 in total) my SQL Server Management Studio starts to slow down and takes several seconds to register key presses. I wondered if I am nesting too many. Is there another way to approach this problem that is more 'best practice'?
CREATE PROCEDURE spu_UpdateStock
-- Add the parameters for the stored procedure here
@ProductID BIGINT,
@NewStockQuantity INT,
@Valid BIT OUTPUT,
@Message NVARCHAR(MAX) OUTPUT
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
-- Insert statements for procedure here
BEGIN TRANSACTION
BEGIN TRY
-- set default output params
SET @Valid = 0
SET @Message = 'An unknown error occurred when booking in the stock'
-- does productid exist
IF EXISTS (SELECT 1 FROM tlkp_Product WHERE fldProductID = @ProductID)
BEGIN
-- does the product exist at the location
IF EXISTS (SELECT 1 FROM tlkp_Location WHERE fldProductID = @ProductID)
BEGIN
-- does the new stock quantity exceed the max allowed quantity?
IF (@NewStockQuantity < 1000)
BEGIN
UPDATE tblLocation SET fldQuantity=@NewStockQuantity WHERE fldProductID=@ProductID
SET @Valid = 1
SET @Message = 'The stock was updated'
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the value entered is too high (Source: SQL).'
END
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the product does not have a location (Source: SQL).'
END
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the product could not be found (Source: SQL).'
END
COMMIT TRANSACTION
END TRY
BEGIN CATCH
ROLLBACK TRANSACTION
DECLARE @Msg NVARCHAR(MAX)
SELECT @Msg=ERROR_MESSAGE()
RAISERROR('Error Occured: %s', 20, 101,@msg) WITH LOG
END CATCH
END
GO
May 16, 2017 at 9:08 am
This can get quite messy very quickly, suggest you first do the checks and then if all is clear, do the insert at the end. This way you can display more than one error message if applicable.
😎
May 16, 2017 at 10:52 am
Also, it's not the IF statements that are likely causing it to slow down, it's all the additional queries validating values. EXISTS is a good way to short circuit the process, but if it's doing scans, it's still going to be slow. Check each individual SELECT statement to ensure they're performing well, using indexes appropriately, etc., and then plug them in to your checks.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
May 16, 2017 at 4:23 pm
I would also recommend getting into the habit of indenting your code so that you can quickly tell at a glance where the beginning and end of each nested statement is. The specifics of how you choose to do it are up to you, but whatever you decide to do, it will probably save you time and confusion down the line.
IF EXISTS (SELECT 1 FROM tlkp_Product WHERE fldProductID = @ProductID)
BEGIN
-- does the product exist at the location
IF EXISTS (SELECT 1 FROM tlkp_Location WHERE fldProductID = @ProductID)
BEGIN
-- does the new stock quantity exceed the max allowed quantity?
IF (@NewStockQuantity < 1000)
BEGIN
UPDATE tblLocation SET fldQuantity=@NewStockQuantity WHERE fldProductID=@ProductID
SET @Valid = 1
SET @Message = 'The stock was updated'
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the value entered is too high (Source: SQL).'
END
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the product does not have a location (Source: SQL).'
END
END
ELSE
BEGIN
SET @Message = 'Cannot book in stock because the product could not be found (Source: SQL).'
END
May 17, 2017 at 2:01 am
Eirikur Eiriksson - Tuesday, May 16, 2017 9:08 AMThis can get quite messy very quickly, suggest you first do the checks and then if all is clear, do the insert at the end. This way you can display more than one error message if applicable.
😎
Thanks Eirikur. I did consider doing the individual tests first, but I decided not to because I thought that would mean every single test would be run every time the stored procedure is called, whereas nesting the tests inside would mean that as soon as one of the tests fails, all the following tests/select statements, will not be run. Although I understand your point about reporting multiple failure reasons.
I can deal with some mess - I'll just comment my code well and indent as suggested by another poster.
May 17, 2017 at 2:04 am
Grant Fritchey - Tuesday, May 16, 2017 10:52 AMAlso, it's not the IF statements that are likely causing it to slow down, it's all the additional queries validating values. EXISTS is a good way to short circuit the process, but if it's doing scans, it's still going to be slow. Check each individual SELECT statement to ensure they're performing well, using indexes appropriately, etc., and then plug them in to your checks.
Thanks Grant. Am I understanding you right here - even though I am only writing the query at this point (not executing it), that SQL will check values in the background while I code? If so, that is very interesting to know.
The query itself runs at an acceptable speed (very fast, in fact). It's just that the user interface itself is what becomes very slow and unresponsive as I am coding the query (not running it).
May 17, 2017 at 2:06 am
autoexcrement - Tuesday, May 16, 2017 4:23 PMI would also recommend getting into the habit of indenting your code so that you can quickly tell at a glance where the beginning and end of each nested statement is. The specifics of how you choose to do it are up to you, but whatever you decide to do, it will probably save you time and confusion down the line.
Thanks SSC-Addicted. Not sure what happened there - I do normally indent the code, but something appears to have gone wrong with the formatting when I pasted it in to my post. Sorry if that made it horrible to read!
May 17, 2017 at 4:29 am
A couple of items to take note of
1 - Parameter @Message ... Why NVARCHAR - Your messages have no unicode characters. Why MAX - Your messages are short enough for MUCH smaller string length.
2 - It is a good idea to not open your transaction until you actually need it.
3 - If you change the order of your validation, you get a potential bonus of being able to kick out of the proc without hitting the DB at all.
4 - By using a RETURN when each test fails, you stop the proc from doing any further work.
5 - Test for the existence of a TRAN before committing or rolling back.
Something like this
CREATE PROCEDURE spu_UpdateStock
-- Add the parameters for the stored procedure here
@ProductID BIGINT
, @NewStockQuantity INT
, @Valid BIT OUTPUT
, @Message NVARCHAR(MAX) OUTPUT /*** NOTE: Why NVARCHAR?, Why MAX? ***/
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
-- Insert statements for procedure here
BEGIN TRY
-- set default output params
SET @Valid = 0;
SET @Message = 'An unknown error occurred when booking in the stock';
/**************************
Do this check first, as there is no need to hit the DB if it fails.
**************************/
-- does the new stock quantity exceed the max allowed quantity?
IF ( @NewStockQuantity >= 1000 )
BEGIN
SET @Message = 'Cannot book in stock because the value entered is too high (Source: SQL).';
RETURN; /*** This will cause the proc to stop processing at this point. ***/
END;
-- does productid exist
IF NOT EXISTS ( SELECT 1 FROM tlkp_Product WHERE fldProductID = @ProductID )
BEGIN
SET @Message = 'Cannot book in stock because the product could not be found (Source: SQL).';
RETURN; /*** This will cause the proc to stop processing at this point. ***/
END;
-- does the product exist at the location
IF NOT EXISTS ( SELECT 1 FROM tlkp_Location WHERE fldProductID = @ProductID )
BEGIN
SET @Message = 'Cannot book in stock because the product does not have a location (Source: SQL).';
RETURN; /*** This will cause the proc to stop processing at this point. ***/
END;
/**************************
1 - Begin your transaction at the last possible moment. That said, in this case,
there is only a single update, so there is no need for an explicit transaction.
**************************/
BEGIN TRANSACTION;
/**************************
1 - The fact that we are here, means that all of the validation checks
have passed and we can go ahead and perform the required actions.
2 - You can set your OUTPUT variables at the same time as the UPDATE.
**************************/
UPDATE tblLocation
SET fldQuantity = @NewStockQuantity
, @Valid = 1
, @Message = 'The stock was updated'
WHERE fldProductID = @ProductID;
IF ( @@TRANCOUNT > 0 ) COMMIT TRANSACTION;
END TRY
BEGIN CATCH
/**************************
You need to test whether there is a valid transaction to roll back before doing the actual rollback.
**************************/
IF ( @@TRANCOUNT > 0 ) ROLLBACK TRANSACTION;
DECLARE @Msg NVARCHAR(MAX);
SELECT @Msg = ERROR_MESSAGE();
RAISERROR('Error Occured: %s', 20, 101,@Msg) WITH LOG;
END CATCH;
END;
GO
May 17, 2017 at 4:49 am
DesNorton - Wednesday, May 17, 2017 4:29 AMA couple of items to take note of
1 - Parameter @Message ... Why NVARCHAR - Your messages have no unicode characters. Why MAX - Your messages are short enough for MUCH smaller string length.
2 - It is a good idea to not open your transaction until you actually need it.
3 - If you change the order of your validation, you get a potential bonus of being able to kick out of the proc without hitting the DB at all.
4 - By using a RETURN when each test fails, you stop the proc from doing any further work.
5 - Test for the existence of a TRAN before committing or rolling back.
DesNorton - thank you very much, very constructive feedback and suggestions! I appreciate you taking the time to note all those points I guess I have picked up bad habits and not grasped certain concepts on my TSQL journey...
To answer your point (1) - The reason I used NVARCHAR(MAX), is because although I am capturing simple/short errors, if there are any other reason for a failure (such as a deadlock on table records) I was just being cautious when capturing the message returned by SQL as I didn't know how long it might be - I have seen some before that are quite complex and several thousand characters long). I started to wrap all my stored procedures in transactions because it allowed me to capture silly things such as permissions not being granted on the stored procedure, or deadlocks on tables (I haven't got in to the workings of locks/hinting).
May 17, 2017 at 5:53 am
r.gall - Wednesday, May 17, 2017 2:04 AMThanks Grant. Am I understanding you right here - even though I am only writing the query at this point (not executing it), that SQL will check values in the background while I code? If so, that is very interesting to know.
The query itself runs at an acceptable speed (very fast, in fact). It's just that the user interface itself is what becomes very slow and unresponsive as I am coding the query (not running it).
OK. I misunderstood. You're saying that the "IF" statements are causing the GUI to run slower and slower and you're not executing the query?
That is seriously messed up. It sounds like a bug. Do you have third party tools installed in SSMS? It's not normal behavior.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
May 17, 2017 at 6:53 am
Grant Fritchey - Wednesday, May 17, 2017 5:53 AMr.gall - Wednesday, May 17, 2017 2:04 AMThanks Grant. Am I understanding you right here - even though I am only writing the query at this point (not executing it), that SQL will check values in the background while I code? If so, that is very interesting to know.
The query itself runs at an acceptable speed (very fast, in fact). It's just that the user interface itself is what becomes very slow and unresponsive as I am coding the query (not running it).
OK. I misunderstood. You're saying that the "IF" statements are causing the GUI to run slower and slower and you're not executing the query?
That is seriously messed up. It sounds like a bug. Do you have third party tools installed in SSMS? It's not normal behavior.
Not to worry Grant! In answer to your question it is just the default install of SSMS 2014 in Windows 7 64 bit. No third party tools. I did wonder if the intelli-sence was causing the problem. But there was definitely something weird going on because I have encountered it several times when writing stored procedures that contain several levels of nested IF statements. My colleague has the same problem to, its very weird. I can replicate the problem too, because when SSMS slows down, I cut out my most recent nested IF statement I was working on, and SSMS starts working normally again, but I paste it back in and it slows down again. But anyway I'm going off topic. Thanks for your help.
May 17, 2017 at 7:54 am
Have you patched your Management Studio. I'm using the 2017 version now, but I just wrote an IF script nested 10 layers deep and it's not affecting anything and I am running third party tools (I love SQL Prompt). You're not looking at normal behavior.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
Viewing 12 posts - 1 through 11 (of 11 total)
You must be logged in to reply to this topic. Login to reply