Skip to main content
Code Review

Return to Answer

added an example of some replacement code
Source Link
Kris Lawton
Kris Lawton
  • 150
  • 9

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
SELECT
    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 (
        select
            HistoryID, EpisodeID, eOwner, eDescription,
            row_number() over (partition by EpisodeID, eOwner, eDescription ORDER BY HistoryID) as RowNumber
        from EpisodeHistory
        where eDescription <> ''
    ) h
    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 EpisodeID, eOwner and eDescription, it will only join to the one with the lowest HistoryID.

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 NULL. Therefore, 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 output clause.

This code above also has the added bonus of working for multiple-row updates.

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
SELECT
    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 (
        select
            HistoryID, EpisodeID, eOwner, eDescription,
            row_number() over (partition by EpisodeID, eOwner, eDescription ORDER BY HistoryID) as RowNumber
        from EpisodeHistory
        where eDescription <> ''
    ) h
    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 EpisodeID, eOwner and eDescription, it will only join to the one with the lowest HistoryID.

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 NULL. Therefore, 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 output clause.

This code above also has the added bonus of working for multiple-row updates.

Source Link
Kris Lawton
Kris Lawton
  • 150
  • 9

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 inserted and deleted tables).

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'
from OtherTable

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.

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.

Checksum?

I'm going to assume since performance is an issue that you've considered indexes, and have an index set up to optimise this Episode and 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 (n)varchar column.

Minor issue

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.