37

I hope this is a simple enough question for any SQL people out there...

We have a table which hold system configuration data, and this is tied to a history table via triggers so we can track who changed what, and when.

I have a requirement to add another value in to this table, but it is one that will change frequently from code, and has a requirement that we don't track it's history (we don't want to clog the table with many thousands of updates per day.

At present, our trigger is a little like this...

CREATE TRIGGER 
    [dbo].[SystemParameterInsertUpdate]
ON 
    [dbo].[SystemParameter]
FOR INSERT, UPDATE 
AS
  BEGIN
    SET NOCOUNT ON
      INSERT INTO SystemParameterHistory 
      (
        Attribute,
        ParameterValue,
        ParameterDescription,
        ChangeDate
      )
    SELECT
      Attribute,
      ParameterValue,
      ParameterDescription,
      ChangeDate
    FROM Inserted AS I
END

I'd like to be able to add some logic to stop it creating the record if an Attribute colum value is prefixed with a specific string (e.g. "NoHist_")

Given that I have almost no experience working with triggers, I was wondering how it would be best to implement this... I have tried a where clause like the following

where I.Attribute NOT LIKE 'NoHist_%'

but it doesn't seem to work. The value is still copied over into the history table.

Any help you could offer would be appreciated.


OK - as predicted by Cade Roux, this fail spectacularly on multiple updates. I'm going to have to take a new approach to this. Does anyone have any other suggestions, please?


Guys - Please educate me here... Why would LEFT() be preferable to LIKE in this scenario? I know I've accepted the answer, but I'd like to know for my own education.

Brian Webster
  • 30,033
  • 48
  • 152
  • 225
ZombieSheep
  • 29,603
  • 12
  • 67
  • 114

7 Answers7

50

Given that a WHERE clause did not work, maybe this will:

CREATE TRIGGER 
    [dbo].[SystemParameterInsertUpdate]
ON 
    [dbo].[SystemParameter]
FOR INSERT, UPDATE 
AS
  BEGIN
    SET NOCOUNT ON

      If (SELECT Attribute FROM INSERTED) LIKE 'NoHist_%'
      Begin
          Return
      End

      INSERT INTO SystemParameterHistory 
      (
        Attribute,
        ParameterValue,
        ParameterDescription,
        ChangeDate
      )
    SELECT
      Attribute,
      ParameterValue,
      ParameterDescription,
      ChangeDate
    FROM Inserted AS I
END
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
  • 5
    with this trigger you're only handling one row. what happens if someone inserts/updates 2 or more rows at once? data will be wrong. – Mladen Prajdic Nov 11 '08 at 17:13
  • 3
    _ is also a wildcard, so LIKE 'NoHist_%' will also match 'NoHistX' – Cade Roux Nov 11 '08 at 20:27
  • Edit queue is full, some please update the syntax. should be LIKE 'NoHist[_]' to take the underscore. Left and Like makes no difference anymore, in older version of Sql Sever, sql was not always good at stop searching after 6 characters. if you run both query's execution plan, it will be identical. – e-Fungus May 03 '21 at 20:48
  • The _ was left alone on purpose. Even knowing it would match any character, I'd leave it like this in real code because it's easier to read and you're extremely unlikely to get false positives. – Joel Coehoorn Aug 08 '23 at 18:56
7

How about this?

CREATE TRIGGER 
[dbo].[SystemParameterInsertUpdate]
ON 
[dbo].[SystemParameter]
FOR INSERT, UPDATE 
AS
BEGIN
SET NOCOUNT ON
  IF (LEFT((SELECT Attribute FROM INSERTED), 7) <> 'NoHist_') 
  BEGIN
      INSERT INTO SystemParameterHistory 
      (
        Attribute,
        ParameterValue,
        ParameterDescription,
        ChangeDate
      )
    SELECT
      Attribute,
      ParameterValue,
      ParameterDescription,
      ChangeDate
   FROM Inserted AS I
END
END
Matty
  • 1,973
  • 4
  • 25
  • 29
  • 1
    I like LEFT() better than my own LIKE() suggestion for this. – Joel Coehoorn Nov 11 '08 at 14:16
  • I don't know enough about the what's going on internally with SQL server to know which approach is better from a performance perspective. I actually like yours a bit better because you exit immediately rather than wrapping the entire INSERT in a conditional statement. – Matty Nov 11 '08 at 14:34
  • 1
    This one will also fail if more than one record is being inserted or updated. – Cade Roux Nov 11 '08 at 19:01
  • 1
    Good point Cade. For this to work properly it would have to iterate over the insertions/updates. This would add unnecessary overhead which could be avoided using your code snippet. – Matty Nov 11 '08 at 19:15
6

The _ character is also a wildcard, BTW, but I'm not sure why this wasn't working for you:

CREATE TRIGGER 
    [dbo].[SystemParameterInsertUpdate]
ON 
    [dbo].[SystemParameter]
FOR INSERT, UPDATE 
AS
  BEGIN
    SET NOCOUNT ON
      INSERT INTO SystemParameterHistory 
      (
        Attribute,
        ParameterValue,
        ParameterDescription,
        ChangeDate
      )
    SELECT
      I.Attribute,
      I.ParameterValue,
      I.ParameterDescription,
      I.ChangeDate
    FROM Inserted AS I
    WHERE I.Attribute NOT LIKE 'NoHist[_]%'
END
Cade Roux
  • 88,164
  • 40
  • 182
  • 265
5

Your where clause should have worked. I am at a loss as to why it didn't. Let me show you how I would have figured out the problem with the where clause as it might help you for the future.

When I create triggers, I start at the query window by creating a temp table called #inserted (and or #deleted) with all the columns of the table. Then I popultae it with typical values (Always multiple records and I try to hit the test cases in the values)

Then I write my triggers logic and I can test without it actually being in a trigger. In a case like your where clause not doing what was expected, I could easily test by commenting out the insert to see what the select was returning. I would then probably be easily able to see what the problem was. I assure you that where clasues do work in triggers if they are written correctly.

Once I know that the code works properly for all the cases, I global replace #inserted with inserted and add the create trigger code around it and voila, a tested trigger.

AS I said in a comment, I have a concern that the solution you picked will not work properly in a multiple record insert or update. Triggers should always be written to account for that as you cannot predict if and when they will happen (and they do happen eventually to pretty much every table.)

HLGEM
  • 94,695
  • 15
  • 113
  • 186
  • When you say you have a concern, could you elaborate, please? I can't see another comment from you in this thread... If you mean there is a possibility that non-'NoHist_' records are dropped incorrectly, could you proposed a solution? – ZombieSheep Nov 11 '08 at 14:30
  • Yes, that's why I think it is worth it to get the where clause to work properly. If you look at the solution you took you will see that it returns if it finds a record with NoHist, therefore I suspect it wouldn't process any records in the same batch that do have a different value. Test to confirm. – HLGEM Nov 11 '08 at 15:16
  • Thanks HLGEM, I'll bear that in mind when putting some load on the app. – ZombieSheep Nov 11 '08 at 16:02
2
CREATE TRIGGER
    [dbo].[SystemParameterInsertUpdate]
ON 
    [dbo].[SystemParameter]
FOR INSERT, UPDATE 
AS
  BEGIN
    SET NOCOUNT ON 

    DECLARE @StartRow int
    DECLARE @EndRow int
    DECLARE @CurrentRow int

    SET @StartRow = 1
    SET @EndRow = (SELECT count(*) FROM inserted)
    SET @CurrentRow = @StartRow

    WHILE @CurrentRow <= @EndRow BEGIN

        IF (SELECT Attribute FROM (SELECT ROW_NUMBER() OVER (ORDER BY Attribute ASC) AS 'RowNum', Attribute FROM inserted) AS INS WHERE RowNum = @CurrentRow) LIKE 'NoHist_%' BEGIN

            INSERT INTO SystemParameterHistory(
                Attribute,
                ParameterValue,
                ParameterDescription,
                ChangeDate)
            SELECT
                I.Attribute,
                I.ParameterValue,
                I.ParameterDescription,
                I.ChangeDate
            FROM
                (SELECT Attribute, ParameterValue, ParameterDescription, ChangeDate FROM (
                                                                                            SELECT ROW_NUMBER() OVER (ORDER BY Attribute ASC) AS 'RowNum', * 
                                                                                            FROM inserted)
                                                                                    AS I 
            WHERE RowNum = @CurrentRow

        END --END IF

    SET @CurrentRow = @CurrentRow + 1

    END --END WHILE
END --END TRIGGER
-1

Using LIKE will give you options for defining what the rest of the string should look like, but if the rule is just starts with 'NoHist_' it doesn't really matter.

Daniel M
  • 1,547
  • 12
  • 12
-1

For triggers in general, you need to use a cursor to handle inserts or updates of multiple rows. For example:

DECLARE @Attribute;
DECLARE @ParameterValue;
DECLARE mycursor CURSOR FOR SELECT Attribute, ParameterValue FROM inserted;
OPEN mycursor;
FETCH NEXT FROM mycursor into @Attribute, @ParameterValue;
WHILE @@FETCH_STATUS = 0
BEGIN

If @Attribute LIKE 'NoHist_%'
      Begin
          Return
      End

etc.

FETCH NEXT FROM mycursor into @Attribute, @ParameterValue;
END

Triggers, at least in SQL Server, are a big pain and I avoid using them at all.

  • You can deal with multiple inserts or updates just fine if, from the outset, you design your triggers with multi-row data in mind. IMHO the OP's first attempt ("where I.Attribute NOT LIKE 'NoHist_%'") should have done just that... no idea what went wrong. – Christian Severin Nov 04 '10 at 13:42
  • `cursor`s are highly discouraged, but if using one, you should at least (a) declare it as `for read only` where possible to optimise it, (b) use a cursor variable for automatic closure and deallocation within the batch, (c) or if you use a manually managed `cursor`, actually bother to `close` and `deallocate` it – underscore_d Mar 21 '17 at 23:06