Triggers-Implementation Issue

Question

I am making 2 tables for implementing triggers –

create table emp(id int primary key identity(1,1),
name char(40),
salary varchar(50),
gender char(40),
departmentid int);

insert into emp(name,salary,gender,departmentid) values ('jimmi',4800,'Male',4);

create table emp_audit
(
id int primary key identity(1,1),
audit varchar(60)
);
alter trigger trigger_em_update on emp for update 
as begin
Declare @Id int
Declare @oldname char(40),@newname char(40)
Declare @oldsal int,@newsal int
Declare @oldgen char(40),@newgen char(40)
Declare @olddeptid int,@newdeptid int

Declare @auditstring nvarchar(max)


--select * from deleted;

select  * into #temptable from inserted;

while(Exists(select id from #temptable)) --boolean condition if there are rows are not
Begin
    set @auditstring =''

    --if there are many rows we still select the first one

    select Top 1 @Id =id,@newname=name,@newgen=gender,@newsal=salary,@newdeptid=departmentid
    from #temptable;

    select Top 1 @Id =id,@oldname=name,@oldgen=gender,@oldsal=salary,@olddeptid=departmentid
    from deleted where @Id=id;

    set @auditstring=' Employee with id= '+CAST(@Id as varchar(20))+ ' changed '
    if(@oldname<>@newname)
        set @auditstring=@auditstring + 'name from '+ @oldname +' to' +@newname

    if(@oldsal<>@newsal)
        set @auditstring=@auditstring + ' salary from '+ @oldsal +' to  ' +@newsal

    if(@oldgen<>@newgen)
    set @auditstring=@auditstring + '   gender from ' + @oldgen + ' to  ' + @newgen

--  if(@olddeptid<>@newdeptid)
    --set @auditstring=@auditstring + ' departmentid from ' + cast(@olddeptid as nvarchar(5))+' to  ' 

insert into emp_audit values(@auditstring)

delete from #temptable where id=@Id

end
end
when i use update query
update emp set name='vishi',gender='male',salary='4000',departmentid=3 where id=3;

It gives an error

"Conversion failed when converting the nvarchar value ' Employee with id= 3 changed name from james tovishi salary from ' to data type int. " i don't know how to solve this..can you solve this..

In `emp` you declare `salary varchar(50)`, but in the trigger `Declare @oldsal int,@newsal int`. You ought to decide on a single datatype. Adding quotes around values makes interpreting the audit easier, e.g. `from 'Foo ' to 'Foo'`. As mentioned [here](https://stackoverflow.com/questions/11890868/how-to-store-historical-records-in-a-history-table-in-sql-server/11891352#11891352): "Logging triggers should always be set to fire last. Otherwise, a subsequent trigger may rollback the original transaction, but the log table will have already been updated. This is a confusing state of affairs." — HABO, Jul 01 '18 at 15:44

Lukasz Szozda · Answer 1 · 2018-07-01T10:10:55.943

The problem is in line:

if(@oldsal<>@newsal)
    set @auditstring=@auditstring + ' salary from '+ @oldsal +' to  ' +@newsal

Should be:

if(@oldsal<>@newsal)
    set @auditstring=@auditstring+ ' salary from '+CAST(@oldsal AS NVARCHAR(100)) 
       +' to  ' +CAST(@newsal AS NVARCHAR(100))

A couple of thoughts:

It is a good practice to end each statement with semicolon
@oldsal<>@newsal won't detect changing from NULL to value or value to NULL
Row-by-row processing is not best practice from performance perspective(especially inside trigger's body)
set @auditstring=@auditstring +.... could be replaced with set @auditstring += ...
If any value that you are using to concatenate auditstring is NULL variable will be set to NULL.
I do not recommend to store name/variable as CHAR(size), it is better to use VARCHAR(size)
Tracking what has changed as single string will require parsing in the future(unless you only want to display it).

Thanks Lukasz for giving me some tips regarding this.Once i do update in my trigger if problem occurs i will ask again. — Avish, Jul 01 '18 at 10:06
All good thoughts. This can be done with a set-based operation by joining the `inserted` and `deleted` virtual tables on `Id`, using `CASE` expressions to detect changed values, and using `FOR XML` to build the audit string. — Dan Guzman, Jul 01 '18 at 10:09

Gordon Linoff · Answer 2 · 2018-07-01T12:05:08.927

The problem is that one of the columns used with + is a number, probably salary. But your trigger is way too complicated.

SQL is a set-based language. So use the sets when you can:

alter trigger trigger_em_update on emp for update 
as
begin
    insert into emp_audit (auditstring) -- guessing the name
        select ('Employee with id k= ' + CAST(@Id as varchar(20))+ ' changed ' +
                (case when @oldname <> @newname
                      then 'name from '+ @oldname +' to ' + @newname + ' '
                      else ''
                 end) +
                (case when @oldsal <> @oldsal
                      then 'salary from '+ convert(varchar(255), @oldsal) +' to ' + concvert(varchar(255), @newsal) + ' '
                      else ''
                 end) +
                (case when @oldgen <> @newgen
                      then 'gender from '+ convert(varchar(255), @oldgen) +' to ' + concvert(varchar(255), @newgen) + ' '
                      else ''
                 end) +
                (case when @olddeptid <> @newdeptid
                      then 'departmentid from '+ convert(varchar(255), @olddeptid) +' to ' + concvert(varchar(255), @newdeptid) + ' '
                      else ''
                 end)
                ) as auditstring
        from inserted i join
             deleted d
             on i.id = d.id;
end;

Triggers-Implementation Issue

2 Answers2