Take the 2-minute tour ×
Database Administrators Stack Exchange is a question and answer site for database professionals who wish to improve their database skills and learn from others in the community. It's 100% free, no registration required.

Currently working on an database Audit project based on triggers that are fired on update on specific tables. The triggers write the changes into a table; information written are: table name, updated column, timestamp, user, old value and new value.

Triggers work fine with single updates, but when it comes to multi-row updates, it is not working.

My code is like this:

IF (UPDATE(Priority))  
BEGIN
    SET @UpdatedColumn = 'Priority'
    INSERT INTO dbo.AuditTable
        ( [TableName] ,
          [Source] ,
          [RecordId] ,
          [User] ,
          [TimeStamp] ,
          [UpdatedColumn] ,
          [OldValue] ,
          [NewValue]
        )
    SELECT 
        N'BookingItem' , -- TableName - nvarchar(max)
        (SELECT CODE FROM TBL_LEG_SOURCE 
                     INNER JOIN INSERTED INS ON LEG_SOURCE_ID = INS.SourceId) ,
        INS.Id , -- RecordId - bigint
        (SELECT USERNAME FROM INSERTED 
                     INNER JOIN TBL_USER 
                     ON ModifiedById = USER_ID) , -- User - nvarchar(max)
        GETDATE() , -- TimeStamp - datetime
        @UpdatedColumn , -- UpdatedColumn - nvarchar(max)
        DEL.Priority , -- OldValue - nvarchar(max)
        INS.Priority  -- NewValue - nvarchar(max)
    FROM 
        INSERTED INS INNER JOIN DELETED DEL ON INS.Id = DEL.Id
    WHERE
        (
            (INS.Priority <> DEL.Priority)
            OR (INS.Priority IS NULL AND DEL.Priority IS NOT NULL)
            OR (INS.Priority IS NOT NULL AND DEL.Priority IS NULL)
        )
END

Error message:

Msg 512, Level 16, State 1, Procedure MyTrigger, Line 818
Subquery returned more than 1 value. This is not permitted when the subquery follows =, !=, <, <= , >, >= or when the subquery is used as an expression.

Any suggestions on how to fix my trigger in order to handle multi-row operations?

share|improve this question
    
What version of SQL2012 are you using - enterprise or standard ? –  Kin Jul 19 '13 at 12:11
    
currently on my developer edition (Enterprise you can say) but it should work on both as deployment is to be done on Standard. –  Hani El Mouallem Jul 19 '13 at 12:21
1  
Then why dont you use CDC - change data capture ? –  Kin Jul 19 '13 at 12:56
    
Not familiar with CDC, not yet and i need a fast solution, at least till i get familiar with the newly added features in SQL12 –  Hani El Mouallem Jul 19 '13 at 13:02
1  
Are you sure this is the only trigger on the table? Is it really on line 818? If so then I think the trigger is a lot more complex than what you've shown here. –  Aaron Bertrand Jul 19 '13 at 13:36
show 12 more comments

2 Answers

up vote 4 down vote accepted

Both subqueries in the query you show join to INSERTED without doing an aggregate or a top(1). So both potentially return more than one row. instead of joining to the INSERTED table again, just reference the column directly. With that the second query would look like this:

(SELECT U.USERNAME FROM TBL_USER U WHERE INS.ModifiedById = U.USER_ID)

The change to the other one is similar.

share|improve this answer
    
Changed all the subqueries and removed the joins as suggested, quite fast and very effective. that solved my case! thanks Sebastian! –  Hani El Mouallem Jul 19 '13 at 14:37
    
So the suggestion here is that an inline subquery is always better than a proper join? I highly doubt this will be "quite fast" on larger updates. –  Aaron Bertrand Jul 19 '13 at 14:38
    
Also note that the other one could quite possibly still yield the error if there is more than one code for any source. –  Aaron Bertrand Jul 19 '13 at 14:40
add comment

Here is how to fix the errors, using proper joins (and if this isn't "fast enough" then look at your indexing):

INSERT dbo.AuditTable
(
  [TableName],
  [Source],
  [RecordId],
  [User],
  [TimeStamp],
  [UpdatedColumn],
  [OldValue],
  [NewValue]
)
SELECT 
  N'BookingItem', -- TableName - nvarchar(max)
  ls.CODE,
  INS.Id, -- RecordId - bigint
  u.USERNAME,
  GETDATE(), -- TimeStamp - datetime
  @UpdatedColumn, -- UpdatedColumn - nvarchar(max)
  DEL.Priority, -- OldValue - nvarchar(max)
  INS.Priority  -- NewValue - nvarchar(max)
FROM 
  INSERTED AS INS 
INNER JOIN 
  DELETED AS DEL ON INS.Id = DEL.Id
INNER JOIN 
  dbo.TBL_LEG_SOURCE AS ls ON ls.LEG_SOURCE_ID = INS.SourceId
INNER JOIN
  dbo.TBL_USER AS u ON INS.ModifiedById = u.USER_ID
WHERE
(
  (INS.Priority <> DEL.Priority)
  OR (INS.Priority IS NULL AND DEL.Priority IS NOT NULL)
  OR (INS.Priority IS NOT NULL AND DEL.Priority IS NULL)
);

However I think it is quite foolish to run 50+ different inserts of this variety in order to capture every single column change. Why not just create a table with columns for time and table name (you don't need to store username because you can always look that up later), and then whenever there is an update, store the old and new version of the row? You can even use a SEQUENCE to ensure that you can identify the set of rows that were modified together (since timestamp may not be unique enough to do that).

CREATE SEQUENCE dbo.AuditSequence
  AS INT START WITH 1 INCREMENT BY 1;

CREATE TABLE dbo.AuditData
(
  AuditSequenceID INT,
  TableName SYSNAME,
  [TimeStamp] DATETIME,
  RowState CHAR(1), -- e.g. 'B' = before, 'A' = after
  ... all your 50 columns, including ModifidById ...
);

Now in your trigger:

CREATE TRIGGER dbo.MyTrigger
  ON dbo.BookingItem
  FOR UPDATE
AS
BEGIN
  SET NOCOUNT ON;

  DECLARE @as INT = NEXT VALUE FOR dbo.AuditSequence,
          @now DATETIME = CURRENT_TIMESTAMP;

  INSERT dbo.AuditData(AuditSequenceID, TableName, [TimeStamp], RowState,
    ... the rest of your 50 columns)
  SELECT @as, N'BookingItem', @now, 'B', * FROM deleted;

  INSERT dbo.AuditData(AuditSequenceID, TableName, [TimeStamp], RowState,
    ... the rest of your 50 columns)
  SELECT @as, N'BookingItem', @now, 'A', * FROM inserted;
END
GO

Now, write complicated queries against this much simpler audit structure that are inefficient and try to track exactly which columns have changed and all of that. Much better to pay that price when you're reviewing audit data than to pay that price on every single update operation.

share|improve this answer
    
That was quite good, though adding few more joins was reflected on the speed of execution; but great solution for smaller tables i think. thanks Aaron –  Hani El Mouallem Jul 19 '13 at 14:35
2  
Well, the logic needs to be correct first, then you worry about performance. You don't keep code efficient by discarding logic, you implement proper indexing (or design) to support the types of queries you need to run. –  Aaron Bertrand Jul 19 '13 at 14:37
4  
I also think it's, well, funny might be the word, that you are running 50+ different insert statements in this trigger, each one to capture a single changed column, and then worried about efficiency. Why not just store a copy of the old row and a copy of the new row, with a few extra columns (such as the time and user id of the change). The username doesn't really need to be stored with the audit data anyway, does it? Can't you look that up when reviewing the audit data? –  Aaron Bertrand Jul 19 '13 at 14:49
    
that is another workaround, iwll be trying it and keep you posted with the outcome but the reason for me to use such structure is to have a single table for audit containing updates from different tables; that is why i chose to have 1 table, and triggers on targetted tables that write data to AuditTable. by what you have suggested is limiting the audit table to a single table, and therefore each table will have its own AuditTable? –  Hani El Mouallem Jul 20 '13 at 18:47
1  
Well you seem to be doing a lot of work that other technologies have already been written for you to do (CDC, Change Tracking). –  Aaron Bertrand Jul 20 '13 at 23:53
show 5 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.