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

I completely agree with gbn's answer, but would like to add that you can easily figure this out yourself - you can just run your stored procedure concurrently from two or more connections in a loop mnay times (>1mln), and see for yourself. — A-K, Sep 27 '09 at 20:28

David Hall · Accepted Answer · 2014-02-13T22:14:04.543

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 it is best to use the OUTPUT clause to retrieve it. It used to be usual to use the SCOPE_IDENTITY() for this but there are problems with that under parallel execution plans.

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. This 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.

-1 for recommending SCOPE_IDENTITY; http://support.microsoft.com/kb/2019779. Y'all should be using the OUTPUT clause in all cases. — Michael J. Gray, Feb 13 '14 at 15:32
@MichaelJ.Gray thanks - I've fixed that. Had not heard of the problem before. Do you happen to know when it was discovered? — David Hall, Feb 13 '14 at 22:14
@DavidHall In the release of SQL Server 2005 is when they introduced `OUTPUT`. I'll retract my downvote and provide an upvote :) — Michael J. Gray, Feb 15 '14 at 17:36

score 7 · Answer 2 · edited Sep 27 '12 at 12:10

The safest way to go here would probably be to use the Output clause, since there is a known bug in scope_idendity under certain circumstances ( multi/parallel processing ).

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

  BEGIN TRANSACTION
  BEGIN TRY
    declare @ttIdTable TABLE (ID INT)
    INSERT INTO 
      [dbo].[TicketNumber]([CreatedDateTime], [CreatedBy])
    output inserted.id into @ttIdTable(ID)
    VALUES
      (GETDATE(), SUSER_SNAME())

    SET @NewID = (SELECT id FROM @ttIdTable)

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

  RETURN @NewID
END

This way you should be thread safe, since the output clause uses the data that the insert actually inserts, and you won't have problems across scopes or sessions.

Link to MSKB scope_identity: http://support.microsoft.com/default.aspx?scid=kb;en-US;2019779 — Mark Sowul, Aug 07 '13 at 18:25

score 3 · Answer 3 · answered Sep 27 '09 at 14:13

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.

marc_s · Answer 4 · 2009-09-27T14:40:36.793

1

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

edited Sep 27 '09 at 14:40

answered Sep 27 '09 at 13:15

marc_s

732,580
175
1,330
1,459

You'd never use SET NOCOUNT ON? Really? – gbn Sep 27 '09 at 14:05
Well, if you're using ADO.NET's DataAdapter, using SET NOCOUNT ON can cause the DataAdapter to erroneously think a stored proc failed since it interprets the return value as the "rows affected" count - and if it's 0 (in the case of SET NOCOUNT ON), then the DataAdapter interprets that as "stored proc failed". – marc_s Sep 27 '09 at 14:11
This article is 8 years old so perhaps applies to v1.0 dot net. I've not seen this error for years: I'd assume it was fixed by now... – gbn Sep 27 '09 at 14:17
gbn: this behavior still seems to apply - this is from the .NET 3.5 docs: http://msdn.microsoft.com/en-us/library/59x02y99.aspx – marc_s Sep 27 '09 at 14:39
Microsoft specifically says **not to use** the SET NOCOUNT ON in stored procs if you use them with ADO.NET SqlDataAdapters. – marc_s Sep 27 '09 at 14:39
I'm maintaining some SP's that are being used by classic ADO(COM/C++) & ADO.NET SqlDataAdapters & this conflict causing me potential problems - does anyone else have this issue & have they solved it yet? – Andrina Websdale Oct 23 '11 at 17:34
-1 for recommending SCOPE_IDENTITY; http://support.microsoft.com/kb/2019779. This has plagued my team this week, so we now use OUTPUT in all cases. I recommend it, Microsoft recommends it. I never thought it would matter, ever. – Michael J. Gray Feb 13 '14 at 15:33

score 1 · Answer 5 · answered Sep 28 '09 at 14:41

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.

That's exactly the type of thing I was trying to avoid so thanks very much for adding it. — serialhobbyist, Oct 01 '09 at 09:54

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

5 Answers5

Linked