views:

244

answers:

4

I have what looked to me at first glance a very simple problem. I want to be able to obtain a unique key value with a prefix. I have a table which contains 'Prefix' and 'Next_Value' columns.

So you'd think you just start a transaction, get the next value from this table, increment the next value in the table and commit, concatenate the prefix to the value, and you are guaranteed a series of unique alphanumeric keys.

However under load, with various servers hitting this stored proc via ADO.NET, I've discovered that from time to time it will return the same key to different clients. This subsequently causes an error of course when the key is used as a primary key!

I had naively assumed BEGIN TRAN...COMMIT TRAN ensured the atomicity of data accesses within the scope. In looking into this I discovered about transaction isolation levels and added SERIALIZABLE as the most restrictive - with no joy.

  Create proc [dbo].[sp_get_key] 
  @prefix nvarchar(3)
  as
  set tran isolation level SERIALIZABLE
  declare       @result nvarchar(32)  

  BEGIN TRY
   begin tran

   if (select count(*) from key_generation_table where prefix = @prefix) = 0 begin
   insert into key_generation_table (prefix, next_value) values (@prefix,1)
   end

   declare @next_value int

   select @next_value = next_value
   from key_generation_table
   where prefix = @prefix

   update key_generation_table
   set next_value = next_value + 1 
   where prefix = @prefix

   declare @string_next_value nvarchar(32)
   select @string_next_value = convert(nvarchar(32),@next_value)

   commit tran

   select @result = @prefix + substring('000000000000000000000000000000',1,10-len(@string_next_value)) + @string_next_value

   select @result

  END TRY
   BEGIN CATCH
  IF @@TRANCOUNT > 0 ROLLBACK TRAN

     DECLARE @ErrorMessage NVARCHAR(400);
  DECLARE @ErrorNumber INT;
  DECLARE @ErrorSeverity INT;
  DECLARE @ErrorState INT;
  DECLARE @ErrorLine INT;

  SELECT @ErrorMessage = N'{' + convert(nvarchar(32),ERROR_NUMBER()) + N'} ' + N'%d, Line %d, Text: ' + ERROR_MESSAGE();
  SELECT @ErrorNumber = ERROR_NUMBER();
  SELECT @ErrorSeverity = ERROR_SEVERITY();
  SELECT @ErrorState = ERROR_STATE();
  SELECT @ErrorLine = ERROR_LINE();
  RAISERROR (@ErrorMessage, @ErrorSeverity, @ErrorState, @ErrorNumber,@ErrorLine)


    END CATCH

Here's the key generation table...

CREATE TABLE [dbo].[Key_Generation_Table](
        [prefix] [nvarchar](3) NOT NULL,
       [next_value] [int] NULL,
  CONSTRAINT [PK__Key_Generation_T__236943A5] PRIMARY KEY CLUSTERED 
  (
    [prefix] ASC
  )WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, IGNORE_DUP_KEY = OFF,      
      ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
  ) ON [PRIMARY]
A: 

Thinking outside the box, could you just add a row to a table with an AUTO_INCREMENT id and then use the id? That's guaranteed to be unique under load. You could then delete the row (so as not to have a table growing endlessly).

To answer your question about what's happening, transactions aren't critical regions.

SERIALIZABLE

Most restrictive isolation level. When it's used, the phantom values cannot occur. It prevents other users from updating or inserting rows into the data set until the transaction will be completed.

The problem this mechanism is designed to prevent is different from the one you are experiencing.

If you want to go with the approach you outlined above, you should be getting an exclusive lock for the critical region.

MarkusQ
I'd prefer to understand what's going on here before doing a workround.
rc1
I'll add an explanation then.
MarkusQ
Thank you - appreciated!
rc1
+1  A: 

Couple of things you have a race condition on your if block. Two requests come in for a new prefix at the same time, both could pass the if block. You should change this around to always insert into your table but in your where clause for the insert do the check to make sure it doesn't exist. Also I'd recommend using Exists instead of count(*)=0. With Exists once sql finds a row it can stop looking.

This same thing can happen with your select, you could have two threads both select the same value, then one gets blocked waiting on the update, but then when it returns it will return the old id.

Modify your logic to update the row first, then get the value you updated it too

   update key_generation_table          
     set next_value = next_value + 1
     where   prefix = @prefix

       select @next_value = next_value -1         
       from key_generation_table          
       where prefix = @prefix

I'd also look at using the ouput statement instead of doing the second select.

EDIT

I'd probally change this to use output since yoru on SQL2005:

declare @keyTable as table  (next_value int)

UPDATE key_generation_Table
set next_value=next_value+1
OUTPUT DELETED.next_value into @keyTable(next_value)
WHERE prefix=@prefix

/* Update the following to use your formating */
select next_value from @keyTable
JoshBerke
You nailed it. Thanks Josh !!
rc1
Welcome don't forget to fix your insert statement that will bite you one day as well
JoshBerke
I'd never seen that OUTPUT DELETED thing... very interesting. Thanks
rc1
You can also use OUTPUT INSERTED.next_value which would get you the value you inserted or in this case the one you updated with. And you can intermix them.
JoshBerke
+1  A: 

Try hinting with UPDLOCK.

      select @next_value = next_value
      from key_generation_table WITH(UPDLOCK)
      where prefix = @prefix

key_generation_table is ideally only used with this specific stored proc. Otherwise UPDLOCK can increase the probability of deadlocks.

Tobias Hertkorn
Thanks but this did not fix the problem
rc1
A: 

Serializable holds locks but reads are allowed. So, the select/update in the middle under load could give the same result if the proc is called very quickly in parallel. I think...

If you do it this way, using the valid syntax, you can combine the 2. The tablock ensures the whole table is locked. This is different to serializable which is concurrency.. tablock is granularity. Also, you assume the key is there.. add the missing prefix after if needed.

update
    key_generation_table WITH (TABLOCK)
set
    @next_value = next_value, next_value = next_value + 1 
where
    prefix = @prefix

if @@ROWCOUNT = 0
begin
    set @next_value  = 1
    insert into key_generation_table (prefix, next_value) values (@prefix, 1)
end
select @string_next_value = convert(nvarchar(32),@next_value)
gbn