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
LVL 1
EastThamesAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

dqmqCommented:
Sorry to rain on the parade, but I don't like the logical structure of that trigger.  Let me try to describe my complaint by describing the logic that I see:

When one or more rows are inserted to the audit table, generate zero, one, or two emails identifying a userid randomly chosen from the inserted rows.

While you certainly can consolidate the two IF EXISTS statements, it produces a logically different result in the case multiple audit rows are inserted at the same time.  If you want to claim that will never happen, then I offer this:

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

declare @userID varchar(1000)
declare @body varchar(8000)
SELECT @userID=userID from inserted
IF @@rowcount <> 1
     begin
     RAISERROR (N'Too many rows inserted at one time',10, 1);
     rollback tran;
    end

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 in ('Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''0''', 'Profile=''*VMDIR\VM.INI'', Section=''Info'', Key=''SystemOffSiteTransfer'', Value=''1'''))
BEGIN
EXEC msdb.dbo.sp_send_dbmail
 @recipients='',
 @blind_copy_recipients = '',
 @sensitivity ='Personal',
 @body = @body,
 @importance ='High',
 @subject ='OffSite Transfer',
 @profile_name ='SQLAlerts';
END
0
Scott PletcherSenior DBACommented:
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
0
dqmqCommented:
>>@@ROWCOUNT gets reset by every SELECT statement, so if using @@ROWCOUNT in a trigger it should be captured FIRST thing in the trigger.

I disagree--it's a bad practice to capture @@ROWCOUNT without immediately preceding it by an SQL statement.  Inside a trigger, I would not put it first, precisely because there is some @@ROWCOUNT can represent something besides the number of affected rows in the triggered table.

To count the number of inserted rows, this works quite well:
SELECT @userID=userID from inserted
IF @@rowcount <> 1

>>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.

Can you cite any evidence?  Or do we just need to better-understand what it represents in the case of new SQL statements that can insert and update in a single operation?


>IF (SELECT COUNT(*) FROM (SELECT TOP 2 * FROM sys.objects) AS derived) = 2
Huh?
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
Ultimate Tool Kit for Technology Solution Provider

Broken down into practical pointers and step-by-step instructions, the IT Service Excellence Tool Kit delivers expert advice for technology solution providers. Get your free copy now.

Scott PletcherSenior DBACommented:
>> >>Can you cite any evidence?  Or do we just need to better-understand what it represents in the case of new SQL statements that can insert and update in a single operation?
<<

In SQL 2000, you could use @@ROWCOUNT first thing in a trigger to get the count of rows affected by the trigger.  It WAS 100% accurate in SQL 2000.

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

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.


>> 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.
0
dqmqCommented:
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.
0
Scott PletcherSenior DBACommented:
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.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Microsoft SQL Server

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.