Solved

Cleaning up

Posted on 2012-03-14
6
159 Views
Last Modified: 2013-01-23
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
0
Comment
Question by:EastThames
  • 3
  • 3
6 Comments
 
LVL 42

Assisted Solution

by:dqmq
dqmq earned 333 total points
Comment Utility
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
 
LVL 69

Expert Comment

by:ScottPletcher
Comment Utility
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
 
LVL 42

Accepted Solution

by:
dqmq earned 333 total points
Comment Utility
>>@@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
Highfive Gives IT Their Time Back

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 
LVL 69

Assisted Solution

by:ScottPletcher
ScottPletcher earned 167 total points
Comment Utility
>> >>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
 
LVL 42

Expert Comment

by:dqmq
Comment Utility
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
 
LVL 69

Expert Comment

by:ScottPletcher
Comment Utility
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

Featured Post

How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

Join & Write a Comment

Composite queries are used to retrieve the results from joining multiple queries after applying any filters. UNION, INTERSECT, MINUS, and UNION ALL are some of the operators used to get certain desired results.‚Äč
Slowly Changing Dimension Transformation component in data task flow is very useful for us to manage and control how data changes in SSIS.
This video shows, step by step, how to configure Oracle Heterogeneous Services via the Generic Gateway Agent in order to make a connection from an Oracle session and access a remote SQL Server database table.
Via a live example, show how to setup several different housekeeping processes for a SQL Server.

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

17 Experts available now in Live!

Get 1:1 Help Now