2

I have read this thread: Is it bad coding practice to create something in a get if it does not exist?

But, my question involve a method which gets a record from a database or creates it if it doesn't exist. I have two separate methods for checking if a record exists and the second to insert if it doesn't exists. But consequently, it sometimes creates many the same records if I call my api very fast. So, I created one method GetOrCreate and used the transaction. Do you think in that case the GetOrCreate method is a good approach?

public async Task<int> GetOrCreateRegionIdAsync(int companyId)
{
    using (SqlConnection connection = new SqlConnection("....."))
    {
        await connection.OpenAsync();
        using (var transaction = connection.BeginTransaction())
        {
            var regionId = await GetRegionIdAsync(connection, transaction, companyId); // GET
            if (regionId.HasValue)
            {
                return regionId.Value;
            }

            var region = new Region()
            {
                CompanyID = companyId
            };
            var newRegionid = await connection.InsertAsync<int>(region, transaction); // CREATE

            return newRegionid;
        }
    }
}

I need to get the region id (or create it if it doesn't exist) because then I need to save a record in a table called FormLinks which has a column 'regionId'.

-- the Regions table
SELECT [id]
      ,[name]
      ,[companyId]
      ,[available]
      ,[displayOrder]      
      ,[createDate]      
FROM [Regions]

-- the FormLinks table:
SELECT [id]
       [regionId]
      ,[available]
      ,[createDate]
      ,[url]
FROM [FormLinks]
4
  • 1
    You still have a race condition here between getting the record and inserting it. This code just makes that window of time more narrow. It does not eliminate the problem. To be honest, this might be a good use case for a stored procedure in the database. Even then, you are not eliminating the race condition. You just make that window of time even narrower. Jul 1 at 12:48
  • Isn't there a unique constraint on the company Id in the region table? If not, there should be. Jul 1 at 17:27
  • Without knowing more about the database tables it is difficult to recommend a solution. I have a few ideas, but the data model is crucial to know if my ideas are any good. Can you update your question? Jul 1 at 17:29
  • @GregBurghardt I could add the unique constraint but I think it doesn't solve my problem because I need a method which always gets the region id, I need it to save in an another table.
    – MrChudz
    Jul 2 at 5:30
0

No matter how you structure this, a race condition between processes will exist. The best thing you can do is attempt the INSERT, and if it fails, query again.

First, you will need a unique constraint on the table to eliminate duplicate entries for each company. The C# code will need to query for that region, and attempt an insert if the region was not found (the emphasis here is "attempt"). If an exception gets thrown, assume another process inserted that record in between your initial SELECT. Then SELECT the region again:

var regionId = await GetRegionIdAsync(connection, transaction, companyId); // GET

if (regionId.HasValue)
{
    return regionId.Value;
}

var region = new Region()
{
    CompanyID = companyId
};

try
{
    var newRegionid = await connection.InsertAsync<int>(region, transaction); // CREATE

    return newRegionid;
}
catch (SqlException)
{
    regionId = await GetRegionIdAsync(connection, transaction, companyId); // GET again

    return regionId.Value;
}
5
  • There should be no race condition if the SQL is written properly. You query for the value first, taking and holding an appropriate lock, then insert it if it doesn't exist. You then release the lock, and thus release any others who were waiting to query, who will then find that the value exists.
    – Steve
    Jul 2 at 16:43
  • @Steve, yes, but then you are locking records in the database. With a SELECT, attempt INSERT and SELECT-again-if-necessary strategy you avoid gumming up your database. Jul 2 at 17:22
  • Additionally, if you are calling GetOrCreateRegionIdAsync repeatedly inside a loop, or from many different processes, you can have multiple records locked in the same table. That seems like a recipe for frustration, especially if your C# code blows up and the lock isn't released. I typically only use locks when running a SQL script that performs destructive operations on data. Jul 2 at 17:25
  • There's no way to avoid locking at some level when performing a conditional insert concurrently. Virtually any work on the database involves the database engine taking locks, so I wouldn't worry about "gumming up". The crucial point is that a lock sufficient to permit the insert which might occur, must be taken at the outset, and it must be held over either until the insert is complete, or until the check has determined that no insert is necessary (because the record already exists). (1/2)
    – Steve
    Jul 3 at 11:27
  • I always find clerical analogies work best. You have a filing cabinet containing indexed paper files. Two people approach the cabinet with the intention of checking whether a file exists, and if not, then drafting and inserting a new one. Clearly, only one person must be permitted to check at once - the other must wait until the first has completed both the check and any remedial insertion. Real clerks would do this naturally, rather than fight over the drawer. But if they both check concurrently, and find the file doesn't exist, they will both attempt to insert the missing record. (2/2)
    – Steve
    Jul 3 at 11:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.