I don’t have a problem with triggers. They get the job done when you need to implement business logic in a hurry, and you’re not allowed to change the application. As long as you keep the number of statements to a minimum (say, 2-3), and don’t try to do something really slow like fire up a cursor, triggers can be an efficient way to solve hard problems quickly.
However, most triggers I run across have a really, really dangerous bug.
Let’s say we want to add a trigger on the Stack Overflow database‘s Users table. Whenever someone’s Reputation is over 1,000 points, we’re going to set their AboutMe to declare that they’re famous:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
CREATE OR ALTER TRIGGER trUserIsFamous ON dbo.Users AFTER UPDATE AS BEGIN DECLARE @Id INT, @Reputation INT; SELECT @Id = Id, @Reputation = Reputation FROM INSERTED; IF @Reputation > 1000 UPDATE dbo.Users SET AboutMe = 'FAMOUS FOR A HIGH REPUTATION!' FROM dbo.Users u WHERE Id = @Id; END GO |
That trigger assumes only one row will be in the INSERTED table, which would be true – as long as we only update one row at a time. But what if a query updates multiple rows in a single transaction?
Let’s say we want to award a free reputation point to everyone in Brazil. Here are some of the people we’re going to affect:
|
1 2 3 4 |
SELECT TOP 20 * FROM dbo.Users WHERE Location = 'Brazil' ORDER BY Reputation DESC; |
There are lots of people involved:
Many of them have reputations over 1,000 points, so those folks are all going to be set to famous, right? Let’s see what happens when we run the update:
And then check to see their new famous AboutMe:
Wait…something went wrong. There are users with a reputation over 1,000, but don’t have “FAMOUS” in their AboutMe. A bunch of ’em simply got ignored.
That’s the bug.
When you declare variables and set them using one row from the INSERTED or DELETED virtual table, you have no idea which row you’re going to get. Even worse, sometimes this trigger will update one row, and sometimes it won’t – because it might happen to grab a row with a reputation under 1,000!
Here’s a better way to write that trigger.
Don’t use variables – instead, think set-based:
|
1 2 3 4 5 6 7 8 9 10 11 |
CREATE OR ALTER TRIGGER trUserIsFamous ON dbo.Users AFTER UPDATE AS BEGIN UPDATE dbo.Users SET AboutMe = 'FAMOUS FOR A HIGH REPUTATION!' FROM dbo.Users u INNER JOIN INSERTED i ON u.Id = i.Id WHERE i.Reputation > 1000; END GO |
In that version, we’re simply joining with the INSERTED table, thereby ensuring that we get all of the updated rows. We operate on ’em all at once, too – think set-based, not a cursor.
Then, when we update multiple rows at once:
The right ones are affected:
tl;dr: when you see the variables being set with the contents of a single row from the DELETED and INSERTED tables, you need to raise an urgent alarm because that trigger probably produces incorrect results when multiple rows are deleted/updated/inserted in a single statement.
“Hey, that’s not SSMS in those pictures.”
Yeah, you caught me: I’m using Azure Data Studio on my Mac, and building SQL notebooks with it. Here’s a quick runthrough of how it works:
How to get started:
- Official Azure Data Studio download, and Github repo with insiders (frequent) builds
- Support: Github issues, #AzureDataStudio channel in SQL Server community Slack (free instant invite)
- Markdown text formatting cheat sheet
- Download the notebook for today’s post, and notebook FAQ for my own stuff
- Register for my Developer’s SQL Server Recipe Book workshop at SQL Intersection in Orlando, where I’ll be sharing my favorite T-SQL starting points for different business requirements like pagination, row-level security, building and maintaining denormalized reporting tables, and passing lists of parameters into procedures. Attendees will get SQL notebooks with the code so they can follow along with me in Azure Data Studio, plus their choice of a Recorded Class Season Pass or the Consultant Toolkit.
34 Comments. Leave new
If I saw someone write a trigger like that I think I would pass out lol.
Chad – yeah, they come up a lot, surprisingly.
Starting to read that.. I assumed you were going to write about the following…. that is the one I see most.. but at least it blows up and doesn’t silently update a bunch of rows 🙂
Server: Msg 512, Level 16, State 1, Procedure trUserIsFamous, Line 11
Subquery returned more than 1 value. This is not permitted when the subquery follows =, !=, <, , >= or when the subquery is used as an expression.
The statement has been terminated.
Denis – hahaha, that’s a good one too.
Pegged it from the title. Probably the most irritating, easiest to fix and most common trigger bug I see.
great, great point about triggers! This should really be shouted from the rooftops! I too have seen this defect in pretty much every legacy trigger I have ever encountered.
The last time I saw this, my boss had written a trigger to generate line items for invoices, and this defect was present in the code he wrote. Needless to say, it cost the company millions in lost invoice line items over about a 10 year period, and also needless to say, when I pointed out the issue and corrected it, HIS bosses were never informed.
Yeah, it’s pretty ugly. I kept swearing I was going to write a blog post about it, and this last time I saw it, I just stopped what I was doing and wrote it.
nobody checked to make sure that it was working or checked for specific failure cases?
correct. nobody thought to implement validation of processing for this and most other data processing at this organization (a top 50 law firm).
The code and processing was managed by accountants, not by programmers…
Of course,I implemented such data processing validation for this and many other processes that I created and/or fixed once I was involved in them. Lay firm- rolling in dough anyways…
Thank you so much! What a great reminder to think set-based. You guys have helped me learn so much!
Awesome, glad you enjoyed it!
We had a couple of tables like this. There were triggers designed to update a family name elsewhere, but all of the triggers were designed with single-row updates/inserts in mind. The logic was really complex so not something we could easily switch around short of using a cursor or some other complex logic.
I was also expecting the “subquery” issue as that was much more prevalent when I encountered triggers like this. The fix for most is easy enough with the “inserted/deleted” sets, though sometimes it gets more complex.
This is definitely a good reminder if people need to start writing triggers, though.
IMHO the trigger should also respond to zero rows.
at start something like that
select @numrows = @@rowcount
if @numrows = 0
return
however, you have to be careful
set nocount on
sets @@rowcount to 1
Why not Select @numRows = count(*) from Inserted.
Wouldn’t that protect you against a future “improvement” to the code that some unknowing newbie might insert?
I don’t quite understand why someone would use variables in a trigger like that in the first place, but I would also try to avoid updating rows that don’t need updating. There are lots of things that can change, and I would assume that rows where Reputation isn’t changed wouldn’t need updating. Something like
INNER JOIN INSERTED i ON u.Id = i.Id
INNER JOIN DELETED d ON u.Id = d.Id
WHERE i.Reputation > 1000 and d.Reputation <= 1000;
I have a trigger somewhat like this, but checking several columns for d.column i.column. It logs changes to the most important columns in a core table to a separate table, along with whodunnit and what application he was using, where, and when. I implemented it as a trigger to make it impossible for any application to ‘forget’ or circumvent the logging.
And it’s in a TRY block, so something that goes wrong with logging doesn’t roll back the original change.
The comment editor ate my ‘less than’ and ‘larger than’ symbols between d.column and i.column above. It must have looked too much line an empty html tag.
I wonder you call it a bug. I thing it’s an expected behavior. Other – normal -tables would behave the same.
And another problem 🙂
Msg 217, Level 16, State 1, Procedure trUserIsFamous, Line 6 [Batch Start Line 13]
Maximum stored procedure, function, trigger, or view nesting level exceeded (limit 32).
Maybee like that:
CREATE OR ALTER TRIGGER trUserIsFamous ON dbo.Users
AFTER UPDATE
AS
BEGIN
declare @numrows int
select @numrows = @@rowcount
if @numrows = 0
return –case update where 1=0
set nocount on –no message rows affected
if (select trigger_nestlevel( object_ID(‘trUserIsFamous’) ) ) > 1
return –udpate Users inside if recursive trigger enabled is set true
if update(Reputation) –no action for update Location for sample
begin
select *
FROM dbo.Users u
INNER JOIN INSERTED i ON u.Id = i.Id
WHERE i.Reputation > 1000 and
not exists(select * from DELETED d where d.Id=i.Id and d.Reputation > 1000);
UPDATE dbo.Users
SET AboutMe = ‘FAMOUS FOR A HIGH REPUTATION!’
FROM dbo.Users u
INNER JOIN INSERTED i ON u.Id = i.Id
WHERE i.Reputation > 1000 and
not exists(select * from DELETED d where d.Id=i.Id and d.Reputation > 1000);
–case update from 2000 to 3000
end
END
GO
of course select *
FROM dbo.Users u
INNER JOIN INSERTED i ON u.Id = i.Id
WHERE i.Reputation > 1000 and
not exists(select * from DELETED d where d.Id=i.Id and d.Reputation > 1000);
its for remove
I think Oracle triggers fire once for each row, some developers could assume the same for SQLServer and be very, very wrong.
OTOH, some guy at my company produced this juicy piece of code at the beginning of their triggers:
/*Verify if any of the columns has been Inserted, Updated, Deleted*/
IF NOT EXISTS (
SELECT TOP 1 1 FROM (
SELECT PK_Column FROM inserted
UNION
SELECT PK_Column FROM deleted
) UPD
LEFT JOIN inserted i on UPD.PK_Column = i.PK_Column
LEFT JOIN deleted d on UPD.PK_Column = d.PK_Column
WHERE
ISNULL(CONVERT(varchar(3000), i.[Column1]), Some_GUID) ISNULL(CONVERT(varchar(3000), d.[Column1]), Same_GUID)
OR ISNULL(CONVERT(varchar(3000), i.[Column2]), Same_GUID) ISNULL(CONVERT(varchar(3000), d.[Column2]), Same_GUID)
OR repeat for another 30 columns
)
RETURN
I had an issue recently with Triggers and replication. I had a table that was being replicated, and on the subscriber database the table had a trigger. When the publisher table was updated by a single row, the transaction would be replicated over, and the trigger would fire once. However, when the publisher table was updated by a 1,000 rows in a SINGLE statement, the trigger would fire 1,000 times on the subscriber!
When you understand how replication works, this behaviour makes sense. However it was still a surprise at the time so be wary!
[…] Brent Ozar points out a common problem with trigger design: […]
Thanks a lot, very good article.
Another common issue I have found with SQL Server triggers is the assumption that if the trigger code fails, the triggering code will also fail. This resulted in a whole heap of business logic failing without anyone noticing it (in the case I particularly remember). To be fair, this was SQL Server 2005; I don’t know if that is still the case.
This is one of the reasons why I am not a fan of implementing business logic within triggers.
“This is one of the reasons why I am not a fan of implementing business logic within triggers.”
100% agree! The logic of triggers is arcane and generally not well understood by businesses and many people who are writing sql in business environments.
Not to mention IMHO using triggers for business rules scatters usually very changeable business logic in too many layers. I support use of triggers only for enforcing integrity constraints if there was no other way although there almost always is (although the accountants who are the bosses do overrule me on this). I have a simple rule for determing the difference between changeable business rules and data integrity constraints but I won’t pollute this blog with it unless asked).
[…] The Silent Bug I Find in Most Triggers […]
Is this confined to triggers only?
It seems that source of the problem is with the deleted temp table,
which can be used outside of triggers as well.
To the best of my knowledge (and the rest of us I guess) ,updated = Delete (old value) and Insert (new value)
This blog post is about triggers. You might totally find these bugs in other places, but I’m just writing about triggers today. Thanks!
I’d also filter the set-based trigger to not update rows where the famous comment was already in there, and to ignore rows where the reputation wasn’t updated. Simple mod to the Where clause, but it would cut I/O and locks on rows where it’s not needed. Also, as a possible logic flaw, do you remove the famous comment if the reputation falls below the threshold, after going over in a previous transaction? Or leave it there? Need a business-rule call on that one.
In 2017 I wrote some articles in the Portuguese language about the most common errors in trigger programming and one of them is exactly what you quote. In the introduction there is the following sentence: “A forma como o SQL Server manipula os eventos que acionam gatilhos (trigger) pode se transformar em verdadeira armadilha para quem programa o procedimento de gatilho e desconhece os detalhes específicos do SQL Server, pois enquanto vários gerenciadores de banco de dados acionam o gatilho uma vez para cada linha (for each row), no SQL Server o gatilho é acionado uma vez para cada instrução DML (DELETE, INSERT, UPDATE).”, which can be translated into something like “The way SQL Server handles trigger events can become a real trap for those who program the trigger procedure and is unaware of the specifics of SQL Server, because as many database managers database executes the trigger SQL code once “for each row”, in SQL Server the trigger is fired once for each DML statement (DELETE, INSERT, UPDATE).”.
Armadilhas na programação de trigger
https://portosql.wordpress.com/2018/08/18/armadilhas-na-programacao-de-trigger/
Cool! I wish there was only one language that we could all read & understand – might make it easier for folks to discover these kinds of things along the way.
hah rookie mistake! I remember this one back in MSSQL in OS/2 1.3 when there was no type of identity or auto increment. We used triggers all the time to “get next” from a counter table (life sure was difficult then!). The rookies (myself included, this was the early 90s) would forget to join inserted and boom, null sequences generated. Accounting system goes down!
Very true, and I’ve seen articles written by experts ( that you know) who have that very bug.
Love to learn this kind of stuff! Thanks for sharing!