Will all transaction in this stored procedure be rolled back

Question

I have created a stored procedure (shown below) in SQL Server and tried to include a Rollback Transaction as I need to have a "stored procedure that has a transaction around it, so that if/when it fails all inserts will be rolled back."

I am unsure if this work or not, or will work, I cannot test yet as only developing locally, but wondered if someone wouldn't mind looking over the Rollback Transaction part of the stored procedure and advise if on the right path?

USE AutomatedTesting
GO

ALTER PROCEDURE [dbo].[spInsertTestCases] 
    (@AddedTFS INT,
     @Scenario NVARCHAR(500),
     @TargetTableName NVARCHAR(100),
     @TargetFieldName NVARCHAR(100),
     @ExpectedResult NVARCHAR(100),
     @TargetTableDBName NVARCHAR(100),
     @TargetTableSchema NVARCHAR(100),
     @TargetFieldIsDateTime NVARCHAR(1),
     @TestCaseIdentifiers dbo.TestCaseIdentifiers READONLY )  -- can only be READONLY. meaning you cannot amend the param
     /* @TestCaseIdentifiers var will be prepopulated
        TestDataIdentifiersNEW is a custom data type which has  fields (TestSequence ColumnName ColumnValue IsAlphaNumeric)

        so stored procedure is called like:
        EXEC [dbo].[spTest_UserDefinedDatatype] 'param1','param2' @temp_testdata 

        @temp_testdata  will already be defined and popualted(INSERT INTO ) before exec to add in 1 to many rows.
        for example:
        ColumnName  ColumnValue
        PATIENTID   123456
        SOURCESYS   PAS

        in simple terms above EXEC is:
        EXEC [dbo].[spTest_UserDefinedDatatype] 'param1','param2'   'PATIENTID  123456'
                                                                    'SOURCESYS  PAS'
    */
AS
BEGIN TRY
    BEGIN TRANSACTION 
        BEGIN
            --DECLARE @TableNameUpdate SYSNAME = @TargetTableName
            --DECLARE @CDI SYSNAME = REPLACE(@TargetTableName,'CDO','CDI')  -- so if targettablename param is CDO then swap it to CDI.  why?

            DECLARE @sql VARCHAR(MAX) = '  INSERT INTO [dbo].[TestCasesIdentifier] ([TestCaseId], [TestCaseSequence], [FieldName], [FieldValue], [AlphaNumeric]) VALUES '

            DECLARE @i INT = 1
            DECLARE @TableNameUpdate SYSNAME = @TargetTableName
            DECLARE @CDI SYSNAME = REPLACE(@TargetTableName,'CDO','CDI')
            DECLARE @ColName SYSNAME
            DECLARE @Ret NVARCHAR(256)
            DECLARE @sql2 NVARCHAR(MAX)
            DECLARE @TestCaseID INT = -1   --does this need default variable?
            DECLARE @ErrorCode INT = @@error
            DECLARE @TestSequence INT
            DECLARE @ColumnName VARCHAR(100)
            DECLARE @ColumnValue VARCHAR(100)
            DECLARE @IsAlphaNumeric BIT
            DECLARE @TableTestSequence INT =  ISNULL((SELECT MAX([TableTestSequence]) + 1 FROM TestCases WHERE @TargetTableName = [TargetTableName]), 1)

            -- INSERT into TestCases.   1 record

            -- An assumption that a number of fields will have defaults on them -            if not, extra fields will need adding
            INSERT INTO [dbo].[TestCases] ([AddedTFS], [TableTestSequence], [Scenario], 
                                           [TargetTableName], [TargetFieldName], [ExpectedResult],
                                           [TargetTableDBName], [TargetTableSchema], [TargetFieldIsDateTime])
            VALUES (@AddedTFS,      -- AddedTFS (The TFS Number of the Development carried out)
                    ISNULL((SELECT MAX([TableTestSequence]) + 1     -- TableTestSequence (Generates the next Sequence Number for a Table)
                            FROM TestCases                          -- if table doesnt exist in TestCases then sets to 1
                            WHERE @TargetTableName = [TargetTableName]), 1),
                    @Scenario,                                  -- Scenario (A description of the scenario use GIVEN and WHERE)
                    @TargetTableName,                           -- TargetTableName (References the Target Table entered at the top of this SQL - SET @TableName = 'CDO_APC_ELECTIVE_ADMISSION_LIST')
                    @TargetFieldName,                                   -- TargetFieldName (The Field in which we want to test)
                    @ExpectedResult,                                    -- ExpectedResult (The expected output/result of the field in which we want to test)
                    @TargetTableDBName,                             -- The DB to be used
                    @TargetTableSchema,                             -- the schema to be used
                    @TargetFieldIsDateTime)                         ---- 1 = Yes, 0 = No (Is Target field a datetime field) 

            -- Grab the identity value just generated by the last statement and the last error code generated
            -- in order to reference TestCases PK when adding to TestCaseIdentifiers
            SELECT @TestCaseID = SCOPE_IDENTITY(), @ErrorCode = @@error

            IF @ErrorCode = 0 --OR @TestCaseID <> -1      -- @ErrorCode <> 0  if error then back out testcases INSERT?  surely should use BEGIN/ROLLBACK tran
            --IF @ErrorCode = 0 OR @TestCaseID <> -1
            -- If there was no error creating the TestCase record, create the records for the WHERE clause
            BEGIN
                /*
                 rollback insert if no matching records
                 rollback insert if SQL returns more than 1 record
                 return error message to user
                */
                SELECT 
                    ic.index_column_id, c.name
                INTO #tmp
                FROM sys.indexes i
                JOIN sys.index_columns ic ON i.object_id = ic.object_id 
                                          AND i.index_id = ic.index_id
                JOIN sys.columns c ON c.column_id = ic.column_id 
                                   AND c.object_id = ic.object_id
                JOIN sys.tables t ON c.object_id = t.object_id
                WHERE t.name = @CDI
                  AND i.is_primary_key = 1

        IF (SELECT COUNT(*) FROM @TestCaseIdentifiers) = 0
        --IF @PKValues IS NULL
        BEGIN

            WHILE @i <= (SELECT COUNT(*) FROM #tmp)
            BEGIN
                   SELECT @ColName = [name]
                   FROM   #tmp
                   WHERE  index_column_id = @i

                   -- if @expectedvalue IS NULL
                   SET @sql2 = 'SELECT TOP 1 @RetvalOut = ' + QUOTENAME(@ColName) + ' FROM ' + QUOTENAME(@CDI) + ' ORDER BY NEWID()'
                   -- else
                   -- SET @sql2 = ''

                   EXECUTE sp_executesql @command = @sql2, @ParmDefinition = N'@RetvalOut NVARCHAR(MAX) OUTPUT', @retvalOut = @Ret OUTPUT

                   SET @sql += '(' + CONVERT(VARCHAR(100),@TestCaseID) + ',' + CONVERT(VARCHAR(10),@i) + ',''' + @ColName + ''',''' + @Ret + ''',1),'

                   SET @i+=1


    SELECT @sql = REVERSE(SUBSTRING(REVERSE(@sql),2,8000))

    PRINT @sql
    EXEC @sql

            END
        END

    ELSE


    BEGIN
--PRINT 'got here'
        DECLARE csr_TestCaseIdentifierInsert CURSOR FOR
            SELECT  [TestSequence],[ColumnName],[ColumnValue],[IsAlphaNumeric]
            FROM @TestCaseIdentifiers
            ORDER BY [TestSequence]

            OPEN csr_TestCaseIdentifierInsert

                FETCH NEXT FROM csr_TestCaseIdentifierInsert INTO @TestSequence, @ColumnName, @ColumnValue, @IsAlphaNumeric
                WHILE @@fetch_status = 0
                BEGIN

                    INSERT INTO [dbo].[TestCasesIdentifier] 
                    ([TestCaseId], 
                    [TestCaseSequence], 
                    [FieldName], 
                    [FieldValue], 
                    [AlphaNumeric])
                    VALUES 
                    (@TestCaseID,  @TestSequence, @ColumnName, @ColumnValue,@IsAlphaNumeric)
                    FETCH NEXT FROM csr_TestCaseIdentifierInsert INTO @TestSequence, @ColumnName, @ColumnValue, @IsAlphaNumeric
                END
            CLOSE csr_TestCaseIdentifierInsert
            DEALLOCATE csr_TestCaseIdentifierInsert


    END -- loop to add records to testcasesidentifier
END
END
    COMMIT
END TRY
BEGIN CATCH
    IF @@TRANCOUNT > 0
        ROLLBACK TRAN

        DECLARE @ErrorMessage NVARCHAR(4000) = ERROR_MESSAGE()
        DECLARE @ErrorSeverity INT = ERROR_SEVERITY()
        DECLARE @ErrorState INT = ERROR_STATE()

    -- Use RAISERROR inside the CATCH block to return error  
    -- information about the original error that caused  
    -- execution to jump to the CATCH block.  
    RAISERROR (@ErrorMessage, @ErrorSeverity, @ErrorState);
END CATCH

Answer 1

You are almost there. I usually wrap the stored proc code within a BEGIN..END block as well, then next comes the most important part: you must add SET XACT_ABORT ON; before your TRY..CATCH and BEGIN TRAN, as SQL Server defaults the XACT_ABORT to OFF. Otherwise not everything will be rolled back. Example setup:

CREATE PROCEDURE dbo.uspMyTestProc
AS
  BEGIN
  SET NOCOUNT, XACT_ABORT ON;

  BEGIN TRY
  BEGIN TRANSACTION;

  -- Do your magic stuff here before committing...

  COMMIT;
  END TRY
  BEGIN CATCH
  IF @@trancount > 0
  ROLLBACK TRANSACTION;

  -- Add extra error logging here if you want...

  END CATCH;
  END;
GO

Also, if you want to add a possible stacktrace if you are using nested procedures et cetera you might want to consider using a generic error handler à la Erland Sommerskog. We adapted this approach completely. See for more details How to handle Transaction in Nested procedure in SQL server?