[RESOLVED] Please fact-check my simple trigger

[MMock]

I have a small table that has a column called SystemRelease with date/id created and date/id modified.
I don't need a history, but whenever a user edits the SystemRelease I want to capture who is the user and when they updated it.
So I believe it would be this:

CREATE TRIGGER  [dbo].[tg_xtblSystemReleaseRecordUpdated] 
   ON  [dbo].[xtblSystemRelease]
   FOR UPDATE
AS 
DECLARE @Control int
SELECT @Control = [Control] FROM Inserted

UPDATE [xtblSystemRelease] SET ModifiedBy = SUSER_SNAME(), ModifiedDate = getdate() WHERE Control = @Control

GO

I just want to make sure,
1. It's "FOR UPDATE" not "AFTER UPDATE"
2. It's updating the right record. How does it know which record to grab from Inserted if it's such a non-specific name as Control?

Does anything else look wrong? We only have a handful of existing triggers currently in our database, and honestly I am nervous copying one if I don't understand it 100%.
Thanks.

[Zvoni]

Uhuh.

Is it just me, or does this look like an infinite loop?
Someone changes something in xtblSystemRelease (successfully!), the trigger activates, and within the trigger the same table is updated again (trigger activates again?).

As for your question with that "Control"-Thing: Control has to be a column of xtblSystemRelease (PrimaryKey?) otherwise the UPDATE-Statement wouldn't work (Try the UPDATE-Statement manually).

Syntax:
https://docs.microsoft.com/en-us/sql...ql-server-2017

FOR | AFTER
AFTER specifies that the DML trigger fires only when all operations specified in the triggering SQL statement have launched successfully. All referential cascade actions and constraint checks must also succeed before this trigger fires.
AFTER is the default when FOR is the only keyword specified.

So, basically it is an AFTER UPDATE-Statement (twice so, because you're checking the INSERTED-Table)

[Zvoni]

*GROWL*
Couldn't edit my own post (too short?!?!?!?!)


EDIT: Here is a solution to avoid Recursion:
https://dev.to/ravenous_baboon/check...update-trigger

[Zvoni]

OH, you gotta be kidding me.
What's wrong with the forum today????

EDIT2: You do realize, that your trigger is going to fail, if more than one row is UPDATED?
I'm assuming it's MS SQL-Server, so you would have to implement a FOR EACH ROW-Syntax (which MS SQL doesn't support in that way)
See here: https://social.msdn.microsoft.com/Fo...=sqlgetstarted

[MMock]

Okay, thanks. This is why I asked. There are some triggers in our database already so I was using them as an example, but I didn't want to blindly use them without fully understanding.
Regarding recursion, here is an existing trigger:

CREATE TRIGGER [dbo].[tg_EquipmentRecordUpdated] ON [dbo].[tblEquipment] 
FOR INSERT, UPDATE 
AS
DECLARE @EquipmentControl int
SELECT @EquipmentControl = EquipmentControl FROM Inserted

UPDATE tblEquipment SET LastModifiedBy = SUSER_SNAME(), LastModifiedOn = getdate() WHERE EquipmentControl = @EquipmentControl

GO

So you are saying there is a problem with it, because it is updating the same table which an update to caused the trigger in the first place???

[Zvoni]

Yes, but i have to revise my argument with the infinite loop.
Your Trigger gets triggered twice (and not as with my first assumption in an endless loop).

First Trigger --> EquipmentControl has been inserted/updated (as in your current example) --> Update LastMod-Fields
Second Trigger --> LastMod-Fields have been updated. But since you're checking against EquipmentControl, which hasn't been changed in this second step, nothing happens in the UPDATE-Part.

Neverthless, i'd implement some sanity-check
Something like
Aircode in red

CREATE TRIGGER [dbo].[tg_EquipmentRecordUpdated] ON [dbo].[tblEquipment] 
FOR INSERT, UPDATE 
AS
BEGIN
DECLARE @EquipmentControl int
IF EXISTS(SELECT @EquipmentControl = EquipmentControl FROM Inserted) THEN
UPDATE tblEquipment SET LastModifiedBy = SUSER_SNAME(), LastModifiedOn = getdate() WHERE EquipmentControl = @EquipmentControl
END

GO

That way, only if EquipmentControl has been changed/inserted, it enters into the UPDATE-part to update the LastMod-Fields.
It will still get triggered twice, but the second pass will be forked by the IF-Clause, and jumps directly to the end without trying to execute the UPDATE-Statement (as in your version right now).

And you should definitely check for multiple rows being inserted/updated. A trigger gets triggered per Event, not per Row

[FunkyDexter]

so you would have to implement a FOR EACH ROW-Syntax

You don't need to put a loop in, just join to the Inserted table in your update:-

Update T
Set LastModifiedBy = SUSER_SNAME(),
    LastModifiedOn = getdate()
From Inserted I
Join tblEquipment T
    on I.Control = T.Control  --<--I'm assuming Control is the primary key.  If not, do the join on the primary key.
 
--and here's a quick and dirty way of preventing any recursion/double firing etc.
--The trigger will fire twice but only result in an update once:-
Where I.LastModifiedBy <> T.LastModifiedBy
And I.LastModifiedOn <> T.LastModifiedOn
--I'm assuming neither column can contain nulls.  If they can you should handle nulls apropriately

[MMock]

Awesome! Thank you!

[Zvoni]

FD,
Nice!
+1