CURSOR v WHILE for beginners

Question

CURSOR v WHILE for beginners

Viewing 11 posts - 16 through 25 (of 25 total)

You must be logged in to reply to this topic. Login to reply

Jeff Moden SSC Guru Points: 1004406 More actions · Answer 1

Heh... feed the vendor a nice juicy pork-chop... with a sling-shot! 😉

--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)

DonaldW SSCarpal Tunnel Points: 4525 More actions · Answer 2

Jeff Moden (12/19/2007)
Heh... feed the vendor a nice juicy pork-chop... with a sling-shot! 😉

ROFL! That will at least get their attention.

Jeff Moden SSC Guru Points: 1004406 More actions · Answer 3

Brett,

Obviously, a couple of us have run into the problems associated with "Sequence Tables" such as you've described... if you would, please, generate the table creation for your sequence table and check out the following URL to generate some data we can test with, please...

http://www.sqlservercentral.com/articles/Best+Practices/61537/

--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)

stuart.adair SSC-Addicted Points: 416 More actions · Answer 4

-- there may be loss of formatting

-- I've used 4 sapce tab

-- firstly I don't like loops but do prefer them over cursors

-- for data movement set based solutions are what you should be trying to do

-- below is response to you question how do I do this while loop

-- personally I would not use a loop unless I had to

SET NOCOUNT ON;

DECLARE -- one declare it less typing

@PHONEID INT

,@PERSONID INT

,@PERSLEGACYACCOUNTID INT-- for loop

,@PHONETYPE VARCHAR(40)-- for loop

,@PHONENUMBER VARCHAR(20)-- for loop

,@LoopCount int-- for loop

,@BatchDatedatetime-- for loop

-- Create # table to act as working list

-- Would normally create this with a CREATE TABLE statement but do not have the details

-- if smallish record set and particulary if X64 machine consider a Table variable here - this will be in memory

SELECT

PHON_TYPE

,LEFT(PHON_NUMBER,20)AS PHON_NUMBER

,PHON_PERSLEGACYACCOUNTID

INTO #loopList

FROM

dbo.ZZIMPPHONE a

WHERE

EXISTS (SELECT 1 FROM dbo.PERSON z WHERE a.PHON_PERSLEGACYACCOUNTID = b.PERS_LEGACYACCOUNTID);

-- this would normally be part of the create table statement

ALTER TABLE #loopList

ADD RowID int IDENTITY (1,1);

--Add an index on RowID if a large data set and it would benefit from an index

-- Prepare cycle count -- prefer this method over deleting the records and using select top 1 less I/O

-- always use SELECT over SET it far more efficient at allocating values to variables and you can set more than one value at a time

SELECT

@LoopCount = 0

,@BatchDate = GETDATE()-- more efficient to get the value once

--While loop - use exists because it most efficient for checking if one or more records exists

WHILE EXISTS(SELECT 1 FROM #loopList WHERE RowID > @LoopCount)

BEGIN

SELECT @LoopCount = @LoopCount + 1-- to pick the next record in the list

-- get working values

SELECT

@PHONETYPE = PHON_TYPE

,@PHONENUMBER = PHON_NUMBER

,@PERSLEGACYACCOUNTID = PHON_PERSLEGACYACCOUNTID

FROM

#loopList

WHERE

RowID = @LoopCount

-- In the next line of code

-- Are you getting a unique sequence value that you are going to use to insert into the table?

-- Or are you getting the same value every time?

-- If it is the latter, I believe there is no need for a loop at all. But I would need more info to show you why.

EXEC @PHONEID = dbo.eware_get_identity_id 'Phone';

SELECT -- don't like top 1 you cannot guarantee the same result every time and it is not efficient approach

@PERSONID = (SELECT TOP 1 PERS_PERSONID FROM PERSON WHERE PERS_LEGACYACCOUNTID = @PERSLEGACYACCOUNTID);

-- this should be a proc that you are calling so your code is modular

INSERT INTO dbo.Phone

(

Phon_PhoneId,

,Phon_PersonID

,Phon_Type

,Phon_Number

,Phon_CreatedBy

,Phon_CreatedDate

,Phon_UpdatedBy

,Phon_UpdatedDate

,Phon_TimeStamp

)

SELECT -- use SELECT as it allows me see data when troubleshooting I always lable as it helps the reader (efficiency should stop at the code performance)

@PHONEIDAS Phon_PhoneId

,@PERSONIDAS Phon_PersonID

,@PHONETYPEAS Phon_Type

,@PHONENUMBERAS Phon_Number

,1AS Phon_CreatedBy

,@BatchDateAS Phon_CreatedDate

,1AS Phon_UpdatedBy

,@BatchDateAS Phon_UpdatedDate

,@BatchDateAS Phon_TimeStamp;

END

-- don't forget to drop your temp table

DROP TABLE #loopList

/*

I don't concur with others in the use of a function as this is not efficient and should be only followed if

this the optimal way to achieve the result. If you are not shore then I suggest you avoid if you have

not got time to investigate multiple solutions.

I recently re-wrote someone's code that had used functions in the select list columns and

reduced its execution time from over 10 minutes to less than 3. The reason lies in the way the SQL Server

engine builds the execution plan. The performance issue lies in the number of time the function is called

or whether it can use an index. It is even worse if the function is porely writen.

Also avoid them in where clauses or joins. There is a time and palce for everything and this is a subject that we could spend pages discussing.

*/

I hope you found this useful

Jeff Moden SSC Guru Points: 1004406 More actions · Answer 5

Basically it is a proprietary SP provided by a SQL based web application for the purpose of generating unique integer based row IDs. It does several things including calling other SP’s for record locking, remote client id range updates as well index updates… so I don’t think so, but here is the code nonetheless:

Oh my... I missed that large chunk of code... I also have to say that's gotta be the worst "GetNextID" proc I've ever seen.

I don't know if I'll have the time to write a decent code example for a replacement to be used by your procs, Brett, but I'll try to make some.

--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)

brett.davis1 SSChasing Mays Points: 608 More actions · Answer 6

Wow guys thanks so much for everyones help.

In regards to the questions around get_next_CRM_ID, I dont really have a level of skills that would make me feel comfortable bypassing and/or optimising it. Aside from that, the vendor has explicitly said that not using it is unsupported.

That being said, if someone feels like having a stab at optimising the procedure or writing insert procedures as per the original topic that can use a more efficient alternative, I'd be happy to take it on board and maybe even put forward some suggestions to the vendor - due to our current status with the vendor we have a considerable amount of input in to these kind of things so may be able to make a difference.

In terms of the original script, I unfortunately haven't noticed any significant difference worth writing home about in terms of execution time after changing it from a cursor to a while loop, but at least it has definitely opened some doors for me in terms of dealing with some other similar situations.

Next challenge for me is getting my head around how to do my code in a set based fashion instead of row by row... but hey, everyone has to start somewhere right?

And compliments of the season to you all!!

Brett

Jeff Moden SSC Guru Points: 1004406 More actions · Answer 7

In terms of the original script, I unfortunately haven't noticed any significant difference worth writing home about in terms of execution time after changing it from a cursor to a while loop, but at least it has definitely opened some doors for me in terms of dealing with some other similar situations.

Heh... that's because a cursor isn't much more than a Temp table with a While loop... the cursor uses slightly more resources so it's a bit slower, but replacing cursors with While loops is a bit like spitting into a strong wind. 😀

Next challenge for me is getting my head around how to do my code in a set based fashion instead of row by row... but hey, everyone has to start somewhere right?

The only way you could do that is to have the vendor make the NextID code so that you can "reserve" groups of ID's... I think they have the basic structure to allow that but we'd need more info on how to reserve by "increment".

They do a heck of a lot of unnecessary locking... like I said, I'll see if I can come up with a working example of how sequence tables should be incremented.

--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)

DonaldW SSCarpal Tunnel Points: 4525 More actions · Answer 8

Jeff said: "They do a heck of a lot of unnecessary locking... like I said, I'll see if I can come up with a working example of how sequence tables should be incremented."

One thing that puzzles me is the range of ids reserved for each table. It appears that they all dip from the same pool of numbers instead of each table being encapsulated beginning with 1 and going to n.

If there were not that reserved range business one could dump the records to be added to a temp table with an Identity column whose seed is the last id used + 1. That would be the next phoneid. From there the data could be inserted into the Phone table en mass and the last id used table would be updated. Looks to me like they are going around the world for something that should be pretty straightforward. Of course, that's looking at it from the outside.

Jeff Moden SSC Guru Points: 1004406 More actions · Answer 9

The seed should probably the value returned from the NextID proc...

Other problem is, you have to increment the NextID in the NextIDtable by the number of rows you have in the temp table... in order to keep from messing up the works when multiple connections call the NextID proc, you must provide the increment at that time or deadlocks are sure to occur.

--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)

DonaldW SSCarpal Tunnel Points: 4525 More actions · Answer 10

Jeff Moden (12/21/2007)
The seed should probably the value returned from the NextID proc...
Other problem is, you have to increment the NextID in the NextIDtable by the number of rows you have in the temp table... in order to keep from messing up the works when multiple connections call the NextID proc, you must provide the increment at that time or deadlocks are sure to occur.

You are right, I hadn't thought about multiple connections.

Assuming the eware_get_identity_id sproc can be updated: get the number of records to be inserted, then pass the record count to the eware_get_identity_id sproc along with the table name. Eware_get_identity_id gets the current value and locks the table then updates it to that value + record count passed in and returns the starting value which is then used as the seed value for Identity. Only one lock to the NextID per group of records to be inserted instead of one per RBAR. The calculation of the phoneid is then done as a set op and the insert performed as one transaction.

This of course ignores the whole range-of-values thing. If that has to be maintained things get tricker. Might have to create a Values table ... Hmm, requires more thought.

I'm working on this in my head and I have a feeling I've overlooked something else. When I get the chance to hammer at it in SSMS I'll see how it works.

"I'd make a mental note but I don't have anything to write on."

Matt Miller (4) SSC Guru Points: 124210 More actions · Answer 11

Jeff Moden (12/21/2007)
The only way you could do that is to have the vendor make the NextID code so that you can "reserve" groups of ID's... I think they have the basic structure to allow that but we'd need more info on how to reserve by "increment".
They do a heck of a lot of unnecessary locking... like I said, I'll see if I can come up with a working example of how sequence tables should be incremented.

That's what I thought, but then again - I would lock things too if I had a process run for 20 minutes adding records. Like you said - it's sad to see - but it's all related.

----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?