2

I am embarking on on a large project and have an inquiry about stored procedures in general.

Let's say I have a User table.

UserKey IDENTITY NOT NULL PRIMARY KEY
FirstName nvarchar(50)NOT NULL
LastName nvarchar(50) NOT NULL
Email nvarchar(50) NOT NULL
Birthdate date NULL
EffectiveFrom datetimeoffset NOT NULL
EffectiveThru datetimeoffset NULL

In reality, there are some more fields here, but for the sake of brevity I have skimmed it down to the basics.

Now, I am faced with creating stored procedures to allow basic CRUD functionality for this table.

The Insert sproc is simple enough.

usp_INSERT_User:

CREATE PROCEDURE [dbo].[usp_INSERT_User]
(
    --@UserKey BIGINT,
    @FirstName NVARCHAR(50),
    @LastName NVARCHAR(50),
    @Email NVARCHAR(50),
    @BirthDate DATE = NULL,
    @EffectiveFromDtTm DATETIMEOFFSET(7),
    @EffectiveThruDtTm DATETIMEOFFSET(7) = NULL,
)

My main inquiry is how I should write an update sproc.

Since there can be pretty granular updates, should I create an update stored procedure for each of those granular updates?

For example, let's say I only want to update the user's email after processing a verified email update for the user, would I write a sproc with one parameter?

usp_UPDATE_UserEmail @NewEmail

If the user updated their profile, I could write a sproc like:

usp_UPDATE_User @UserKey, @FirstName, @LastName, @Email, @Birthdate

So, to cover my most common update operations, I might end up with a few sprocs to cover use cases of updates to the User table. Obviously, the larger the table and/or the higher number of business requirements would have a direct correlation to the number of update sprocs written for a particular table. This gets me thinking about maintainability of a ton of sprocs.

Yesterday, I went down the rabbit hole of

"oh, why don't I just write ONE end-all-be-all update sproc that can take whatever parameter I want to update via an XML param"

Luckily, without using dynamic sql, I was able to create that monstrosity. And it works.

I then decided to test speeds between my fancy xml sproc and just a regular update sproc (with multiple params) with very similar updates.

So, I created a mock user table with 12 million records and tested both approaches. Unsurprisingly, the XML approach on average, took 7x longer than the vanilla update sproc.

There is also additional overhead on the other side of the wire serializing parameters into XML, which could be of concern with a large update batch.

Now, I say all of that to wrap up and ask which type of approach in your experience is most often (if at all) seen? Is is most common practice to have granular sprocs like usp_UPDATE_Field1 / usp_UPDATE_Field2 / usp_UPDATE_Field3&4 OR usp_UPDATE_whateveryouwant

I like the simplicity of having a golden sproc that can take however many number of parameters you want to pass it and update them accordingly, but there are also performance drawbacks.

To give more context: running SQL Server 2019. I am also pretty new at this stuff.

Thanks for any help!

6
  • What problem are you trying to solve by wrapping simple updates in SPs at all? What's wrong with plain SQL issued by the application? – mustaccio Mar 2 at 13:12
  • Not trying to solve any problem by using sprocs. Nothing in particular is wrong with plain SQL issued by the application. I just want to keep data access layers consistent since I will be using sprocs for more complex data operations. – spencer741 Mar 2 at 13:31
  • If they are not solving any problems, I suggest you KISS them good-bye. – mustaccio Mar 2 at 13:34
  • Is it really a terrible thing to wrap simple updates in a sproc? Although my explorative post emits questionable complexity, I don't really see the issue of creating small and tightly focused SPs wrapping updates. – spencer741 Mar 2 at 13:41
  • As an aside, SQL shreds JSON 2-10x faster than XML codingsight.com/… – Jonathan Fite Mar 2 at 13:56
6

Stored procs are the programming API of the database, the methods you invoke to mutate the system state. As such, I prefer to have SPs names reflect the business outcome desired rather than the implementation - change_of_address rather than update_customer. Each SP has the parameters to perform that business change and no others. Each does one thing and does it well.

If the business supports two processes that both update a common column then so be it. That column is an input to each of those SPs. Say there's a web based "change profile" and a mobile-only "register new phone". They both write to customer.phone_number. I would have separate SPs for each business purpose.

There is no measurable overhead to having many small, tightly focused stored procedures. Changes to data types are automatic on the next recompile. Changes to column names are simple search-and-replace. SSMS's built-in dependency analysis can help, too. If you use local variables that mirror columns use the column's name e.g. customer_name is read into @old_customer_name.

1
  • 4
    "There is no measurable overhead to having many small, tightly focused stored procedures" - in fact there is sometimes a benefit over more complex procedures, which are more likely to suffer from intermittent bad plans due to "parameter sniffing" issues. – David Spillett Mar 2 at 11:22
2

Personally I don't understand why people write stored procedures that only perform CRUD. For me SQL is the interface to the database. Just write an update statement with placeholders for the column values and send that to the database -- along with the bound variables.

Someone will argue about SQL injection, but a) you're just moving the point where that can happen (the stored procedure could still be written badly), and b) you still have to guard for other forms of injection (eg a malicious user entering HTML and JavaScript in a text field).

5
  • Have you seen some of the crap that LINQ and EntityFramework generate? Also, can be used to obscure database implementation details that you don't necessarily want exposed to the app. That said, I don't see the point of doing ALL of your CRUD that way, but there are use-cases for it. – Jonathan Fite Mar 2 at 13:54
  • I'd argue it's easy to write a stored procedure that disallows injection (both through parameters and proper security scoping) than it would be to have your app "safe" each statement and somehow execute under the proper context/permission level. – bbaird Mar 2 at 14:17
  • @bbaird You don't need to "safe" it up. It's pretty easy to manufacture the correct SQL in most client languages, each column gets a proper parameter, and pass through the necessary values as parameters – Charlieface Mar 2 at 23:59
  • @Charlieface The difference being one is hardened by the DBMS, the other is just sort of left to whomever has assumed they know how it should be done. – bbaird Mar 3 at 3:43
  • @bbaird I don't get it. Shouldn't the same devs who can write basic CRUD sprocs also be the ones to write client-side CRUD? Maybe I'm just not familiar with enterprise setups (very likely...) – Charlieface Mar 3 at 3:46
0

I would definitely avoid the complication of packing and unpacking XML, effectively turning a single input parameter into a DSL. It is something extra that can have bugs at either end (and a bug is in the calling code, you may end up having to prove it isn't in your DB-side code before that is accepted!).

Would extra parameters be acceptable for columns that can be set to NULL? If so then you could do something like:

CREATE OR ALTER PROCEDURE update_some_table (
      @recordId BIGINT
    , @col1 NVARCHAR(50) = NULL
    , @col2 NVARCHAR(50) = NULL
    , ...
    , @col1NULL BIT = 0
    , @col2NULL BIT = 0
    , ...
) AS
BEGIN
    UPDATE some_table
    SET    col1 = CASE 
                  WHEN @col1 IS NOT NULL THEN @col1 
                  WHEN @col1NULL = 1     THEN NULL
                  ELSE col1 -- no new value and no instruction to NULL, so keep present value
         , col1 = CASE 
                  WHEN @col2 IS NOT NULL THEN @col2
                  WHEN @col2NULL = 1     THEN NULL
                  ELSE col2 -- no new value and no instruction to NULL, so keep present value
         , ...
    WHERE  id = @recordId
END

Then you can call this with a mix of patterns:

EXEC update_some_table @recordId = 1, @col1 = 'new value'
EXEC update_some_table @recordId = 1, @col1 = 'new value', @col2 = 'another new value'
EXEC update_some_table @recordId = 1, @col2 = 'another new value'
EXEC update_some_table @recordId = 1, @col1NULL = 1, @col2 = 'another new value'
EXEC update_some_table @recordId = 1, @col2NULL = 1

In any case there is only one search for the row to update rather than potentially having one update statement per property (which might be part of why your solution is slower, along with the XML unpacking jigger-pokery).

I think (I've not checked) that this will result in the columns always looking to be updated if you inspect the relevant bits of COLUMNS_UPDATED inside a trigger that fires because of this procedure, which may be a consideration if you are using trigger based auditing.

Of course if all you are doing is forming an UPDATE statement from the provided parameters, you just let the application issue its own UPDATE statement (using constraints, triggers, and perhaps temporal tables, for any validation and auditing requirements)?

4
  • Brilliant insight! The one thing that was also driving me toward the xml sproc I created was the "setting nullables" problem when using optional parameters. Your solution solves that perfectly (albeit the extra params, but quite elegant). Before I accept your answer, can I also ask if you have generally seen the approach you mentioned used as commonplace in production environments? I hate to ask "best practices", but I am trying to figure out what the millions of DBAs out there are doing to solve this problem that seems common, but then again, there aren't many resources elaborating. – spencer741 Mar 2 at 11:02
  • 1
    @spencer741 - it is more a DB developer question than a DBA question. The question is "what benefit does the sproc interface layer actually give the developer?". If the answer is "none" or "not sure" then why not just let the app or its ORM layer access the table/view directly? That is probably the most common solution. Access control, validation, and auditing, can still be done at the DB level with other features. If you are trying to abstract away the base table structure, then Michael's suggestion of task based procedures with appropriate names is a better option, and not uncommon. – David Spillett Mar 2 at 11:18
  • I see no reason to construct such a complicated update statement. If one column changes, the DBMS will write out a new record anyway (assuming basic datatypes). The only case you should consider is if the user hasn't changed anything, but that is something that is probably best taken care of client-side. – Colin 't Hart Mar 2 at 11:23
  • @Colin'tHart - the intention of using CASE in an update like this is removing the need for the calling code to know all the other values, within the constraints of the design requiring changes to be made by stored procedure, not wanting individual calls for each column or set of columns, but wanting all (or many) columns to possibly be updated. Without those constraints, yes there are less complicated approaches. – David Spillett Mar 2 at 11:27
0

The "granular updates" question is orthogonal to the question of whether to put it in a stored procedure, as you can break it down both in client code or in a stored procedure.

I must say, I only ever see a point to breaking down update queries like this when dealing with very wide and/or heavily indexed tables, or where triggers are involved. The point being that a column in the UPDATE statement is considered an update even if no change happened.

In such a situation, I would actually tend towards dynamic SQL. Build up the query in your favourite client-side language, using proper parameterization, and execute. It is pretty trivial, for example, in C#.

// class User {

public async Task Update(string connectionString)
{
    SqlParameters[] params = GetColsToUpdate();
    var query = $@"
UPDATE User
SET
{string.Join("/r/n", params.Select(p => $"{p.ParameterName} = @{p.ParameterName},"))}
WHERE UserKey = @UserKey;
";
    using(var conn = new SqlConnection(connectionString))
    using(var comm = new SqlCommand(conn, query))
    {
        comm.Parameters.AddRange(params);
        comm.Parameters.Add("@UserKey", SqlDbType.BigInt).Value = UserKey;
        await conn.OpenAsync();
        await comm.ExecuteNonQueryAsync();
    }
}

It does have the side-effect of recompilation and adding more queries to the plan cache, but that is actually the point: we may want different plans for different updated columns. What we don't want, and won't get, is a different plan for every actual value being passed in.

If you want to restrict yourself to a stored procedure, you could do perhaps something like this:

CREATE PROCEDURE [dbo].[User_UPDATE]
(
    @ColumnsToUpdate OneColumnSysnameTableType,

    @UserKey BIGINT,
    @FirstName NVARCHAR(50) = NULL,
    @LastName NVARCHAR(50) = NULL,
    @Email NVARCHAR(50) = NULL,
    @BirthDate DATE = NULL
)
AS

DECLARE @sql nvarchar(max) = N'

UPDATE dbo.User
SET
' +
(
SELECT STRING_AGG(CAST(v.name + N' = ' + v.var AS nvarchar(max)), N'
    ')
FROM (VALUES
  ('UserKey',  '@UserKey'),
  ('FirstName','@FirstName'),
  ('LastName', '@LastName'),
  ('Email',    '@Email'),
  ('BirthDate','@BirthDate')
) v(name, var)
JOIN @ColumnsToUpdate c ON c.val = v.name
);

EXEC sp_executesql @sql,
  N'@UserKey BIGINT,
    @FirstName NVARCHAR(50),
    @LastName NVARCHAR(50),
    @Email NVARCHAR(50),
    @BirthDate DATE
',
    @UserKey = @UserKey,
    @FirstName = @FirstName ,
    @LastName = @LastName,
    @Email = @Email,
    @BirthDate = @BirthDate;


GO

As you can see, it is a lot of boiler-plate, and probably should itself be generated via a script.

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.