CURSOR question

I have written my first stored proc using a cursor and am wondering if I did
it correctly. It does work, but I wanted to make sure I am using it
correctly and that this code is optimized or if there was a simpler way to
do it (maybe without cursors). Basically what I needed to do was loop
through a recordset and execute an SP against each record found. Any advice
is appreciated, here is my SP...
--create stored procedure
create proc udpTest
@.cdid integer --CDID
AS
DECLARE @.eid integer --EventID
--create the cursor
DECLARE CursEvent CURSOR FOR
SELECT EventID FROM tblEvent
WHERE EventCDID = @.cdid
OPEN CursEvent
-- Perform the first fetch
FETCH NEXT FROM CursEvent INTO @.eid
-- Check @.@.FETCH_STATUS to see if there are any more rows to fetch
WHILE @.@.FETCH_STATUS = 0
BEGIN
--Run SP
EXECUTE udpTwo @.eid
FETCH NEXT FROM CursEvent INTO @.eid
END
--close the cursor
CLOSE CursEvent
DEALLOCATE CursEvent
GOOn Wed, 10 Aug 2005 02:21:05 -0400, Mark Hoffy wrote:

>I have written my first stored proc using a cursor and am wondering if I di
d
>it correctly.
Hi Mark,
Probably not. In 99% of all cases, the choice to use a cursor is
incorrect.

>It does work, but I wanted to make sure I am using it
>correctly and that this code is optimized or if there was a simpler way to
>do it (maybe without cursors).
Exactly. Use the strength of SQL to process the whole set at once and
let the optimizer figure out the most efficient way to do it.

>Basically what I needed to do was loop
>through a recordset and execute an SP against each record found.
No - what you needed to do was write one single query that will do
whatever the SP does, but for all rows at once.

>Any advice
>is appreciated, here is my SP...
If you really want some good advice, then post the stored proc that you
are calling for each row in the cursor as well. Add to that the
structure of all tables involved (as CREATE TABLE statements - omit
columns that are irrelevant in this case, but do include all constraints
and properties), some illustrative sample data (as INSERT statements)
and a short description of the business requirements you are trying to
satisfy.
See www.aspfaq.com/5006 for some more comments on the best way to get
help from the groups.
Best, Hugo
--
(Remove _NO_ and _SPAM_ to get my e-mail address)|||No - what I needed to do was loop through a recordset and execute an SP
against each record found. I can ask my own questions - thanks.
The stored proc that is being called is called from several different places
in different forms. Even if my solution was to create an SP for each
instance, it would mean writing several versions of nearly the same proc and
having to maintain the logic in multiple places. Also, the proc called is
very complex, incorporating a number of lookups while updating several
tables. If it were a simple single-line-of-logic sp, I would agree and
would never have approached a cursor solution.
If anyone has any comments on my cursor code or other ways to implement
this, I would be happy to read them. Thanks.
"Hugo Kornelis" <hugo@.pe_NO_rFact.in_SPAM_fo> wrote in message
news:p8cnf15huc04sohldc31qimc8qufl09o6i@.
4ax.com...
> On Wed, 10 Aug 2005 02:21:05 -0400, Mark Hoffy wrote:
>
did
> Hi Mark,
> Probably not. In 99% of all cases, the choice to use a cursor is
> incorrect.
>
to
> Exactly. Use the strength of SQL to process the whole set at once and
> let the optimizer figure out the most efficient way to do it.
>
> No - what you needed to do was write one single query that will do
> whatever the SP does, but for all rows at once.
>
> If you really want some good advice, then post the stored proc that you
> are calling for each row in the cursor as well. Add to that the
> structure of all tables involved (as CREATE TABLE statements - omit
> columns that are irrelevant in this case, but do include all constraints
> and properties), some illustrative sample data (as INSERT statements)
> and a short description of the business requirements you are trying to
> satisfy.
> See www.aspfaq.com/5006 for some more comments on the best way to get
> help from the groups.
> Best, Hugo
> --
> (Remove _NO_ and _SPAM_ to get my e-mail address)|||On Thu, 11 Aug 2005 17:51:40 -0400, Mark Hoffy wrote:

>No - what I needed to do was loop through a recordset and execute an SP
>against each record found. I can ask my own questions - thanks.
Well, you did write "any advice is appreciated".
Sorry for bothering you.
Best, Hugo
--
(Remove _NO_ and _SPAM_ to get my e-mail address)|||>> No - what I needed to do was loop through a recordset and execute an SP agains
t each record,[sic] found. I can ask my own questions - thanks. <<
Please post DDL, so that people do not have to guess what the keys,
constraints, Declarative Referential Integrity, data types, etc. in
your schema are. Sample data is also a good idea, along with clear
specifications. It is very hard to debug code when you do not let us
see it.
I agree with Hugo.
In all the DECADES -- not years-- I have been involved with SQL, I have
written five cursors. If the CASE expression had been available, I
would have avoid three of the five. Is this an NP-Complete problem?
So what? If the procedures are logically different, then they nee dtpo
be in separate modules. This is basic software engineering and is far,
far more fundamental than SQL practices.
Because you do not know a record is not a row, and describe a procedure
with poor cohesion, I think you are a procedural programmer that cannot
think of a relational solution. You also put "tbl-" prefixes on
singular names, and have other signs of OO and 1960's BASIC thinking.
Again, we cannot fix code that we cannot see. Post the procedure and a
clear spec.|||I'll refrain from the "do not use cursors" bit (you've seen that in the
other replies ;o) )
Other than that and assuming that in this case you do need a cursor to call
the second sp, you might consider adding
LOCAL FAST_FORWARD
to your cursor call. Other than that I think it's basically right...
Greets, Lee-Z
"Mark Hoffy" <mark@.here.com> wrote in message news:OmKKe.6$F_7.1@.fe06.lga...
>I have written my first stored proc using a cursor and am wondering if I
>did
> it correctly. It does work, but I wanted to make sure I am using it
> correctly and that this code is optimized or if there was a simpler way to
> do it (maybe without cursors). Basically what I needed to do was loop
> through a recordset and execute an SP against each record found. Any
> advice
> is appreciated, here is my SP...
> --create stored procedure
> create proc udpTest
> @.cdid integer --CDID
> AS
> DECLARE @.eid integer --EventID
> --create the cursor
> DECLARE CursEvent CURSOR FOR
> SELECT EventID FROM tblEvent
> WHERE EventCDID = @.cdid
> OPEN CursEvent
> -- Perform the first fetch
> FETCH NEXT FROM CursEvent INTO @.eid
> -- Check @.@.FETCH_STATUS to see if there are any more rows to fetch
> WHILE @.@.FETCH_STATUS = 0
> BEGIN
> --Run SP
> EXECUTE udpTwo @.eid
> FETCH NEXT FROM CursEvent INTO @.eid
> END
> --close the cursor
> CLOSE CursEvent
> DEALLOCATE CursEvent
> GO
>|||None of what you've said here rules out a set-based solution. If you
have a legacy proc designed to work only with single rows then you have
to decide whether it's worth a re-write or if iyou can live with the
cursor solution. The "better" solution is to avoid designing-in these
limitations in the first place.
David Portas
SQL Server MVP
--|||You should use a local cursor. A local cursor is closed and deallocated
when it goes out of scope, whereas a global cursor stays allocated and open
for as long as the connection is open, unless explicitly closed and
deallocated. (DECLARE @.CursEvent CURSOR SET @.CursEvent = CURSOR LOCAL
FAST_FORWARD FOR SELECT ...). If an error occurs that terminates the batch,
a local cursor will be closed and deallocated, whereas a global cursor
remains open. Note: you should still explicitly close and deallocate a
local cursor, because you should free resources as soon as you're finished
with them.
You should add error handling. If the SP returns a value (in a RETURN
statement) then you should check that value (EXEC @.RC = udpTwo @.eid). You
should also check @.@.ERROR after the EXEC statement (IF @.RC != 0 OR @.@.ERROR
!= 0 GOTO ERROR). In addition, you should check @.@.CURSOR_ROWS before
executing the first fetch. There's no point in entering the fetch loop if
there's nothing to do. (Note that @.@.CURSOR_ROWS may return a negative
number if the cursor is asynchronously populated, so you should use
@.@.CURSOR_ROWS != 0 instead of @.@.CURSOR_ROWS > 0 to determine whether you
should proceed into the fetch loop.)
"Mark Hoffy" <mark@.here.com> wrote in message news:OmKKe.6$F_7.1@.fe06.lga...
> I have written my first stored proc using a cursor and am wondering if I
did
> it correctly. It does work, but I wanted to make sure I am using it
> correctly and that this code is optimized or if there was a simpler way to
> do it (maybe without cursors). Basically what I needed to do was loop
> through a recordset and execute an SP against each record found. Any
advice
> is appreciated, here is my SP...
> --create stored procedure
> create proc udpTest
> @.cdid integer --CDID
> AS
> DECLARE @.eid integer --EventID
> --create the cursor
> DECLARE CursEvent CURSOR FOR
> SELECT EventID FROM tblEvent
> WHERE EventCDID = @.cdid
> OPEN CursEvent
> -- Perform the first fetch
> FETCH NEXT FROM CursEvent INTO @.eid
> -- Check @.@.FETCH_STATUS to see if there are any more rows to fetch
> WHILE @.@.FETCH_STATUS = 0
> BEGIN
> --Run SP
> EXECUTE udpTwo @.eid
> FETCH NEXT FROM CursEvent INTO @.eid
> END
> --close the cursor
> CLOSE CursEvent
> DEALLOCATE CursEvent
> GO
>