December 20, 2006 at 11:41 am
Comments posted here are about the content posted at http://www.sqlservercentral.com/columnists/kramot/2773.asp
February 12, 2007 at 1:53 am
Nigel Rivett's technique is worth taking a look at.. http://www.simple-talk.com/sql/database-administration/pop-rivetts-sql-server-faq-no.5-pop-on-the-audit-trail/
Best wishes,
Phil Factor
February 12, 2007 at 3:26 am
we had a long discussion about cursors vs loops, a while loop is still a cursor and I doubt the impact within a single trigger would be much different.
My view = if you want to destroy your database performance add triggers - note that update triggers force all updates to be deferred , so no inplace updates, not sure if that is still so in sql 2005, this can give problems with transactional replication for instance.
[font="Comic Sans MS"]The GrumpyOldDBA[/font]
www.grumpyolddba.co.uk
http://sqlblogcasts.com/blogs/grumpyolddba/
February 13, 2007 at 7:57 am
I have to say that like colin this sort of operations are to be considered *very* well before you jump the gun. High Performance DB don't take this sort of things easily , you have to *work* to make it happen. (Been there done that)
For a small thing that is not too heavily abused you could use whatever makes you happy.
* Noel
February 13, 2007 at 5:41 pm
February 14, 2007 at 1:09 am
to be honest cursor / loop = row by agonising row operations within triggers would be my worst nightmare for a production database. The overhead of deferred updates is also critical as this will cause fragmentation of table/index structures thus degrading performance further. We did the threads over loops and cursors so I'm not going to comment further. Itzik Ben-Gan has some good examples of where a cursor works faster and better, however, that's not really part of this thread.
I agree auditing is important, SOX, and it's tricky, and there are not too many alternatives, but triggers have, in my experience, not been the best solution.
[font="Comic Sans MS"]The GrumpyOldDBA[/font]
www.grumpyolddba.co.uk
http://sqlblogcasts.com/blogs/grumpyolddba/
February 14, 2007 at 2:20 am
I think that Keren has a good point. A lot of database developers are dealing with the task of revising existing code in difficult circumstances, or trying to tie down a tricky bug, and I have experienced times myself when such code has been extremely useful. However, I'd never actually design a database with this sort of code in it. Who would?
Best wishes,
Phil Factor
February 15, 2007 at 6:07 am
A few other questions, as someone mentioned this isn't the ideal and I have to put auditing into our applications sometime soon!
When designing from scratch is the auditing best done as part of the calling stored procedure or is it best in the calling application?
Assuming the database structure is large and complex would the data still be pivoted or replicate the table structure (perhaps in a separate database/server) or storing versions in the same table?
How important is user friendliness when viewing of the audit trail (does anyone have any experience of end users wanting to view a record of changes without tech support)?
What additional functionality would you expect to provide such as archiving etc.
Phil Nicholas
August 8, 2007 at 12:49 pm
Hello forum,
I have a couple comments on Karen's posting. First, bravo for taking on the hotly controversial topic of handling change logging with triggers - only the brave tread here. And I think that you've adequately cautioned your readers that this is not the ideal solution, as most would agree that if you designed a brand new database with change logging you would abstract the application's direct access to the tables entirely and handle both the updates and change logging entirely through a batch of sp's.
I have taken Karen's code as a template and devised a solution in a client's existing database. This is a case where I cannot change all the client-sourced update commands (which quite often reference the tables directly), so the solution here had to be based on triggers. This is also a database that typically updates relatively few rows at a time and has plenty of horsepower on the server so there will be no appreciable performance degredation to the end user. There are rare situations where bulk updates are performed, but I was careful to make sure that all these new triggers were diabled prior to - and reenabled after - the bulk update process is run.
I did also find one teensy little error in Karen's code: Just before the WHILE loop, where the @CurrentRowID and @tmpname variables are initialized with this statement.
SELECT @CurrentRowId = colid,
@tmpName = name
FROM syscolumns
WHERE colid = @NextRowId
The where clause is not specific enough to zero in on one row. The colid column in syscolumns is definitely not unique in the table. Here's a suggestion:
SELECT @CurrentRowId = colid, @tmpName = name
FROM syscolumns
WHERE id = object_id('''+@TableName+''') and objectproperty(id, ''IsUserTable'') = 1 and colid = @NextRowId
I think this is what Karen had in mind.
Lastly, thanks Karen for a great article! Even though this solution has a specific niche where it might fit perfectly, we should know that there are very few "silver bullet" solutions out there for SQL Server and should carefully measure EVERY solution we borrow from someone else before throwing it into production!
August 8, 2007 at 1:23 pm
Sorry I forgot one more thing: I noticed that Karen's tbl_log (and the code that writes to it) doesn't include the PK(s) for the table the change came from. This makes it difficult at best to use the information gathered in your audit table to trace back to the row in the base table that the change was performed on.
This was not difficult to solve. In my particular database, I know I have composite PK's with a maximum of 5 columns. So I added PK1, PK2, PK3, PK4 and PK5 columns to my version of tbl_log, datatypes of varchar(60) or so. Then in the trigger code, just before spinning through the columns in the table to see which ones have changed, I get the PK column values like so:
--Loop through the PK columns for the table and grab values from the temp tables for each
select @PKpos = min(ordinal_position), @PKposmax = max(ordinal_position)
from information_schema.key_column_usage
where objectproperty(object_id(constraint_name),''IsPrimaryKey'') = 1
and table_name = '''+@TableName+'''
while @PKpos <= @PKposmax
begin
select @PKval = null
select @PKcolname = quotename(column_name)
from information_schema.key_column_usage
where objectproperty(object_id(constraint_name),''IsPrimaryKey'') = 1
and table_name = '''+@TableName+''' and ordinal_position = @PKpos
if exists(select * from #inserted'+@VarRandom+')
select @subsql = N''select @PKval = convert(varchar(60), d.''
+ @PKcolname+'') from #inserted'+@VarRandom+' d ''
else
select @subsql = N''select @PKval = convert(varchar(60), d.''
+ @PKcolname+'') from #deleted'+@VarRandom+' d ''
exec sp_executesql @subsql, N''@PKval varchar(60) output'', @PKval output
if @PKpos = 1
set @PK1val = @PKval
else if @PKpos = 2
set @PK2val = @PKval
else if @PKpos = 3
set @PK3val = @PKval
else if @PKpos = 4
set @PK4val = @PKval
else if @PKpos = 5
set @PK5val = @PKval
select @PKpos = @PKpos+1
end
As you might guess I then wrote the @PK1val, @PK2val,... values to my ChangeLog table in corresponding columns. It was not really necessary to record what the column name was for each part of the composite because I can derive that easily by the table name. I can now join the ChangeLog table directly back to the row in the base table where the change was originally performed for reporting.
October 30, 2007 at 2:09 am
Thank you Bob. I've been feeling a little ganged up on till you and Phil Factor came along. I knew that there must be someone out there who had or will encounter similar circumstances as I have and may find this article useful. Over time I've grown skeptical of that assumption, but your feedback had really made it worth my while. I am very glad you enjoyed the article and found it useful and I appreciate you posting your elaboration on the code.
Thank you very much
K
January 18, 2008 at 2:25 pm
I unfortunately have ntext fields in the table(s) that need to be audited and I've been trying to find a way to get around it.
My attempt (I can't call it a solution because it doesn't work) is to try to just insert the fields that aren't ntext, text or image into a temp table. I do this by getting the names of the fields for the table being audited from the syscolumns table and put them in a variable.
My code looks like this:
CREATE TABLE #TableCache (
columnName varchar(400)
)
INSERT INTO #TableCache
SELECT '[' + sc.name + ']' as columnName
FROM sysobjects as so JOIN syscolumns as sc
ON so.id = sc.id
WHERE so.xtype = 'U' AND sc.xtype NOT IN ( 99, 35, 34) AND so.name = @TableName
AND sc.name not in ('Created', 'Modified', 'RowVersion')
SET @FieldList = ''
-- create a cursor
DECLARE tableColumn CURSOR FOR
SELECT *
FROM #TableCache
-- loop over the cursor
-- and get a list of the columns in the table that are not ntext, text or image
OPEN tableColumn
FETCH NEXT FROM tableColumn INTO @columnName
WHILE @@FETCH_STATUS = 0
BEGIN
if len(@FieldList) = 0
begin
SET @FieldList = '[' + @columnName + ']'
end
else
begin
SET @FieldList = @FieldList + ', [' + @ColumnName + ']'
end
-- next row, please
FETCH NEXT FROM tableColumn INTO @columnName
END
print @FieldList
-- get list of columns
Set @sql ='SELECT ' + @FieldList + ' INTO #ins FROM inserted'
EXEC (@SQL)
The trigger errors on the last line stating invalid object inserted. (NOTE: This code is inside a trigger)
If I try to use this:
SELECT @FieldList INTO #ins FROM inserted
I get No column was specified for column 1 of '#ins'.
Any ideas?
July 30, 2008 at 9:44 am
i get Incorrect syntax near '+@VarRandom+'.
July 30, 2008 at 6:42 pm
I would say you’d have to be more specific then that. But before you do that, it’s possible the formatting on the page changed the code. So use the following... see how it does.
CREATE PROCEDURE dbo.SPRouterForLog
(
@SPName nvarchar(50),
@RecordId nvarchar(10),
@str1 nvarchar(4000),
@TableName nvarchar(100),
@OpName nvarchar(50),
@PageName nvarchar(50)
) AS
declare @TrriggerCreate varchar(8000)
declare @VarRandom nvarchar(50)
set @VarRandom=ltrim(str(replace(Rand(), '.', '')))
set @TrriggerCreate='CREATE TRIGGER [PreUpdateTrigger] ON dbo.'+@TableName+'
FOR UPDATE
AS
DECLARE
@iReturnCode int,
@iNextRowId int,
@iCurrentRowId int,
@iLoopControl int,
@old varchar(75),
@new varchar(75),
@tmpName sysname,
@data_type nvarchar(30)
SELECT * INTO #deleted'+@VarRandom+' FROM deleted
SELECT * INTO #inserted'+@VarRandom+' FROM inserted
SELECT @iLoopControl = 1
SELECT @iNextRowId = MIN(colid)
FROM syscolumns
where id = object_id('''+@TableName+''')
SELECT @iCurrentRowId = colid,
@tmpName = name
FROM syscolumns
WHERE colid = @iNextRowId
WHILE @iLoopControl = 1
BEGIN
SELECT @iNextRowId = NULL
SELECT @iNextRowId = MIN(colid)
FROM syscolumns
WHERE colid > @iCurrentRowId and id = object_id('''+@TableName+''')
if (substring (columns_updated(), 1+ round ((@iCurrentRowId - 1) / 8, 0), 1) & power (2, (@iCurrentRowId - 1) % 8) <> 0 )
begin
create table #pass'+@VarRandom+' (tempvar sysname null)
create table #pass1'+@VarRandom+' (tempvar sysname null)
set @data_type=null
select @data_type=data_type FROM information_schema.columns WHERE table_name='''+@TableName+''' and Column_Name=@tmpName
if (@data_type <> ''money'' )
Begin
exec (''insert #pass'+@VarRandom+' select d.''+@tmpName+'' from #deleted'+@VarRandom+' as d
insert #pass1'+@VarRandom+' select i.''+@tmpName+'' from #inserted'+@VarRandom+' as i'')
end
else
begin
exec (''insert #pass'+@VarRandom+' select cast(d.''+@tmpName+'' as varchar) from #deleted'+@VarRandom+' as d
insert #pass1'+@VarRandom+' select cast(i.''+@tmpName+'' as varchar) from #inserted'+@VarRandom+' as i'')
End
select @old = tempvar from #pass'+@VarRandom+'
select @new = tempvar from #pass1'+@VarRandom+'
drop table #pass1'+@VarRandom+'
drop table #pass'+@VarRandom+'
if @old is null
begin
set @old='' ''
end
if @new<>@old
begin
insert into tbl_log (TableName,RecordNumber,ActionBy,ActionPage,ChangeClmn,OldValue,NewValue,ActionDate) values ('''+@TableName+''','+@RecordId+','''+@OpName+''','''+@PageName+''',@tmpName,@old,@new,getdate())
end
end
IF ISNULL(@iNextRowId,0) = 0
BEGIN
BREAK
END
SELECT @iCurrentRowId = colid,
@tmpName = name
FROM syscolumns
WHERE colid = @iNextRowId and id = object_id('''+@TableName+''')
end
drop table #deleted'+@VarRandom+'
drop table #inserted'+@VarRandom+'
'
EXEC(@TrriggerCreate)
declare @strSqlExec nvarchar(4000)
set @strSqlExec=@SPName+' '+@str1
EXEC (@strSqlExec)
drop trigger PreUpdateTrigger
GO
July 31, 2008 at 4:03 pm
Thank you for your reply Keren.
I believe I see the error of my ways....I was attempting to use a portion of your code within a trigger.
To be precise the section:
declare @VarRandom NVARCHAR(50)
set @VarRandom=ltrim(str(replace(Rand(), '.', '')))
SELECT * INTO #deleted'+@VarRandom+' FROM deleted
SELECT * INTO #inserted'+@VarRandom+' FROM inserted
this is where I get the syntax error. Is the above possible within a trigger?
Viewing 15 posts - 1 through 15 (of 15 total)
You must be logged in to reply to this topic. Login to reply