Reputation: 1
ALTER PROCEDURE Add_Edit_Courses_new
@CourseCode VARCHAR,
... other params ...
AS
BEGIN TRY
DECLARE @ErrorCode INT =0, @ErrorMessage VARCHAR(25) = 'Action failed'
IF @TaskType > 2
BEGIN
RAISERROR('Wrong action key',16,1)
END
ELSE
BEGIN TRANSACTION
BEGIN
DECLARE @message VARCHAR(MAX)
IF @TaskType = 1
BEGIN
INSERT INTO Courses(...) VALUES(@CourseCode,...)
SET @message = 'Added Successfully'
END
ELSE IF @TaskType = 2
BEGIN
UPDATE Courses SET CourseCode=@CourseCode,...;
SET @message = 'Modified Successfully'
END
END
COMMIT TRANSACTION
END TRY
BEGIN CATCH
ROLLBACK TRANSACTION
SELECT ERROR_NUMBER() AS ErrorNumber, ...
END CATCH
I wrote this stored procedure to insert and update and I used (1 & 2) to differentiate the task while using a try and catch, but every time I try to execute this stored procedure I keep getting that error, please can you help me with where I am wrong, I am just learning this principle for the first time.
Upvotes: 0
Views: 607
Reputation: 6015
Imo there are some major issues with this code. First, if the intention is for the transaction to be atomic then SET XACT_ABORT ON
should be specified. Per the Docs RAISERROR
does not honor SET XACT_ABORT ON
so it could be converted to use THROW
. Second, in cases where the ELSE
block in the code is hit then COMMIT
will always be hit (regardless of what happens to the commitable state of the transaction).
Also, the code performs the ROLLBACK
before the code:
SELECT ERROR_NUMBER() AS ErrorNumber, ...
It's the ROLLBACK
which clears the error messages and returns the state to normal. To catch the error metadata SELECT
it before the ROLLBACK
happens.
Upvotes: 1
Reputation: 280520
Why is BEGIN TRANSACTION
before BEGIN
? I feel like BEGIN TRANSACTION
/COMMIT TRANSACTION
should be inside the ELSE
conditional. With some noise removed:
IF @TaskType > 2
BEGIN
RAISERROR('Wrong action key',16,1);
END
ELSE
BEGIN -- moved this here
BEGIN TRANSACTION;
-- BEGIN -- removed this
DECLARE @message varchar(max);
IF @TaskType = 1
BEGIN
INSERT INTO Courses(...
SET @message = 'Added Successfully';
END
IF @TaskType = 2 -- don't really need ELSE there
BEGIN
UPDATE Courses SET ...
SET @message = 'Modified Successfully';
END
-- END -- removed this
COMMIT TRANSACTION;
SELECT @message;
END -- moved this here
In your catch you just blindly say:
ROLLBACK TRANSACTION;
This should be updated to:
IF @@TRANCOUNT > 0
BEGIN
ROLLBACK TRANSACTION;
END
Note that if you have a conditional where multiple statements are not wrapped correctly in BEGIN / END
, they won't execute like you think. Consider:
IF 1 = 0
PRINT 'foo';
PRINT 'bar';
You get bar
output every time, regardless of the result of the conditional.
You had something similar:
IF 1 = 1
-- do stuff
ELSE
BEGIN TRANSACTION;
BEGIN
-- do stuff
END
COMMIT TRANSACTION;
In that case, -- do stuff
and the commit happened every time, even if the begin transaction did not, because that BEGIN
/END
wrapper (and anything that followed it) was not associated with the ELSE
.
Perhaps a little dry and wordy if you're new to the topic, but Erland Sommarskog has a very comprehensive series on error handling here, might be worth a bookmark:
Upvotes: 2