Is this stored procedure thread-safe? (or whatever the equiv is on SQL Server)

Question

With the help of others on SO I've knocked up a couple of Tables and Stored Procedures, this morning, as I'm far from a DB programmer.

Would someone mind casting an eye over this and telling me if it's thread-safe? I guess that's probably not the term DBAs/DB developers use but I hope you get the idea: basically, what happens if this sp is executing and another comes along at the same time? Could one interfere with the other? Is this even an issue in SQL/SPs?

CREATE PROCEDURE [dbo].[usp_NewTicketNumber]
 @ticketNumber int OUTPUT
AS
BEGIN
 SET NOCOUNT ON;
 INSERT INTO [TEST_Db42].[dbo].[TicketNumber]
      ([CreatedDateTime], [CreatedBy])
   VALUES
    (GETDATE(), SUSER_SNAME())
 SELECT @ticketNumber = IDENT_CURRENT('[dbo].[TicketNumber]');
 RETURN 0;
END

Answer 1

You probably do not want to be using IDENT_CURRENT - this returns the latest identity generated on the table in question, in any session and any scope. If someone else does an insert at the wrong time you will get their id instead!

If you want to get the identity generated by the insert that you just performed then you need to use SCOPE_IDENTITY. Scope identity returns the last identiy generated for any table in the current session and the current scope.

For more discussion in this see MSDN.

The main SQL equivalent of thread safety is when multiple statements are executed that cause unexpected or undesirable behaviour. The two main types of such behaviour I can think of are locking (in particular deadlocks) and concurrency issues.

Locking problems occur when a statement stops other statements from accessing the rows it is working with. The can affect performance and in the worst scenario two statements make changes that cannot be reconciled and a deadlock occurs, causing one statement to be terminated.

However, a simple insert like the one you have should not cause locks unless something else is involved (like database transactions).

Concurrency issues (describing them very poorly) are caused by one set of changes to database records overwriting other changes to the same records. Again, this should not be a problem when inserting a record.

Answer 2

First off - why don't you just return the new ticket number instead of 0 all the time? Any particular reason for that?

Secondly, to be absolutely sure, you should wrap your INSERT and SELECT statement into a TRANSACTION so that nothing from the outside can intervene.

Thirdly, with SQL Server 2005 and up, I'd wrap my statements into a TRY....CATCH block and roll back the transaction if it fails.

Next, I would try to avoid specifying the database server (TestDB42) in my procedures whenever possible - what if you want to deploy that proc to a new server (TestDB43) ??

And lastly, I'd never use a SET NOCOUNT in a stored procedure - it can cause the caller to erroneously think the stored proc failed (see my comment to gbn below - this is a potential problem if you're using ADO.NET SqlDataAdapter objects only; see the MSDN docs on how to modify ADO.NET data with SqlDataAdapter for more explanations).

So my suggestion for your stored proc would be:

CREATE PROCEDURE [dbo].[usp_NewTicketNumber]
AS
BEGIN
  DECLARE @NewID INT

  BEGIN TRANSACTION
  BEGIN TRY
    INSERT INTO 
      [dbo].[TicketNumber]([CreatedDateTime], [CreatedBy])
    VALUES
      (GETDATE(), SUSER_SNAME())

    SET @NewID = SCOPE_IDENTITY()

    COMMIT TRANSACTION
  END TRY
  BEGIN CATCH
    ROLLBACK TRANSACTION
    SET @NewID = -1
  END CATCH

  RETURN @NewID
END

Marc

Answer 3

CREATE PROCEDURE [dbo].[usp_NewTicketNumber]
    @NewID int OUTPUT
AS
BEGIN
    SET NOCOUNT ON;
    BEGIN TRY
        BEGIN TRANSACTION
        INSERT INTO 
            [dbo].[TicketNumber] ([CreatedDateTime], [CreatedBy])
        VALUES
            (GETDATE(), SUSER_SNAME())

        SET @NewID = SCOPE_IDENTITY()

        COMMIT TRANSACTION;
    END TRY
    BEGIN CATCH
        IF XACT_STATE() <> 0
            ROLLBACK TRANSACTION;
        SET @NewID = NULL;
    END CATCH
END

I would not use RETURN for meaningful use data: either recordset or output parameter. RETURN would normally be used for error states (like system stored procs do in most cases):

EXEC @rtn = EXEC dbo.uspFoo
IF @rtn <> 0
    --do error stuff

You can also use the OUTPUT clause to return a recordset instead.

This is "thread safe", that is it can be run concurrently.

Answer 4

I agree with David Hall's answer, I just want to expand a bit on why ident_current is absolutely the wrong thing to use in this situation.

We had a developer here who used it. The insert from the client application happened at the same time the database was importing millions of records through an automated import. The id returned to him was from one of the records my process imported. He used this id to create records for some child tables which were now attached to the wrong record. Worse we now have no idea how many times this happened before someone couldn't find the information that should have been in the child tables (his change had been on prod for several months). Not only could my automated import have interfered with his code, but another user inserting a record at the smae time could have done the same thing. Ident_current should never be used to return the identity of a record just inserted as it is not limited to the process that calls it.