Please start any new threads on our new
site at https://forums.sqlteam.com. We've got lots of great SQL Server
experts to answer whatever question you can come up with.
Author |
Topic |
Cre8tivedaze
Starting Member
8 Posts |
Posted - 2013-08-21 : 11:17:59
|
Hi all,I've been using SQL on and off for probably about 10 years but nothing as advanced as the likes of some of you. I would probably rate myself somewhere between a beginner and Intermediate user but a recent project has seen me needing to really refine my sql skills. With that said, I've got this stored proc below that I put together using this tutorial (of sorts) [url]http://technet.microsoft.com/en-us/library/aa175920%28v=sql.80%29.aspx[/url] mainly for it's error handling and proper code writing practices. My question really is simple - it this statement massively over engineered or is pretty close to how someone who lives and breathes SQL everyday would consider a well structured SP. I'd appreciate any and all feedback.CREATE PROCEDURE [dbo].[CreateJob] @TransactionCountOnEntry INT, @ErrorCode INT, @SPreturnVAL VARCHAR (20) OUTPUT, @userID UNIQUEIDENTIFIER, @creationDate DATETIME, @jobID INT, @TIfileName NVARCHAR(100), @jobNumber NVARCHAR(50), @authorName NVARCHAR(256), @authorID INT, @title NVARCHAR(256), @intervieweeID INT, @intervieweeName NVARCHAR(256), @intervieweeCommonName NVARCHAR(256), @interviewerID INT, @interviewerName NVARCHAR(256), @interviewerCommonName NVARCHAR(256), @statusID INTASBEGIN SET NOCOUNT ON; SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN SELECT @TransactionCountOnEntry = @@TRANCOUNT BEGIN TRANSACTION END BEGIN SELECT * FROM NLA_JobRegister WHERE jobNumber = @jobNumber IF @@ROWCOUNT = 0 BEGIN SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN BEGIN /* CHECK WHETHER OR NOT THE AUTHOR ALREADY EXISTS IN THE DB */ SELECT * FROM Authors WHERE authorName = @authorName IF @@ROWCOUNT = 0 SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN /* THE AUTHOR DOESN'T EXISTS SO INSERT IT AND RETURN ITS ID */ INSERT INTO Authors VALUES (@authorName) SET @authorID = SCOPE_IDENTITY() SELECT @ErrorCode = @@ERROR END ELSE SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN /* THE AUTHOR ALREADY EXISTS SO GRAB ITS ID VALUE */ SET @authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName) SELECT @ErrorCode = @@ERROR END END BEGIN IF @ErrorCode = 0 /* CHECK WHETHER OR NOT THE INTERVIEWEE ALREADY EXISTS IN THE DB*/ SELECT * FROM Interviewee WHERE intervieweeName = @intervieweeName IF @@ROWCOUNT = 0 /* INTERVIEWEE DOESN'T EXIST SO INSERT IT AND RETURN ITS ID */ SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN INSERT INTO Interviewee VALUES (@intervieweeName, @intervieweeCommonName) SET @intervieweeID = SCOPE_IDENTITY() SELECT @ErrorCode = @@ERROR END ELSE SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN /* THE INTERVIEWEE ALREADY EXISTS SO GRABS ITS ID VALUE */ SET @intervieweeID = (SELECT intervieweeID FROM Interviewee WHERE intervieweeName = @intervieweeName) SELECT @ErrorCode = @@ERROR END END BEGIN IF @ErrorCode = 0 /* CHECK WHETHER OR NOT THE INTERVIEWER ALREADY EXISTS IN THE DB */ SELECT * FROM Interviewer WHERE interviewerName = @interviewerName IF @@ROWCOUNT = 0 /* INTERVIEWER DOESN'T EXIST SO INSERT IT AND RETURN ITS ID */ SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN INSERT INTOInterviewer VALUES (@interviewerName, @interviewerCommonName) SET @interviewerID = SCOPE_IDENTITY() SELECT @ErrorCode = @@ERROR END ELSE SELECT @ErrorCode = @@ERROR IF @ErrorCode = 0 BEGIN /* THE INTERVIEWER ALREADY EXISTS SO GRABS ITS ID VALUE */ SET @interviewerID = (SELECT interviewerID FROM Interviewer WHERE interviewerName = @interviewerName) SELECT @ErrorCode = @@ERROR END END BEGIN IF @ErrorCode = 0 BEGIN /* CREATE A NEW RECORD IN THE JOB REGISTER */ INSERT INTO JobRegister VALUES (@jobNumber, @TIfileName, @authorID, @title, @intervieweeID, @interviewerID, @creationDate, @userID, @statusID) SET @jobID = SCOPE_IDENTITY() SELECT @ErrorCode = @@ERROR END END BEGIN IF @ErrorCode = 0 BEGIN /* CREATE A NEW LOG OF THE TRANSACTION */ INSERT INTO Logging VALUES (@jobID, @userID, 1, @creationDate) SELECT @ErrorCode = @@ERROR END END END END ELSE BEGIN SET @SPreturnVAL = 'Already Exists' END END BEGIN IF @@TRANCOUNT > @TransactionCountOnEntry BEGIN IF @ErrorCode = 0 BEGIN SET @SPreturnVAL = 'Success' COMMIT TRANSACTION END ELSE BEGIN SET @SPreturnVAL = 'Something Died' ROLLBACK TRANSACTION END END END END |
|
James K
Master Smack Fu Yak Hacker
3873 Posts |
Posted - 2013-08-21 : 11:29:58
|
Two or three things from scanning through the code:1. You could shorten the amount of code - for example, for the authors, something like this:SET @authorId = NULL;SELECT @authorId = authorId FROM Authors WHERE authorName = @authorName;IF (@authorId IS NULL) BEGIN INSERT INTO Authors VALUES (@authorName); SET @authorId = SCOPE_IDENTITY();END 2. Does this assume that authorName is always unique? Could there be two authors with the same name?3. Since you are on SQL 2008, the preferred method for error handling is using TRY-CATCH blocks. There are examples here: http://msdn.microsoft.com/en-us/library/ms175976.aspx |
|
|
Cre8tivedaze
Starting Member
8 Posts |
Posted - 2013-08-21 : 11:37:03
|
Hi James,Good idea on the shortening - my head gets a bit lost in these somewhat nested stored procs.As for the Author name always being unique, yes it will be. Because I'm using SQL 2008 (it's actually 2008 R2) are you suggesting that maybe what I have put together isn't really optimal?Cheers. |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
Posted - 2013-08-21 : 12:16:12
|
Here are some thoughts; Instead of selecting the COUNT and then checking that value, it might be faster to do an IF NOT EXISTS. Since the EXISTS can and will short-circut (stop after finding a row) instead of having to count all the rows in the table. But, it looks like you expect the table to be empty anyway, so not a big deal.AS James mentioned, TRY..CATCHHere is another way to do your insert/populate AuthorID: SET @authorID = NULL;----------INSERT Authors SELECT @authorNameWHERE NOT EXISTS ( SELECT authorId FROM Authors WHERE authorName = @authorName; )-- ORMERGE Authors AS TargetUSING (SELECT @authorName) AS Source (authorName) ON Target.authorName = Source.authorNameWHEN NOT MATCHED THEN INSERT (authorName) VALUES (Source.authorName);---------------SET @authorID = SCOPE_IDENTITY()IF @authorID IS NULLBEGIN SET @authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName)END |
|
|
Cre8tivedaze
Starting Member
8 Posts |
Posted - 2013-08-21 : 12:28:12
|
Thanks for the suggestions Lamprey.I'm getting the feeling that TRY..CATCH blocks are getting the most support here. So....Using a TRY..CATCH kind of operation, I could basically set a variable such as @DeadOrAlive in the CATCH block and set it to SET @DeadOrAlive = 'Dead'Then before executing the next block check that variable to see if it equals 'Alive', if it doesn't then pretty much continue to the end and rollback the transaction. |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
Posted - 2013-08-21 : 12:44:32
|
I'm not 100% sure what you need to handle, but I don't think you need to set/check anotehr variable. The basic form for using the TRY..CATCH is:BEGIN TRANBEGIN TRY -- Do stuff COMMIT TRANEND TRYBEGIN CATCH ROLLBACK TRANEND CATCH You can get "more fancy" if you like and check the XACT_STATE:IF (XACT_STATE()) = -1BEGIN PRINT N'The transaction is in an uncommittable state.' + 'Rolling back transaction.' ROLLBACK TRANSACTION;END;-- Test whether the transaction is committable.IF (XACT_STATE()) = 1BEGIN PRINT N'The transaction is committable.' + 'Committing transaction.' COMMIT TRANSACTION; END; |
|
|
Lamprey
Master Smack Fu Yak Hacker
4614 Posts |
Posted - 2013-08-21 : 12:57:53
|
For Grins, here is a partial mock up of one way to handle things:CREATE PROCEDURE [dbo].[CreateJob] @TransactionCountOnEntry INT, @ErrorCode INT, @SPreturnVAL VARCHAR (20) OUTPUT, @userID UNIQUEIDENTIFIER, @creationDate DATETIME, @jobID INT, @TIfileName NVARCHAR(100), @jobNumber NVARCHAR(50), @authorName NVARCHAR(256), @authorID INT, @title NVARCHAR(256), @intervieweeID INT, @intervieweeName NVARCHAR(256), @intervieweeCommonName NVARCHAR(256), @interviewerID INT, @interviewerName NVARCHAR(256), @interviewerCommonName NVARCHAR(256), @statusID INTASBEGIN SET NOCOUNT ON; SET @TransactionCountOnEntry = @@TRANCOUNT BEGIN TRANSACTION BEGIN TRY IF NOT EXISTS (SELECT * FROM NLA_JobRegister WHERE jobNumber = @jobNumber) BEGIN SET @authorID = NULL; MERGE Authors AS Target USING (SELECT @authorName) AS Source (authorName) ON Target.authorName = Source.authorName WHEN NOT MATCHED THEN INSERT (authorName) VALUES (Source.authorName); SET @authorID = SCOPE_IDENTITY(); IF @authorID IS NULL BEGIN SET @authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName); END END IF NOT EXISTS (SELECT * FROM Interviewee WHERE intervieweeName = @intervieweeName) BEGIN SET @intervieweeID = NULL; MERGE Interviewee AS Target USING (SELECT @intervieweeName, @intervieweeCommonName) AS Source (intervieweeName, intervieweeCommonName) ON Target.intervieweeName = Source.intervieweeName WHEN NOT MATCHED THEN INSERT (intervieweeName, intervieweeCommonName) VALUES (Source.intervieweeName, Source.intervieweeCommonName); SET @intervieweeID = SCOPE_IDENTITY(); IF @intervieweeID IS NULL BEGIN SET @intervieweeID = (SELECT intervieweeID FROM Interviewee WHERE intervieweeName = @intervieweeName); END END --- ETC.. COMMIT TRANSACTION END TRY BEGIN CATCH ROLLBACK TRANSACTION END CATCHEND |
|
|
James K
Master Smack Fu Yak Hacker
3873 Posts |
Posted - 2013-08-21 : 13:19:37
|
More "for grins" type of stuff regarding MERGE, in particular, about a seemingly non-atomic behavior (which really is atomic for the default isolation level): http://sqlmag.com/sql-server/merge-statement-tips |
|
|
kameswararao polireddy
Starting Member
19 Posts |
Posted - 2013-08-28 : 07:45:50
|
Using COUNT is better than using EXISTS or NOT EXISTS. Because count can be used in generating error messages at the end of the procedures.Using MERGE is some what a risk some process because if you don’t get the indexing, joins and other considerations correctly, then it is possible to not only lose the performance benefits you hoped for, but to suffer from performance problems much much worse than if you had written the straight-forward INSERT, UPDATE and/or DELETE statements in the first place. Because of their complexity, I believe MERGE statements seem more vulnerable to bad query plans than other DML statements.SO it is better to use INNER or LEFT JOINS instead of MERGE statements.P.Kameswara rao |
|
|
James K
Master Smack Fu Yak Hacker
3873 Posts |
Posted - 2013-08-28 : 08:41:18
|
quote: Originally posted by kameswararao polireddy Using COUNT is better than using EXISTS or NOT EXISTS. Because count can be used in generating error messages at the end of the procedures.Using MERGE is some what a risk some process because if you don’t get the indexing, joins and other considerations correctly, then it is possible to not only lose the performance benefits you hoped for, but to suffer from performance problems much much worse than if you had written the straight-forward INSERT, UPDATE and/or DELETE statements in the first place. Because of their complexity, I believe MERGE statements seem more vulnerable to bad query plans than other DML statements.SO it is better to use INNER or LEFT JOINS instead of MERGE statements.P.Kameswara rao
While there may be cases where one needs to use COUNT over EXISTS or NOT EXISTS, in many cases, it is beneficial to use EXISTS or NOT EXISTS. As always, it depends on the situation, but I lean in favor of exists/not exists over count in many cases. I can go over the reasons, but it has been well-documented and studied, for example: http://sqlblog.com/blogs/andrew_kelly/archive/2007/12/15/exists-vs-count-the-battle-never-ends.aspxRegarding the statements you made about merge - can you point me to any documentation that backs up what you are saying? |
|
|
|
|
|
|
|