October 5, 2018 at 11:05 am
Hey Everyone,
This is the basic stored procedure error processing and capture routine I have in place. Any holes to poke in it or useful additions?
Define the Error tableCREATE TABLE custom.ErrorLog
(
ErrorLogID INT IDENTITY(1,1) NOT NULL,
ErrorMsg NVARCHAR(4000) NOT NULL,
Error NVARCHAR(4000) NOT NULL,
Server NVARCHAR(128) NOT NULL,
Host NVARCHAR(128) NOT NULL,
DB NVARCHAR(128) NOT NULL,
SP NVARCHAR(128) NOT NULL,
Line INT NOT NULL,
ErrorNumber INT NOT NULL,
ErrorSeverity INT NOT NULL,
ErrorState NVARCHAR(10) NOT NULL,
LocalTime NVARCHAR(128) NOT NULL,
CreateBy NVARCHAR(128) NOT NULL,
CreateDate DATETIME NOT NULL,
CONSTRAINT PK_ErrorLog_ErrorLogID PRIMARY KEY CLUSTERED
(
ErrorLogID ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, FILLFACTOR = 100) ON [PRIMARY]
) ON [PRIMARY]
ALTER TABLE custom.ErrorLog ADD CONSTRAINT DF_ErrorLog_CreateBy DEFAULT (SUSER_SNAME()) FOR CreateBy
ALTER TABLE custom.ErrorLog ADD CONSTRAINT DF_ErrorLog_CreateDate DEFAULT (GETDATE()) FOR CreateDate
Define the Error Logging RoutineCREATE PROCEDURE [custom].[LogError]
(
@Procedure SYSNAME,
@Msg NVARCHAR(4000) OUTPUT,
@Severity INT OUTPUT,
@State INT OUTPUT
)
AS
BEGIN
SET NOCOUNT ON;
SET XACT_ABORT ON;
SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
DECLARE
@Failed BIT = 1,
@Succeeded BIT = 0;
DECLARE
@ExecutionStatus BIT = @Succeeded;
BEGIN TRY
-- If in TRANSACTION, get out
IF @@TRANCOUNT > 0
BEGIN
ROLLBACK TRANSACTION;
END;
-- Get everything into a variable - account for any way I could invoke this
DECLARE
@ErrorNumber INT = ISNULL( ERROR_NUMBER(), 0 ), -- Returns the number of the error
@ErrorProcedure NVARCHAR(128) = COALESCE( @Procedure, ERROR_PROCEDURE(), 'Inline Code' ), -- Returns the name of the stored procedure or trigger where the error occurred
@ErrorLine INT = ISNULL( ERROR_LINE(), 0 ), -- Returns the line number inside the routine that caused the error
@ErrorMessage NVARCHAR(4000) = ISNULL( ERROR_MESSAGE(), 'Error' ), -- Returns the complete text of the error message. The text includes the values supplied for any substitutable parameters, such as lengths, object names, or times
@TAB CHAR(1) = CHAR(9), -- TAB
@Database NVARCHAR(128) = DB_NAME(),
@Server NVARCHAR(128) = @@SERVERNAME,
@Host NVARCHAR(128) = HOST_NAME(),
@UserName NVARCHAR(128) = SUSER_SNAME(),
@LocalTime NVARCHAR(40) = CAST( SYSDATETIMEOFFSET() AS VARCHAR(40));
-- Set for OUTPUT
SELECT
@Severity = ISNULL( ERROR_SEVERITY(), 0 ),
@State = ISNULL( ERROR_STATE(), 0 );
-- This handles coming back up out of the error stack
IF @ErrorMessage LIKE '%Severity%'
BEGIN
SET @Msg = '(Called by ' + @ErrorProcedure + ' )' + NCHAR(13) + NCHAR(10) + @ErrorMessage;
END;
ELSE
-- This handles initial error
BEGIN
SET @Msg = N'
Procedure' + @tab + ': ' + @ErrorProcedure + '
Message' + @tab + @tab + ': ' + @ErrorMessage + '
Line' + @tab + @tab + ': ' + CAST( @ErrorLine AS VARCHAR(40) ) + '
Server' + @tab + @tab + ': ' + @Server + '
User' + @tab + @tab + ': ' + @UserName + '
Database' + @tab + ': ' + @Database + '
User Host' + @tab + ': ' + @Host + '
Error' + @tab + @tab + ': ' + CAST( @ErrorNumber AS VARCHAR(40) ) + '
Severity' + @tab + ': ' + CAST( @Severity AS VARCHAR(40) ) + '
State' + @tab + @tab + ': ' + CAST( @State AS VARCHAR(40) ) + '
Time' + @tab + @tab + ': ' + @LocalTime;
END;
-- Put it in the DB in case it is not returned to the user or lost/eaten
INSERT INTO custom.ErrorLog( ErrorMsg, ERROR, SERVER, HOST, DB, SP, Line, ErrorNumber, ErrorSeverity, ErrorState, LOCALTIME )
SELECT @Msg, @ErrorMessage, @Server, @Host, @Database, @ErrorProcedure, @ErrorLine, @ErrorNumber, @Severity, @State, @LocalTime;
END TRY
BEGIN CATCH
-- Error logging error - do what you can
SELECT
@Msg = 'Error logging error - ' + COALESCE( ERROR_MESSAGE(), '' ),
@Severity = ERROR_SEVERITY(),
@State = ERROR_STATE(),
@ExecutionStatus = @Failed;
END CATCH;
RETURN @ExecutionStatus;
END;
Basic stored procedure templateIF OBJECT_ID('custom.ErrorDemo','P') IS NOT NULL
BEGIN
EXECUTE ('DROP PROCEDURE custom.ErrorDemo')
END
GO
--#region Proc Header
------------------------------------------------------------------------------------------------------
-- Author : Douglas Osborne
-- Name : custom.ErrorDemo
-- Create Date : 05OCT2018
-- Description : Demo of TRAN
-- Example : EXECUTE custom.ErrorDemo
-- Called By : ErrorDemo
--
-- Revision Author JIRA Description
------------------------------------------------------------------------------------------------------
-- 05OCT2018 Ozzie JIRA Created
------------------------------------------------------------------------------------------------------
CREATE PROCEDURE custom.ErrorDemo
AS
BEGIN
SET NOCOUNT ON;
SET XACT_ABORT ON;
SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
DECLARE
@Succeeded BIT = 0;
DECLARE
@ExecutionStatus BIT = @Succeeded;
BEGIN TRY
BEGIN TRAN
-- Do Stuff
COMMIT TRAN
END TRY
BEGIN CATCH
DECLARE
@EMsg NVARCHAR(4000),
@ESeverity INT,
@EState INT,
@CError NVARCHAR(100) = N'',
@Failed BIT = 1,
@StoredProcedure SYSNAME = OBJECT_NAME( @@PROCID ); -- Returns the name of the object where the error occurred
IF XACT_STATE () = -1
BEGIN ;
SET @CError = 'Also note - Transaction was not committable.';
ROLLBACK TRANSACTION;
END;
IF @@TRANCOUNT > 0
BEGIN
ROLLBACK TRANSACTION;
END;
EXECUTE custom.LogError @Procedure = @StoredProcedure, @Msg = @EMsg OUTPUT, @Severity = @ESeverity OUTPUT, @State = @estate OUTPUT;
SET @EMsg += @CError;
RAISERROR( '%s', @ESeverity, @estate, @EMsg ) WITH LOG;
SET @ExecutionStatus = @Failed;
END CATCH
RETURN @ExecutionStatus;
END
A demo of this would be a main program which calls one stored procedure which then calls a second, which fails. With this current
technique, I get a complete stack trace from the failing routine all the way back up to the main routine.
Msg 50000, Level 16, State 1, Procedure Main, Line 48 [Batch Start Line 181]
(Called by Main )
(Called by One )
Procedure : Two
Message : Divide by zero error encountered.
Line : 17
Server : DEVSQL
User : douglas.osborne
Database : DemoDB
User Host : Network1418
Error : 8134
Severity : 16
State : 1
Time : 2018-10-05 12:25:27.3691262 -04:00
Any thoughts on shortcomings or improvements?
TIA,
Doug
October 5, 2018 at 11:27 am
A few quick thoughts:
1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.
If you haven't even tried to resolve your issue, please don't expect the hard-working volunteers here to waste their time providing links to answers which you could easily have found yourself.
October 5, 2018 at 11:42 am
Phil Parkin - Friday, October 5, 2018 11:27 AMA few quick thoughts:1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.
3 - I think in some cases it is definitely needed - one example I found and I am sure others exist (and better ones)
https://social.technet.microsoft.com/wiki/contents/articles/40078.sql-server-set-xact-abort-vs-try-catch.aspx
and https://www.red-gate.com/simple-talk/sql/t-sql-programming/defensive-error-handling/
October 5, 2018 at 12:39 pm
frederico_fonseca - Friday, October 5, 2018 11:42 AMPhil Parkin - Friday, October 5, 2018 11:27 AMA few quick thoughts:1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.3 - I think in some cases it is definitely needed - one example I found and I am sure others exist (and better ones)
https://social.technet.microsoft.com/wiki/contents/articles/40078.sql-server-set-xact-abort-vs-try-catch.aspx
and https://www.red-gate.com/simple-talk/sql/t-sql-programming/defensive-error-handling/
That link seems to suggest that it is not even possible to have both
a) Bullet-proof error logging and
b) No possibility of leaving open transactions hanging
In place at the same time.
If the example quoted in the link were used in the OP's framework, my understanding is that the batch would fail, but no error would be written.
If you haven't even tried to resolve your issue, please don't expect the hard-working volunteers here to waste their time providing links to answers which you could easily have found yourself.
October 5, 2018 at 1:25 pm
Phil Parkin - Friday, October 5, 2018 11:27 AMA few quick thoughts:1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.
So you are saying that if I have a TRY CATCH block around all of my executing code there is no reason to have SET XACT_ABORT ON since having that on negates the actions in the TRY CATCH block?
Doug
October 8, 2018 at 5:28 am
Douglas Osborne-229812 - Friday, October 5, 2018 1:25 PMPhil Parkin - Friday, October 5, 2018 11:27 AMA few quick thoughts:1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.So you are saying that if I have a TRY CATCH block around all of my executing code there is no reason to have SET XACT_ABORT ON since having that on negates the actions in the TRY CATCH block?
Doug
SET XACT_ABORT ON ensures that no transactions are left open after the proc terminates. But as you are already handling the transaction rollback explicitly in your CATCH block, it is almost unnecessary. I say almost because of the, hopefully very unusual, cases mentioned by frederico_fonseca which would not trigger the CATCH block, so leaving it in as a failsafe is probably a good idea after all.
If you haven't even tried to resolve your issue, please don't expect the hard-working volunteers here to waste their time providing links to answers which you could easily have found yourself.
October 8, 2018 at 5:52 am
Phil Parkin - Monday, October 8, 2018 5:28 AMDouglas Osborne-229812 - Friday, October 5, 2018 1:25 PMPhil Parkin - Friday, October 5, 2018 11:27 AMA few quick thoughts:1) Don't put SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED in any sort of template.
2) The @Succeeded variable is redundant
3) If you are going to explicitly roll back transactions in a CATCH block, there's no need for the SET XACT_ABORT ON.So you are saying that if I have a TRY CATCH block around all of my executing code there is no reason to have SET XACT_ABORT ON since having that on negates the actions in the TRY CATCH block?
Doug
SET XACT_ABORT ON ensures that no transactions are left open after the proc terminates. But as you are already handling the transaction rollback explicitly in your CATCH block, it is almost unnecessary. I say almost because of the, hopefully very unusual, cases mentioned by frederico_fonseca which would not trigger the CATCH block, so leaving it in as a failsafe is probably a good idea after all.
Thanks Phil,
I removed SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED. Leaving in the extra error check.
Why is setting @Succeeded redundant? I just have the single return and it is either @Succeeded or @Failed.
Thanks for the analysis.
Doug
October 8, 2018 at 6:39 am
Douglas Osborne-229812 - Monday, October 8, 2018 5:52 AMThanks Phil,
I removed SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED. Leaving in the extra error check.
Why is setting @Succeeded redundant? I just have the single return and it is either @Succeeded or @Failed.
Thanks for the analysis.
Doug
Because instead of this:
DECLARE @Succeeded BIT = 0;
DECLARE @ExecutionStatus BIT = @Succeeded;
You can write this:
DECLARE @ExecutionStatus BIT = 0;
If you haven't even tried to resolve your issue, please don't expect the hard-working volunteers here to waste their time providing links to answers which you could easily have found yourself.
October 8, 2018 at 6:42 am
Thinking about this a little more, if you called the variable @ExecutionSuccess rather than @ExecutionStatus, the zero/one setting becomes slightly clearer.
If you haven't even tried to resolve your issue, please don't expect the hard-working volunteers here to waste their time providing links to answers which you could easily have found yourself.
Viewing 9 posts - 1 through 8 (of 8 total)
You must be logged in to reply to this topic. Login to reply