Trigger assistance

[Matt]

I have a trigger that I created to log changes in one table to another
table but it is horribly inefficient.

I am hoping that someone with more experience than I can see a way to
make this trigger more efficient.

------------

ALTER TRIGGER tContacts_ChangeLog
ON dbo.Contacts
FOR UPDATE
AS

SET NOCOUNT ON

DECLARE @tablename varchar(20),
@record_id_column varchar(30),
@colname varchar(30),
@colvalue varchar(8000),
@insertstmt varchar(1500),
@username varchar(20)

SELECT @tablename = 'Contacts'
SELECT @record_id_column = 'ContactID'

DECLARE columns_cursor CURSOR LOCAL FOR
SELECT COLUMN_NAME
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_NAME = @tablename
AND (POWER(2, (ORDINAL_POSITION-1) % 8) & CONVERT(INT,
SUBSTRING(COLUMNS_UPDATED(), (ORDINAL_POSITION-1)/8 + 1, 1))) <> 0

SELECT * INTO #del FROM deleted
SELECT * INTO #ins FROM inserted
SELECT @username = RIGHT(SYSTEM_USER, LEN(SYSTEM_USER) -
CHARINDEX('\',SYSTEM_USER))

OPEN columns_cursor

FETCH NEXT FROM columns_cursor INTO @colname

WHILE @@FETCH_STATUS = 0
BEGIN
SELECT @insertstmt = 'INSERT INTO ' + @tablename + '_ChangeLog (
recordid, fieldname, changedfrom, changedto, username, datetime ) ' +
'SELECT d.' + @record_id_column + ', ''' + @colname + ''', d.' +
@colname + ', i.' + @colname + ', ''' + @username + ''', GETDATE()' +
'FROM #del d INNER JOIN #ins i ON d.' + @record_id_column + ' = i.' +
@record_id_column + ' WHERE (i.' + @colname + ' <> d.' + @colname + ')'
+
' OR (i.' + @colname + ' IS NOT NULL AND ' + 'd.' + @colname + ' IS
NULL) OR (i.' + @colname + ' IS NULL AND ' + 'd.' + @colname + ' IS NOT
NULL)'

-- INSERT INTO Debug (value) VALUES( @insertstmt )

EXEC( @insertstmt )

FETCH NEXT FROM columns_cursor INTO @colname
END

CLOSE columns_cursor
DEALLOCATE columns_cursor

[Erland Sommarskog]

Matt ([e-mail]) writes:

I have a trigger that I created to log changes in one table to another
table but it is horribly inefficient.

I am hoping that someone with more experience than I can see a way to
make this trigger more efficient.

Looks like you are into auditing. You could possibly make the code
somewhat more efficient by getting the metadata from the system tables
directly, rather than INFORMATION_SHECMA. Also, always make your
cursors STATIC (or INSENSITIVE), unless you have a good reason for
using something else.

You can also improve performance by replacing the temp tables with
table variables. Creating temp tables in triggers can be expensive,
if there are many operations on the table in the same transaction,
particularly if you use SELECT INTO. Declaring primary keys in the
table variables may also be a good thing.

But it's never going to be that effecient. And be aware of that you
need to grant INSERT permissions to all users who will update this
table, even if they access the table through a stored procedure.

So you may have to change approach entirely. Actually, there are
auditing products out there. To mention two, Red Matrix has SQLAudit
(which uses triggers) and Lumigent has Entegra (which gets information
from the transaction log).

--
Erland Sommarskog, SQL Server MVP, es****@sommarskog.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

[Erland Sommarskog]

Matt ([e-mail]) writes:

I have a trigger that I created to log changes in one table to another
table but it is horribly inefficient.

I am hoping that someone with more experience than I can see a way to
make this trigger more efficient.

Looks like you are into auditing. You could possibly make the code
somewhat more efficient by getting the metadata from the system tables
directly, rather than INFORMATION_SHECMA. Also, always make your
cursors STATIC (or INSENSITIVE), unless you have a good reason for
using something else.

You can also improve performance by replacing the temp tables with
table variables. Creating temp tables in triggers can be expensive,
if there are many operations on the table in the same transaction,
particularly if you use SELECT INTO. Declaring primary keys in the
table variables may also be a good thing.

But it's never going to be that effecient. And be aware of that you
need to grant INSERT permissions to all users who will update this
table, even if they access the table through a stored procedure.

So you may have to change approach entirely. Actually, there are
auditing products out there. To mention two, Red Matrix has SQLAudit
(which uses triggers) and Lumigent has Entegra (which gets information
from the transaction log).

--
Erland Sommarskog, SQL Server MVP, es****@sommarskog.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

[David Portas]

You can avoid such horrors by creating static trigger code for each table.
It isn't too difficult to write a scrpt to generate the triggers for you
based on the info schema views. In the long run the time spent designing
such a script is sure to prove worthwhile.

--
David Portas
SQL Server MVP
--

[David Portas]

You can avoid such horrors by creating static trigger code for each table.
It isn't too difficult to write a scrpt to generate the triggers for you
based on the info schema views. In the long run the time spent designing
such a script is sure to prove worthwhile.

--
David Portas
SQL Server MVP
--

[Matt]

The reason I'm using temporary tables is that I have to use EXEC to
execute the select statement, so the inserted and deleted tables aren't
available, but temporary tables are. Can I reference a table variable
like that (in an EXEC statement)? I didn't think that I could.

Fortunately, since I'm using an application role, granting the INSERT
permission to the role is not a big deal.

Thankyou very much for the help.

[Matt]

I am going to stick with this solution until performance on it becomes
an issue, at which point I'll be forced to do as you say. Thankyou for
the advice.

[Erland Sommarskog]

Matt ([e-mail]) writes:

The reason I'm using temporary tables is that I have to use EXEC to
execute the select statement, so the inserted and deleted tables aren't
available, but temporary tables are. Can I reference a table variable
like that (in an EXEC statement)? I didn't think that I could.
That's correct. I was sort of suggesting that you would abandon the
dynamic SQL as well.
I am going to stick with this solution until performance on it becomes
an issue, at which point I'll be forced to do as you say. Thankyou for
the advice.

That's a little funny, since in your first post, you complained that
the current scheme was ineffecient.

--
Erland Sommarskog, SQL Server MVP, es****@sommarskog.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

[Matt]

My apologies for the confusion..right now it is not causing a problem,
but I can see that it certainly could in the future. I like to make
things as efficient as possible, whether they're causing a problem
right now or later...I was hoping someone could suggest something that
was not static. At this moment, it isn't worth it to me to write the
static trigger code because of a lack of time, but I'll be sure to jump
on it if I run into any issues.

Thanks for your time.