I have a SQL Server stored procedure:

CREATE PROCEDURE dbo.GetNextNumber
    @NextNumber int OUT
AS
    UPDATE SystemSettings 
    SET REGISTRY_NUMBER = REGISTRY_NUMBER   1  
    WHERE ID = 1;

    SELECT @NextRegistryNumber = REGISTRY_NUMBER
    FROM SystemSettings
    WHERE ID = 1;
  
    SELECT 'NextNumber' = @NextRegistryNumber;

I call this from a C# API method:

public int GetNextRegistryNumberFC()
{
    string procedure = "GetNextNumber";
        
    // Create ADO.NET objects.
    // SqlConnection con = new SqlConnection(connectionString);
    // SqlCommand cmd = new SqlCommand(procedure, con);

    using (var con = new SqlConnection(ConfigurationManager.ConnectionStrings[<ConnectionString>].ConnectionString))
    using (var cmd = new SqlCommand(procedure, con))

    try
    {
        // Configure command and add input parameters.
        cmd.CommandType = CommandType.StoredProcedure;

        // Add the output parameter.               
        cmd.Parameters.Add("@NextNumber", SqlDbType.Int).Direction = ParameterDirection.Output;                

        // Execute the command.
        con.Open();
        cmd.ExecuteNonQuery();

        NextRegistryNumber = Convert.ToInt32(cmd.Parameters["@NextNumber"].Value);
    }
    catch (Exception ex)
    {
        throw;
    }
    finally
    {
        cmd.Dispose();
        con.Close();
        con.Dispose();
    }

    return NextRegistryNumber;
}

This works the first time I run it. After that it just return the same number each time instead of incrementing it. I am not sure what I happening. If I run the stored procedure by it self it works each time. Does anyone know what is going wrong here?

CodePudding user response：

This stored procedure will produce unexpected results if it's called by two or more connections at the same time. Both executions will increment the field then both return the same value. Even if this was fixed, the update query creates a hotspot in a single row, with concurrent requests blocking while they wait for each other to update that row.

To avoid this, SQL Server (and other databases) have SEQUENCE objects that can be incremented atomically with minimal locking :

CREATE SEQUENCE Registration_Numbers START WITH 1;

The NEXT VALUE FOR function will increment a sequence and return the new value :

SELECT NEXT VALUE FOR Registration_Numbers;

This query is so short it doesn't need a stored procedure :

var sql="SELECT NEXT VALUE FOR Registration_Numbers;";
using(var con=new SqlConnection(connectionString))
using(var cmd=new SqlCommand(sql,con))
{
    var new_id=(long)cmd.ExecuteScalar();
    return new_id;
}

There's no need for try/catch here. A using block ensures the connection and command objects will be disposed even if an exception occurs. using works even in cases where finally wouldn't be called.

Rethrowing isn't needed either. The following catch block is no different than no catch at all.

catch (Exception ex)
{
    throw;
}

Such a block would make sense if the exception was processed, eg logged.

Atomic update and read

If using a table is absolutely necessary (why?), the OUTPUT clause can be used to return the new value right from the UPDATE command:

CREATE PROCEDURE dbo.GetNextNumber
AS
    UPDATE SystemSettings 
    SET REGISTRY_NUMBER = REGISTRY_NUMBER   1  
    OUTPUT inserted.REGISTRY_NUMBER
    WHERE ID = 1;

The same code can be used to call the stored procedure. Only the the CommandType needs to change

var proc_name=" dbo.GetNextNumber";
using(var con=new SqlConnection(connectionString))
using(var cmd=new SqlCommand(proc_name,con))
{
    cmd.CommandType = CommandType.StoredProcedure;

    var new_id=(long)cmd.ExecuteScalar();
    return new_id;
}

CodePudding user response：

Is this a typo?

SELECT 'NextNumber' = @NextRegistryNumber;

This would output a table with a column called 'NextNumber' with the value in @NextRegistryNumber ie: you've done a column alias.

It should be:

SELECT @NextNumber = @NextRegistryNumber;

OR

SET @NextNumber = @NextRegistryNumber;