Stored procedure structure and concurrency issues

Question

Someone wrote the following stored procedure in SQL Server 2008. I think it's just not one the best ways to achieve what he needed to achieve.

As you can see that we need to update MyTable under some conditions but not under the other. And whenever there is a condition that lets a user update MyTable we don't want multiple users to update MyTable concurrently. The thing is the way this procedure is written all the variables are set using select statements and then Begin Transaction starts.

But given the structure of this procedure is it better to execute an sp_getapplock procedure to allow just one user to do the update (that might work well with this procedure given its structure but I read all over that sp_getapplock can lead to deadlocks) and the other options is to do a serializable READ_COMMITED.

But in case of serializable wouldn't this stored procedure allow one of the concurrent user (out of say 2) still select the old values of col1, col2, col3 and col4 and populate variables like @onGoingSession, @timeDiff etc with those old values when the other done updating and released the row (through serializable).Since after the transaction completion it's the turn of second user. And I do wanna block other users from reading the row that is being modified.

Shouldn't we move the begin transaction up right after declaring the variables and the do a serializable after it. So that no concurrent users should read old values and populate the variables while one of the users (apparently who already acquired the row lock) is updating the locked row but hasn't committed as yet.

 USE [myDatabase]
 GO

 SET ANSI_NULLS ON
 GO
 SET QUOTED_IDENTIFIER ON
 GO

CREATE PROCEDURE [dbo].[MyTableProc]
    (@UserId char(20))   
AS
BEGIN
    DECLARE @error varchar(200)
    DECLARE @warning varchar(300)
    DECLARE @timeDiff smallint
    DECLARE @onGoingSession bit = 1
    DECLARE @userName char(20)
    DECLARE @counterOfResync tinyint

    SET @counterOfResync = (SELECT [Col1] FROM MyTable)
    SET @userName = (SELECT [Col2] FROM MyTable)
    SET @onGoingSession = (SELECT [Col3] FROM MyTable)

    SET @timeDiff = (SELECT DATEDIFF(MINUTE, Col4, CURRENT_TIMESTAMP) from MyTable)

    BEGIN TRANSACTION 
        IF(@onGoingSession = 1)
        BEGIN
            IF(@timeDiff >= 360)
            BEGIN
                UPDATE MyTable 
                SET UserId = @UserId, Col4 = CURRENT_TIMESTAMP

                IF(@@ERROR = 0)
                BEGIN
                    SET @warning = RTRIM('An unfinsihed session for ' + 
                                   LTRIM(RTRIM(@userName)) + ' is going on for the past ' +    
                                   LTRIM(RTRIM(@timeDiff)) + ' minutes but updates from ' + LTRIM(RTRIM(@UserId)) + ' are successful')
                    COMMIT

                    RAISERROR(@warning,7, 1) 

                    RETURN
                END
                ELSE
                BEGIN
                    ROLLBACK

                    RETURN @@ERROR
                END
           END  
           ELSE
           BEGIN
               SET @error = RTRIM('A session of updates for '+ LTRIM(RTRIM(@userName))+ ' is already in progress concurrent updates are not allowed')

               IF(@@ERROR = 0)
               BEGIN
                   COMMIT

                   RAISERROR(@error, 8, 1)
                   RETURN
               END
               ELSE
               BEGIN
                   ROLLBACK

                   RETURN @@ERROR
               END
           END
       END
       ELSE IF(@onGoingSession = 0 AND @counterOfResync = 0)
       BEGIN
           UPDATE MyTable 
           SET UserId = @UserId, Col3 = 1, Col4 = CURRENT_TIMESTAMP

           IF(@@ERROR =0)
           BEGIN
               COMMIT

               RETURN
           END
           ELSE
           BEGIN
               ROLLBACK

               RETURN @@ERROR
           END
       END
       ELSE IF(@onGoingSession = 0 AND @counterOfResync > 0 AND @timeDiff >= 5)
       BEGIN
           UPDATE MyTable 
           SET Col3 = 1, CountOfResync = 0, UserId = @UserId, Col4 = 
  CURRENT_TIMESTAMP

           IF(@@ERROR = 0)
           BEGIN
               COMMIT

               RETURN
           END
           ELSE
           BEGIN
               ROLLBACK
               RETURN @@ERROR
           END
       END
       ELSE 
       BEGIN
           SET @error  = RTRIM('A server resync session already in progress, updates can''t be made at the same time')

           IF(@@ERROR = 0)
           BEGIN
               COMMIT
               RAISERROR(@error, 9, 1)
               RETURN
           END
           ELSE
           BEGIN
               ROLLBACK
               RETURN @@ERROR
           END
      END

      RETURN @@ERROR
END

I would appreciate if rather than down voting you guys gimme your valuable suggestions. — user2913184, Feb 07 '17 at 02:47
Double-spaced code without indentation doesn't make it any easier for us to help you. Perhaps you could make it a little tidier. Then there are four `select` statements to get the _only row_ ever allowed in `MyTable`. — HABO, Feb 07 '17 at 03:42
@marc_s I really appreciate that you have edited my code. Thanks a bunch. Can anyone please answer to my questions, will appreciate. — user2913184, Feb 07 '17 at 13:31
Moving the initial queries inside the transaction and getting values with a single `select` with an [`updlock`](https://technet.microsoft.com/en-us/library/ms187373(v=sql.105).aspx) hint should serialize updates without blocking other readers. If you want to prevent reading of stale values while a transaction is active _but not committed_ then you'll want something like an exclusive lock instead. — HABO, Feb 07 '17 at 14:46
Yeah thats what I'm planning right now to move the initial query inside transcation and before that `Set ISOLATION LEVEL SERIALIZABLE` that way I will prevent concurrent updates as well as in case 2 users acquire a shared lock and the update the row in turns. Then while one updating the other is waiting for its turn and as soon as the transaction completes for the first user the second one would hit any of the `IF` statements so either it will bypass the whole transaction and get an error/warning message back to the calling system or might update. — user2913184, Feb 07 '17 at 15:15
I got this idea from this question [link](http://stackoverflow.com/questions/9502273/in-sql-server-how-can-i-lock-a-single-row-in-a-way-similar-to-oracles-select). I don't even need to raise a flag for second user to make him bypass or execute the transaction since I have a bunch of `IF` flags/conditions to pass through. — user2913184, Feb 07 '17 at 15:15
And the thing is I wanna block other readers too as long as the update is happening, Sorry I didn't mention this above in my question though I updated my question. — user2913184, Feb 07 '17 at 15:21
Be careful with [`set isolation level`](https://msdn.microsoft.com/en-us/library/ms173763.aspx?f=255&MSPPError=-2147217396). Read all of the **Remarks** in the documentation re: it works for the duration of the connection, or stored procedure, or trigger. You may want to add a comment regarding the lifetime of the isolation level change in your SP, or use query hints, e.g. `holdlock`, instead. As written the SP should proceed apace. Aside: You might want to look at [`try`/`catch`](https://msdn.microsoft.com/en-us/library/ms175976.aspx?f=255&MSPPError=-2147217396) for error handling. — HABO, Feb 07 '17 at 16:31
Yeah I will have to be careful about the connection issue. Since we might have concurrent users trying to make updates through different connections.But it again goes back to what I said that even if the connection is gone after the transaction completes and in the mean a new user gets another connection from the connection pool and try to run the same transaction but it will read whatever is committed earlier on by the first user and will find itself either passing or failing all the `IF- ELSE` conditions this will force him to either make updates or not. — user2913184, Feb 07 '17 at 20:03
So you read the part that says: "If you issue SET TRANSACTION ISOLATION LEVEL in a stored procedure or trigger, when the object returns control the isolation level is reset to the level in effect when the object was invoked."? — HABO, Feb 07 '17 at 20:36
But in our case a .Net application is the caller where I'm not setting any isolation level the way REPEATABLE READ in a batch. Am I following you right? — user2913184, Feb 07 '17 at 20:48
Or the last resort is to do `SELECT @counterOfResync = somecolumn, @userName = somecolumn, @onGoingSession = somecolumn, @timeDiff = DATEDIFF(MINUTE, somecolumn CURRENT_TIMESTAMP) from MyTable with(updlock, holdlock)`. Where somecolumn are different column names in the database — user2913184, Feb 07 '17 at 20:49
And the other thing is that even if the serializable is good for the connection time it shouldn't hurt us because as long as the connection is alive only 1 user can make updates while other can attempt to but with fail. And as and when the connection not alive whoever connects next with a different connection will again set the isolation level for its transactions and not let any other users to do concurrent updates and that's exactly what we need. — user2913184, Feb 07 '17 at 20:54
Of course, you could just roll the whole thing into a single `update` statement that uses a combination of `case` expressions for the column values and a `where` clause to determine whether to do anything at all. — HABO, Feb 07 '17 at 22:50

score 0 · Answer 1 · answered Feb 08 '17 at 14:55

Something like the following would reduce the transaction to a single update statement with the logic to update individual columns handled by case expressions. Post processing handles any errors and generating the curious variety of results expected.

create procedure dbo.MyTableProc( @UserId as Char(20) )
  as
  begin

  declare @CounterOfResync as TinyInt;
  declare @Error as Int;
  declare @ErrorMessage as VarChar(200);
  declare @OngoingSession as Bit = 1;
  declare @PriorUsername as Char(20);
  declare @TimeDiff as SmallInt;
  declare @WarningMessage as VarChar(300);

  update MyTable
    set
      @CounterOfResync = Col1,
      @PriorUsername = UserId,
      @OngoingSession = Col3,
      @TimeDiff = DateDiff( minute, Col4, Current_Timestamp ),
      UserId = @UserId,
      Col2 = case when Col3 = 0 and Col1 > 0 and DateDiff( minute, Col4, Current_Timestamp ) >= 5 then 1 else Col2 end,
      Col3 = case when Col1 = 0 and Col3 = 0 then 1 else Col3 end,
      Col4 = Current_Timestamp
    set @Error = @@Error;

    if ( @CounterOfResync = 1 )
      if ( @TimeDiff >= 360 )
        begin
        if ( @Error = 0 )
          begin
          set @WarningMessage = 'An unfinished session for user ' + @PriorUsername + ' is going on for the past ' +
            Cast( @TimeDiff as VarChar(10) ) + ' minutes but updates from ' + @UserId + ' are successful';
          RaIsError( @WarningMessage, 7, 1 );
          end
        else
          return @Error;
        end
      else
        begin
        if ( @Error = 0 )
          begin
          set @ErrorMessage = 'A session of updates for '+ @PriorUsername + ' is already in progress concurrent updates are not allowed';
          RaIsError( @ErrorMessage, 8, 1 );
          end
        else
          return @Error;
        end
    else
      if ( @OngoingSession = 0 and @CounterOfResync = 0 )
        return @Error
      else
        -- ...

Apologies for any errors in trying to wade through the existing code and translate it into something stranger. My intent is to provide a (mis)direction that you may choose to follow, not the completed code.

Stored procedure structure and concurrency issues

1 Answers1