| Author |
Topic |
|
mattgosling72
Starting Member
8 Posts |
 Posted - 2012-01-11 : 11:55:34
|
| Hi,I have 'inherited' some stored procedures in a SQL 2000 database, and after succeeding in debugging some problems with one 300+ page thought I would look at streamlining the code and have just got myself confused with how to do it, or if I should just leave it as is.In this particular procedure there is one chunk of code of code that is repeated 400+ times. It is basically trying to get 30 rows of information for any given part. The code gets the latest entry for all parts with a row count higher than 30, puts them into a temp table, deletes the rows in the temp table then decreases a field that holds the rowcount for this part.My immediate thought was to create a loop that would achieve the same in a smaller amount of code, before doing this I thought I would look into potential reasons why this approach would be problematic. Unfortunately all I succeeded in doing was getting myself confused by endless posts about set based (good?) vs cursors(bad?), not really anything on loops and their benefits/problems. Should I just leave it as it is, or could I gain some performance benefits? Are loops in stored procedures generally a bad thing, or is there some other construct I should be looking at? Whilst the code works now I can see problems ahead if any of the parts get to have too many rows for this to work.Let me know if you need any more information, and any advice would be useful.The repeated code is as follows:INSERT INTO injpartlisttemp SELECT MAX(iwserial), iwpart FROM injwocount WHERE iwref > 30 GROUP BY iwpartDELETE FROM injwocountWHERE iwserial = ( SELECT ipserial FROM injpartlisttemp WHERE ipserial = iwserial ) DELETE FROM injpartlisttempthanksMatt |
|
|
Transact Charlie
Master Smack Fu Yak Hacker
3451 Posts |
Posted - 2012-01-11 : 12:11:20
|
Hi there.quote:
Should I just leave it as it is, or could I gain some performance benefits? Are loops in stored procedures generally a bad thing, or is there some other construct I should be looking at?
Almost always - YES.However, if the sp is 300+ lines then working out what the sp *does* (let alone what it should be doing) will be very difficult.What I'd do is:1) work out the interface to the sp. (i.e what it takes in and what it pushes out)2) Write some tests that cover data rules. you could do this either in sql server but they would be better in a c# nunit type test suit. 3) set up some static data that the stored proc operates on (this will be the difficult bit because you want to cover edge cases)4) Then write your own that will satisfy the tests (throwing away the old code)If you already know what the code *SHOULD DO* then you can safely just look at the data you have and the required output. Then work out the best way to do it.I imagine there will be a much, much better way that what you have described that will be much faster as well.If you post sample data and the required output we can help.If you post the current sp's code then someone *may* help but only if they feel like trawling through someone else's terrible code.Charlie===============================================================Msg 3903, Level 16, State 1, Line 1736The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION |
 |
|
|
X002548
Not Just a Number
15586 Posts |
|
|
Transact Charlie
Master Smack Fu Yak Hacker
3451 Posts |
Posted - 2012-01-11 : 12:29:53
|
| Brett is sadly right.If it's not actually a problem then.....why touch it until it is a problem.it still rankles my developer's soul though.Charlie===============================================================Msg 3903, Level 16, State 1, Line 1736The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION |
|
|
|
X002548
Not Just a Number
15586 Posts |
|
|
mattgosling72
Starting Member
8 Posts |
Posted - 2012-01-12 : 03:31:49
|
quote:
Should I just leave it as it is, or could I gain some performance benefits? Are loops in stored procedures generally a bad thing, or is there some other construct I should be looking at?
quote:
Almost always - YES.
Is that YES to all? Should loops be avoided in a sp as a matter of course? Why would that be? Is it just that they are inefficient compared to other methods?As far as this sp is concerned I have worked out what it does and realised that the 'problem' I was getting were down to the script being designed to only run on a Sunday in order to get the correct/expected data. I'll add something to it to automatically change the date variable to the relevant Sunday date, so it can be run on any day.I've also added comments to it.I think I'll (reluctantly) leave it alone and revisit it, if and when there are further problems.Thanks for your help!Matt |
|
|
|
Transact Charlie
Master Smack Fu Yak Hacker
3451 Posts |
Posted - 2012-01-12 : 04:27:55
|
| In general (almost always) a loop is not the 'way'SQL is a declarative language, this means that you should be expressing what you want, not how to get it.This isn't just semantics, it has a real impact.Consider if you wanted to put some sugar into your morning coffeeWhich is the faster way?1) use a spoon to get the sugar into your coffee in one go (set based)2) pick up each individual grain of sugar and place it into your coffee (loop).Relational databases are designed to perform on set based queries. When you write code that only works with individual entities (rows) then you limit yourself in the future when you want to work on sets of those rows.Charlie===============================================================Msg 3903, Level 16, State 1, Line 1736The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION |
|
|
|
X002548
Not Just a Number
15586 Posts |
|
|
mattgosling72
Starting Member
8 Posts |
Posted - 2012-01-13 : 02:57:21
|
quote:
1) use a spoon to get the sugar into your coffee in one go (set based)2) pick up each individual grain of sugar and place it into your coffee (loop).
quote:
You are building a house....you need to start framing the house..you need to start nailing the wood togetherYou nail in the first nail...you then run down to the hardware store and by another nail..you run back to the site and hammer in another nail...you then run BACK to the hardware store and buy another nail..etc
I like the analogies. Makes a lote of sense when put like that!Incidentally it turns out that, although the sp is running correctly, the data being returned by the repeated code is not exactly what is required.It was close enough to have not been noticed for 18 months.So I have to write a new version anyway. At least I have a better understanding now of how to approach it.Thanks again for your help |
|
|
|
Transact Charlie
Master Smack Fu Yak Hacker
3451 Posts |
Posted - 2012-01-13 : 04:15:58
|
| Feel free to post again ( in a different thread ) when you have the requirements and table definitions.Love green-fields!Charlie===============================================================Msg 3903, Level 16, State 1, Line 1736The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION |
|
|
|
|
Loops/Cursors/repeated Code