January 19, 2015 at 11:17 pm
Eirikur Eiriksson (1/19/2015)
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.😎
I don't believe so. If you're auditing a table, it shouldn't be for just one "implementation". It should be for everything including someone making modifications through something like SSMS or a "smart" spreadsheet. Moving the trigger logic to the implementation would just complicate the code.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 20, 2015 at 2:52 am
Hi Jeff Moden,
I am sorry for the late reply, I ran into some issues.
Thanks very much for your detailed explanation !!! Didn't expect this much clarity in explanation 🙂
I am looking into your post, I am a starter in this field, so I need some time in getting all the things you said.
I will come up with doubts shortly 😉
Hi Crmitchell & ScottPletcher,
Thanks very much for your ideas 🙂 , I will check those and post shortly.
January 20, 2015 at 4:04 am
Hi Jeff,
I still need to know what the first 3 non-identity columns are in the EmployeeAudit table
The first columns in the audit table are
ChangeDate (datetime2(3), not null)
ChangeUserId (Varchar(12), not null)
ChangeType (char(1), not null)
January 20, 2015 at 8:12 am
Jeff Moden (1/19/2015)
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.
But a 0 is not a 0 with a MERGE, that is the point. An INSERT trigger may have receive a @@ROWCOUNT value of 3 even if no rows were Inserted (but were UPDATEd or DELETEd instead). Writing triggers differently based on whether they handle all actions or just some actions is a sure recipe for errors later, as the trigger gets split out later, or combined, but the coding isn't changed; that's just a maintenance nightmare.
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 20, 2015 at 9:34 am
ScottPletcher (1/20/2015)
Jeff Moden (1/19/2015)
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.
But a 0 is not a 0 with a MERGE, that is the point. An INSERT trigger may have receive a @@ROWCOUNT value of 3 even if no rows were Inserted (but were UPDATEd or DELETEd instead). Writing triggers differently based on whether they handle all actions or just some actions is a sure recipe for errors later, as the trigger gets split out later, or combined, but the coding isn't changed; that's just a maintenance nightmare.
Understood and thanks for the feedback. But if NO rows were actually affected by any parts of the Merge, then a 0 will still be returned. I think we might be talking two different things because I whole heartedly agree that @@ROWCOUNT should never be used (even in old code) to determine which event caused a trigger to fire but I've always thought that. It's not a recent revelation. It's only good for doing an early out if all the parts of the MERGE affect exactly nothing.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 20, 2015 at 9:58 am
Jeff Moden (1/19/2015)
Eirikur Eiriksson (1/19/2015)
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.😎
I don't believe so. If you're auditing a table, it shouldn't be for just one "implementation". It should be for everything including someone making modifications through something like SSMS or a "smart" spreadsheet. Moving the trigger logic to the implementation would just complicate the code.
I did mention only as often is the case in DWs;-)
😎
January 20, 2015 at 11:08 am
Hi Jeff,
I still need to know what the first 3 non-identity columns are in the EmployeeAudit table
1 . ChangeDate (datetime2(3), not null)
2. ChangeUserId (Varchar(12), not null)
3 . ChangeType (char(1), not null)
Please advise how to modify trigger for items 1-5 and items 1-6
January 21, 2015 at 6:59 am
Hi Jeff,
Please guide me in re writing the trigger for two scenarios which you have advised ... I'm very much interested in doing
That ... I have given the details of the first three columns in my last post...
Thanks in advance 🙂
January 21, 2015 at 7:54 am
lawlietdba (1/21/2015)
Hi Jeff,Please guide me in re writing the trigger for two scenarios which you have advised ... I'm very much interested in doing
That ... I have given the details of the first three columns in my last post...
Thanks in advance 🙂
Apologies. Got nailed by a cold yesterday afternoon and went to bed. Thanks to eating a whole pile of oranges, I feel better. I'll try to get to this today.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 21, 2015 at 8:00 am
Eirikur Eiriksson (1/20/2015)
Jeff Moden (1/19/2015)
Eirikur Eiriksson (1/19/2015)
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.😎
I don't believe so. If you're auditing a table, it shouldn't be for just one "implementation". It should be for everything including someone making modifications through something like SSMS or a "smart" spreadsheet. Moving the trigger logic to the implementation would just complicate the code.
I did mention only as often is the case in DWs;-)
😎
I guess I don't see why a DW would cause a different need here. Trigger(s) would cause coverage for the implementation and any ad hoc updates done outside of the implementation, as well. Could just be me, though. I never bank on a table "only" being maintained by a given implementation because auditors don't want to hear such things. It's bad enough that they know that someone with SA or even DBO privs could tamper with the contents of the table no matter if it's maintained only by implementation or by trigger.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 21, 2015 at 10:11 am
Hi Jeff,
No problem 🙂 thanks very much for looking into it 🙂
January 21, 2015 at 10:23 pm
crmitchell (1/19/2015)
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.
True enough but not just a caveat for #6. If a job recreates the triggers, then the job will overwrite any changes. Disabling and reenabling triggers won't overwrite the code.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 21, 2015 at 10:34 pm
Sorry. Spotted a bug in my code and had to take it down. I'll be back.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 22, 2015 at 12:02 am
Nope. No error. I kept thinking that MERGE would fire triggers simultaneously if it did both an UPDATE and a DELETE. It doesn't. They're treated as separate statements so this works.
As always, details are in the code, which also outline the changes to the 2 tables that I'd previously identified.
CREATE TRIGGER dbo.Employee_Audit ON dbo.Employee AFTER UPDATE, DELETE AS
/**********************************************************************************************************************
Purpose:
This trigger is on the Employee table and audits only modifications of rows to save on huge amounts of duplicated data
as produced by typical audit "systems". Note that INSERTs are never audited. Here's why...
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 a caveat. 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.
This trigger assumes that the Employee table and the EmployeeAudit table have both been modified by adding a
"CreatedOn" DATETIME column that defaults to GETDATE() and a "CreatedBy" column that defaults to ORIGINAL_LOGIN().
That's what's responsible for auditing the INSERTS.
This trigger will work for MERGEs, as well.
Revision History:
Rev 00 - 22 Jan 2015 - Jeff Moden
- Note that this revision was not tested by the author but nearly identical code works for the author. It
wasn't possible to do an insitu test because the author did not have access to either the Employee table
or the EmployeeAudit table to do insitu testing with.
**********************************************************************************************************************/
--===== If no rows have actually been UPDATEd, or DELETEd, exit early because there's nothing to do.
-- Note that this trigger will not respond to INSERTs to begin with so this works just fine.
IF @@ROWCOUNT = 0
RETURN
;
--=====================================================================================================================
-- Presets
--=====================================================================================================================
--===== Disable the auto-display of row counts for performance and to prevent row counts from being mistaken as errors
-- by the application or other code source.
SET NOCOUNT ON
;
--=====================================================================================================================
-- Audit modifications only.
-- Remember that, at this point, only an UPDATE or DELETE has occurred. INSERTs are not audited.
-- If a DELETE fired the trigger, then there will be no rows in the INSERTED table.
-- If an UPDATE fired the trigger, then there will be rows in the INSERTED table.
-- In either case, we only audit what the old data is, which comes from the DELETED table.
--=====================================================================================================================
INSERT INTO dbo.EmployeeAudit --Note that an actual list of columns should be used here for the INSERT and the *.
SELECT ChangeDate = GETDATE()
,ChangeUserId = ORIGINAL_LOGIN
,ChangeType = CASE
WHEN NOT EXISTS (SELECT TOP 1 1 FROM INSERTED)
THEN 'D'
ELSE 'U'
END
,* --Note that this should be expanded to contain the actual list of columns
FROM DELETED
;
GO
--Jeff Moden
Change is inevitable... Change for the better is not.
January 22, 2015 at 1:21 am
Hi Jeff,
This trigger assumes that the Employee table and the EmployeeAudit table have both been modified by adding a
"CreatedOn" DATETIME column that defaults to GETDATE() and a "CreatedBy" column that defaults to ORIGINAL_LOGIN().
That's what's responsible for auditing the INSERTS.
Thanks very much for working out of your way to help me ... !!!! 🙂
You have said i need to modify the original table,
I already have a column named "CreationDateTime" in my Original table and also have a list of User Id in the table .
Please have a look into it. Do i still have to modify original table ...?
O
SET QUOTED_IDENTIFIER ON
GO
SET ANSI_PADDING ON
GO
CREATE TABLE [dbo].[Employee](
[EmployeeNumber] [char](12) NOT NULL,
[Suffix] [char](3) NOT NULL,
[EmployeeTypeCode] [char](2) NOT NULL,
[AppEffDate] [datetime2](3) NOT NULL,
[CreationDateTime] [datetime2](3) NOT NULL,
[AppExpDate] [datetime2](3) NULL,
[EmployeeAmount] [money] NULL,
[ChangeUserId] [varchar](10) NULL,
[ChangeReason] [varchar](255) NULL,
[ChangeManagerId] [varchar](10) NULL,
[ChangeStatus] [char](1) NOT NULL,
[ChangeType] [char](1) NOT NULL,
[ChangeReasonId] [smallint] NULL
) ON [PRIMARY]
GO
Here is my Audit table,
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
SET ANSI_PADDING ON
GO
CREATE TABLE [dbo].[EmployeeAudit](
[ChangeDate] [datetime2](3) NOT NULL,
[ChangeUserId] [varchar](12) NOT NULL,
[ChangeType] [char](1) NOT NULL,
[EmployeeNumber] [char](12) NOT NULL,
[Suffix] [char](3) NOT NULL,
[EmployeeTypeCode] [char](2) NOT NULL,
[AppEffDate] [datetime2](3) NOT NULL,
[CreationDateTime] [datetime2](3) NOT NULL,
[AppExpDate] [datetime2](3) NULL,
[EmployeeAmount] [money] NULL,
[ChangeUserId] [varchar](10) NULL,
[ChangeReason] [varchar](255) NULL,
[ChangeManagerId] [varchar](10) NULL,
[ChangeStatus] [char](1) NOT NULL,
[ChangeType] [char](1) NOT NULL,
[ChangeReasonId] [smallint] NULL
) ON [PRIMARY]
Please look into the table and advise ....
Viewing 15 posts - 16 through 30 (of 60 total)
You must be logged in to reply to this topic. Login to reply