0

This was an old SQL Server 2008 Express database (five of them actually) that I just migrated to SQL Server 2019 Express. Everything seemed to be working fine until my crew got in and we were getting an error everywhere. Turns out we had RAISEERROR in the triggers, and even though my compatibility appears to be set to 2008 (100), we were still getting the error. So I upgraded to THROW. Now everything appears to work fine, but as I'm not a DBA, I'm worried that my upgrade my corrupt some data or leave orphans. Here's an example of one of the triggers:

USE [toddAPB]
GO
/****** Object:  Trigger [dbo].[T_tSaleLineItem_ITrig]    Script Date: 5/18/2021 1:32:55 PM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER TRIGGER [dbo].[T_tSaleLineItem_ITrig] ON [dbo].[tSaleLineItem] FOR INSERT AS
SET NOCOUNT ON
/* * PREVENT INSERTS IF NO MATCHING KEY IN 'tProduct' */
IF (SELECT COUNT(*) FROM inserted) !=
   (SELECT COUNT(*) FROM tProduct, inserted WHERE (tProduct.RecNumP = inserted.RecNumP))
    BEGIN
        ;THROW 44447, 'The record can''t be added or changed. Referential integrity rules require a related record in table ''tProduct''.',1;
        ROLLBACK TRANSACTION
    END

/* * PREVENT INSERTS IF NO MATCHING KEY IN 'tSale' */
IF (SELECT COUNT(*) FROM inserted) !=
   (SELECT COUNT(*) FROM tSale, inserted WHERE (tSale.RecNumS = inserted.RecNumS))
    BEGIN
        ;THROW 44447, 'The record can''t be added or changed. Referential integrity rules require a related record in table ''tSale''.',1;
        ROLLBACK TRANSACTION
    END

Do I need to put SET XACT_ABORT ON on every trigger (hundreds of them)? Should I? Do I still need ROLLBACK TRANSACTION after every THROW?

Before I adjusted to THROW, I was getting the "syntax error near 44447" error. the line previously looked more like this:

RAISERROR 44447 'The record can''t be added or changed. Referential integrity rules require a related record in table ''tProduct''.'

Thanks for the help.

Dale K
  • 25,246
  • 15
  • 42
  • 71
DauntlessRob
  • 765
  • 1
  • 7
  • 17
  • `SET XACT_ABORT ON` is the default for a trigger. How is this related to changing from `raiserror` to `throw`? Also `throw` rolls back by default. Also if you correctly terminate your statements with a `;` you don't need to add one in front of your `throw` (but SSMS will still show it as red because of a bug). And what was the actual error you were getting? `raiserror` is still supported? – Dale K May 18 '21 at 21:52
  • @DaleK From what I understand, it changes whether `THROW` will rollback the transaction or not. So I my assumption is that if it is off, it won't rollback the transaction and it will not execute the `ROLLBACK TRANSACTION` code either. – DauntlessRob May 18 '21 at 21:54
  • As I said `SET XACT_ABORT ON` is implicitly on for a trigger unless you turn it off. – Dale K May 18 '21 at 21:55
  • @DaleK The error I was getting was Incorrect SQL Syntax near 44447. Adjusting to `THROW` fixed it. – DauntlessRob May 18 '21 at 21:56
  • As we can't see your old code we can't assist with that. Not that its a bad thing to move to throw, but as you have worked out it does change the behaviour, so you might have been better off fixing the original error. – Dale K May 18 '21 at 21:58
  • @DaleK according to https://stackoverflow.com/questions/1150032/what-is-the-benefit-of-using-set-xact-abort-on-in-a-stored-procedure?rq=1, `XACT_ABORT` is not on by default.... which is it?! – DauntlessRob May 18 '21 at 22:08
  • 1
    @DauntlessRob I would believe the [official docs](https://learn.microsoft.com/en-us/sql/t-sql/statements/set-xact-abort-transact-sql?view=sql-server-ver15) over a SO question. – Dale K May 18 '21 at 22:10
  • With regard to the link above, it doesn't mention triggers. XACT_ABORT is off by default in other contexts, but not in a trigger. Consequently, I turn it on in every SP. – Dale K May 18 '21 at 22:22
  • 1
    You can configure xact_abort to be on at the server level, which would make more sense if you are setting it for every procedure? – Stu May 18 '21 at 22:29
  • 1
    I suggest implementing the appropriate foreign key constraints and dumping this trigger completely. – SMor May 18 '21 at 22:33
  • 1
    @Stu Thanks! I didn't know that. Just turned it on... waiting for the screams... – Dale K May 18 '21 at 22:44

1 Answers1

2

Well really you should be solving this issue with a Foreign Key Constraint (thanks Charlieface), however if thats not practical read on...

SET XACT_ABORT ON is the default for a trigger.

OFF is the default setting in a T-SQL statement, while ON is the default setting in a trigger.

You should also be using proper joins rather than implicit joins. I have laid it out how I would write a trigger, with correctly terminated statements.

ALTER TRIGGER [dbo].[T_tSaleLineItem_ITrig]
ON [dbo].[tSaleLineItem]
FOR INSERT AS
BEGIN
    SET NOCOUNT ON;

    /* PREVENT INSERTS IF NO MATCHING KEY IN 'tProduct' */
    IF (SELECT COUNT(*) FROM inserted) != (SELECT COUNT(*) FROM tProduct P INNER JOIN inserted I ON P.RecNumP = I.RecNumP)
    BEGIN
        THROW 44447, 'The record can''t be added or changed. Referential integrity rules require a related record in table ''tProduct''.', 1;
    END;

    /* PREVENT INSERTS IF NO MATCHING KEY IN 'tSale' */
    IF (SELECT COUNT(*) FROM inserted) != (SELECT COUNT(*) FROM tSale S INNER JOIN inserted I ON S.RecNumS = I.RecNumS)
    BEGIN
        THROW 44447, 'The record can''t be added or changed. Referential integrity rules require a related record in table ''tSale''.', 1;
    END;
END;

You could of course continue to use RAISERROR but it appears the syntax has changed since you originally wrote it - so you would have to correct that. Since you are having to modify them all anyway, moving to THROW seems appropriate.

I find your integrity check interesting, I would have written it as follows due to how I approach the logic, but I don't think its any better.

IF EXISTS (
    SELECT 1
    FROM Inserted I
    WHERE NOT EXISTS (SELECT 1 FROM tProduct P WHERE P.RecNumP = I.RecNumP)
)
Dale K
  • 25,246
  • 15
  • 42
  • 71