January 30, 2016 at 7:58 am
Thanks for the advise Gail. I tried to modify the script.
January 30, 2016 at 8:15 am
You didn't even try to run that...
Msg 156, Level 15, State 1, Procedure TR_Aud_Update_Datetime_Update_User, Line 15
Incorrect syntax near the keyword 'EXISTS'.
Go back and look at Hugo's example of an EXISTS clause:
In this specific case IN and EXISTS are equivalent so you could leave it at that, but I want my code to be consistent - hence the choice to never use IN with subquery. My WHERE clause would be:
WHERE EXISTS(SELECT *
FROM inserted AS i
WHERE i.User_ID = Users.User_ID);
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
January 30, 2016 at 8:34 am
Sorry I was trying to update this from my phone as I had no access to system.
Please see updated script.
USE [Test]
GO
/****** Object: Trigger [dbo].[TR_Aud_Update_Datetime_Update_User] Script Date: 1/30/2016 10:33:30 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
CREATE TRIGGER [dbo].[TR_Aud_Update_Datetime_Update_User]
ON [dbo].[Users]
FOR UPDATE
AS
IF @@ROWCOUNT = 0
RETURN;
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN;
SET NOCOUNT ON;
BEGIN
UPDATE [dbo].[Users]
SET Aud_Update_Datetime=CURRENT_TIMESTAMP, Aud_Update_User=SUSER_SNAME()
WHERE EXISTS
(SELECT * FROM inserted AS i
WHERE i.User_ID = Users.User_ID);
END
GO
January 30, 2016 at 8:52 am
Looks a lot better this way!
Final remarks:
* Try to avoid using [brackets] where they are not needed. In your case, the brackets around [dbo], [Users], and [TR_Aud_Update_Datetime_Update_User] are not needed. On the other hand, since User_ID is a reserverd words I would recommend using [User_ID] where it is used (unless you have the option to change the colum name, which would be my first preference).
* The BEGIN and END in the trigger are superfluous.
* I recommend that you also include the two lines of comments from my sample in the trigger. You want to make sure that your coworkers and successors never open the trigger and add some code at the start - since that would invalidate the @@ROWCOUNT test in the first statement.
January 30, 2016 at 9:00 am
You don't need both of these, they're checking for the same thing.
IF @@ROWCOUNT = 0
RETURN;
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN;
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
January 30, 2016 at 9:04 am
Appreciate your help on this Hugo & Gail!
Updated the script:
USE [Test]
GO
/****** Object: Trigger [dbo].[TR_Aud_Update_Datetime_Update_User] Script Date: 1/30/2016 10:58:39 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER TRIGGER dbo.TR_Aud_Update_Datetime_Update_User
ON [dbo].[Users]
FOR UPDATE
AS
-- Do not put anything before these lines!
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN; -- Change to deleted for ON / AFTER DELETE trigger
-- Rest of the code follows after this line
UPDATE dbo.Users
SET Aud_Update_Datetime=CURRENT_TIMESTAMP, Aud_Update_User=SUSER_SNAME()
WHERE EXISTS
(SELECT *
FROM inserted AS i
WHERE i.User_ID = Users.User_ID);
January 30, 2016 at 9:08 am
GilaMonster (1/30/2016)
You don't need both of these, they're checking for the same thing.
IF @@ROWCOUNT = 0RETURN;
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN;
True on SQL Server 2005 and older.
With a MERGE statement, @@ROWCOUNT can be non-zero but the inserted pseudo-table can be empty. You could argue that the @@ROWCOUNT test is superfluous but I like to start with this to ensure the fastest return from the stored procedure in the majority of no-rows-affected cases.
January 30, 2016 at 9:10 am
SQL!$@w$0ME (1/30/2016)
Appreciate your help on this Hugo & Gail!Updated the script:
USE [Test]
GO
/****** Object: Trigger [dbo].[TR_Aud_Update_Datetime_Update_User] Script Date: 1/30/2016 10:58:39 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER TRIGGER dbo.TR_Aud_Update_Datetime_Update_User
ON [dbo].[Users]
FOR UPDATE
AS
-- Do not put anything before these lines!
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN; -- Change to deleted for ON / AFTER DELETE trigger
-- Rest of the code follows after this line
UPDATE dbo.Users
SET Aud_Update_Datetime=CURRENT_TIMESTAMP, Aud_Update_User=SUSER_SNAME()
WHERE EXISTS
(SELECT *
FROM inserted AS i
WHERE i.User_ID = Users.User_ID);
As you see, Gail and I disagree on the double test of @@ROWCOUNT and EXISTS FROM inserted. Both approaches are valid, just pick one and stick to it.
I would still include the SET NOCOUNT ON at the start of every trigger (after the @@ROWCOUNT test if you decide to bring it back), to make sure that you never get "xx rows affected" messages from your trigger.
January 30, 2016 at 9:20 am
Thanks!
USE [Test]
GO
/****** Object: Trigger [dbo].[TR_Aud_Update_Datetime_Update_User] Script Date: 1/30/2016 11:16:58 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
CREATE TRIGGER [dbo].[TR_Aud_Update_Datetime_Update_User]
ON [dbo].[Users]
FOR UPDATE
AS
-- Do not put anything before these lines!
IF @@ROWCOUNT = 0 RETURN; -- With a MERGE statement, @@ROWCOUNT can be non-zero but the inserted pseudo-table can be empty. Not required on SQL Server 2005 and older
SET NOCOUNT ON; -- To make sure that you never get "xx rows affected" messages from your trigger
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN; -- Change to deleted for ON / AFTER DELETE trigger
-- Rest of the code follows after this line
BEGIN
UPDATE dbo.Users
SET Aud_Update_Datetime=CURRENT_TIMESTAMP, Aud_Update_User=SUSER_SNAME()
WHERE EXISTS
(SELECT *
FROM INSERTED AS i
WHERE i.User_ID = Users.User_ID);
END
GO
January 30, 2016 at 9:39 am
Hugo Kornelis (1/30/2016)
GilaMonster (1/30/2016)
You don't need both of these, they're checking for the same thing.
IF @@ROWCOUNT = 0RETURN;
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN;
True on SQL Server 2005 and older.
With a MERGE statement, @@ROWCOUNT can be non-zero but the inserted pseudo-table can be empty.
In which case, you probably want the EXISTS check, not the @@Rowcount, as the rowcount can be non-zero when no rows were updated (I assume a MERGE that's only deleting). In fact, in that case, might need to ensure that there are rows in both inserted and deleted as that confirms an update was done.
Yes, exists will be slower than Rowcount, but I suspect that's well down to a ms or less.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
January 30, 2016 at 9:50 am
GilaMonster (1/30/2016)
Hugo Kornelis (1/30/2016)
GilaMonster (1/30/2016)
You don't need both of these, they're checking for the same thing.
IF @@ROWCOUNT = 0RETURN;
IF NOT EXISTS (SELECT * FROM INSERTED) RETURN;
True on SQL Server 2005 and older.
With a MERGE statement, @@ROWCOUNT can be non-zero but the inserted pseudo-table can be empty.
In which case, you probably want the EXISTS check, not the @@Rowcount, as the rowcount can be non-zero when no rows were updated (I assume a MERGE that's only deleting). In fact, in that case, might need to ensure that there are rows in both inserted and deleted as that confirms an update was done.
Yes, exists will be slower than Rowcount, but I suspect that's well down to a ms or less.
No need to test "deleted" as well. The behaviour of MERGE with triggers is funny. If the MERGE can do an operation (insert, update, or delete), then the appropriate trigger will fire. In that trigger, @@ROWCOUNT will reflect the number of rows affected by the entire MERGE statement, but the inserted and deleted pseudo-tables will only include the rows affected by that specific operation. So if one row in inserted and one row updated, then the ON INSERT trigger will have only the new row in inserted, and the ON UPDATE trigger will have only the modified row in inserted and deleted.
And you're right that the difference is small, but in stuff that I reuse as a standard pattern I always try to optimize as much as feasible.
January 30, 2016 at 9:55 am
Kinda makes sense. You may gather that I barely use MERGE. I think the last time I wrote one was when I was teaching a class on T-SQL commands.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
January 30, 2016 at 10:42 am
GilaMonster (1/30/2016)
Kinda makes sense. You may gather that I barely use MERGE. I think the last time I wrote one was when I was teaching a class on T-SQL commands.
Numerous good reasons (including unfixed bugs and STILL have the UPSERT race condition!!) to not use MERGE. Such a shame too. But it wasn't fully baked when it was released and it has never gotten the attention it needed/deserved from the product team.
Best,
Kevin G. Boles
SQL Server Consultant
SQL MVP 2007-2012
TheSQLGuru on googles mail service
January 30, 2016 at 1:05 pm
TheSQLGuru (1/30/2016)
Such a shame too. But it wasn't fully baked when it was released and it has never gotten the attention it needed/deserved from the product team.
I could not agree more. The only place I use MERGE is in place of the proprietary UPDATE...JOIN or DELETE...JOIN statements to add protection from error 8672.
Msg 8672, Level 16, State 1, Line 1
The MERGE statement attempted to UPDATE or DELETE the same row more than once. This happens when a target row matches more than one source row. A MERGE statement cannot UPDATE/DELETE the same row of the target table multiple times. Refine the ON clause to ensure a target row matches at most one source row, or use the GROUP BY clause to group the source rows.
I have frequently recalled an Aaron Bertrand post on MERGE-Bugs as a reason to be suspicious of MERGE. His post was almost 3 years ago and I was curious what was still open. It is absolutely amazing that not one of the Bugs Aaron detailed have actually been fixed...
#773895 : MERGE Incorrectly Reports Unique Key Violations - Closed as Won't fix
#766165 : MERGE evaluates filtered index per row, not post operation, which causes filtered index violation - Closed as Won't fix
#723696 : Basic MERGE upsert causing deadlocks - Closed as By design
#713699 : A system assertion check has failed ("cxrowset.cpp":1528) - Closed as Won't fix
#699055 : MERGE query plans allow FK and CHECK constraint violations - Closed as Won't fix
#685800 : Parameterized DELETE and MERGE Allow Foreign Key Constraint Violations - Closed as Won't fix
#635778 : NOT MATCHED and MATCHED parts of a SQL MERGE statement are not optimized - Active
#633132 : MERGE INTO WITH FILTERED SOURCE does not work properly - Active
#596086 : MERGE statement bug when INSERT/DELETE used and filtered index - Active
#583719 : MERGE statement treats non-nullable computed columns incorrectly in some scenarios - Active
I have not encountered any issues myself with the UPDATE...JOIN-usage or DELETE...JOIN-usage of MERGE and it seems vanilla enough that I have been willing to trade the risk for the benefit...but I am beginning to re-think that.
There are no special teachers of virtue, because virtue is taught by the whole community.
--Plato
January 30, 2016 at 2:10 pm
Orlando Colamatteo (1/30/2016)
TheSQLGuru (1/30/2016)
Such a shame too. But it wasn't fully baked when it was released and it has never gotten the attention it needed/deserved from the product team.I could not agree more. The only place I use MERGE is in place of the proprietary UPDATE...JOIN or DELETE...JOIN statements to add protection from error 8672.
I use UPDATE ... JOIN or DELETE ... JOIN and make very sure that I check what I'm getting back and that I have 1..1 matches to the tables being deleted/updated/
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
Viewing 15 posts - 16 through 29 (of 29 total)
You must be logged in to reply to this topic. Login to reply