January 17, 2015 at 12:01 am
Hi,
Please explain what the below trigger query does after the insert statement.
insert into Employee select * from Employee_DupCk
CREATE TRIGGER [dbo].[tI_employee] ON [dbo].[Employee] FOR INSERT AS BEGIN DECLARE @numrows int, @errno int, @errmsg varchar(255) SELECT @numrows = @@ROWCOUNT IF @numrows > 0 BEGIN INSERT EmployeeAudit SELECT getdate(), user, 'I', i.* FROM inserted AS i SELECT @errno = @@ERROR IF @errno != 0 BEGIN SELECT @errmsg = 'Error detected while inserting EmployeeAudit entry' GOTO error END END RETURN error: RAISERROR(@errmsg, 16, 1) IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION END
Please explain what this trigger does and also advise whether this query just creates the trigger or is it also executes the operation which this trigger is supposed to do ?
This trigger query is fired immediately after the insert statement.
January 17, 2015 at 1:42 am
Quick thoughts, this trigger is supposed to copy the data being inserted into the dbo.Employee table into the dbo.EmployeeAudit table, catch error in the insert and roll back the transaction if the Audit insert fails.
BTW, formatting the code makes it more readable, comments in the code.
😎
CREATE TRIGGER [dbo].[tI_employee] ON [dbo].[Employee]
/* The trigger fires after the data has been inserted into the
table. The trigger has access to the data being inserted
through the "inserted" pseudo table during the execution,
it will fire (execute) for any attempt to insert into the
table.
The CREATE TRIGGER code will not in itself execute the code.
The code copies the data being inserted into the dbo.Employee
table into the dbo.EmployeeAudit table, adding time stamp etc.
*/
FOR INSERT
AS
BEGIN
/* Variable declaration, note that all the variables will have
the value of NULL. On SQL Server 2008 and later, the variable
values can be initialized as a part of the declaration .
*/
DECLARE @numrows int, @errno int, @errmsg varchar(255)
/* The @@ROWCOUNT returns the number of rows affected by
the last statement.
*/
SELECT @numrows = @@ROWCOUNT
/* If the value of the @numrows variable is greater than 1 (one) this
section will be executed.
*/
IF @numrows > 0
BEGIN
INSERT EmployeeAudit
SELECT getdate(), user, 'I', i.*
FROM inserted
AS i
SELECT @errno = @@ERROR
/* Primitive error handling, hides the real error and
replaces the message with something close to meaningless.
*/
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error detected while inserting EmployeeAudit entry'
GOTO error
END
END
RETURN
error:
RAISERROR(@errmsg, 16, 1)
IF @@TRANCOUNT > 0
/* This rollback is not limited to the insert only, it can affected
code and logic further up the chain and possibly cause very hard
do detect errors in any logic controling the insert.
*/
ROLLBACK TRANSACTION
END
GO
Edit: paste error
January 17, 2015 at 11:19 am
Eirikur correctly described what the code in the trigger does. I'll tell you that the trigger uses older methods, can be improved a bit performance-wise, has unnecessary error handling, and single handedly causes all data for the employee table to be unnecessarily doubled.
I'm almost positive that whomever wrote that trigger also wrote one each for Updates and Deletes. If you can find those (should be easy to find if the exist. Just look at triggers on the same table), I'd be happy to demonstrate and explain the claims I just made and additional problems that I think may exist in those other triggers based on what I see in this trigger.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 17, 2015 at 10:51 pm
Hi Eirikur Eiriksson,
Thanks Very much for the clear explanation !!! Couldn't have asked for more 🙂
Hi Jeff,
Thanks verymuch for your suggestion. As you said I found the separate triggers for Update as well as Delete . Please explain what can be done to improve this code performance wise.
/****** Object: Trigger [dbo].[tU_Employee] Script Date: 01/17/2015 23:18:18 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
CREATE TRIGGER [dbo].[tU_Employee]
ON [dbo].[Employee]
FOR UPDATE
AS
/*
* ****************************************************************
* *
* * Name:tU_Employee
* * Date Written: Tue Jan 11, 1999
* * Author:
* * Y/NSystem
* *-------------------------------------
* UPDATE trigger on Employee
*/
BEGIN
DECLARE
@numrows int,
@errno int,
@errmsg varchar(255)
/*
* ***********************************************************************
* ***************** Capture Audit info **********************************
*/
SELECT @numrows = @@ROWCOUNT
IF @numrows > 0
BEGIN
/*
* ******************************************************
* Create Audit copy when Employee is inserted
* ******************************************************
*/
INSERT EmployeeAudit
SELECT getdate(), user, 'D', d.*
FROM deleted AS d
SELECT @errno = @@ERROR
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error detected while inserting EmployeeAudit entry'
GOTO error
END
INSERT EmployeeAudit
SELECT getdate(), user, 'I', i.*
FROM inserted AS i
SELECT @errno = @@ERROR
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error detected while inserting EmployeeAudit entry'
GOTO error
END
END
/*
* ***********************************************************************
* ***********************************************************************
*/
RETURN
error:
RAISERROR(@errmsg, 16, 1)
IF @@TRANCOUNT > 0
ROLLBACK TRANSACTION
END
GO
/****** Object: Trigger [dbo].[tD_Employee] Script Date: 01/17/2015 23:39:12 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
/* 01-Apr-2013 Converted using SSMA - UnitTested
02-Apr-2013 ManualConversions are done and UnitTested
*/
CREATE TRIGGER [dbo].[tD_Employee]
ON [dbo].[Employee]
FOR DELETE
AS
/*
* ****************************************************************
* *
* * Name:tD_Employee
* *
* * Date Written:Tue Jan 11, 1999
* * Purpose/Description:
* *
* ******************************************************************
* * REVISION HISTORY
* * Date
* * Name Revised Description
* * ------------- ---------- ---------------------------------------
* *
* *****************************************************************
* DELETE trigger on Employee
*/
BEGIN
DECLARE
@numrows int,
@errno int,
@errmsg varchar(255)
/*
* ***********************************************************************
* ***************** Capture Audit info **********************************
*/
SELECT @numrows = @@ROWCOUNT
IF @numrows > 0
BEGIN
/*
* *****************************************************
* Create Audit copy when Employee is deleted
* *****************************************************
*/
INSERT EmployeeAudit
SELECT getdate(), user, 'D', d.*
FROM deleted AS d
SELECT @errno = @@ERROR
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error detected while deleting EmployeeAudit entry'
GOTO error
END
END
/*
* ***********************************************************************
* ***********************************************************************
*/
RETURN
error:
RAISERROR(@errmsg, 16, 1)
IF @@TRANCOUNT > 0
ROLLBACK TRANSACTION
END
GO
January 18, 2015 at 11:40 am
lawlietdba (1/17/2015)
Hi Jeff,Thanks verymuch for your suggestion. As you said I found the separate triggers for Update as well as Delete . Please explain what can be done to improve this code performance wise.
First, thank you very much for taking the time to dig those out and post them.
Second, these are on an "Employee" table. That means that this table doesn't suffer many Inserts, Updates, or Deletes and the performance of these triggers isn't paramount. That also means that the ROI of repairing these triggers, which work correctly, may be NIL. The repairs in items 1-5 below will not require any other changes but, the repairs in item #6 will. The repairs suggested by item #6 should probably NOT be made in this case because we don't know how the audit reporting code (if there is any) that reads from the "EmployeeAudit" will be affected. If there is no such audit reporting code, then it would be worth making changes if, for no other reason, than to provide a "model" for how such audit triggers should be written, especially on tables that suffer a lot of Inserts, Updates, and/or Deletes.
Ok... let's get started.
1. @@ROWCOUNT should NOT be assigned to a variable. Instead, there should be an IF statement at the very beginning to test for a value of "0" and immediately exit if the value is "0". Although DECLAREs, variable assignment, and testing the variable for a value doesn't take much time or resources, it still takes some and for high hit ratios, it can become a bit obnoxious. Testing @@ROWCOUNT right out of the gate eliminates all that unnecessary processing.
2. SET NOCOUNT ON is a must for all triggers. It will prevent the relatively long time it takes to return a rowcount even when it's not used. Further, it will prevent such rowcounts from being interpreted as an error by a GUI.
3. The "user" function should be changed to "ORIGINAL_LOGIN()". This will more accurately return who the real user was if any form of "impersonation" comes into play. It will not, however, identify the real user of an application unless the application uses the original user's login to access the database. Typically, most applications have their own login and that's what will be captured by "user" or "ORIGINAL_LOGIN()". Since "ORIGINAL_LOGIN()" does have the added functionality of correctly identifying changes made via SSMS and SQLCMD, even when impersonation is in effect, and they both take the same amount of time to process, it's better to go with the more robust "ORIGINAL_LOGIN".
4. Doing error checking in an audit trigger is a bit like nuking the same spot twice. It's usually a huge waste and serves only to make the code longer. AFTER/FOR triggers (like these) are a part of the implicit transaction formed by any Insert, Update, or Delete. By the time they fire, the data is already in the table which also means that any naturally occurring errors, such as fatal datatype mismatches, truncation errors, etc, etc, etc, will have occurred long before the trigger actually fires. About the only time these triggers would fail is if someone made a change to the Employee table and the code that feeds it but forgot to change the EmployeeAudit table or the EmployeeAudit table was missing. Guess what happens then? SQL Server will raise an error and the error message it produces will more correctly identify the problem than 'Error detected while inserting EmployeeAudit entry', and, since the trigger is a part of the transaction that caused the trigger to fire, will do a correct rollback on it's own. There's absolutely no need for any error detection code in these triggers! If there was, a correct BEGIN TRY/CATCH would be the way to go rather than IF/GOTO.
5. Having these 3 triggers is fine but it also means that you have 3 triggers. If you have many such audits, you'll have a shedload of code and, if a change comes about that requires the triggers to be changed, you'll have 3 triggers to modify and test. If you ever create a list of all triggers in the database, it'll be 3 times longer than it needs to be. While such triggers aren't likely to need a change (especially because of the SELECT *), it won't hurt to consolidate such audit triggers into 1. The test to figure out if it was an Insert, Update, or Delete won't take any longer than SQL Server figuring out which of the 3 separate triggers to fire, if it's written correctly.
6. Ok. None of the changes above will require any changes in any audit code and they can all be done without any impact except for making the triggers a little faster, a little less resource intensive, and a little easier to maintain (if ever needed). This one (Item #6) will require changes to any audit code if such audit code actually exists.
The way the triggers currently stand, here's what happens if a new row is inserted into the Employee table.
a. The data is inserted into the Employee table.
b. The trigger fires.
c. An exact copy of the row is made along with who and when the insert was made.
The WHOLE row is copied + some other stuff. Whether the table has many columns or not, we've just duplicated ALL of the data, which doubles disk space usage, doubles the memory required when the EmployeeAudit table is searched, doubles the backup space and time it takes to do, doubles the restore time in a DR event, and nothing has yet changed in the row! The original row lives in two places!
If a row is updated, it's much worse. Here's what happens there.
a. A previously inserted row is updated. That means the original row lives not only in the Employee table, but it also already lives in the EmployeeAudit table. Keep THAT in mind.
b. That previous row on the Employee table is updated. All the changes that were made now live in the Employee table, as they should.
c. The trigger fires.
d. A copy of the DELETED row is made and inserted into the EmployeeAudit table. That DELETED row is exactly identical to the original row that was already inserted into the EmployeeAudit table, which means that we've now got 3 times the amount of the original data and IT'S ALL DUPLICATE DATA!!!
e. A copy of the INSERTED row is made and inserted into the EmployeeAudit table. That INSERTED row is exactly identical to the modified row in the Employee table, which means we now have 4 times the amount of data and, this too, is a duplication of data that already exists!
Quick recap of the duplications...
a. The insert lives in the original table and a copy is made in the audit table. That's 1 duplication.
b. Every update makes a copy of the original row, which already exists in the audit table for 1 duplication and a copy of the updated row, which now lives in the original table, is made which constitutes another duplication for a total of 2 duplications for every update.
Considering the data in the original table and the 3 duplicates in the audit table, just one insert and 1 update cause 4 rows of data to be stored.
In my humble opinion, that's totally insane. It's a totally unnecessary duplication of data (4 rows for a single insert and just one update) that affects the capacity of many different areas and makes interrogation of the EmployeeAudit table much more difficult and resource intensive because it has to look through 3 times the amount of data! And every update will add 2 more rows of duplicated data!
Delete's aren't so much of a problem. Done correctly, there will be no duplication of data because the data in the original table will be deleted. It will only exist once, in the audit table. Understanding that is also the key to fixing this massive growth problem.
Here's what I recommend (and this is what I do in production for such "whole row" audit systems) to solve this problem especially for tables that suffer many Inserts, Updates, and Deletes. Here's the summary of what to do...
a. Add two columns to the original table (Employee table, in this case). One for "CreatedOn" with a default of GETDATE() and one for "CreatedBy" with a default of ORIGINAL_LOGIN(). Yes, that will take extra space in all the areas that I previously mentioned but it will take MUCH LESS space than a 3X duplication that can and will get much worse with every UPDATE. The addition of these two columns is a bit of self-auditing that will let you know when the row was first inserted. The freshly inserted row does NOT need to be copied to the audit table.
b. Audit ONLY the data from the DELETED trigger table. There's no need to audit the INSERTs because an inserted row will ALWAYS exist in the original table until it's updated or deleted. If a row is updated, you only need to record what it used to be because the modified row will ALWAYS exist in the original table. What the row used to be is located in the DELETED trigger table and should be copied to the audit table. Of course, if you do a DELETE, the DELETED trigger table will contain that deleted row and it should be copied to the audit table.
d. To make life easier for people writing "audit related code", create a UNION ALL view that returns the results from both the original table (Employee, in this case) and the audit table (EmployeeAudit, in this case). Any underlying indexes on the tables will come into play on this view.
Here's a recap of what happens after making such modifications.
a. A row is inserted into the original table. The CreatedBy and CreatedOn columns record who and when the row was inserted. The row is not copied to the audit table. NO duplication occurs. If the row is never updated, the original INSERT is preserved in the original table and the view picks that up when needed.
b. If a row is UPDATED, the info from the DELETED trigger table is copied to the audit table to preserve what the originally inserted data looks like and the original table now contains the modified data. Both are available through the view and NO exact duplication occurs because we're only recording what rows looked like BEFORE any given update in the audit table.
c. Whether a row has been updated or not, the final condition of the row is recorded in the original table. NO exact duplication occurs. DELETEs, of course, are audited but, still, NO exact duplication occurs.
To greatly simplify ("Executive Overview"?), if a row never changes, only that row exists and no duplication exists. If a row is updated, only the changed row (in the original table) and the original row (now, in the audit table) exists. That's only two instances of the row instead of 4 and there is no exact duplication of the row. After that, each update only records one row (the condition of the row BEFORE the update) instead of two and none of the rows are exact duplicates. The last update to the row ALWAYS exists in the original table and doesn't need to be audited.
Now, for the only real problem. People who write code to interrogate the audit information (which now lives in both the original table {Employee} and the audit table {EmployeeAudit}) now have to realize and code for the fact that the current state of a row in the original table occurred on the last audit date available in the audit table for that given row. In fact, every row in the audit table has an audit date on it that says "This is the condition of the row BEFORE it was changed". It's not difficult to program around that (especially with the "Preceding Row" stuff available in 2012 but also not difficult in versions before that) and is usually well worth it because of the incredible reduction in storage requirements and the fact that going back to interrogate audit data is only done in an extreme case where you have to prove something. Audit data is normally not interrogated at all so even if performance challenged audit interrogation code is written, it's not going to impact anything that's important.
Whew! Sorry about the long winded explanation but I needed to be sure that you could sell the changes to management, especially on more frequently inserted/modified tables. Your DBA and your backups will love you for it.
I'm going to take a break for now. In the meantime, I need for you to tell me what the first 3 non-IDENTITY column names of the EmployeeAudit table are. When I get that information, I'll show you how to write a trigger that covers only items 1-5 and a trigger that covers items 1-6.
Hmmm.... I think I just wrote an "article". Guess I should get it ready for publication and submit it. 🙂
--Jeff Moden
Change is inevitable... Change for the better is not.
January 18, 2015 at 12:20 pm
P.S. I almost forgot... because Item #6 causes only one row to be inserted into the audit table during UPDATEs, it doubles the performance of UPDATEs. Because Item #6 causes no rows to be inserted into the audit table during INSERTs, INSERTs will also be twice the speed. Deletes will be the same as they were with the old triggers.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 19, 2015 at 2:40 am
another caveat on no 6.
Check that you don't have any jobs which disable or worse delete and then later reenable or recreate those triggers.
If you do then they will break your audit trail.
Most likely culprits would be jobs to do batch processing.
You may want to audit any such action as well.
January 19, 2015 at 9:26 am
@@ROWCOUNT is no longer reliable in triggers and should not be used to determine if/how many rows were affected. You have an easy alternative anyway, since you just want to see if any row exists.
I made a few other corrections, some minor, as noted in bold below.
CREATE TRIGGER [dbo].[tI_employee]
ON [dbo].[Employee]
AFTER INSERT
AS
IF EXISTS(SELECT TOP (1) * FROM inserted)
BEGIN
DECLARE @errno int, @errmsg varchar(255)
INSERT EmployeeAudit
SELECT GETDATE(), user, 'I', i.*
FROM inserted AS i
SELECT @errno = @@ERROR
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error #' + CAST(@errno AS varchar(10)) + ' detected while inserting EmployeeAudit entry.'
GOTO error
END --IF
END --IF
RETURN
error:
RAISERROR(@errmsg, 16, 1)
IF XACT_STATE() <> 0
BEGIN
/* This rollback is not limited to the insert only, it can affected
code and logic further up the chain and possibly cause very hard
do detect errors in any logic controling the insert.
*/
ROLLBACK TRANSACTION
END --IF
GO
SQL DBA,SQL Server MVP(07, 08, 09) "It's a dog-eat-dog world, and I'm wearing Milk-Bone underwear." "Norm", on "Cheers". Also from "Cheers", from "Carla": "You need to know 3 things about Tortelli men: Tortelli men draw women like flies; Tortelli men treat women like flies; Tortelli men's brains are in their flies".
January 19, 2015 at 1:42 pm
ScottPletcher (1/19/2015)
@@ROWCOUNT is no longer reliable in triggers and should not be used to determine if/how many rows were affected. You have an easy alternative anyway, since you just want to see if any row exists.
Didn't know that. Have you a link for that information that I could study?
--Jeff Moden
Change is inevitable... Change for the better is not.
January 19, 2015 at 1:49 pm
Jeff Moden (1/19/2015)
ScottPletcher (1/19/2015)
@@ROWCOUNT is no longer reliable in triggers and should not be used to determine if/how many rows were affected. You have an easy alternative anyway, since you just want to see if any row exists.Didn't know that. Have you a link for that information that I could study?
I think it's primary related to MERGE.
SQL DBA,SQL Server MVP(07, 08, 09) "It's a dog-eat-dog world, and I'm wearing Milk-Bone underwear." "Norm", on "Cheers". Also from "Cheers", from "Carla": "You need to know 3 things about Tortelli men: Tortelli men draw women like flies; Tortelli men treat women like flies; Tortelli men's brains are in their flies".
January 19, 2015 at 1:58 pm
ScottPletcher (1/19/2015)
@@ROWCOUNT is no longer reliable in triggers and should not be used to determine if/how many rows were affected. You have an easy alternative anyway, since you just want to see if any row exists.I made a few other corrections, some minor, as noted in bold below.
CREATE TRIGGER [dbo].[tI_employee]
ON [dbo].[Employee]
AFTER INSERT
AS
IF EXISTS(SELECT TOP (1) * FROM inserted)
BEGIN
DECLARE @errno int, @errmsg varchar(255)
INSERT EmployeeAudit
SELECT GETDATE(), user, 'I', i.*
FROM inserted AS i
SELECT @errno = @@ERROR
IF @errno != 0
BEGIN
SELECT @errmsg = 'Error #' + CAST(@errno AS varchar(10)) + ' detected while inserting EmployeeAudit entry.'
GOTO error
END --IF
END --IF
RETURN
error:
RAISERROR(@errmsg, 16, 1)
IF XACT_STATE() <> 0
BEGIN
/* This rollback is not limited to the insert only, it can affected
code and logic further up the chain and possibly cause very hard
do detect errors in any logic controling the insert.
*/
ROLLBACK TRANSACTION
END --IF
GO
Never mind. I found it. It's because of MERGE that such claims are made. That's the whole thing with the trigger that I'm getting ready to replace the 3 triggers with... it's not going to matter if it's an Insert, Update, Delete, or one or more of the three from a merge. @@ROWCOUNT is still reliable but the use of MERGE puts a slightly different spin on it. In truth, I'm not sure that it's changed its functionality (@@ROWCOUNT still returns the rowcount of the last completed action and MERGE is an action despite being made up of Insert, Update, and Delete) at all for the purpose that it's used in the triggers. I'll have to do my own testing and an additional proof using 2005 vs 2008.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 19, 2015 at 2:03 pm
I still see no reason to risk @@ROWCOUNT if you just want to know if a row was modified or not -- that easy enough to check with EXISTS(), which will always be accurate. I particularly don't want to have to re-write triggers just because someone used MERGE for the first time!!
SQL DBA,SQL Server MVP(07, 08, 09) "It's a dog-eat-dog world, and I'm wearing Milk-Bone underwear." "Norm", on "Cheers". Also from "Cheers", from "Carla": "You need to know 3 things about Tortelli men: Tortelli men draw women like flies; Tortelli men treat women like flies; Tortelli men's brains are in their flies".
January 19, 2015 at 5:17 pm
ScottPletcher (1/19/2015)
I still see no reason to risk @@ROWCOUNT if you just want to know if a row was modified or not -- that easy enough to check with EXISTS(), which will always be accurate. I particularly don't want to have to re-write triggers just because someone used MERGE for the first time!!
Exists won't be any more accurate than @@ROWCOUNT when it comes to deciding if you want to do an extremely early return (exit) if no rows have been Inserted, Updated, or Deleted. A 0 is a 0. I agree that EXISTS should be used to determine the type of triggering action but only if the trigger handles all 3 actions.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 19, 2015 at 5:20 pm
I still need to know what the first 3 non-identity columns are in the EmployeeAudit table to be able to write a replacement for you.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 19, 2015 at 10:03 pm
Quick thought, if the table's content is only being maintained with the MERGE statement, it would be better to move the trigger's logic into that implementation, that's straight forward with the OUTPUT clause.
😎
Viewing 15 posts - 1 through 15 (of 60 total)
You must be logged in to reply to this topic. Login to reply