SQL Server Stored Procedure Best Practices

2020-09-18 · mssql, t-sql, stored procedures, performance

The same few mistakes keep getting me paged: a procedure that's fast for one customer and times out for another, one that swallows errors, one that leaves a transaction open when it fails. None of it needs advanced tuning, only habits applied every time.

This is my checklist when I review procedures on SQL Server 2016 through 2019, with the reasons.

The header I paste into everything

CREATE OR ALTER PROCEDURE dbo.usp_GetOrdersByCustomer
    @CustomerID int,
    @FromDate date = NULL
AS
BEGIN
    SET NOCOUNT ON;
    SET XACT_ABORT ON;
    ...
END;

CREATE OR ALTER (2016 SP1 and later) makes deployments idempotent and keeps permissions, unlike DROP followed by CREATE.
SET NOCOUNT ON suppresses the "n rows affected" messages. Some client libraries treat each of them as a result set, and it saves a network round trip per statement.
SET XACT_ABORT ON makes any run-time error roll back the whole transaction and abort the batch, instead of leaving a half-finished transaction open when the caller's connection drops. It also makes TRY...CATCH behaviour predictable.

Name procedures with a schema and a prefix other than sp_. sp_ tells the engine to look in master first, which costs a cache miss per call and occasionally collides with a system procedure.

Schema-qualify everything

SELECT o.OrderID, o.OrderDate FROM dbo.Orders AS o WHERE o.CustomerID = @CustomerID;

Not FROM Orders. Without the schema, name resolution depends on the caller's default schema, which means an extra lookup at runtime and, worse, a separate plan per default schema because the engine cannot be sure the references are the same. It is also how a user with a table called Orders in their own schema silently changes what a procedure does.

Parameter sniffing

The plan compiled for the first set of parameter values is reused for every subsequent call. A procedure compiled for a customer with 5 orders gets a nested loops plan that then runs for a customer with 5 million orders. Options, in the order I reach for them:

-- 1. Statement-level recompile: a fresh plan every call; fine for procedures that run a few times a minute
SELECT ... FROM dbo.Orders WHERE CustomerID = @CustomerID
OPTION (RECOMPILE);

-- 2. Optimize for a typical value or for the average
SELECT ... FROM dbo.Orders WHERE CustomerID = @CustomerID
OPTION (OPTIMIZE FOR (@CustomerID UNKNOWN));

-- 3. Branch to separate procedures for known skewed cases
IF @CustomerID IN (SELECT CustomerID FROM dbo.LargeCustomers)
    EXEC dbo.usp_GetOrdersByCustomer_Large @CustomerID, @FromDate;
ELSE
    EXEC dbo.usp_GetOrdersByCustomer_Small @CustomerID, @FromDate;

What I don't do: copy parameters into local variables to "disable sniffing". I did that early on. It works by accident (the optimizer uses density instead of the histogram) and produces mediocre plans for everyone. Query Store makes the whole problem visible; if you see two plans for one query with wildly different durations, this is why.

Catch-all queries with optional parameters

-- slow: one plan for every combination
WHERE (@CustomerID IS NULL OR o.CustomerID = @CustomerID)
  AND (@FromDate IS NULL OR o.OrderDate >= @FromDate)

This forces a scan because the optimizer must produce a plan valid for all NULL combinations. Either add OPTION (RECOMPILE) to that statement, which lets the optimizer eliminate the NULL branches at compile time, or build the query with sp_executesql and only the predicates that apply:

DECLARE @sql nvarchar(max) = N'SELECT o.OrderID, o.OrderDate FROM dbo.Orders AS o WHERE 1 = 1';
IF @CustomerID IS NOT NULL SET @sql += N' AND o.CustomerID = @CustomerID';
IF @FromDate IS NOT NULL SET @sql += N' AND o.OrderDate >= @FromDate';
EXEC sp_executesql @sql, N'@CustomerID int, @FromDate date', @CustomerID, @FromDate;

Always sp_executesql with parameters, never EXEC (@sql) with values concatenated in. The former gets plan reuse and is immune to injection; the latter gets neither.

Errors and transactions

BEGIN TRY
    BEGIN TRANSACTION;
    UPDATE dbo.Accounts SET Balance -= @Amount WHERE AccountID = @From;
    UPDATE dbo.Accounts SET Balance += @Amount WHERE AccountID = @To;
    COMMIT TRANSACTION;
END TRY
BEGIN CATCH
    IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION;
    THROW;
END CATCH;

THROW (2012+) re-raises the original error with its number, message and line, so the caller and the Agent job history see the real cause. RAISERROR with a generic message hides it. Do not return error codes in output parameters and hope the application checks them; let the error propagate. Keep transactions short: no WAITFOR, no calls to linked servers, no sending mail inside a transaction.

Return only what's needed

SELECT * couples the procedure to the table layout; a new column breaks the client's mapping or adds bandwidth silently. List columns. Return one result set per procedure unless the caller genuinely handles multiple. Avoid using the procedure's return value for data; it is an int meant for status.

Measuring afterwards

Look at the procedure's runtime from the DMVs rather than from a single manual execution:

SELECT OBJECT_NAME(ps.object_id) AS procedure_name, ps.execution_count,
       ps.total_worker_time / ps.execution_count / 1000 AS avg_cpu_ms,
       ps.total_elapsed_time / ps.execution_count / 1000 AS avg_elapsed_ms,
       ps.total_logical_reads / ps.execution_count AS avg_reads,
       ps.cached_time, ps.last_execution_time
FROM sys.dm_exec_procedure_stats ps
WHERE ps.database_id = DB_ID()
ORDER BY ps.total_worker_time DESC;

And use SET STATISTICS IO, TIME ON in SSMS while testing the extreme parameter values, not just the convenient ones.

Errors that point at bad habits

"Msg 266: Transaction count after EXECUTE indicates a mismatching number of BEGIN and COMMIT statements. Previous count = 0, current count = 1." A code path leaves a transaction open, usually a RETURN inside the TRY before the COMMIT. XACT_ABORT ON plus the CATCH pattern above prevents it.
"Msg 3930: The current transaction cannot be committed and cannot support operations that write to the log file. Roll back the transaction." The CATCH block tried to do work after a doomed transaction. Check XACT_STATE(); if it returns -1, roll back before doing anything else.
"Msg 2812: Could not find stored procedure 'usp_GetOrders'." Missing schema in the EXEC call and the caller's default schema is not dbo. Always EXEC dbo.usp_GetOrders.
A procedure is fast in SSMS and slow from the application with the same parameters. The two sessions have different SET options (typically ARITHABORT), so they get different cached plans, one of them sniffed badly. Compare with sys.dm_exec_plan_attributes and apply the fixes from the sniffing section.

None of this is clever. It's just boring consistency, and boring is what I want from a procedure at 3 a.m.