Dear SQLers:

I have a basic trigger that does most of what I want. I am
wondering if it is good design.

I prefer to have the one trigger instead of two nearly-identical
triggers. That is why I have @AccessType. Good or bad? Is there a
better way of determining between insert and update?

What sort of error handling should I have? While this code does
work for multiple rows, I need a handling for if one or more
inserts/updates fail, and it should be something that returns
reasonable detail to the application.

***** Start of Code *****
create trigger trgIUAccounts on Accounts
instead of insert, update
as
begin
declare @AccessType int -- 0: row insert, 1: row update
if (select count(*) from Deleted)>0
select @AccessType=1
else
select @AccessType=0

-- NiceString() Parameters
declare @RetVal int
declare @strRaw nvarchar(max)
declare @strNice nvarchar(max)

declare #Inserted insensitive cursor
for select ACUK,ACOrd,ACName,ACNr from Inserted

-- Current Row's Column Values
declare @ACUK nvarchar(4)
declare @ACOrd int
declare @ACName nvarchar(40)
declare @ACNr nvarchar(20)

open #Inserted

fetch next from #Inserted into @ACUK,@ACOrd,@ACName,@ACNr

while @@fetch_status=0
begin
select @strRaw=@ACName
execute @RetVal=NiceString @strRaw,@strNice output,0,1,1
if @RetVal>0 and @strRaw<>@strNice
select @ACName=@strNice
-- else
-- begin
-- --***** error
-- end
--*****TODO: Do the same for ACNr.
if @AccessType=0
insert into Accounts
(ACUK,ACOrd,ACName,ACNr)
values
(@ACUK,@ACOrd,@ACName,@ACNr)
else
update Accounts
set ACOrd=@ACOrd,ACName=@ACName,ACNr=@ACNr
where ACUK=@ACUK

fetch next from #Inserted into @ACUK,@ACOrd,@ACName,@ACNr
end

close #Inserted
end
***** End of Code *****

Sincerely,

Gene Wirchenko

Gene Wirchenko (genew (AT) ocis (DOT) net) writes:
You are on the right track, but this is better:

IF exists (SELECT * FROM deleted)

No need to actually count the rows!

By default when you are in a trigger, the first error will abort the batch
and rollback the transaction.

Good boy!

This is actually better in a way:

DECLARE cur CURSOR STATIC LOCAL

STATIC and INSENSITIVE have - as far as I know - the same meaning, the
rows are copied to a work table in tempdb. This gives a predictable and
understandable behaviour. The default cursor is a dynamic cursor, and
it's good that avoided this trap.

The reason that STATIC is better is that this proprietary keyword
(INSENSITIVE is ANSI) permits you to specify LOCAL which means that
the cursor disappears when then scope exits, no matter how it exits.
The default is global which means that the cursor still exists. Which
can cause nasty surprises next time.

In fact, I would expect this to happen to you, because you only have
CLOSE at the end. You need DEALLOCATE instead. With CLOSE the cursor
still exists.

# in the beginning of the cursor name is a little odd.


I prefer to write this as


WHILE 1 = 1
BEGIN
FETCH cur ...
IF @@fetch_status <> 0
BREAK

Then you only need FETCH statement, and one less line to change if you add
one more column to the cursor.



--
Erland Sommarskog, SQL Server MVP, esquel (AT) sommarskog (DOT) se

Links for SQL Server Books Online:
SQL 2008: http://msdn.microsoft.com/en-us/sqlserver/cc514207.aspx
SQL 2005: http://msdn.microsoft.com/en-us/sqlserver/bb895970.aspx

On Wed, 13 Apr 2011 00:09:39 +0200, Erland Sommarskog
<esquel (AT) sommarskog (DOT) se> wrote:

Lazy is good. Thank you.

That will do for now.

[snip]

I was thinking it would then be temporary. Does that not work
with cursors?

[snip]

Ugly, but yes on maintainability.

Sincerely,

Gene Wirchenko

Gene Wirchenko (genew (AT) ocis (DOT) net) writes:
Nope. Actually, since you asked, I had to test:

create procedure testis as
declare #cur cursor for select name from sys.columns
open #cur
fetch #cur
go
exec testis
exec testis
go
deallocate #cur
drop procedure testis



--
Erland Sommarskog, SQL Server MVP, esquel (AT) sommarskog (DOT) se

Books Online for SQL Server 2005 at
http://www.microsoft.com/technet/pro...ads/books.mspx
Books Online for SQL Server 2000 at
http://www.microsoft.com/sql/prodinf...ons/books.mspx