Code Review This Procedure

October 1, 2015

Filed under: SQLServerPedia Syndication,Technical Articles — Michael J. Swart @ 10:28 am

Earlier this week we encountered a web-site outage caused by a database procedure. I’m going to blog about that problem (in detail) in a post scheduled for next week. But before I publish that post, I want to know how subtle or obvious the problem was. It seems obvious to me now, but I have the benefit of hindsight. I wonder whether we could we have avoided this during the code review stage of development.

So before I publish the details, I invite you to do a code review of this procedure in the comment section.

The Procedure

Here’s the procedure. It suffers from the same thing that burned us this week. Do you see any issues with it? Tell me in the comment section.

CREATE PROCEDURE dbo.s_EVENT_TICKETS_GetOrCreate (
  @EventId BIGINT,
  @VenueSeatId BIGINT,
  @PurchaserId BIGINT,
  @PurchaseMethodId BIGINT
)
AS
  SET NOCOUNT ON;
 
  DECLARE @pid BIGINT;
  DECLARE @pmid BIGINT;
  DECLARE @dt DATETIME2
 
  SET TRANSACTION ISOLATION LEVEL SERIALIZABLE
  BEGIN TRAN
 
    -- If the row exists, grab details about the purchaser
    SELECT
      @pid = PurchaserId,
      @pmid = PurchaseMethodId,
      @dt = PurchaseDate
    FROM dbo.EVENT_TICKETS WITH (UPDLOCK)
    WHERE EventId = @EventId
      AND VenueSeatId = @VenueSeatId;
 
    IF ( @pid IS NULL )
    BEGIN
 
      -- The row doesn't exist, insert the row
      SET @dt = SYSDATETIME()
 
      INSERT INTO dbo.EVENT_TICKETS 
        ( EventId, VenueSeatId, PurchaserId, PurchaseMethodId, PurchaseDate )
      VALUES 
        ( @EventId, @VenueSeatId, @PurchaserId, @PurchaseMethodId, @dt );
 
      SELECT @pid = @PurchaserId,
             @pmid = @PurchaseMethodId;
    END
 
  COMMIT TRAN
 
  -- return details about the purchaser
  SELECT 
    @pid as PurchaserId,
    @pmid as PurchaseMethodId,
    @dt as PurchaseDate;

The Schema

Here’s a subset of the table definitions that this procedure is meant to use.

CREATE TABLE dbo.[EVENTS]
(
  EventId BIGINT NOT NULL IDENTITY,
  CONSTRAINT PK_EVENTS 
    PRIMARY KEY (EventId)
  -- etc...
);
 
CREATE TABLE dbo.VENUE_SEATS
(
  VenueSeatId BIGINT NOT NULL IDENTITY,
  CONSTRAINT PK_VENUE_SEATS 
    PRIMARY KEY (VenueSeatId)
  -- etc...
);
 
CREATE TABLE dbo.PURCHASERS
(
  PurchaserId BIGINT NOT NULL IDENTITY,
  CONSTRAINT PK_PURCHASERS 
    PRIMARY KEY (PurchaserId)
  -- etc...
);
 
CREATE TABLE dbo.PURCHASE_METHODS
(
  PurchaseMethodId BIGINT NOT NULL IDENTITY,
  CONSTRAINT PK_PURCHASE_METHODS 
    PRIMARY KEY (PurchaseMethodId)
  -- etc...
);
 
CREATE TABLE dbo.EVENT_TICKETS
(
  EventId BIGINT NOT NULL,
  VenueSeatId BIGINT NOT NULL,
  PurchaserId BIGINT NOT NULL,
  PurchaseMethodId BIGINT NOT NULL,
  PurchaseDate DATETIME2 NOT NULL,
  CONSTRAINT PK_EventId 
    PRIMARY KEY CLUSTERED (EventId, VenueSeatId),
  CONSTRAINT FK_EVENT_TICKETS_EVENTS
    FOREIGN KEY (EventId) REFERENCES dbo.[EVENTS] (EventId),
  CONSTRAINT FK_EVENT_TICKETS_VENUE_SEATS 
    FOREIGN KEY (VenueSeatId) REFERENCES dbo.VENUE_SEATS (VenueSeatId),
  CONSTRAINT FK_EVENT_TICKETS_PURCHASERS 
    FOREIGN KEY (PurchaserId) REFERENCES dbo.PURCHASERS (PurchaserId),
  CONSTRAINT FK_EVENT_TICKETS_PURCHASE_METHODS 
    FOREIGN KEY (PurchaseMethodId) REFERENCES dbo.PURCHASE_METHODS (PurchaseMethodId),
);
GO

-- Comments (30)

30 Comments »

I don’t think inserted rows get a legit value for @dt – I don’t see where it’s getting set for newly inserted rows. (Might be missing something obvious though.)

When setting the variables with a SELECT, you wanna use SELECT TOP 1 and have an ORDER BY. Otherwise, if you’ve got multiple matching rows, you’re getting a random one. In this case you’re okay because it’s the primary key, but I try to force people into that for discipline’s sake.

Comment by Brent Ozar — October 1, 2015 @ 10:43 am
Yeah, the more I look at it, the more I don’t think inserts will work. @dt doesn’t get set before the insert, and the field is not null, so inserts should fail.

Comment by Brent Ozar — October 1, 2015 @ 10:45 am
Oh of course NOW I see where it’s set. (sigh) The perils of doing code review on a phone.

Comment by Brent Ozar — October 1, 2015 @ 11:33 am
The serializable thing is really bothering me. Here’s how I’d rather see them do it: lose the transaction, lose the serializable, and just do:

WITH Incoming AS (SELECT (field list of incoming parameters)
INSERT dbo.EVENT_TICKETS (field list)
SELECT (field list)
FROM Incoming i
LEFT OUTER JOIN dbo.EVENT_TICKETS et ON (event and seat fields)
WHERE et.PurchaserID IS NULL

Then you can either use an output tablets check the results, or grab the purchaser and other fields out of the table again with the select. Should cut down on lock durations.

Comment by Brent Ozar — October 1, 2015 @ 11:47 am
If I were reviewing, I’d be asking these questions:

1 – What is the purpose of using SERIALIZABLE? As this is a “higher” isolation level, it could create locking/blocking issues.
2 – Why the explicit query hint for asserting an update lock? Query hints should be used with great care and I’d like to know why the dev doesn’t trust the engine to handle locking (especially considering it’s already at a higher isolation level).

Comment by Mike Fal — October 1, 2015 @ 12:01 pm
Since you’ve already suggested that the problem is obvious now, maybe I’ll suggest something that should probably be obvious during development:
I see an EVENT_TICKETS table with a bunch of FK constraints, but I don’t see anything in this procedures code to ensure those values actually exist in the referenced table. Are we to assume the app is handling that? I get where dev’s would assert that the app checking this is good enough, and checking would certainly make the code more verbose, but as reviewer it would certainly make me more comfortable to at least have a TRY CATCH there in case something goes wrong.

#myTwoCents

Comment by Bill — October 1, 2015 @ 12:20 pm
If a Null is passed in for @PurchaserId and/or @PurchaseMethodId while an error will occur, the SP will still return a values, which means that the App will have to know to throw them away.

Comment by Aaron Lowe — October 1, 2015 @ 12:20 pm
The problem is the combination of the SERIALIZABLE transaction level, and the transaction wrapping both a read and update to the same table. The combination of these things will mean deadlocks to your EVENT_TICKETS table.

Per the MS SQL Server documentation:
SERIALIZABLE
Specifies the following:

•Statements cannot read data that has been modified but not yet committed by other transactions.

•No other transactions can modify data that has been read by the current transaction until the current transaction completes.

•Other transactions cannot insert new rows with key values that would fall in the range of keys read by any statements in the current transaction until the current transaction completes.

That last bullet is what brought your system down, because you are reading and making an update to the same table, with keys that would likely have been read by other uncommitted transactions in progress.

I’m writing this quickly, so hopefully it’s clear. 🙂

Comment by Jeff — October 1, 2015 @ 12:32 pm
The Serializable thing bothers me too. That made me squirm a bit when I saw it.

I don’t have much practical experience with this isolation level, but my instinct is that there could be a potential impact on concurrency for inserts. I’m not sure *how much* of an impact it would have… but if nothing else seeing Brent’s comment about the same issue makes me feel that my squirminess is justified.

Comment by Andy — October 1, 2015 @ 12:34 pm
@Jeff – there’s so many interesting things about the “reading and making an update” – for example, note that the select is going by EventID and VenueSeatID. Say you didn’t have a covering non-clustered index to support that query – you would end up locking the whole table for your select, and it’d be dog slow. Since it’s serializable, other queries wouldn’t even be able to read the table while you scanned it. Ouch.

Comment by Brent Ozar — October 1, 2015 @ 1:05 pm
Isn’t the issue that the UPDLOCK and Serializable give you the *illusion* of preventing a concurrent insert if the row doesn’t yet exist in EVENT_TICKETS? The Select gets no row, ergo there is nothing to lock, ergo no range lock to prevent someone else succeeding at that insert and stealing your seat.

Comment by Ewald Cress — October 1, 2015 @ 1:25 pm
@Ewald, Actually, the range lock is taken to cover the row that isn’t there.
Range locks are taken and held something like what I describe in this post: https://michaeljswart.com/2010/04/transaction-phenomena-part-4-serializable-vs-snapshot/
When there’s nothing to lock, a RangeS-U lock is held on a key with resource_description “(ffffffffffff)”.

@All
Thanks everyone for the great comments. I’m trying to avoid engaging with these comments too much in order to not bias other readers. But thanks for the feedback. I will address all your feedback soon.

Comment by Michael J. Swart — October 1, 2015 @ 1:47 pm
Since your EVENT_TICKETS table has required foreign keys (evidenced by NOT NULL on all columns with foreign key references) the proc should be validating that the input parameter values exist in the foreign key tables before trying to insert into EVENT_TICKETS. If it doesn’t find any one of them it should throw an error and gracefully rollback the transaction and return from the proc.

The way it’s designed currently I think you could get an error on inserting to EVENT_TICKETS that would fail the proc and leave the transaction open.

Comment by Aaron — October 1, 2015 @ 4:40 pm
I would fire the kid that wrote this mess. Just start with the proc header, the design is wrong. Remember freshman software engineering? Coupling and cohesion? A module with strong cohesion (like a math function) does one and only one task, but in your world this thing can “get_or_create”; perhaps it also makes eggs?

We only use numeric data types for computation; identifiers are strings with validation rules. We never use IDENTITY for anything in RDBMS; why is the physical insertion attempt count part of a logical data model? Because it looks like the old Unix/mag tape sequentail record numbers? Not a valid reason in the year 2015 and in RDBMS.

I think you want the purchase method, not the identifier for the method. Ans certainly not a BIGINT! I would guess {‘cash’, ‘cheque’, ‘Mastercard’, ‘Amex’, etc.} instead 12390764567. And why do you want nanoseconds; did you mean to use DATETIME2(0)?

What are some of the first principles of any declarative language?
1) No local variables. But you filled this with 1960’s FORTRAN I local registers in a bad disguise. And the names are awful! Why do you that “@dt” is clear and precise? NO! It is both cryptic and generic, the worst of software engineer!
2) No control flow. That means no loops or if-then control logic.

The one I like was the spaces around parentheses. We did that when I was a FORTRAN programmer! We put them on their own punch card, so we could slip in new cards and re-arrange the parameter lists (each parameter had its own card, too). It gave me flashbacks 🙂

We do not use Pascal case today. ISO standards can be case sensitive or insensitive, so the data elements will not port the same way from product to product. The underscore and ISO-11179 naming rules are preferred.

I know you are trying to demonstrate the transaction levels, but I just hate to look at crappy SQL. Especially when I can see that it is really old FORTRAN and poor design 🙁
Quick stab at a skeleton:

CREATE PROCEDURE Create_Event_Ticket
(@in_event_id CHAR(10), –TicketMaster codes?
@in_venue_seat_nbr CHAR(4), –TicketMaster codes?
@in_purchase_id CHAR(16), -– credit card size
@in_purchase_method CHAR(10)) — list in CHECK() constraint
AS
INSERT INTO Event_tickets
(event_id, venue_seat_nbr, purchase_id, purchase_method, purchase_date)
SELECT @in_event_id, @venue_seat_nbr, @in_purchase_id, @in_purchase_method, CURRENT_TIMESTAMP
FROM Event_Tickets
WHERE event_id = @in_event_id
AND venue_seat_nbr = @in_venue_seat_nbr;

Which will insert an empty set (i.e. do nothing) that can be caught at the invoking tier.

CREATE PROCEDURE Find_Event_Ticket_Purchaser
(@in_event_id CHAR(10),
@in_venue_seat_nbr CHAR(4))
AS
SELECT event_id, venue_seat_nbr, purchase_id, purchase_method, purchase_date
FROM Event_Tickets
WHERE event_id = @in_event_id
AND venue_seat_nbr = @in_venue_seat_nbr;

Comment by Joe Celko — October 3, 2015 @ 1:27 pm
TRANSACTION ISOLATION LEVEL SERIALIZABLE is the biggest issue here. In a multi-threaded web application this is equivalent to having a synchronized block in the code. This can cause deadlock/blocking issue.
I would remove this. Can use a SQL merge statement with output clause to rewrite this? Basically if the the row exists return the row if not insert and return the row.
Also I would turn on RCSI at the database level and allow only row level lock for the indices.

Comment by Sati Putra — October 3, 2015 @ 7:40 pm
Guess from a newbie, : there is no error handling code, and no input validation for parameters of proc.
So if the input purchaserid is NULL, it will proceed on the INSERT but fail due to NOT NULL cirteria.
However, as INSERT is part of the transaction, (and have serializable means the lock becoming table lock ?) the lock is not released due to COMMIT TRANSACTION never executed, and thus would block subsequent processing on the concerned table (session pooling on web app ?) until that session is clear up when next user reuse that session again

Comment by Eric — October 4, 2015 @ 11:05 pm
I’m going to guess that you started getting lots of deadlocks because of the SERIALIZABLE isolation level. Because that isolation level places range locks after doing a read on everything it read, I’m guessing that in a multi-threaded environment like a website, you ended up with a number of requests that came in at around the same time for the an EventID, VenueSeatID that were in the same range. They all put locks on the same range, and since in SERIALIZABLE mode no other transaction can modify data that has been read by this one, that would make all the threads block each other when they get to the insert.

Comment by Dan Brown — October 5, 2015 @ 9:57 am
@Eric (of Comment #16)
@Aaron (of Comment #13)

I think you’re on to something regarding what’s wrong with the procedure. I’d like to hear more about what an improvement could look like.

Comment by Michael J. Swart — October 5, 2015 @ 1:48 pm
SERIALIZABLE will take range locks and an insert on a table with an identity column is bad news because the range is the previous key to the end of the table, ie. the inserts become single threaded, throw in some relationships and you are probably heading for deadlocks. I wouldn’t look further than that normally. Note that when I saw this happen before SERIALIZABLE wasn’t explicitly specified, it was some default from .NET

Comment by Colin Allen — October 5, 2015 @ 5:41 pm
Is it better to check if the @eventid instead of @pid? To trigger an insert the @eventid should be null but that’s contradicting to the insert statement.

Comment by Alan — October 5, 2015 @ 6:51 pm
Sorry forget about my previous post, didn’t pay attention to the event_ticket table primary key. The stored proc should do basic validation for creating tickets, things like checking the purchaser table for purchaserid; purchase_method for PurchaseMethodId; event table for eventid and venue_seats for VenueSeatId.

Comment by Alan — October 5, 2015 @ 7:41 pm
A Trial to rewrite with INSERT statement first, before the SELECT statement
so as to avoid the required retained lock with SERIALIZABLE transaction level to handle race condition.

(Below assume it is acceptable to use SQL default error handling behavior for any not validated input condition.
Otherwise, there is need to add parameters to allow Stored proc to return error during the validation stage for graceful error handling)

P.S.

I see from web search another Aaron “https://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/”
did mention about the danger of MERGE, so, I use the INSERT statement as is.

I may not have enough knowledge to be sure if the single INSERT statement is atomic enough to overcome the race condition as he point out there.

CREATE PROCEDURE dbo.s_EVENT_TICKETS_GetOrCreate (
@EventId BIGINT,
@VenueSeatId BIGINT,
@PurchaserId BIGINT,
@PurchaseMethodId BIGINT
)
AS
SET NOCOUNT ON;

DECLARE @dt DATETIME2

SET @dt = SYSDATETIME()

INSERT INTO dbo.EVENT_TICKETS
( EventId, VenueSeatId, PurchaserId, PurchaseMethodId, PurchaseDate )
VALUES
( @EventId, @VenueSeatId, @PurchaserId, @PurchaseMethodId, @dt )
WHERE NO EXIST (
SELECT EventId, VenueSeatID
FROM dbo.EVENT_TICKETS
WHERE (EventId = @EventId AND VenueSeatId = @VenueSeatId) )

SELECT
PurchaserId,
PurchaseMethodId,
PurchaseDate
FROM dbo.EVENT_TICKETS
WHERE EventId = @EventId AND VenueSeatId = @VenueSeatId;

Comment by Eric — October 6, 2015 @ 12:23 am
Sorry ! Syntax error after trial, revised :

CREATE PROCEDURE dbo.s_EVENT_TICKETS_GetOrCreate (
@EventId BIGINT,
@VenueSeatId BIGINT,
@PurchaserId BIGINT,
@PurchaseMethodId BIGINT
)
AS
SET NOCOUNT ON;

DECLARE @dt DATETIME2

SET @dt = SYSDATETIME()

INSERT INTO dbo.EVENT_TICKETS
(EventId, VenueSeatId, PurchaserId, PurchaseMethodId, PurchaseDate )
SELECT
@EventId, @VenueSeatId, @PurchaserId, @PurchaseMethodId, @dt
WHERE NOT EXISTS (
SELECT EventId, VenueSeatID
FROM dbo.EVENT_TICKETS
WHERE (EventId = @EventId AND VenueSeatId = @VenueSeatId) )

SELECT
PurchaserId,
PurchaseMethodId,
PurchaseDate
FROM dbo.EVENT_TICKETS
WHERE EventId = @EventId AND VenueSeatId = @VenueSeatId;

Comment by Eric — October 6, 2015 @ 2:28 am
I often can’t disentangle what the actual impact of various isolation levels would be so I go a different route; which is to create a quick and dirty load test (I just whipped this up, please don’t re-use it, it could easily have bugs).

Create Database Test
Go
Alter Database Test Set Recovery Simple
Go
Use Test
Go
— Run provided creation script
— Run provided table creation script

— Reset the tables between tests
Delete From Event_Tickets
Delete From Events
Delete From Venue_Seats
Delete From Purchasers
Delete From Purchase_Methods
Go

Dbcc Checkident(‘Events’, Reseed, 0)
Dbcc Checkident(‘Venue_Seats’, Reseed, 0)
Dbcc Checkident(‘Purchasers’, Reseed, 0)
Dbcc Checkident(‘Purchase_Methods’, Reseed, 0)
Go

— Insert some defaults
Insert Events Default Values;
Go 100
Insert Venue_Seats Default Values;
Go 100
Insert Purchasers Default Values;
Go 100
Insert Purchase_Methods Default Values;
Go 100

—-

Then I ran this script:

Set-StrictMode -Version Latest
$ErrorActionPreference = “Stop”

# Spin up 20 processes
for ($i = 1; $i -le 20; $i++) {
Start-Job -ScriptBlock {
Set-StrictMode -Version Latest
$ErrorActionPreference = “Stop”

Import-Module SQLPS -DisableNameChecking
$serverInstance = “localhost”
$databaseName = “Test”
$records = 100

# Run each thread 1000 times and abort on the first error; Invoke-SqlCmd is known for being picky about what it aborts on though
try {
for ($i = 1; $i -le 1000; $i++) {
$a = Get-Random -Minimum 1 -Maximum ($records + 1)
$b = Get-Random -Minimum 1 -Maximum ($records + 1)
$c = Get-Random -Minimum 1 -Maximum ($records + 1)
$d = Get-Random -Minimum 1 -Maximum ($records + 1)

$query = “Exec dbo.s_EVENT_TICKETS_GetOrCreate $a, $b, $c, $d”

Invoke-SqlCmd -ServerInstance $serverInstance -Database $databaseName -Query $query -IncludeSqlUserErrors -AbortOnError
}
} catch {
“Error on $a $b $c $d”
Write-Error $_
}
}
}

# Watch what happens for a while
while ((Get-Job -State “Running”)) {
Write-Host “.” -NoNewLine

if (Get-Job -State Failed) {
Get-Job | Stop-Job
Write-Host “Something failed, ready to investigate”
break
}

Start-Sleep -Seconds 1
}

# Set a breakpoint here, for clean up once you’ve investigated the root cause
Get-Job | Stop-Job -PassThru | Remove-Job

—

So I had loaded the foreign key tables with 100 entries then hammered the stored procedure 1000 times each across 20 concurrent threads with random figures 1..100 for each of the parameters. Unfortunately none of them ever failed.

I also re-ran it focusing on much fewer entries so there’d be more chance of it hitting some conflicts and it still never failed.

Whatever is causing the problem is outside of this stored procedure and provided schema; it could still be caused by differences between the simplified schema and production (i.e. longer rows) or the other queries that are running concurrently.

Comment by Cody — October 6, 2015 @ 6:04 am
if @PurchaserId is null, it will cause an error as you are trying to insert a null into a non-nullable column

Comment by morshed — October 6, 2015 @ 7:04 am
Can the insert be wrapped in a try/catch block to allow for a rollback upon insert failure due to FK violations?

The procedure itself seems a bit odd for a ticketing system; people usually purchase more than 1 ticket but this procedure seems to account for a single seat at an event at a time. Curious how the framework handles someone wanting to purchase say a 4 ticket block. Does this system have the “available seats” concept so that sold and “in progress” tickets are marked unavailable when the user is trying to select tickets to purchase?

Comment by Tim Schulze — October 6, 2015 @ 10:14 am
@Tim, Yes it can. A try/catch block would allow for a rollback which would help immensely

Comment by Michael J. Swart — October 6, 2015 @ 12:03 pm
[…] week, I invited readers to have a look at a procedure in a post called Code Review This Procedure. I was looking for anyone to suggest turning on XACT_ABORT as a best practice. It’s a best […]

Pingback by Don’t Abandon Your Transactions | Michael J. Swart — October 6, 2015 @ 12:00 pm
Thanks all for reviewing the procedure. Who knew that a 50 line procedure could generate so much discussion. I just posted more about this procedure in a new post called Don’t Abandon Your Transactions.

Comment by Michael J. Swart — October 6, 2015 @ 12:05 pm
Just in case fellow readers not continue to read on Michael’s new post “Don’t Adandon …”, Michael has kindly point out my trial of code for posting made on Oct 6 has fault and could fail due to concurrency issue.

Thanks a lot Michael !

Comment by Eric — October 9, 2015 @ 3:23 am

RSS feed for comments on this post. TrackBack URL

Michael J. Swart

October 1, 2015