Link to home
Start Free TrialLog in
Avatar of EastThames
EastThamesFlag for United Kingdom of Great Britain and Northern Ireland

asked on

Cleaning up

Hi all,

Over the last few days I have asked a number of questions on writing this trigger and have got some really really good help which has made it all come together, now I would like some expert advice on the code I have wrote and if it can be cleaned up? to me it looks really crowded. Any help would be greatly appreciated! :)

Dan

ALTER TRIGGER [dbo].[test1]
   ON  [dbo].[Audit_Trg]
   AFTER INSERT
AS

declare @userID varchar(1000)
declare @body varchar(8000)
SET @userID = (SELECT userID from inserted)
SET @body = '**OFFSITE TRANSFER HAS BEEN ENABLED** Please REENABLE through the console!!! ' + ' ' + 'The user who did this is: ' + @userID

IF EXISTS (SELECT Data FROM inserted WHERE Data = 'Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''0''')
BEGIN
EXEC msdb.dbo.sp_send_dbmail
 @recipients='',
 @blind_copy_recipients = '',
 @sensitivity ='Personal',
 @body = @body,
 @importance ='High',
 @subject ='OffSite Transfer',
 @profile_name ='SQLAlerts';
END
IF EXISTS (SELECT Data FROM inserted WHERE Data = 'Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''1''')
BEGIN
EXEC msdb.dbo.sp_send_dbmail
 @recipients='',
 @blind_copy_recipients = '',
 @sensitivity ='Personal',
 @body = '**OFFSITE TRANSFER IS DISABLED**',
 @importance ='High',
 @subject ='OffSite Transfer',
 @profile_name ='SQLAlerts';
END
SOLUTION
Avatar of dqmq
dqmq
Flag of United States of America image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Avatar of Scott Pletcher
Hmm, I'm not quite sure you've gotten "really really good" help :-) , but here's my clean up of what you have.

@@ROWCOUNT gets reset by every SELECT statement, so if using @@ROWCOUNT in a trigger it should be captured FIRST thing in the trigger.

However, from SQL 2005 on, @@ROWCOUNT is not necessarily accurate anyway, so obviously it's best not to rely on it -- too bad, it was a convenient technique.




ALTER TRIGGER [dbo].[test1]
   ON  [dbo].[Audit_Trg]
   AFTER INSERT
AS
SET NOCOUNT ON

--if more than one row inserted, must force rollback, since this trigger cannot accurately handle multiple rows
IF (SELECT COUNT(*) FROM (SELECT TOP 2 * FROM sys.objects) AS derived) = 2
BEGIN
    RAISERROR('Trigger "Audit_Trg" cannot handle multiple rows INSERTed at one time, abending INSERTs.', 16, 1)
    ROLLBACK TRANSACTION
END --IF

DECLARE @body_0 varchar(8000)
SELECT
    @body = '**OFFSITE TRANSFER HAS BEEN ENABLED** Please REENABLE through the console!!! ' + ' ' +
        'The user who did this is: ' + userID

IF EXISTS (SELECT Data FROM inserted WHERE Data = 'Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''0''')
BEGIN
    EXEC msdb.dbo.sp_send_dbmail
         @recipients='',
        @blind_copy_recipients = '',
        @sensitivity ='Personal',
        @body = @body_0,
        @importance ='High',
        @subject ='OffSite Transfer',
        @profile_name ='SQLAlerts'
END --IF
ELSE
IF EXISTS (SELECT Data FROM inserted WHERE Data = 'Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''1''')
BEGIN
    EXEC msdb.dbo.sp_send_dbmail
        @recipients='',
        @blind_copy_recipients = '',
        @sensitivity ='Personal',
        @body = '**OFFSITE TRANSFER IS DISABLED**',
        @importance ='High',
        @subject ='OffSite Transfer',
        @profile_name ='SQLAlerts'
END --IF

GO
ASKER CERTIFIED SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Scott,
Not trying to be adversarial; just trapped between learning and teaching with the same brush stroke.  

>>However, in SQL 2005 and beyond @@ROWCOUNT does not necessarily contain the count of rows affected by the trigger.  

@@ROWCOUNT has always represented the number of rows affected by the preceding SQL statement; never, the number of rows affected by the trigger. That is why it is bad practice to capture it first line of the trigger:  because it's meaningful only in context of the preceding SQL, statement and at the first line of a trigger that's indeterminant

Indeed, @@ROWCOUNT was 100% accurate in v2000 and AFAIK, it's 100% accurate now.  Driven by the advent of SQL statements that are capable of firing multiple triggers, what's changed is our awareness of what @@ROWCOUNT actually represents,  As a result, we've learned that number of rows affected by the SQL statement and number of rows affected by the trigger were equal by coincincence, not by design.

I'm sure we agree that it's no longer a good practice to put @@ROWCOUNT first in a trigger where it is disassociated from the SQL statement preceding it.  I'm just saying that it never was.

 
>>Not sure why you assume I don't know that i means in new statements, but for the record, @@ROWCOUNT gives the *total* rows affected by the statement, both INSERT and UPDATE.

I was not assuming anything, just trying to understand your claim that @@rowcount is inaccurate.  I mean, are there situations where it does not work as documented?


>>huh?
>>For efficiency.  Unlike you, I don't want to go thru every row in inserted just to determine if more than one row was affected.

I still don't understand how your query against the sys.objects table returns and indication of how many rows were updated.  Can you clarify?

As for efficiency, I just used code that was already present to pull in the userid.  I suppose this would have been a trifle more efficient:
select top 2 userid from inserted
But there's already a need to retrieve userid from the inserted table, I don't understand how we gain efficiency by counting rows from sys.objects.

FWIW, I don't think we really have any disagreement here.  The only response I'm seeking is a clarification about your use of sys.objects to count the rows affected by a trigger.
Should be the inserted table, of course:

IF (SELECT COUNT(*) FROM (SELECT TOP 2 * FROM inserted) AS derived) = 2



>> and at the first line of a trigger that's indeterminant <<

That's just false.  In SQL 2000, when entering a trigger, @@ROWCOUNT initially contained the number of rows affected by the statement that fired the trigger.  You seem to be overlooking the fact that the ONLY way to get to a trigger is from a SQL statement that modifies a row(s).  Since a SQL statement could only be one type of modification -- INSERT UPDATE or DELETE -- it was safe to use that value in the trigger.

I think @@ROWCOUNT still contains the number of rows.  The problem now is that the statement may be a MERGE statement, which means @@ROWCOUNT could be a combined total of both INSERTs and UPDATEs.