It will only work as intended for single-row updates
The fact that you use
TOP 1 and
SCOPE_IDENTITY() tells me this was designed with one-row updates/inserts in mind. But what happens when you update multiple rows? (In case you don't know, the trigger only fires once for the whole changed set, rather than once for each row. The changed rows are added to the
To solve this, I'd recommend using a temp table or variable table instead of
@CMP, and the
output clause instead of
@NEWID. For example:
-- Example table with identity
create table Things (KeyId int identity(1, 1), Value varchar(31))`
-- Table to store identity ints which get created
declare @NewIds table (NewKeyId int)
insert into Things (Value)
output inserted.KeyId into @Keys (NewKeyId) -- this line gets the new identities
select 'Value text'
Now you can access the created IDs using
@Keys. Of course you'd want to store the
EpisodeId or something in the same table, to relate them.
EDIT: Untested replacement
So I've just roughly written some SQL I think should work instead of everything including and below the
SELECT Top 1 statement. The main issue I have with that part of your trigger is that it will only work for one-row updates, and it performs an INSERT then an UPDATE on that inserted data, which can be optimised.
INSERT into EpisodeHistory
EpisodeID = d.EpisodeID,
eOwner = d.eOwner,
eDescription = iif(h.HistoryID is null, d.eDescription, ''),
eDescRef = isnull(h.HistoryID, 0),
hWhen = getDate(),
from deleted d
left join (
HistoryID, EpisodeID, eOwner, eDescription,
row_number() over (partition by EpisodeID, eOwner, eDescription ORDER BY HistoryID) as RowNumber
where eDescription <> ''
on d.EpisodeID = h.EpisodeID
and d.eOwner = h.eOwner
and d.eDescription = h.eDescription
and h.RowNumber = 1
Rather than inserting the EpisodeHistory, then checking and updating based on the data we've just inserted, this code just does the operation all in one.
First I'll explain the left join. The point of the join is to find the
HistoryID of the earliest matching
EpisodeHistory row (since we want to put it in
eDescRef, right?). So we do a normal select of the
EpisodeHistory table, except we also add a column for
row_number(), which, when combined with the fact that we've put
RowNumber = 1 in the join condition, will means it will only join to the first
HistoryID that matches. This makes sure that if there are two rows with the same
eDescription, it will only join to the one with the lowest
Now the actual columns that are inserted are fairly simple. If the left join fails (if no rows in EpisodeHistory match), selecting
h.HistoryID will return
eDescription = iif(h.HistoryID is null, d.eDescription, '') means that if a
HistoryID was not found in the join, it will just do a normal insert of
eDescription. If one was found, it will blank that field. The next line is fairly simple in that it inserts
HistoryID if the join happened, otherwise it puts in 0.
Since the entire operation can be done in one statement like above, so you can ignore that bit I said above about temporary tables and an
This code above also has the added bonus of working for multiple-row updates.
It's a trigger...
You may already know, but the SQL Server community is fairly divided when it comes to whether triggers should be avoided or not. They can be easy to forget, hard to debug, and can hinder performance as they become part of the write operation.
Since you mention performance I'll elaborate on that aspect, and I'll make some assumptions I hope you can forgive. I'm going to assume there's an application that sits in front of this database, and the application prompts the insert/update. Say the user is on a form and makes changes to an episode, and submits the change. Usually the application will wait for confirmation of the update/insert operation before proceeding (before loading the next page, for instance). With the trigger, the application now has to wait for the initial insert/update, as well as the trigger to complete its operation too.
Therefore, if you haven't already, consider the possibility of separating out this process. In this example I gave above, you could have it so that the application still waits for the initial insert/update operations, loading the next page normally, but then in the background (asynchronously) it tells the database to perform audit operations. Boom, you've reduced page load time, better UX. This also has the benefit of you not having to worry so much about performance tuning the auditing operation.
I'm going to assume since performance is an issue that you've considered indexes, and have an index set up to optimise this
EpisodeHistory comparison. Have you considered the use of
CHECKSUM()? I don't have any experience with it myself, however I'd imagine you could use it in combination with persistent computed columns to improve the load when write and maybe reading to the indexes.
In this instance you'd have an extra column on both tables called something like
eDescriptionChecksum, which would be a checksum of eDescription, and you'd put that column in the index instead of
eDescription. Then when you perform the comparison to see if they're the same, you'd be comparing the relatively small and light
int checksum column instead of the potentially-huge
I see when you set the
@CMP, you select from multiple tables. It's recommended to use joins instead – use of multiple tables in the
from clause is being depreciated by all DBMSs.
If anything's unclear, let me know. Hope you found this helpful.