Trigger issue
-
Hi,
I'm new and trying to get a trigger to save changes, deletes on POPHD to xPOPHD.
The following trigger is updating every occurance of DELWho and DELWHEN for a particular record number - I only want it to update the latest.
CREATE trigger POPHDDelete
on POPHD
after delete,update
as
insert into Xpophd select * from deleted
update Xpophd
set DELWHEN = current_timestamp,
DELWHO = system_user
from deleted join Xpophd
on deleted.id=Xpophd.id
TIA
Richard
-
That would be because that is exactly what you are asking it to do.
In you where clause for your update statement, your only join condition is that the IDs match.
It just looks all wrong to me. That could be something to do with the fact that you INSERT all you deleted records, then try to update them.
Why do you need to update them, by inserting, haven't you actually done exactly what you need?
To test my theory, just comment out the update, delete a record and see what you output is. That could really simplify things for you.
-
The thing I'd like to add to Steve's good comments is that you should not use SELECT *
It's, well... a bit lazy... and the updates will be comparitively slow and you have to find the latest, yada-yada-yada... 
Use the following code to generate a list of the columns and add it to your Insert/Select... then do the timestamp and user name thing during the insert...
SELECT Column_Name + ','
FROM INFORMATION_SCHEMA.Columns
WHERE Table_Name = 'ACNServiceProgression'
ORDER BY Ordinal_Position
--Jeff Moden
RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.Change is inevitable... Change for the better is not.
Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally) -
Using select * on the Inserted/Deleted tables gives me what I want - a copy of the record before Updated/Deleted. This is saved to the xPOPHD table. My timestamp fields DELWHO and DELWHEN are being updated but for all occurances (previous updates) of that record.
Is there some way to link the tables using only the MAX or latest ID of the xPOPHD table.
Richie
-
I agree with Jeff that you should not use SELECT * in the trigger code. Also it isn't a good idea to insert something and then update it, if you can insert it correctly right away.
If you name all the columns explicitly, you can assign DELWHO and DELWHEN required values when inserting the data and there is no need for another update. Only then you can say that it "gives you what you want".
-
The best idea is to add the updates to the insert statement.
INSERT INTO Xpophd
SELECT colA, colB, colC, current_timestamp, system_user
FROM deleted
Fix the column list so that it contains all the columns in xpophd.
FYI, if you had to use the update statement, here's how to do it:
UPDATE Xpophd
SET delwhen = current_timestamp, delwho = system_user
WHERE id IN (SELECT id FROM deleted)
-
Stephanie,
Many thanks for a Complete Answer. I'll work on your suggestions.
I really appreciate the sample code - its the sort of answer i was hoping for.
The other responses were accurate in confirming that I still had a problem but contributed little to solving it.
I'll drop an note back on progress.
Many thanks to all who replied.
Richie
-
Stephanie,
The following code resolved what I was looking for thanks to yourself. It may be lazy code but it works - thanks to your pointers.
I'm relatively new to this so being told that my approach is not the best doesn't help without proper direction - so many thanks for your FYI bit. A simple example was what I was looking for and you gave that.
I agree it's not the best option to Update after Insert and one visit would be perfect so if anyone can improve (by example) I'd really appreciate it. Can I update the DELWHEN, DELWHO fields during a select ?
This forum is a brilliant source of information for all participants but please bear in mind some of us are fairly new and examples help - so please lead by example.
BTW only difference between POPHD and xPOPHD is ARCID (autoincrement)
CREATE trigger POPHDDelete
on POPHD
after delete,update
as
insert into Xpophd select * from deleted
update Xpophd
set DELWHEN = current_timestamp,
DELWHO = system_user
from deleted join Xpophd
on deleted.id=Xpophd.id and
xpophd.arcid IN (SELECT max(arcid) FROM xpophd where deleted.id=Xpophd.id )
Richie
-
Richie
Try this
CREATE trigger POPHDDelete
on POPHD
after delete,update
as
insert into Xpophd
(
col list
, DELWHEN
, DELWHO
)
select
col list
, current_timestamp
, system_user
from
deleted
Stephanie alluded to it earlier that you don't really need the update as you can do it all form the insert.
Also, by specifying the columns as they exist now, you protect you trigger from maintenance issues - for instance someone adds a colun to POPHD, but not to xPOPHD. This prevents a column mismatch.
-
If you run this code, you might just want to have your DBA turn on Trace Flag 1204 just to be safe
one visit would be perfect so if anyone can improve (by example) I'm thinking that Stephanie already gave you a great example... and, I even gave you a way to get all the column names so you don't actually have to type any of them. You need to get cracking and implement what Steve and I suggested and she did.
--Jeff Moden
RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.Change is inevitable... Change for the better is not.
Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally) -
Jeff,
There you go again - providing suggestions without an explanation of their purpose or effect. What does the trace flag do? How do you set it on and what or how do you analyse its output.
In your example what does this table mean ?
WHERE Table_Name = 'ACNServiceProgression'
Richie
-
Yep, sorry about that... I got a little frustrated with you... everyone has been trying to tell you that you're asking for "Death by SQL" by doing the UPDATE right after the INSERT and you keep telling us that you're going to use the code that "works" (your Insert/Update). They also told you that the SELECT * is a bad thing (same reason, would require an Update instead of just an Insert) and that you need to make an explicit column list but you said "it gives me what I want". I understand the tendancy of a newbie to gravitate to the first thing they find that solves their problem but you came here for an answer... would you stop fighting the suggestions and just do them? After all, you did ask for help... why are you fighting it?
Since you're obviously not a DBA, I suggested that you tell him or her to turn on Trace FLAG 1204 so he/she can keep track of all the deadlocks your code may cause... thought that you might even take the time to look it up in Books Online (in Query Analyzer, click on [Help][Trasact-SQL Help], then follow your nose). It's that bad because the code will make a very hot spot (lot's of page locks... maybe even extent locks... ) near the proverbial "end" of the table. And, it's just one table so there will be lot's of contention even though you haven't put the code in an explicit transaction (BEGIN/COMMIT).
And, sorry about the fat fingering... I copied a piece of code I had and forgot to make a change in it for so you could more easily figure out what's going on...
SELECT Column_Name + ','
FROM INFORMATION_SCHEMA.Columns
WHERE Table_Name = 'PutYourTableNameBetweenTheseTwoQuotesRichard'
ORDER BY Ordinal_Position
So, the bottom line is, both Stephanie and Steve have posted the correct code (except for YOUR column names) without knowing what the table your Inserting into looks like (get into the habit of posting the schema for those types of things)... Stepanie implied a full column list with ColA,ColB,ColC. Steve also implied a full column list with ColList. You just need to read what the "generic" example code says and fill in a blank or two. I know you're frustrated (an understandable common newbie ailment) but you have to stop fighting the people that are trying to help you... use either Stepanie's INSERT example (without the UPDATE) or Steve's example...
And get used to thinking things like "Hmmm, that's new... I wonder what INFORMATION_SCHEMA is... lemme look it up in Books Online"...
One final thought...
This forum is a brilliant source of information for all participants but please bear in mind some of us are fairly new and examples help - so please lead by example.
We ARE... you're just not listening
--Jeff Moden
RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.Change is inevitable... Change for the better is not.
Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally) -
Just one more suggestion to the code you posted:
and xpophd.arcid IN (SELECT max(arcid) FROM xpophd where deleted.id=Xpophd.id )
would better be
and xpophd.arcid = (SELECT max(arcid) FROM xpophd where deleted.id=Xpophd.id )
Yes, both will work, but MAX(arcid) will always return only one value, so why treat it as a list? By using = you make the code more readable. (But I still think you should not update anything, but insert the correct values right away).
-
I'd sincerely like to thank you all for your contributions and I do apologise for seeming to fight back at the people suggesting different options but by doing so you all have contributed to delivering a workable solution and food for though for me to develop on.
Put yourself in my shoes - some of the earlier responses only confirmed that I was doing it wrong but only when examples arrive did it begin to make more sense. I have great admiration for the wealth of knowledge in this forum and I will take all suggestions on board and try resolve it the proper way.
So thanks again for all your help.
Best Regards
Richie
-
Understood and appreciate the feed-back but riddle me this... did you change your code so that it does NOT use an UPDATE, yet
--Jeff Moden
RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.Change is inevitable... Change for the better is not.
Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)
Viewing 15 posts - 1 through 15 (of 15 total)
You must be logged in to reply to this topic. Login to reply