2

In our application at the database level, I have a table called Installments in schema Billing and Billing_History.

The trigger shown is on the Installments table in the Billing Schema.

What this does is everytime a record is inserted/updated in the billing schema it is also written into the history file.

If the record is deleted from the billing table it is written to the history table with a "Deleted" indicator = true.

I think that the "If Not Exists (Select * from Inserted) is killing my performance as more records get added.

Is there a more effecient was to write this trigger?

Create TRIGGER [Billing].[Installments_InsertDeleteUpdate_History]
ON [Billing].[Installments]
AFTER INSERT, DELETE, UPDATE
AS BEGIN
Insert Into Billing_History.Installments
    Select *, GetDate(), 0 From Inserted

If Not Exists (Select * From Inserted)
    Insert Into Billing_History.Installments
        Select *, GetDate(), 1 From Deleted

SET NOCOUNT ON;

-- Insert statements for trigger here

END

AWeim
  • 259
  • 4
  • 12
  • Suggest you add a tag for what RDBMS you are using. – Smandoli Sep 27 '12 at 20:13
  • `If Not Exists (Select * From Inserted)` won't be killing your performance but you can replace it with a `IF @@ROWCOUNT > 0` check for the same semantics. – Martin Smith Sep 27 '12 at 20:13
  • 1
    Select * in an insert is an extremely poor practice. – HLGEM Sep 27 '12 at 20:18
  • Suggest destroying the trigger with a large hammer and using stored procs. – Nick Vaccaro Sep 27 '12 at 20:25
  • Wait. Does that even work? There's no "WHERE" in the IF clause. – Nick Vaccaro Sep 27 '12 at 20:26
  • Using `GetDate()` within a query is chasing a moving target, impacts performance, and may produce curious results, e.g. as the date changes. It is almost always a better idea to capture the current date/time in a variable and then use that value as needed. This is more important across multiple statements as in a stored procedure. In this case having a single date/time for the inserted and deleted rows from a single trigger firing makes it easier to attempt to correlate data in the history table. – HABO Sep 27 '12 at 20:30
  • @Richardakacyberkiwi - Could you suggest a book that will explain the benefits of assigning different date/times to events that appear to occur simultaneously? Having made the mistake of applying a single timestamp to events reported together in numerous industrial automation systems, I would very much like to understand the error of my ways. – HABO Sep 27 '12 at 20:51
  • 1
    @HABO - `getdate()` is a [runtime constant](http://blogs.msdn.com/b/conor_cunningham_msft/archive/2010/04/23/conor-vs-runtime-constant-functions.aspx) so will only be evaluated once per statement here and as the two branches of code are mutually exclusive in this case I can't see that issue. – Martin Smith Sep 27 '12 at 21:05
  • @Martin Smith: Isn't using @@ROWCOUNT problematic because of the possible presence of other triggers? – RBarryYoung Sep 27 '12 at 21:34
  • @RBarryYoung - I meant immediately after the `Insert Into Billing_History.Installments Select *, GetDate(), 0 From Inserted`. Are you saying a trigger on Installments might mess that? (Edit yes just tested and that does seem like it might) – Martin Smith Sep 27 '12 at 21:36
  • No, I'm saying that there could be more than one AFTER trigger on `Billing.Installments`, and there's no trigger ordering on this one, so it might go second or later. – RBarryYoung Sep 27 '12 at 21:38
  • I don't see how that would affect things regarding `@@ROWCOUNT`? It is testing the rowcount from the `insert Into Billing_History.Installments` statement which is equal to the number of rows in `inserted` and I don't see that a future after trigger can affect the number of these rows? – Martin Smith Sep 27 '12 at 21:46
  • Oops, oh, I see, Sorry, I was thinking that you were intending to apply it to the triggering DML statement on `Billing.Installments`. My bad. – RBarryYoung Sep 27 '12 at 21:50
  • @MartinSmith - Runtime constant nondeterministic function. That's a good one. One might think that a feature like that would warrant documentation. By just mentioning it in a blog there is no obligation to support the feature. (And with a teaser that it might apply elsewhere.) As we know so well, observed behavior is no guarantee of future results. Sigh. Thanks for yet another bit of scary enlightenment. I'll just go back to being horrified by: `select 30/3/5, 30/3/-5, 30/(-3)/5; select 30/-3/5` – HABO Sep 28 '12 at 19:43

3 Answers3

1

I would suggest that the trigger form you have is the best performing, given it's required tasks. There really aren't much better ways to achieve the same auditing result.

The answer here would agree Creating audit triggers in SQL Server and here's a long discussion about performance of audit solutions.

Your situation is slightly different, because you actually DON'T want the deleted (original) table in UPDATE situations, hence the IF.

Community
  • 1
  • 1
RichardTheKiwi
  • 105,798
  • 26
  • 196
  • 262
  • Thank's for all the comments. At this point I don't think there is enough compelling evidence to change/remove the trigger. I am going to look other places for performance gain. – AWeim Sep 28 '12 at 12:49
0

Create one trigger for INSERTs and UPDATEs and a second for DELETEs. Then you don't have to use an IF statement and a slow query to check where to log.

From a design perspective, see if you can eliminate triggers. They're a mess.

Nick Vaccaro
  • 5,428
  • 6
  • 38
  • 60
  • The `IF EXISTS(...)` is not a slow query. If the OP is getting performance problems I suggest they look elsewhere than that statement. – Martin Smith Sep 27 '12 at 20:29
  • @MartinSmith I think you are correct. That being said, it still makes sense to use multiple triggers to eliminate unnecessary IFs, IMO. – Nick Vaccaro Sep 27 '12 at 20:31
0

Well you could make this simple change:

Create TRIGGER [Billing].[Installments_InsertDeleteUpdate_History]
ON [Billing].[Installments]
AFTER INSERT, DELETE, UPDATE
AS BEGIN

If Not Exists (Select * From Inserted)
    Insert Into Billing_History.Installments
        Select *, GetDate(), 1 From Deleted
ELSE
    Insert Into Billing_History.Installments
        Select *, GetDate(), 0 From Inserted

SET NOCOUNT ON;

-- Insert statements for trigger here

Which is logically more efficient, but whether it's physically more performant is an open question. If it is actually faster, it sure won't be by very much.

RBarryYoung
  • 55,398
  • 14
  • 96
  • 137