views:

221

answers:

4

Is the following stored procedure code robust for multi-user application. It is working fine. I was wondering if there is any better way to do it and if there is any performance issues.

The proc has three sql statements together like 1.Updating hardware status in Allocation Table 2. Calculating Next appropriate Primary Key value for new record to be inserted in DEFECT_LOG table 3.Inserting the values into DEFECT_LOG table. I am also using a variable to return 1 if the transaction was successful.

ALTER PROCEDURE spCreateDefective

(

    @alloc_ID nvarchar(50),
    @cur_date datetime,
    @problem_desc nvarchar(MAX),
    @got_defect_date datetime,
    @trans_status tinyint OUTPUT --Used to check transaction status
)

AS

/*  Transaction Maintainer Statements */

BEGIN TRAN transac1

    SET XACT_ABORT ON

    SET TRANSACTION ISOLATION LEVEL SERIALIZABLE

/*  Declaration Area */

    DECLARE @temp nvarchar(10)

    DECLARE @def_ID nvarchar(20)

/*  Updating Inventory Status to 'Defective' and Updating Release Date to current System date */ 

    UPDATE INV_Allocation_DB

    SET INV_STATUS = 'Defective' , RELEASE_DATE=@cur_date
    WHERE INV_ID=@alloc_ID


/*  Calculates New Primary Key for the DEFECT_LOG Table */

--  Returns the largest number or 1 if no records are present in the table 

    SELECT @temp=COALESCE(CONVERT(int,SUBSTRING(MAX(DEFECT_ID),5,5))+1,1) FROM DEFECT_LOG


    SET @def_ID = 'DEF_'+ RIGHT(replicate('0',5)+ convert(varchar(5),@temp),5)

/*  Insert statement for inserting data into DEFECT_LOG */

    INSERT INTO DEFECT_LOG (DEFECT_ID,INV_ID,PROB_DESC,GOT_DEFECT_DATE) 
    VALUES(@def_ID,@alloc_ID,@problem_desc,@got_defect_date)


    SET @trans_status = 1

    COMMIT TRAN transac1

/*  Returns 1 if transaction successful */

RETURN @trans_status
+1  A: 

Using a SERIALIZABLE transaction level is not recommended unless you absolutely have to have one. It will increase the likelihood of blocking and decrease throughput.

You seem to be using it in order to guarantee a unique DEFECT_ID? Why not use an IDENTITY column for DEFECT_ID instead?

Mitch Wheat
A: 

i wanted a alphanumeric primary key so that primary keys of different tables can be differentiated.

kk
@sathish: I can't think of many situations where that would be desirable (although there are 2 camps on the subject of surrogate primary keys).
Mitch Wheat
A: 

since iam reading the current largest primary key value from the table,isnt it necessary to serialize the operation so that no read error occurs; say if someone inserts a new record in the defect_log table in the time between the two stages 1.getting the largest primary key value and 2.inserting into defect_log table ?

kk
@sathish: Yes and as I mentioned that seems to be why you have choosen a serizable transaction. If you can remove the need to distinguish primary keys from different tables, then use an IDENTITY column and use the default READ COMMITED isolation.
Mitch Wheat
+1  A: 

Personally I would use an IDENTITY field as a true primary key, but then have an additional column with the alphanumeric identifier. (Possibly as a persisted computed field)

This should reduce the number of issues with regards to concurrency.

Dems