|
database
newsgroups
|
|||||||||||||||||||||||
|
|||||||||||||||||||||||
trigger doesn't seem to work w/cursorThe purpose of this trigger is this; When "UNITNBR" gets updated, I want to change the ID field to a concatenation of the new UNITNBR and some other fields. So, this is what I've got: =============================== CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS FOR UPDATE AS DECLARE @NEWID VARCHAR(100), @OLDID VARCHAR(100), @XTEST VARCHAR(50), @XUNIT CHAR(10), @XRESULT VARCHAR(50), @DT DATETIME, @YYYY CHAR(4), @MM CHAR(2), @DD CHAR(2) DECLARE CUR_UPD CURSOR FOR SELECT ID,UNITNBR,TEST,RESULT,DRAWDATE FROM Inserted IF UPDATE(UNITNBR) BEGIN OPEN CUR_UPD FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT WHILE @@FETCH_STATUS=0 SET @YYYY=CAST(DATEPART(YEAR,@DT) AS CHAR(4)) SET @MM=CAST(DATEPART(MONTH,@DT) AS CHAR(2)) SET @DD=CAST(DATEPART(DAY, @DT) AS CHAR(2)) IF LEN(@DD)=1 BEGIN SET @DD='0'+@DD END IF LEN(@MM)=1 BEGIN SET @MM='0'+@MM END SET @NEWID=@XUNIT+@YYYY+@MM+@DD+@XTEST+@XRESULT UPDATE TBLRESULTS SET TBLRESULTS.ID=@NEWID WHERE ID=@OLDID FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT END CLOSE CUR_UPD DEALLOCATE CUR_UPD =============================== The table has approx 150,000 records. To test, I've used the query analyzer to execute "UPDATE TBLRESULTS SET UNITNBR='XXXXXXXXXX' WHERE UNITNBR='YYYYYYYYYY'" There are only a few records that should be updated. Yet, when I run this, it runs forever. Previously, I had this set up without the cursor, and it would only update the very last 'Inserted' record. Could someone give me a pointer on what's wrong here? Thanks Keith Keith,
All you are doing is setting the value @YYYY over and over and over and over again... Oops. :( You need to put what you want to happen within WHILE @@FETCH_STATUS=0 inside a BEGIN..END block. Just indenting the code won't do it. (We've all done this kind of thing at some point... ) Steve Kass Drew University Keith wrote: Show quote >Hi >The purpose of this trigger is this; >When "UNITNBR" gets updated, I want to change the ID field to a >concatenation of the new UNITNBR and some other fields. So, this is what >I've got: > >=============================== >CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS >FOR UPDATE > >AS > >DECLARE @NEWID VARCHAR(100), > @OLDID VARCHAR(100), > @XTEST VARCHAR(50), > @XUNIT CHAR(10), > @XRESULT VARCHAR(50), > @DT DATETIME, > @YYYY CHAR(4), > @MM CHAR(2), > @DD CHAR(2) > >DECLARE CUR_UPD CURSOR FOR >SELECT ID,UNITNBR,TEST,RESULT,DRAWDATE FROM Inserted > >IF UPDATE(UNITNBR) >BEGIN > >OPEN CUR_UPD >FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > >WHILE @@FETCH_STATUS=0 > > SET @YYYY=CAST(DATEPART(YEAR,@DT) AS CHAR(4)) > SET @MM=CAST(DATEPART(MONTH,@DT) AS CHAR(2)) > SET @DD=CAST(DATEPART(DAY, @DT) AS CHAR(2)) > > IF LEN(@DD)=1 > BEGIN > SET @DD='0'+@DD > END > IF LEN(@MM)=1 > BEGIN > SET @MM='0'+@MM > END > > SET @NEWID=@XUNIT+@YYYY+@MM+@DD+@XTEST+@XRESULT > > UPDATE TBLRESULTS > SET TBLRESULTS.ID=@NEWID > WHERE ID=@OLDID > > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > >END > >CLOSE CUR_UPD >DEALLOCATE CUR_UPD >=============================== > >The table has approx 150,000 records. To test, I've used the query analyzer >to execute "UPDATE TBLRESULTS SET UNITNBR='XXXXXXXXXX' WHERE >UNITNBR='YYYYYYYYYY'" >There are only a few records that should be updated. Yet, when I run this, >it runs forever. >Previously, I had this set up without the cursor, and it would only update >the very last 'Inserted' record. >Could someone give me a pointer on what's wrong here? >Thanks >Keith > > > > > > > > > > > > > Thanks,
That was what I needed to get it to work. A second pair of eyes... It took less than 2 minutes to update 1036 records out of approx 150,000 in the table, which is perfectly acceptible as this will only be triggered occasionally. Keith Show quote "Steve Kass" wrote: > Keith, > > All you are doing is setting the value @YYYY over and over > and over and over again... Oops. :( > > You need to put what you want to happen within > WHILE @@FETCH_STATUS=0 inside a BEGIN..END > block. Just indenting the code won't do it. > > (We've all done this kind of thing at some point... ) > > Steve Kass > Drew University > > > Keith wrote: > > >Hi > >The purpose of this trigger is this; > >When "UNITNBR" gets updated, I want to change the ID field to a > >concatenation of the new UNITNBR and some other fields. So, this is what > >I've got: > > > >=============================== > >CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS > >FOR UPDATE > > > >AS > > > >DECLARE @NEWID VARCHAR(100), > > @OLDID VARCHAR(100), > > @XTEST VARCHAR(50), > > @XUNIT CHAR(10), > > @XRESULT VARCHAR(50), > > @DT DATETIME, > > @YYYY CHAR(4), > > @MM CHAR(2), > > @DD CHAR(2) > > > >DECLARE CUR_UPD CURSOR FOR > >SELECT ID,UNITNBR,TEST,RESULT,DRAWDATE FROM Inserted > > > >IF UPDATE(UNITNBR) > >BEGIN > > > >OPEN CUR_UPD > >FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > > >WHILE @@FETCH_STATUS=0 > > > > SET @YYYY=CAST(DATEPART(YEAR,@DT) AS CHAR(4)) > > SET @MM=CAST(DATEPART(MONTH,@DT) AS CHAR(2)) > > SET @DD=CAST(DATEPART(DAY, @DT) AS CHAR(2)) > > > > IF LEN(@DD)=1 > > BEGIN > > SET @DD='0'+@DD > > END > > IF LEN(@MM)=1 > > BEGIN > > SET @MM='0'+@MM > > END > > > > SET @NEWID=@XUNIT+@YYYY+@MM+@DD+@XTEST+@XRESULT > > > > UPDATE TBLRESULTS > > SET TBLRESULTS.ID=@NEWID > > WHERE ID=@OLDID > > > > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > > >END > > > >CLOSE CUR_UPD > >DEALLOCATE CUR_UPD > >=============================== > > > >The table has approx 150,000 records. To test, I've used the query analyzer > >to execute "UPDATE TBLRESULTS SET UNITNBR='XXXXXXXXXX' WHERE > >UNITNBR='YYYYYYYYYY'" > >There are only a few records that should be updated. Yet, when I run this, > >it runs forever. > >Previously, I had this set up without the cursor, and it would only update > >the very last 'Inserted' record. > >Could someone give me a pointer on what's wrong here? > >Thanks > >Keith > > > > > > > > > > > > > > > > > > > > > > > > > > > Due do not knwoing anything about your DDL and logical structure of the
database, i want to give you a common advise not to use cursors in that example. That´s a quite good example not to use cursors, because there arent any operations which can´t be done in a setbased statement. Yesterday I had a cursor issue where we switched over to a setbased operations which saved us for 170000 rows just about 1 3/4 (Ran with the cursor in 1 3/4 hours, with a set based in 5 Minutes) hours of time. Show quote "Keith" <Ke***@discussions.microsoft.com> schrieb im Newsbeitrag news:470B36FE-9F77-472A-B620-02A3EC5F4D06@microsoft.com... > Hi > The purpose of this trigger is this; > When "UNITNBR" gets updated, I want to change the ID field to a > concatenation of the new UNITNBR and some other fields. So, this is what > I've got: > > =============================== > CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS > FOR UPDATE > > AS > > DECLARE @NEWID VARCHAR(100), > @OLDID VARCHAR(100), > @XTEST VARCHAR(50), > @XUNIT CHAR(10), > @XRESULT VARCHAR(50), > @DT DATETIME, > @YYYY CHAR(4), > @MM CHAR(2), > @DD CHAR(2) > > DECLARE CUR_UPD CURSOR FOR > SELECT ID,UNITNBR,TEST,RESULT,DRAWDATE FROM Inserted > > IF UPDATE(UNITNBR) > BEGIN > > OPEN CUR_UPD > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > WHILE @@FETCH_STATUS=0 > > SET @YYYY=CAST(DATEPART(YEAR,@DT) AS CHAR(4)) > SET @MM=CAST(DATEPART(MONTH,@DT) AS CHAR(2)) > SET @DD=CAST(DATEPART(DAY, @DT) AS CHAR(2)) > > IF LEN(@DD)=1 > BEGIN > SET @DD='0'+@DD > END > IF LEN(@MM)=1 > BEGIN > SET @MM='0'+@MM > END > > SET @NEWID=@XUNIT+@YYYY+@MM+@DD+@XTEST+@XRESULT > > UPDATE TBLRESULTS > SET TBLRESULTS.ID=@NEWID > WHERE ID=@OLDID > > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > END > > CLOSE CUR_UPD > DEALLOCATE CUR_UPD > =============================== > > The table has approx 150,000 records. To test, I've used the query > analyzer > to execute "UPDATE TBLRESULTS SET UNITNBR='XXXXXXXXXX' WHERE > UNITNBR='YYYYYYYYYY'" > There are only a few records that should be updated. Yet, when I run > this, > it runs forever. > Previously, I had this set up without the cursor, and it would only update > the very last 'Inserted' record. > Could someone give me a pointer on what's wrong here? > Thanks > Keith > > > > > > > > > > > Hi
Thanks for the tip. Could you elaborate on what you mean by "set based" ? Keith Show quote "Jens Süßmeyer" wrote: > Due do not knwoing anything about your DDL and logical structure of the > database, i want to give you a common advise not to use cursors in that > example. That´s a quite good example not to use cursors, because there arent > any operations which can´t be done in a setbased statement. Yesterday I had > a cursor issue where we switched over to a setbased operations which saved > us for 170000 rows just about 1 3/4 (Ran with the cursor in 1 3/4 hours, > with a set based in 5 Minutes) hours of time. > > -- > HTH, Jens Suessmeyer. > > --- > http://www.sqlserver2005.de > --- > "Keith" <Ke***@discussions.microsoft.com> schrieb im Newsbeitrag > news:470B36FE-9F77-472A-B620-02A3EC5F4D06@microsoft.com... > > Hi > > The purpose of this trigger is this; > > When "UNITNBR" gets updated, I want to change the ID field to a > > concatenation of the new UNITNBR and some other fields. So, this is what > > I've got: > > > > =============================== > > CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS > > FOR UPDATE > > > > AS > > > > DECLARE @NEWID VARCHAR(100), > > @OLDID VARCHAR(100), > > @XTEST VARCHAR(50), > > @XUNIT CHAR(10), > > @XRESULT VARCHAR(50), > > @DT DATETIME, > > @YYYY CHAR(4), > > @MM CHAR(2), > > @DD CHAR(2) > > > > DECLARE CUR_UPD CURSOR FOR > > SELECT ID,UNITNBR,TEST,RESULT,DRAWDATE FROM Inserted > > > > IF UPDATE(UNITNBR) > > BEGIN > > > > OPEN CUR_UPD > > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > > > WHILE @@FETCH_STATUS=0 > > > > SET @YYYY=CAST(DATEPART(YEAR,@DT) AS CHAR(4)) > > SET @MM=CAST(DATEPART(MONTH,@DT) AS CHAR(2)) > > SET @DD=CAST(DATEPART(DAY, @DT) AS CHAR(2)) > > > > IF LEN(@DD)=1 > > BEGIN > > SET @DD='0'+@DD > > END > > IF LEN(@MM)=1 > > BEGIN > > SET @MM='0'+@MM > > END > > > > SET @NEWID=@XUNIT+@YYYY+@MM+@DD+@XTEST+@XRESULT > > > > UPDATE TBLRESULTS > > SET TBLRESULTS.ID=@NEWID > > WHERE ID=@OLDID > > > > FETCH NEXT FROM CUR_UPD INTO @OLDID,@XUNIT,@XTEST,@XRESULT,@DT > > > > END > > > > CLOSE CUR_UPD > > DEALLOCATE CUR_UPD > > =============================== > > > > The table has approx 150,000 records. To test, I've used the query > > analyzer > > to execute "UPDATE TBLRESULTS SET UNITNBR='XXXXXXXXXX' WHERE > > UNITNBR='YYYYYYYYYY'" > > There are only a few records that should be updated. Yet, when I run > > this, > > it runs forever. > > Previously, I had this set up without the cursor, and it would only update > > the very last 'Inserted' record. > > Could someone give me a pointer on what's wrong here? > > Thanks > > Keith > > > > > > > > > > > > > > > > > > > > > > > > > Something like this. Don't use cursors in triggers:
CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS FOR UPDATE AS UPDATE dbo.tblresults SET id = unitnbr + CAST(DATEPART(YEAR,drawdate) AS CHAR(4))+ + RIGHT('0'+CAST(DATEPART(MONTH,drawdate) AS VARCHAR(2)),2) + RIGHT('0'+CAST(DATEPART(DAY, drawdate) AS VARCHAR(2)),2) + test + result WHERE EXISTS (SELECT * FROM Inserted WHERE id = dbo.tblresults.id) GO This is a bit better than what you had originally but still not good. Why don't you put this "ID" column in a view rather than computing it on-the-fly? However you construct the value this seems a very poor choice of "key" if that's what it's supposed to be. -- David Portas SQL Server MVP -- Thanks for the info, I'll give that a shot, though I was able to get the
trigger to work right, and it took less than 2 minutes to find and change 1036 records out of approx 150,000. The ID isn't a key, it's just a quick reference used by another process to tell if a record should be imported. It's a shortcut; the other process combines the columns into a string and does a search for a match in the ID column rather than a match across 4 columns... for example... SELECT * FROM TBLRESULTS WHERE ID=THISSTRING, vs SELECT * FROM TBLRESULTS WHERE COL1=stringA AND COL2=stringB AND COL3=stringC AND COL4=stringD. Thanks again Keith Show quote "David Portas" wrote: > Something like this. Don't use cursors in triggers: > > CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS > FOR UPDATE > AS > > UPDATE dbo.tblresults > SET id = unitnbr > + CAST(DATEPART(YEAR,drawdate) AS CHAR(4))+ > + RIGHT('0'+CAST(DATEPART(MONTH,drawdate) AS VARCHAR(2)),2) > + RIGHT('0'+CAST(DATEPART(DAY, drawdate) AS VARCHAR(2)),2) > + test > + result > WHERE EXISTS > (SELECT * > FROM Inserted > WHERE id = dbo.tblresults.id) > > GO > > This is a bit better than what you had originally but still not good. > Why don't you put this "ID" column in a view rather than computing it > on-the-fly? However you construct the value this seems a very poor > choice of "key" if that's what it's supposed to be. > > -- > David Portas > SQL Server MVP > -- > > > SELECT * FROM The second of those is the more efficient option since the index and> TBLRESULTS WHERE ID=THISSTRING > vs > > SELECT * FROM TBLRESULTS WHERE > COL1=stringA AND COL2=stringB AND COL3=stringC AND COL4=stringD statistics for those columns can be used for other queries plus it avoids bloating your indexes and tables with redundant data. If the ID column isn't indexed then there's no benefit to creating it at all, just put it in a view. In general, it doesn't make sense to concatenate columns for searches. -- David Portas SQL Server MVP -- Thanks again David,
I tried out your method, without the cursor, and it worked well, about 1 second, versus just under 2 minutes for the cursor method. I'll take into consideration your notes on the ID column as well, but for now, it would be a bit of work to get rid of it, with not much real reward; performance isn't much of an issue here. Thanks Keith Show quote "David Portas" wrote: > Something like this. Don't use cursors in triggers: > > CREATE TRIGGER TRG_UPDATE_ID ON dbo.tblRESULTS > FOR UPDATE > AS > > UPDATE dbo.tblresults > SET id = unitnbr > + CAST(DATEPART(YEAR,drawdate) AS CHAR(4))+ > + RIGHT('0'+CAST(DATEPART(MONTH,drawdate) AS VARCHAR(2)),2) > + RIGHT('0'+CAST(DATEPART(DAY, drawdate) AS VARCHAR(2)),2) > + test > + result > WHERE EXISTS > (SELECT * > FROM Inserted > WHERE id = dbo.tblresults.id) > > GO > > This is a bit better than what you had originally but still not good. > Why don't you put this "ID" column in a view rather than computing it > on-the-fly? However you construct the value this seems a very poor > choice of "key" if that's what it's supposed to be. > > -- > David Portas > SQL Server MVP > -- > > >> Could you elaborate on what you mean by "set based" ? << Get a copy of SQL PROGRAMMING STYLE; I have a chapter on how to startthinking in sets instead of procedures. When you can make the jump, your code will improve by orders of magnitude.  |
|||||||||||||||||||||||
Other interesting topics