SQL Begin Tran and Commit /Roll Back Tran clauses placement Issue

Question

Below is my code I am trying to develop and facing below error , please guide where am I wrong.
To get rid of Locks I am trying to use WITH (NOLOCK) clause , I cannot use it . Is this expected!

Msg 266, Level 16, State 2, Procedure uspMyProc, Line 0 [Batch Start Line 71]
Transaction count after EXECUTE indicates a mismatching number of BEGIN and COMMIT statements. Previous count = 119, current count = 158.

USE MyDB  
go  
ALTER PROCEDURE dbo.MyProc  
(  
   @maxrow    INT = 600000  
  ,@batch     INT = 20000       
  ,@rows      INT = 0           
  ,@rowsdel   INT = 1           
 ) AS  
 WHILE @maxrow >= @rows and @rowsdel > 0   
 BEGIN TRY  
   SET DEADLOCK_PRIORITY LOW;  
 IF (EXISTS (SELECT *   
                 FROM INFORMATION_SCHEMA.TABLES   
          WHERE TABLE_SCHEMA = 'dbo' AND  TABLE_NAME = 'MyTable'))  
  WAITFOR DELAY '00:00:00:01'  
 SET IDENTITY_INSERT dbo.MyTablebkp ON   
 BEGIN TRAN  
    DELETE TOP(@batch) FROM dbo.MyTable --WITH (NOLOCK)  
    OUTPUT   
    DELETED.[Column1],DELETED.[Column2]  
         INTO dbo.MyTablebkp  
   (Column1,Column2)   
     
 --WHERE CONDITION MANDATORY   
 SET @rowsdel = @@ROWCOUNT  
 SET @rows += @rowsdel  
 IF @rows = @maxrow  
 BREAK  
 SET IDENTITY_INSERT dbo.MyTablebkp OFF  
 PRINT  'Looped :' + CONVERT (VARCHAR(10), @rows)  
 PRINT  'Per Batch :'  + CONVERT (VARCHAR(10), @rowsdel)  
 END TRY  
 BEGIN CATCH  
         SELECT    
             ERROR_NUMBER()    AS ErrorNumber  ,ERROR_SEVERITY()  AS ErrorSeverity    
            ,ERROR_STATE()     AS ErrorState   ,ERROR_PROCEDURE() AS ErrorProcedure    
            ,ERROR_LINE()      AS ErrorLine    ,ERROR_MESSAGE()   AS ErrorMessage;   
    END CATCH  
 IF @@TRANCOUNT > 0  
 BEGIN  
 COMMIT TRAN  
 RETURN 0  
 END  
   


  

Answer 1

Please stop using NOLOCK instead take a look at allow_snapshot_isolation vs read_committed_snapshot isolation levels

Your control flow has cases where neither COMMIT nor ROLLBACK may be executed. Move the BEGIN TRANSACTION statement up so that it is placed just below BEGIN TRY.
Begin Try
Begin Transaction

Also consider results of XACT_STATE() function. Check the block Uncommittable Transactions and XACT_STATE on MSDN

Answer 2

The problem I see is you are not using XACT_STATE() anywhere to check the status of transaction. Plus you are beginning a transaction and not committing it. The whole problem as I see, arises because of improper error handling. Can you rewrite your SP in below format of code and see if that helps. The format is taken from Remus Rusanu's blog

create procedure [usp_my_procedure_name]
as
begin
 set nocount on;
 declare @trancount int;
 set @trancount = @@trancount;
 begin try
 if @trancount = 0
 begin transaction
 else
 save transaction usp_my_procedure_name;

 -- Do the actual work here

lbexit:
 if @trancount = 0
 commit;
 end try
 begin catch
 declare @error int, @message varchar(4000), @xstate int;
 select @error = ERROR_NUMBER(), @message = ERROR_MESSAGE(), @xstate = XACT_STATE();
 if @xstate = -1
 rollback;
 if @xstate = 1 and @trancount = 0
 rollback
 if @xstate = 1 and @trancount > 0
 rollback transaction usp_my_procedure_name;

 raiserror ('usp_my_procedure_name: %d: %s', 16, 1, @error, @message) ;
 end catch
end
go

Answer 3

Transaction count after EXECUTE indicates a mismatching number of BEGIN and COMMIT statements

You should format you SQL code more proper, then you see your failure:
You start the transaction inside the WHILE loop and this for every loop, but commits it outside of the loop at the end.

If you get 5 loops, it produce 5 open transactions, at the end you commit 1 transaction, 4 stay open and on the RETURN you get that error message; 5 BEGIN only 1 COMMIT = mismatching number

Answer 4

For a batch delete process - do not use a transaction around the WHILE loop. The goal of a batch delete process is to limit the impact to the transaction log - and to reduce the blocking on the table being deleted.

By using a transaction you are doing the same thing as deleting all of the rows in a single command, which is not what batch deleting is supposed to accomplish.

A few more items:

1. What is the purpose of checking for the existence of the table - and waiting? I would remove that as it isn't needed
2. SET IDENTITY_INSERT ON for your backup table...remove the IDENTITY property on that column in the backup table - create a separate column with an identity property if needed and do not specify that column in your OUTPUT statement.
3. Get rid of the BREAK - it isn't needed. You exit the loop when you have either exceeded the max number of rows or there are no rows affected by the delete. If the delete executes and no rows found - there are no more rows to be deleted (based on your where clause).
4. If you want to force an early out of the loop - check the number of rows affected (@rowdel), and if that value is less than the batch size reset @rowsdel to 0 after adding the value to @rows. This will force the condition @rowsdel > 0 to be evaluated as false and exit the loop.
5. Add a SET NOCOUNT ON as the first command - this will eliminate the rowcount for each iteration
6. Remove @rows and @rowsdel as parameters...these should be defined as variables in the code.
7. Move SET DEADLOCK_PRIORITY LOW before the BEGIN TRY, right after SET NOCOUNT ON

And - start using semi-colon as a statement terminator. It is required for some statements and is a good habit to get into...

Answer 5

In addition to other posts:

• NOLOCK is completely pointless here. If you are updating or deleting data, you are taking locks, no way around it!
• BEGIN TRANSACTION and COMMIT TRANSACTION must match up. So if you have BEGIN TRANSACTION in the beginning of the loop, you need it at the end as well. As long as @@trancount is > 1, COMMIT TRANSACTION commits nothing, it only decrements @@trancount.