Write buffering in SQL Server

Question

I have a program running on SQL Server 2008. There is a function to create a new shipment. The minimum data needed to create a shipment is the idno, subno and class. The Primary Key is idno + idsub. I created a stored procedure to create a new shipment as follows

ALTER Proc [dbo].[spDP_CreateNewShipment](@Class char(10),@idno int = 0, @idsub smallint = 1)
as

If @idno = 0
Begin
Select @idno = Max(idno)+1 From Shipment
End 
Insert Into Shipment
(idno,idsub, class)
Values (@idno,@idsub,@class)

If I pass it an @idno of 0, it should create a shipment with the next available idno. This works great except sometimes two new shipments will get the same new idno. It would seem that the timing would have to be so exactly the same that this should be close to impossible, yet it happens. The only other possibility I can think of is that the Insert might be getting buffered and might not happen immediately. I know very little about SQL Server settings. Does anyone know of a setting that might cause the write not to happen immediately?

This is what *identity* columns are for, you should use one. As you have observed if process 1 reads Max(idno) after process 2 has read Max(idno) but not yet inserted you will get duplication. — Alex K., Dec 20 '17 at 18:27
Don't reinvent the wheel. Use IDENTITY columns. It will be simpler, quicker, more reliable, easier to maintain, harder to accidentally corrupt, etc, etc... — MatBailie, Dec 20 '17 at 18:46
@MatBailie Identity won't work because the idno is not unique. The primary key is idno + idsub. — Jim Wilcox, Dec 20 '17 at 19:12
If you explain a bit more about your structure I can guarantee that an IDENTITY column will work. You may need one more table, but that itself would be an indication that the current design has a flaw (conflating dimensions and facts is a possible example). That you're trying to do this at all really is a significant code-smell. It should be taken as a warning that you're likely trying to work around a symptom of another problem, a problem that really should be resolved first. — MatBailie, Dec 20 '17 at 22:45
@MatBailie The primary key consists of two columns; idno and idsub. We have shipments that are identified by idno and idsub. In certain situations, we have to create a sub shipment so say we have a shipment 12345/1 and it goes into storage, when it comes back out we create a shipment 12345/2. I inherited this and it would not be easy to change. — Jim Wilcox, Dec 21 '17 at 15:08
You're saying is that `12345,1` and `12345,2` are the same shipment, but different positions in a process? This should mean that you have "real" `shipment` table, with one row per shipment *(`idno` as an identity column)*. A `shipment_sub` table would then have changes in state over time. In your case you could perhaps have `shipment_master` and `shipment` instead. Your procedure then inserts in to `shipment_master` to get the new `idno`? *(If you update your question with example data then I can give a concrete example in an answer, accounting for minimal changes to your existing system.)* — MatBailie, Dec 21 '17 at 15:22
@MatBailie I see what you are saying and you are correct. Even if shipment_master only had shipment numbers on it, it would guarantee they are unique. I have tried putting the current process in a transaction with the highest possible locking. If that doesn't work, I will try your idea. Thanks! — Jim Wilcox, Dec 21 '17 at 15:28

pkuderov · Answer 1 · 2017-12-20T18:41:58.327

2

Short answer is : you don't need it in this case, use identity primary key instead.

You got into this problem because of race condition. After you generated @idno some another thread generates the same @idno too. Then both threads are trying to insert rows with the same @idno and therefore the second insert violates primary key uniqueness rule.

Identity primary key allow you to just insert non-Id-data-columns and leave next-id-generation to the database. Then you can ask it what Id was produced.

For the insert code example look at this SO answer. Something like that:

if @idno = 0 begin
    Insert Into Shipment(idsub, class)
    Output inserted.idno   -- it outputs to the client idno of the inserted row
    Values (@idno,@idsub,@class)
end
else begin
    -- @idno != 0 case
    -- are you SURE you wanna insert with @idno generated on client?

    -- if you HAVE TO, then you probably should use 
    -- GUID (aka uniqueidentifier) for idno column
    -- instead of identity int
end

And for more information about identity columns look at msdn. In short you declare that next value for this column is generated based on some initial value seed with proposed step. The most common (seed, step) is (1, 1) but you can set whatever you want.

edited Dec 20 '17 at 18:41

answered Dec 20 '17 at 18:27

pkuderov

3,501
2
28
46

I can't use an identity column because idno is not unique. The way the function works is if you pass idno = 0, it should create a new idno. If you pass it idno <> 0, then it is creating a sub shipment with the same idno but different idsub which is passed as a parameter. – Jim Wilcox Dec 20 '17 at 19:18
The "use transaction" answer obviously will work - but it definetely has a flaw. Another way is to separate idno to another table where idno is identity column with foreign key to it from your main table. In this case, when idno = 0, you insert new row to idno table and obtain its id then use it further as in "idno != 0" case. – pkuderov Dec 22 '17 at 09:42
oh, just seen the comment from MatBailie with the same advice :) – pkuderov Dec 22 '17 at 09:44
Thanks for your response. What is the down side to putting the whole thing in a transaction? – Jim Wilcox Dec 22 '17 at 15:10

score 1 · Accepted Answer · answered Dec 20 '17 at 18:45

1

Wrap your statements into transaction and set the highest isolation level. Something like this.

ALTER Proc [dbo].[spDP_CreateNewShipment](@Class char(10),@idno int = 0, @idsub smallint = 1)
as
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN TRANSACTION
If @idno = 0
Begin
Select @idno = Max(idno)+1 From Shipment
End 
Insert Into Shipment
(idno,idsub, class)
Values (@idno,@idsub,@class)
COMMIT TRANSACTION

answered Dec 20 '17 at 18:45

Alex Kudryashev

9,120
3
27
36

I wrapped it in a transaction as you suggested. If the problem goes away, I will mark this as the answer. – Jim Wilcox Dec 21 '17 at 15:10

lucky · Answer 3 · 2017-12-20T18:39:31.880

0

It could be better to perform actions within a transaction;

begin transaction
    If @idno = 0
    Begin
        Select @idno = Max(idno)+1 From Shipment
    End 
    Insert Into Shipment (idno,idsub, class) Values (@idno,@idsub,@class)
commit transaction

edited Dec 20 '17 at 18:39

answered Dec 20 '17 at 18:33

lucky

12,734
4
24
46

Write buffering in SQL Server

3 Answers3