Reviewing Stored Procedure template for handling transaction?

Question

I've prepare a template for SQL Server stored procedures which will have Insert, delete, Update transaction.

I have a stored procedure to log error in catch block in a table.

How can I improve my stored procedure to handle transactional error or any kind of error where stored procedure should not give error on screen.

1. Can this be improved to handle transaction in proper way or it sufficient?
2. First Begin tran block or Begin Try block?
3. Rollback in catch block if @@trancount > 0 sufficient?
4. Use of Xact_abort on in every Insert, delete, update transaction procedure is ok?

Code:

ALTER PROCEDURE [dbo].[DB_INSERT_MY_LOG]
(@Param1 varchar(30))
AS
BEGIN
    SET NOCOUNT ON;
    SET XACT_ABORT ON;

    DECLARE          @ErrNumber         NVARCHAR(100)   
    ,@ErrDescription    NVARCHAR(4000)  
    ,@ErrorProcedure    NVARCHAR(200)
    ,@ErrState          INT             
    ,@ErrSeverity       INT             
    ,@ErrLine           INT             
    ,@ErrTime           DATETIME

    BEGIN TRAN 
    BEGIN TRY   
                   --Insert or Update or Delete or (Insert & Update together and vice  versa) & multiple insert into different tables.
    COMMIT TRAN
    END TRY

    BEGIN CATCH

    IF (@@TRANCOUNT > 0)  
    ROLLBACK TRAN

    SET @ErrNumber          = ERROR_NUMBER()
    SET @ErrDescription = ERROR_MESSAGE()
    SET @ErrorProcedure    = ERROR_PROCEDURE()
    SET @ErrState           = ERROR_STATE()
    SET @ErrSeverity        = ERROR_SEVERITY()
    SET @ErrLine            = ERROR_LINE()
    SET @ErrTime            = GETDATE()

    EXEC    MyErrorLogInsert    
    @ErrorNumber                = @ErrNumber
    ,@ErrorDescription          = @ErrDescription
    ,@ErrorProcedure            = 'DB_INSERT_MY_LOG'
    ,@ErrorState                = @ErrState
    ,@ErrorSeverity             = @ErrSeverity
    ,@ErrorLine                 = @ErrLine
    ,@ErrorTime                 = @ErrTime

    END CATCH

    SET NOCOUNT OFF;
    SET XACT_ABORT OFF;

Answer 1

It might be worth looking at XACT_STATE instead of @@TRANCOUNT.

From BOL:

Both the XACT_STATE and @@TRANCOUNT functions can be used to detect whether the current request has an active user transaction. @@TRANCOUNT cannot be used to determine whether that transaction has been classified as an uncommittable transaction.

So there may be some cases where, even though there was an error, the transaction should still be committed to the database.

IF (XACT_STATE()) = -1
BEGIN
    PRINT 'The transaction is in an uncommittable state.' +
          ' Rolling back transaction.'
    ROLLBACK TRANSACTION;
END;

-- Test whether the transaction is active and valid.
IF (XACT_STATE()) = 1
BEGIN
    PRINT 'The transaction is committable.' + 
          ' Committing transaction.'
    COMMIT TRANSACTION;   
END;

Of course, it's entirely up to you whether you want to commit any transaction where an error was raised, even if it is committable... Just because you can, doesn't mean it's always a good idea!

Answer 2

I would use something like the following. This is pretty much what I have been using for years, with your error message logging worked into the CATCH block.

    CREATE PROCEDURE [SchemaName].[ProcedureName]
    (
      @Param  DataType
      ...
    )
    AS
    SET NOCOUNT ON;
    DECLARE @InNestedTransaction BIT;

    BEGIN TRY

      IF (@@TRANCOUNT = 0)
      BEGIN
         SET @InNestedTransaction = 0;
         BEGIN TRAN; -- only start a transaction if not already in one
      END;
      ELSE
      BEGIN
         SET @InNestedTransaction = 1;
      END;

      -- { 1 or more SQL statements }

      IF (@@TRANCOUNT > 0 AND @InNestedTransaction = 0)
      BEGIN
         COMMIT;
      END;

    END TRY
    BEGIN CATCH

      IF (@@TRANCOUNT > 0 AND @InNestedTransaction = 0)
      BEGIN
         ROLLBACK;
      END;

      DECLARE
              @ErrorNumber    INT = ERROR_NUMBER(),
              @ErrorMessage   NVARCHAR(4000) = ERROR_MESSAGE(),
              @ErrorState     INT = ERROR_STATE(),
              @ErrorSeverity  INT = ERROR_SEVERITY(),
              @ErrorLine      INT = ERROR_LINE(),
              @ErrorTime      DATETIME = GETDATE();

      EXEC dbo.MyErrorLogInsert    
        @ErrorNumber      = @ErrorNumber,
        @ErrorDescription = @ErrorMessage,
        @ErrorProcedure   = N'DB_INSERT_MY_LOG',
        @ErrorState       = @ErrorState,
        @ErrorSeverity    = @ErrorSeverity,
        @ErrorLine        = @ErrorLine,
        @ErrorTime        = @ErrorTime;

      RAISERROR(@ErrorMessage, @ErrorSeverity, @ErrorState);
      RETURN;

    END CATCH;

Some notes:

• I don't see a need to use SET XACT_ABORT ON;
• Since the COMMIT and ROLLBACK are in the TRY / CATCH structure, I prefer to also have the BEGIN TRAN there as well.
• In the CATCH block you need to use RAISERROR (can't use THROW as that was not introduced until SQL Server 2012) to let the calling process know there was a problem. That could either be app code or even another proc that called this one, or a SQL Agent job step, etc. It is up to the top-most (i.e. parent) process to determine whether to display error info or not. But you cannot just exit the CATCH block without notifying the calling process of the error.
• You don't need to declare @ErrorProcedure since you pass in a string literal for that one to the logging proc.
• Starting in SQL Server 2008 you can set a variable in the DECLARE so you don't need all of those separate SET statements (in the CATCH block). Even if not setting in the DECLARE, you could set them all in a single SELECT (e.g. SELECT @var1 = 'val', @var2 = 'val2', ...)
• The @InNestedTransaction variable and the places where it is used handle cases when a transaction was started prior to this proc being called, whether it was in another proc, or at the app layer, etc.