I'm fairly sure there is a better way to do this. Since the table is going to be small (less 100 entries) it works fine but the idea of locking the whole table seems really bad but I wasn't sure how else to do this and no one at work was able to help me out.
Basically the idea is to return the ID number of a certain ISP. If it does not exist insert it and return the new id. Ideally I'd like to make this sproc only be able to run one at a time but I was unable to find anyway to do that.
ALTER PROCEDURE [dbo].[usp_InsertISPName] @ISPName VARCHAR(50) ASBEGIN -- SET NOCOUNT ON added to prevent extra result sets from -- interfering with SELECT statements. SET NOCOUNT ON; DECLARE @RecordID int BEGIN TRANSACTION SELECT @RecordID = (SELECT recordid FROM l_ISPName WITH (TABLOCKX) WHERE ISPName = @ISPName) IF @RecordID IS NULL BEGIN INSERT INTO l_ISPName (ISPName) VALUES (@ISPName) SELECT @RecordID = SCOPE_IDENTITY() END COMMIT TRANSACTION RETURN @RecordIDEND
What about this? Note that I changed it so that @RecordID is not a locally declared variable, rather an output parameter. You should not use process exit codes to pass data values back to an application (or calling procedure).
ALTER PROCEDURE [dbo].[usp_InsertISPName]@ISPName VARCHAR(50) ,@RecordID int OUTPUTASBEGIN -- SET NOCOUNT ON added to prevent extra result sets from -- interfering with SELECT statements. SET NOCOUNT ON; SELECT @RecordID = (SELECT recordid FROM l_ISPName WHERE ISPName = @ISPName) IF @RecordID IS NULL BEGIN
INSERT INTO l_ISPName (ISPName) select @ISPName where not exists (select 1 FROM l_ISPName WHERE ISPName = @ISPName) SELECT @RecordID = SCOPE_IDENTITY() END END
Co-FounderSQL Server MVPBrandonGalderisiSQL Server Nation
Hey Tom,
No reason to lock the table when you're just reading to see if a value for a particular record exists. By default, SQL Server will use the read committed isolation level, meaning that it will only read those records that have been committed. This doesn't really save you from phantom reads and nonreapeatable reads, but it works fine for what you're doing. So, no reason for the lock hint.
SQL Server MVP
Brandon
My question to you would be couldn't we have two independent process try to insert the same thing?
Thread 1 gets to the if statement and sees that @RecordID is null for ISP "BellSouth". The thread is then stopped and thread 2 begins
Thread 2 gets to the if statement and sees that @RecordID is null for ISP "BellSouth". It then proceeds to insert BellSouth into the table.
Thread 2 completes and Thread 1 picks up where it left off. It proceeds to insert BellSouth into the table and now we have two instances of BellSouth.
I locked the table so that each process reads the table and then inserts if needed while not allowing any other process to insert data in the middle of it process. My understanding of how threading works in SQL may be incorrect but isn't this a possibility in your code?
Tim
I wasn't worried about getting dirty reads or anything. This sproc is the only one that has access to insert data into the table and the table lock was the only way I knew how to ensure I didn't get two instances inserting data at the same time. I'm sure that this has had to come up somewhere before. Pick a website that lets you register, like ebay, and you'll have a race condition where the process has to check to see if the name exists and then create the user. Potentially two people could check and see that a name doesn't exist and then both try to insert the name. One would succeed and the other would fail. I guess you could catch the fail and return that back to the user. In my case I could catch the fail and just requery the table and get the new ID but I was hoping for a cleaner solution such as the Singleton patter or a flag on the sproc that says only let 1 instance run at a time. How you all handle these situations?
What about changing the order of the procedure to insert where not exists then if @@rowcount = 0 (record exists) retrieve it from the lookup, else from scope_identity().
ALTER PROCEDURE [dbo].[usp_InsertISPName]@ISPName VARCHAR(50) ,@RecordID int OUTPUTASBEGIN -- SET NOCOUNT ON added to prevent extra result sets from -- interfering with SELECT statements. SET NOCOUNT ON; INSERT INTO l_ISPName (ISPName) select @ISPName where not exists (select 1 FROM l_ISPName WHERE ISPName = @ISPName)if @@rowcount=0 SELECT @RecordID = (SELECT recordid FROM l_ISPName WHERE ISPName = @ISPName)else SELECT @RecordID = SCOPE_IDENTITY()
END
I would still assume that we could have a race condition where 2 threads both read the table and not find the isp. The issue stems from that we can query the same table at the same time. By locking the table I eliminate the abliity of anything else to read the table (unless I do a nolock). This ensures that the 2nd thread can't obtain a lock so it can't begin looking for information untl the 1st thread completes. At this point the information is already inserted and the 2nd thread will just read the id of the ISP.
In java their is a key word called Synchronized that can be used on a method to ensure that only one instance is ran at a time. I was hopeing for something like this but was unable to fine anything. In C# I would just create an object to lock on and lock that object. Subsequent threads would wait until the object was release and then once they obtain the lock proceed.
Maybe my fears of cross threading is unfounded and it wouldn't be an issue. I didn't try to do any extensive testing to find out if it would be or not.
Well it seems from a business prospective you cannot have multiple ISPs with the same name. why not carry that into the database with a unique constraint?
The contraint does exist which is why I'm doing the insert this way. The table serves as a self populating ISP lookup table. Originally I had it pull all the names when the interface form loaded. When it was time to insert the data it had to recognize the ISP name was new and inset it into the table before it could link the main table to the ISP name through the recordID. What happened was two people opened the form and added the same ISP. The 1st went through but the 2nd threw an error. At that point I decided to go ahead and put all the logic in the DB as it made more sense to do that anyway. The main insert sproc call this sproc to get the ISP number and then uses that to form the link. However I'm afraid I could still run into the same issue if I remove the lock on the table. It would be very minuscule and I probably would never hit it but it would be there.
since the insert happens as part of the same command as the not exists, it is EXTREMELY unlikely. We have encountered problems where in a high usage system the not exists passes for two items that are executed at same EXACT time. But that only ever happened to us when inserting hundreds of thousands of records at a time. in the end, we turned on the ignore duplicate flag for the unique index to allow it to fail gracefully.