The Silent Bug I Find in Most Triggers

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:

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:

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:

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:

Previous Post
Are your CPU-intensive queries running slowly? Check your VM type.
Next Post
Developers: Azure SQL DB Serverless can save you money.

28 Comments. Leave new

  • If I saw someone write a trigger like that I think I would pass out lol.

    Reply
  • Denis Gobo
    May 6, 2019 9:22 am

    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.

    Reply
  • Kenneth Fisher
    May 6, 2019 9:34 am

    Pegged it from the title. Probably the most irritating, easiest to fix and most common trigger bug I see.

    Reply
  • ken j ambrose
    May 6, 2019 9:52 am

    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.

    Reply
    • 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.

      Reply
    • nobody checked to make sure that it was working or checked for specific failure cases?

      Reply
      • 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…

        Reply
  • Thank you so much! What a great reminder to think set-based. You guys have helped me learn so much!

    Reply
  • 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.

    Reply
  • Wojciech Sawicki
    May 6, 2019 10:29 pm

    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

    Reply
    • Ray Herring
      May 23, 2019 1:59 pm

      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?

      Reply
  • Luc Van der Veken
    May 6, 2019 11:18 pm

    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.

    Reply
    • Luc Van der Veken
      May 6, 2019 11:25 pm

      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.

      Reply
  • I wonder you call it a bug. I thing it’s an expected behavior. Other – normal -tables would behave the same.

    Reply
  • Wojciech Sawicki
    May 7, 2019 12:27 am

    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

    Reply
    • Wojciech Sawicki
      May 7, 2019 12:30 am

      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

      Reply
  • 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

    Reply
  • Shaun Austin
    May 7, 2019 2:28 am

    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!

    Reply
  • […] Brent Ozar points out a common problem with trigger design: […]

    Reply
  • Thanks a lot, very good article.

    Reply
  • Bruce Cassidy
    May 17, 2019 2:37 pm

    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.

    Reply
  • ken j ambrose
    May 17, 2019 2:57 pm

    “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).

    Reply
  • […] The Silent Bug I Find in Most Triggers […]

    Reply
  • 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)

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *

Fill out this field
Fill out this field
Please enter a valid email address.

Menu
{"cart_token":"","hash":"","cart_data":""}