Procedure resuming even after the error

Question

Below is my procedure in SQL Server 2005

PROCEDURE [dbo].[sp_ProjectBackup_Insert]
    @prj_id bigint
    AS
    BEGIN
     DECLARE @MSG varchar(200)
     DECLARE @TranName varchar(200)
     DECLARE @return_value int


    -- 1. Starting the transaction 
     begin transaction @TranName

    -- 2. Insert the records

     SET IDENTITY_INSERT [PMS_BACKUP].[Common].[PROJECT] ON  INSERT INTO [PMS_BACKUP].[Common].[PROJECT] ([PRJ_ID],[PRJ_NO1],[PRJ_NO2],[PRJ_NO3],[PRJ_DESC],[IS_TASKFORCE],[DATE_CREATED],[IS_APPROVED],[DATE_APPROVED],[IS_HANDEDOVER],[DATE_HANDEDOVER],[DATE_START],[DATE_FINISH],[YEAR_OF_ORDER],[CLIENT_DETAILS],[SCOPE_OF_WORK],[IS_PROPOSAL],[PRJ_MANAGER],[PRJ_NAME],[MANAGER_VALDEL],[MANAGER_CLIENT],[DEPT_ID],[locationid],[cut_off_date]) SELECT * FROM  [pms].[Common].[PROJECT]  T WHERE T.PRJ_ID =  (@prj_id) SET IDENTITY_INSERT [PMS_BACKUP].[Common].[PROJECT] OFF        IF @@ERROR <> 0 GOTO HANDLE_ERROR
      SET IDENTITY_INSERT [PMS_BACKUP].[Common].[DEPARTMENT_CAP] ON  INSERT INTO [PMS_BACKUP].[Common].[DEPARTMENT_CAP] ([CAP_ID],[DEPT_ID],[PRJ_ID],[IS_CAPPED],[DATE_CAPPED],[CAPPED_BY],[CAP_APPROVED_BY],[STATUS],[UNCAPPED_BY],[DATE_UNCAPPED],[DESCRIPTION],[UNCAP_APPROVED_BY],[LOCATIONID]) SELECT * FROM  [pms].[Common].[DEPARTMENT_CAP]  T WHERE T.PRJ_ID =  (@prj_id) SET IDENTITY_INSERT [PMS_BACKUP].[Common].[DEPARTMENT_CAP] OFF        IF @@ERROR <> 0 GOTO HANDLE_ERROR
       INSERT INTO [PMS_BACKUP].[Common].[DOC_REG]  SELECT * FROM  [pms].[Common].[DOC_REG]  T WHERE T.PRJ_ID =  (@prj_id)    IF @@ERROR <> 0 GOTO HANDLE_ERROR 



    -- 3. Commit transaction

     COMMIT TRANSACTION @TranName;

        return @@trancount;

    HANDLE_ERROR: 
     rollback transaction @TranName
     RETURN 1 
    END

and the issue is even if the first insert query fails, its not stopping the processing and resume the rest of the insert queries. The return value I am getting is 1, but in the results window I can see the log like this

(0 row(s) affected) Msg 2627, Level 14, State 1, Procedure sp_ProjectBackup_Insert, Line 35 Violation of PRIMARY KEY constraint 'PK_PROJECT'. Cannot insert duplicate key in object 'Common.PROJECT'. The statement has been terminated.

(0 row(s) affected)

(0 row(s) affected)

I thought the return 1 will make the exit from error handling code but not happening. Any problem with my error handling?

Answer 1

You need to put proper error handling around your statements. With SQL 2005 and up, that means try/catch:

PROCEDURE [dbo].[sp_ProjectBackup_Insert] 
    @prj_id bigint 
    AS 
    BEGIN 
     DECLARE @MSG varchar(200) 
     DECLARE @TranName varchar(200) 
     DECLARE @return_value int 


    -- 1. Starting the transaction  
    BEGIN TRANSACTION @TranName 

    -- 2. Insert the records 

    BEGIN TRY

         SET IDENTITY_INSERT [PMS_BACKUP].[Common].[PROJECT] ON  INSERT INTO [PMS_BACKUP].[Common].[PROJECT] ([PRJ_ID],[PRJ_NO1],[PRJ_NO2],[PRJ_NO3],[PRJ_DESC],[IS_TASKFORCE],[DATE_CREATED],[IS_APPROVED],[DATE_APPROVED],[IS_HANDEDOVER],[DATE_HANDEDOVER],[DATE_START],[DATE_FINISH],[YEAR_OF_ORDER],[CLIENT_DETAILS],[SCOPE_OF_WORK],[IS_PROPOSAL],[PRJ_MANAGER],[PRJ_NAME],[MANAGER_VALDEL],[MANAGER_CLIENT],[DEPT_ID],[locationid],[cut_off_date]) SELECT * FROM  [pms].[Common].[PROJECT]  T WHERE T.PRJ_ID =  (@prj_id) SET IDENTITY_INSERT [PMS_BACKUP].[Common].[PROJECT] OFF        IF @@ERROR <> 0 GOTO HANDLE_ERROR 
         SET IDENTITY_INSERT [PMS_BACKUP].[Common].[DEPARTMENT_CAP] ON  INSERT INTO [PMS_BACKUP].[Common].[DEPARTMENT_CAP] ([CAP_ID],[DEPT_ID],[PRJ_ID],[IS_CAPPED],[DATE_CAPPED],[CAPPED_BY],[CAP_APPROVED_BY],[STATUS],[UNCAPPED_BY],[DATE_UNCAPPED],[DESCRIPTION],[UNCAP_APPROVED_BY],[LOCATIONID]) SELECT * FROM  [pms].[Common].[DEPARTMENT_CAP]  T WHERE T.PRJ_ID =  (@prj_id) SET IDENTITY_INSERT [PMS_BACKUP].[Common].[DEPARTMENT_CAP] OFF        IF @@ERROR <> 0 GOTO HANDLE_ERROR 
           INSERT INTO [PMS_BACKUP].[Common].[DOC_REG]  SELECT * FROM  [pms].[Common].[DOC_REG]  T WHERE T.PRJ_ID =  (@prj_id)    IF @@ERROR <> 0 GOTO HANDLE_ERROR  

        -- 3. Commit transaction 

        COMMIT TRANSACTION @TranName; 
        RETURN 0

    END TRY 

    BEGIN CATCH

        --HANDLE_ERROR
        ROLLBACK TRANSACTION @TranName 
        RETURN 1  

    END CATCH 

    END 

(Be sure to test and debug this -- should be good, but you never know.)

The RETURN value is only relevant to whatever called the procedure -- if it's not checking for success or failure, then you may have a problem.

Answer 2

There are so many things wrong with this I don't know where to start.

As far as your error call, you are trapping whether there is an error on the last step run before the error, not if any error has occurred so far. Since the last step is not the insert but the set_identity_insert statement, there is no error to trap.

Now, on to what needs fixing besides that.

If this is a backup table and is only used as a backup table, get rid of the identity property all together. No need to keep turning the insert on and off, just fix the table, it is not being directly written to by users data is alawys coming from another table, so why does it need an identity at all?

Next, the error you got indicates to me that what you need to be doing is inserting only records that don't already exist in the backup table not all records. You may also need to update existing records. Or you need to truncate the table first before doing the insert if you only need the most current data period and the data table being copied is not that large (you don't want to re-enter a million records when only 100 were new and 10 were changed).

In SQL Server 2005 you have TRY CATCH blocks available, you should start using those instead of goto.

Never, ever, ever use SELECT * in an insert. Or any time the code will go to production. Select * is a very poor programming technique. In the insert for instance it will cause problems when the initial table is changed as you define the columns to insert into but not those in the select.

Finally, you should not name stored procedures with sp at the start. System procs start with sp and SQL Server will look there first for the proc before looking at user procs. It's a little wasted time every time you call a proc. Overall it's bad for the system and if they happen to have a system proc with the same name yours will never be called.