Hi,
My table design as following,
CREATE TABLE [dbo].[SFAProject](
[Id] [int] IDENTITY(1,1) NOT NULL,
[TenderName] [varchar](200) NULL,
[ClientsId] [int] NOT NULL,
[SalesPersonId] [nvarchar](450) NULL,
[SFAStageId] [int] NOT NULL,
[SFATenderCategoryId] [int] NOT NULL,
[SubmitDate] [date] NOT NULL,
[TenderValue] [decimal](18, 2) NULL,
[TenderCost] [decimal](18, 2) NULL,
[TenderRemark] [varchar](500) NULL,
[CrtBy] [nvarchar](450) NULL,
[CrtDte] [datetime] NULL,
[UpdBy] [nvarchar](450) NULL,
[UpdDte] [datetime] NULL,
CONSTRAINT [PK_SFAProject] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]
GO
Then, this
CREATE TABLE [dbo].[SFAProject_Activity](
[TrnxId] [int] IDENTITY(1,1) NOT NULL,
[Id] [int] NOT NULL,
[TrnxUserId] [nvarchar](450) NULL,
[TrnxType] [char](1) NULL,
[TrnxDte] [datetime] NULL,
[TenderName] [varchar](200) NULL,
[ClientsId] [int] NOT NULL,
[SalesPersonId] [nvarchar](450) NULL,
[SFAStageId] [int] NOT NULL,
[SFATenderCategoryId] [int] NOT NULL,
[SubmitDate] [date] NOT NULL,
[TenderValue] [decimal](18, 2) NULL,
[TenderCost] [decimal](18, 2) NULL,
[TenderRemark] [varchar](500) NULL,
CONSTRAINT [PK_SFAProject_Activity] PRIMARY KEY CLUSTERED
(
[TrnxId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]
GO
ALTER TABLE [dbo].[SFAProject_Activity] ADD CONSTRAINT [DF_SFAProject_Activity_TrnxDte] DEFAULT (getdate()) FOR [TrnxDte]
GO
I've trigger on SFAProject like this,
CREATE TRIGGER [dbo].[TR_Audit_SFAProject]
ON [dbo].[SFAProject]
FOR INSERT, UPDATE, DELETE
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
IF EXISTS ( SELECT 0 FROM Deleted )
BEGIN
IF EXISTS ( SELECT 0 FROM Inserted )
BEGIN
INSERT INTO [dbo].[SFAProject_Activity]
( [Id], TrnxUserId, [TrnxType], [TenderName], [ClientsId], [SalesPersonId],
[SFAStageId], [SFATenderCategoryId], [SubmitDate],
[TenderValue], [TenderCost], [TenderRemark]
)
SELECT D.Id ,
D.UpdBy ,
'U' ,
D.TenderName ,
D.ClientsId ,
D.SalesPersonId ,
D.SFAStageId ,
D.SFATenderCategoryId ,
D.SubmitDate ,
D.TenderValue,
D.TenderCost ,
D.TenderRemark
FROM Deleted D
END
ELSE
BEGIN
INSERT INTO [dbo].[SFAProject_Activity]
( [Id], TrnxUserId, [TrnxType], [TenderName], [ClientsId], [SalesPersonId],
[SFAStageId], [SFATenderCategoryId], [SubmitDate],
[TenderValue], [TenderCost], [TenderRemark]
)
SELECT D.Id ,
D.UpdBy ,
'D' ,
D.TenderName ,
D.ClientsId ,
D.SalesPersonId ,
D.SFAStageId ,
D.SFATenderCategoryId ,
D.SubmitDate ,
D.TenderValue,
D.TenderCost ,
D.TenderRemark
FROM Deleted D
END
END
ELSE
BEGIN
INSERT INTO [dbo].[SFAProject_Activity]
( [Id], TrnxUserId, [TrnxType], [TenderName], [ClientsId], [SalesPersonId],
[SFAStageId], [SFATenderCategoryId], [SubmitDate],
[TenderValue], [TenderCost], [TenderRemark]
)
SELECT I.Id ,
I.CrtBy ,
'I' ,
I.TenderName ,
I.ClientsId ,
I.SalesPersonId ,
I.SFAStageId ,
I.SFATenderCategoryId ,
I.SubmitDate ,
I.TenderValue,
I.TenderCost ,
I.TenderRemark
FROM Inserted I
END
END
GO
ALTER TABLE [dbo].[SFAProject] ENABLE TRIGGER [TR_Audit_SFAProject]
GO
It successful working on Insert and Update
I need help on Delete Activity
Please help. Need help to modify my Trigger ( if any )
I firmly believe that one shouldn't have to read all of the code to figure out what it does to figure out what section of the code to work with. I also believe in having a meaningful flower box complete with revision history even when supposed source control is being used because it does help prevent deploying regressions in code.
In the following code, search for the word "concern".
Here's the redaction I did on your code and it includes the ORGINAL_LOGIN() value for DELETEs, READ THE COMMENTS and also remove the "Concern" comments for your final production code.
Also, I did materialize both of your tables so I could check the syntax of the trigger and make sure it would compile but I have NOT tested it for actual runtime. You still need to do that testing.
CREATE TRIGGER dbo.TR_Audit_SFAProject ON dbo.SFAProject
FOR INSERT,UPDATE,DELETE
AS
/**********************************************************************************************************************
Purpose:
Audit the dbo.SFAProject table for INSERTs, UPDATEs, and DELETES.
Note that the TrnxUserId is the UpdBy column from the source table for INSERTs and UPDATEs. Since that column is NOT
updated during a DELETE, the ORIGINAL_LOGIN() function will be used, instead.
-----------------------------------------------------------------------------------------------------------------------
Revision History:
Rev 00 - 27 Mar 2022 - Adelia
- Original development and unit test.
Rev 01 - 27 Mar 2022 - Adelia
- Simplify the trigger code. Basically, a rewrite including some modified original code.
This includes
- Added early exit if no rows affected by the triggering action.
- Added XACT_ABORT to force automatic transaction rollbacks on any qualifying failure in the
trigger.
- Change the code for DELETEs to use the value from the ORIGINAL_LOGIN() instead of the value
from the deleted row.
**********************************************************************************************************************/--=====================================================================================================================
-- Early actions and presets
--=====================================================================================================================
--===== If an action occurred but no rows were affected, exit early because there are no rows to audit.
IF @@ROWCOUNT = 0 RETURN
;
--===== Environmental presets
SET XACT_ABORT ON; --Force an automatic transaction rollback on any qualifying failure.
SET NOCOUNT ON; --Prevent the return of rowcounts which would interfere with external result sets.
--===== Determine the trigger action to "DRY" the code out.
DECLARE @TriggerAction CHAR(1);
SELECT @TriggerAction = CASE
WHEN NOT EXISTS (SELECT 1 FROM DELETED) THEN 'I' --If nothing in DELETED , is an INSERT.
WHEN NOT EXISTS (SELECT 1 FROM INSERTED) THEN 'D' --If nothing in INSERTED, is a DELETE.
ELSE 'U' --There was 1 or more rows in both logical tables so it's an UPDATE.
END
;
--=====================================================================================================================
-- Record the original rows for UPDATEs and DELETEs from the DELETED logical trigger table.
-- Record the new rows for INSERTs from the INSERTED logical trigger table.
--=====================================================================================================================
IF @TriggerAction IN ('U','D')
BEGIN --===== Record the activity for UPDATEs and DELETEs.
INSERT INTO dbo.SFAProject_Activity
(
Id
,TrnxUserId
,TrnxType
,TenderName
,ClientsId
,SalesPersonId
,SFAStageId
,SFATenderCategoryId
,SubmitDate
,TenderValue
,TenderCost
,TenderRemark
)
SELECT d.Id
,TrnxUserId = IIF(@TriggerAction = 'U',d.UpdBy,ORIGINAL_LOGIN()) --"Guess" who did DELETES. Also, concerned about someone "faking it" here.
,TrnxType = @TriggerAction
,d.TenderName
,d.ClientsId
,d.SalesPersonId
,d.SFAStageId
,d.SFATenderCategoryId
,d.SubmitDate --I'm concerned about this... a person could "fake it". It should be GETDATE() here.
,d.TenderValue
,d.TenderCost
,d.TenderRemark
FROM DELETED d
;
END --End of UPDATE/DELETE audit action
ELSE ---------------------------------------------------------------------------------------------------------------
BEGIN --===== Record the activity for INSERTs, which is what the trigger action has to be at this point.
INSERT INTO dbo.SFAProject_Activity
(
Id
,TrnxUserId
,TrnxType
,TenderName
,ClientsId
,SalesPersonId
,SFAStageId
,SFATenderCategoryId
,SubmitDate
,TenderValue
,TenderCost
,TenderRemark
)
SELECT i.Id
,TrnxUserId = i.CrtBy --I'm concerned that someone cou;d "fake it" here.
,TrnxType = @TriggerAction
,i.TenderName
,i.ClientsId
,i.SalesPersonId
,i.SFAStageId
,i.SFATenderCategoryId
,i.SubmitDate --I'm concerned about this... a person could "fake it". It should be GETDATE() here.
,i.TenderValue
,i.TenderCost
,i.TenderRemark
FROM INSERTED i
;
END --End of INSERT audit action
;
GO
Last but not least, I always question audit systems that audit INSERTs because it's an instant duplication of data. Yep, it makes life a little easier unless you make a view to read from both the source and audit tables (which is also uber easy) and you'll need to do a trick to audit the INSERT and mark it as such in the audit table, but audit tables become HUGE even without instantly doubling the space requirements. Remember that if you insert a row, it will be in the source table until something else happens to the row. There's just no need to make a copy of the inserted row. It's just a waste of space.
--Jeff Moden
Change is inevitable... Change for the better is not.
March 27, 2022 at 6:33 pm
p.s. For those getting ready to recommend temporal tables... I love them because they auto-magically do a lot of what I said especially when it comes time to doing point-in-time lookups and auto-magically making an updateable system view to keep from having to audit inserted rows that are never updated or deleted.
Despite my love for them, I hate them. My problem with them is that it has NO WAY to add extra columns for things like capturing the value of ORIGINAL_LOGIN() at runtime and Microsoft made the horrible error of not automatically including such a thing in the system temporal table. Having code that requires the population of a "ModifiedBy" or similar column in the source table is an invitation to "faking it" and this is also one of the reasons that temporal tables will not stand up in court. It's a bloody shame that more consideration wasn't done in the design of what could have been an incredible feature that has been reduced to spent cannon fodder.
The same goes with those bloody "Instead Of" triggers. I don't have much love for Oracle but their BEFORE triggers would have made this much more effective.
--Jeff Moden
Change is inevitable... Change for the better is not.
March 28, 2022 at 12:01 am
Speculating regarding temporal tables what if you defined ORIGINAL_LOGIN() as the default value of a column in the base table and use a trigger to prevent it from being overridden with a value provided to an INSERT/UPDATE statement? Although the structure of the temporal history table is fixed it's still possible to create additional table(s) with foreign key relations also maintained by TRIGGER(S). The transaction datetime would reference the version row in the history table afaik
The code follows the OP's convention as a "catch all" across INSERT, UPDATE, DELETE. Imo because the code is tailored for each case it would be preferable to create 3 triggers: one for each situation. If the code were actually identical across all 3 cases it could make more sense to remain as one. In the future, for this table, when expanding the Triggers icon in SSMS would the OP be happier with 1 balled up piece of code or 3 functionally separate simple statements? Imo it could be a good tradeoff to substitute framework for code
Aus dem Paradies, das Cantor uns geschaffen, soll uns niemand vertreiben können
March 28, 2022 at 1:32 am
Hi Jeff Moden,
Your Trigger looks working (Insert & Update). But Delete, I need TrnxUserId same like CrtBy and UpdBy. Not User in SQL Server
This is the Front-End looks like
Current SFAProject table as following,
CREATE TABLE [dbo].[SFAProject](
[Id] [int] IDENTITY(1,1) NOT NULL,
[TenderName] [varchar](200) NULL,
[ClientsId] [int] NOT NULL,
[SalesPersonId] [nvarchar](450) NULL,
[SFAStageId] [int] NOT NULL,
[SFATenderCategoryId] [int] NOT NULL,
[SubmitDate] [date] NOT NULL,
[TenderValue] [decimal](18, 2) NULL,
[TenderCost] [decimal](18, 2) NULL,
[TenderRemark] [varchar](500) NULL,
[CrtBy] [nvarchar](450) NULL,
[CrtDte] [datetime] NULL,
[UpdBy] [nvarchar](450) NULL,
[UpdDte] [datetime] NULL,
CONSTRAINT [PK_SFAProject] PRIMARY KEY CLUSTERED
(
[Id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]
GO
Did I need to adding column - DelBy same as CrtBy and UpdBy to access the value for - Who Deleted ?
Please help
March 28, 2022 at 1:55 am
Hi Jeff Moden,
Your Trigger looks working (Insert & Update). But Delete, I need TrnxUserId same like CrtBy and UpdBy. Not User in SQL Server
To do that, you'd first have to update the UpdBy column (or new DelBy) and then do the DELETE or update the row that the DELETE puts in the audit table. Deletes don't provide anything anywhere to capture the "who did the DELETE aspect".
We might also want to change the code for the TrnxUserId in the trigger for the INSERT to get its info from the CrtBy instead of the UpdBy. Have a look at that part of the code, please.
Speculating regarding temporal tables what if you defined ORIGINAL_LOGIN() as the default value of a column in the base table and use a trigger to prevent it from being overridden with a value provided to an INSERT/UPDATE statement? Although the structure of the temporal history table is fixed it's still possible to create additional table(s) with foreign key relations also maintained by TRIGGER(S). The transaction datetime would reference the version row in the history table afaik
The code follows the OP's convention as a "catch all" across INSERT, UPDATE, DELETE. Imo because the code is tailored for each case it would be preferable to create 3 triggers: one for each situation. If the code were actually identical across all 3 cases it could make more sense to remain as one. In the future, for this table, when expanding the Triggers icon in SSMS would the OP be happier with 1 balled up piece of code or 3 functionally separate simple statements? Imo it could be a good tradeoff to substitute framework for code
A default value doesn't do an update once the entry is populated by a default and it needs to change with every action.
And, no... there's really no need for 3 triggers in this case. Having just one audit trigger per table seems good enough, especially if you make a change to the table itself. Then you'd have to update 3 triggers instead of two places in just one.
But, to each there own.
As to putting FKs on an audit table... don't EVER do that. There's not reason to because the rows have already survived the INSERTs and UPDATEs for FKs and FKs would slow down the process even more that having the trigger to begin with and then be further slowed down by writing rows to the audit table. There is zero reason to ever put FKs on the audit table.
--Jeff Moden
Change is inevitable... Change for the better is not.
March 28, 2022 at 2:11 am
I correct the code in the INSERTED section of the trigger. Changed to...
, TrnxUserId = i.CrtBy
--Jeff Moden
Change is inevitable... Change for the better is not.
March 28, 2022 at 2:24 am
As a bit of a sidebar, the "poor man's" method of auditing that includes a variable width NVARCHAR or VARCHAR column to store the name of the person or thing that did an update is the worst idea in the world. Even if everything else were absolutely perfect and there were nothing to cause any page-splits and the ensuing fragmentation, that one column virtually guarantees pages splits ESPECIALLY ON EVER INCREASING INDEXES because the pages on those fill to 100% due to the nature of INSERTs regardless of what the Fill Factor is set to if the most recent rows are the ones that are updated (where a Fill Factor won't help at all!).
If you're going to go through all of this and the trigger, etc, etc, consider using a "user table" that has a unique integer column in it and use the integers for the CrtBy, UpdBy, DelBy, whatever you want to call them. The CrtBy column is NOT an "ExpAnsive Update" issue because it doesn't ever change size after the initial INSERT but it does help to be consistent. 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
March 28, 2022 at 4:08 am
Thanks Jeff
March 28, 2022 at 1:27 pm
A default value doesn't do an update once the entry is populated by a default and it needs to change with every action.
The trigger "prevents" (assuming READ COMMITTED only) overriding the default? It also denies multi-row actions
drop table if exists dbo.temp_orig_login;
go
create table dbo.temp_orig_login(pkCol int identity(1, 1) primary key not null,
origLogin sysname not null default original_login());
go
create or alter trigger dbo.trg_ins_temp_orig_login on dbo.temp_orig_login
after insert, update
as
declare
@row_count int=@@rowcount;
if @row_count=0
return;
if @row_count>1
rollback;
set nocount on
update tl
set origLogin = original_login()
from dbo.temp_orig_login tl
join inserted i on tl.pkCol=i.pkCol
where tl.origLogin<>original_login();
go
/* insert */
insert dbo.temp_orig_login(origLogin) values('John Doe');
select * from dbo.temp_orig_login;
/* update */
update dbo.temp_orig_login
set origLogin = 'Frank Johnson'
where pkCol=1;
select * from dbo.temp_orig_login;
/* insert multi-row*/
/*
insert dbo.temp_orig_login(origLogin) values
('John Doe'),
('John Jay');
select * from dbo.temp_orig_login;
*/
Aus dem Paradies, das Cantor uns geschaffen, soll uns niemand vertreiben können
March 28, 2022 at 4:52 pm
Jeff Moden wrote:A default value doesn't do an update once the entry is populated by a default and it needs to change with every action.
The trigger "prevents" (assuming READ COMMITTED only) overriding the default?
Nope. Not what I said. To simplify what I said, where are you going to get the name of the person that did the DELETE and have it so that the trigger can actually record it? It's NOT going to come from the source table unless you first update the source table and then that will also create and extra row in the audit.
You COULD (which I've done in the past) update only whatever the "Modified By" column is and have the trigger ignore that update if only the "Modified_By" column was changed but, then, if the DELETE is unsuccessful, you have the wrong data in the source column.
The OP wants the trigger to record who did the DELETE and there's no way that I know of to do that directly. You either have to do the trick above in a transaction to prevent bad data if the DELETE fails (and that is twice the work) or you have to use something like the ORIGINAL_LOGIN() like I did.
--Jeff Moden
Change is inevitable... Change for the better is not.
March 28, 2022 at 5:44 pm
As to putting FKs on an audit table... don't EVER do that. There's not reason to because the rows have already survived the INSERTs and UPDATEs for FKs and FKs would slow down the process even more that having the trigger to begin with and then be further slowed down by writing rows to the audit table. There is zero reason to ever put FKs on the audit table.
I disagree, at least in some cases. In some cases I think it's worth having disabled FKs there as documentation (only). They give info to readers but don't cause any system overhead, since they are disabled.
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".
March 28, 2022 at 6:35 pm
As to putting FKs on an audit table... don't EVER do that. There's not reason to because the rows have already survived the INSERTs and UPDATEs for FKs and FKs would slow down the process even more that having the trigger to begin with and then be further slowed down by writing rows to the audit table. There is zero reason to ever put FKs on the audit table.
I disagree, at least in some cases. In some cases I think it's worth having disabled FKs there as documentation (only). They give info to readers but don't cause any system overhead, since they are disabled.
Then what is the point of creating them in the first place? Creating a FK, disabling it, and using it as a documentation mechanism seems like a very round about way of doing things.
Michael L John
If you assassinate a DBA, would you pull a trigger?
To properly post on a forum:
http://www.sqlservercentral.com/articles/61537/
March 28, 2022 at 8:07 pm
ScottPletcher wrote:As to putting FKs on an audit table... don't EVER do that. There's not reason to because the rows have already survived the INSERTs and UPDATEs for FKs and FKs would slow down the process even more that having the trigger to begin with and then be further slowed down by writing rows to the audit table. There is zero reason to ever put FKs on the audit table.
I disagree, at least in some cases. In some cases I think it's worth having disabled FKs there as documentation (only). They give info to readers but don't cause any system overhead, since they are disabled.
Then what is the point of creating them in the first place? Creating a FK, disabling it, and using it as a documentation mechanism seems like a very round about way of doing things.
I stated the reason for creating them - for documentation purposes.
What else would you suggest to document that the relationship between the two tables exists but to prevent it from having any overhead?
I suspect that folks trying later to determine the relationships between tables will find it useful.
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".
March 28, 2022 at 9:06 pm
As to putting FKs on an audit table... don't EVER do that. There's not reason to because the rows have already survived the INSERTs and UPDATEs for FKs and FKs would slow down the process even more that having the trigger to begin with and then be further slowed down by writing rows to the audit table. There is zero reason to ever put FKs on the audit table.
I disagree, at least in some cases. In some cases I think it's worth having disabled FKs there as documentation (only). They give info to readers but don't cause any system overhead, since they are disabled.
I didn't say to remove rows from the audit table. I have seen reference tables change and even be deleted before. There's just no sense in having the audit table extend FK's to any reference tables. All that's going to do is slow things down even if none of FKs are ever changed.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 15 posts - 1 through 15 (of 15 total)
You must be logged in to reply to this topic. Login to reply