Go Premium for a chance to win a PS4. Enter to Win

x
?
Solved

Cleaning up

Posted on 2012-03-14
6
Medium Priority
?
206 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 1332 total points
ID: 37720818
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 70

Expert Comment

by:Scott Pletcher
ID: 37721196
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 1332 total points
ID: 37721630
>>@@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
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 70

Assisted Solution

by:Scott Pletcher
Scott Pletcher earned 668 total points
ID: 37721674
>> >>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
ID: 37726313
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 70

Expert Comment

by:Scott Pletcher
ID: 37726629
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

Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Ready to get certified? Check out some courses that help you prepare for third-party exams.
One of the most important things in an application is the query performance. This article intends to give you good tips to improve the performance of your queries.
Via a live example, show how to backup a database, simulate a failure backup the tail of the database transaction log and perform the restore.
Via a live example, show how to setup several different housekeeping processes for a SQL Server.

782 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