Tek-Tips is the largest IT community on the Internet today!

Members share and learn making Tek-Tips Forums the best source of peer-reviewed technical information on the Internet!

Register Log in
  • Congratulations SkipVought on being selected by the Tek-Tips community for having the most helpful posts in the forums last week. Way to Go!

Cursor Error - is my assumption correct? 2

Status
Not open for further replies.
normm

normm

Programmer
Dec 9, 2004
46
0
0
GB
Hi all,

I have written a series of stored procedures intended to process a large amount of data and have errors that I now feel that I have fixed.

However due to the insanely large amount of processing conducted the process takes a very long time to run fully (around 13 million update statements). I just want to ask if my assumptions are correct before I waste a large amount of time running it again.


The main stored procedure works as follows.


Start

Cursor while loop

Execute stored procedure 1

Execute stored procedure 2

Execute stored procedure 3

End of cursor while loop

End


Each stored procedure has at least one cursor inside it.

My first error occurs as I forgot to create a function required by stored procedure 1 on the test server - this has been resolved. All of the processing in this function occurs within a cursor named cli_cursor.

The second error occurred during stored procedure 2 which also has a cursor named cli_cursor - the error stated that cli_cursor already existed.

Am I correct assuming that when the first stored procedure fails due to the missing function that this cursor is left open causing the cursor in my second stored procedure to throw an error as it already exists?

If I could just get a yes from somebody then it would give me enough confidence to try the long drawn out test again.

However if my assumption is incorrect then does anyone have any advice on the quickest way to diagnose what the real problem is?

Thanks in advance for any help anyone can give.

Normm.
 
Sort by date Sort by votes
Upvote 0 Downvote
Hi Mark - the cursors are neccecary - by design there are only ever 2 cursors open accross the whole process at any one time. 13 million is not the number of rows processed it is the number of update statements executed by the system as a whole. all cursors only hold a limited amount of data so the memory requirements shouldn't be that high.

the process is unlikely to run for as long a period in real world usage as it is limited to a date range, I just have a very large dataset for testing purposes with roughly 10 years worth of records. typical usage would involve re - processing the last month's worth of transactions.

do you feel my assumption about the cause of the second error correct?

Thanks for responding.

Normm
 
Upvote 0 Downvote
If you are willing to show the code perhaps someone can come up with a way in how to avoid cursors. IN 99,9% of the cases cursors aren't necessary.

Christiaan Baes
Belgium

My Blog
 
Upvote 0 Downvote
Hi Christiaan,

Here is my code - I must point out that I am dealing with a badly designed legacy database previously held in an ISAM Clarion TPS database - the code follows. I am also aware that the manual generation of new RecordID variables is a stupid way to do it but I do not have control of the database structure and the field is not an auto incrementing primary key - to allow this system to interact with older technologies.

first of all the main procedure

Code: 
the second error - the one regarding the cli_cursor that already exists comes from the redobedanal function


/*re do Group Anal
top level procedure
inside this proceedure 
reDoFeeAnal
reDoBedAnal
reDoHomeAnal
and then builds the group anal*/
create procedure ReDoGrpAnal(@GLO_ADMIN tinyint)
as
begin

declare @HID_ID int
declare @GRP_RecordID int
declare @Date int 
declare @TotBeds int 
declare @TotOcc int
declare @Occ decimal(9,2) 
declare @Income decimal(9,2) 
declare @Private int 
declare @LA int 
declare @DSS int 
declare @TotRes int 
declare @AveIncome decimal(9,2)

select @GRP_RecordID =	case when ((max(RecordID) = 0) or (max(RecordID) is NULL)) then 
							1 
						else max(RecordID) + 1 end 
from GrpAnal

declare hom_cursor cursor
for
select HID
from Homes

open hom_cursor

fetch next from hom_cursor into @HID_ID

while (@@FETCH_STATUS <> - 1)
begin
	if (@@FETCH_STATUS <> - 2)
	begin
		
		execute ReDoFeeAnal @HID_ID, @GLO_ADMIN

		execute ReDoBedAnalysis @HID_ID, @GLO_ADMIN
		
		execute redoHomeAnal @HID_ID, @GLO_ADMIN
		
		fetch next from hom_cursor into @HID_ID
	end
end
close hom_cursor
deallocate hom_cursor


delete from GrpAnal

declare grp_cursor cursor
for
select Date as Date, sum(TotNights) as TotBeds, sum(Nights) as TotOcc, (sum(Nights) / sum(TotNights)) * 100 as Occ,
sum(Income) as Income, sum(Private) as Private, sum(LA) as LA, sum(DSS) as DSS, sum(TotRes) as TotRes,
(sum(Income) / sum(Nights)) * 7 as AveIncome from HomeAnal group by Date

open grp_cursor

fetch next from grp_cursor into @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome 

while (@@FETCH_STATUS <> - 1)
begin
	if (@@FETCH_STATUS <> - 2)
	begin
		
		select @GRP_RecordID =	case when ((max(RecordID) = 0) or (max(RecordID) is NULL)) then 
									1 
								else 
									max(RecordID) + 1 
								end
		from GrpAnal 

		insert GrpAnal
		select @GRP_RecordID, @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome
		
		fetch next from grp_cursor into @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome
	end
end
close grp_cursor
deallocate grp_cursor
  

end

the section my question is based around is :-

Code: 
execute ReDoFeeAnal @HID_ID, @GLO_ADMIN
execute ReDoBedAnalysis @HID_ID, @GLO_ADMIN        
execute redoHomeAnal @HID_ID, @GLO_ADMIN


The first error I have encountered is from the following procedure - It was because I failed to include the definition of the getclifeeHistDates function.

Code: 
/*redo fee anal procedure
this procedure is used as a cursor for the ufa procedure
that in turn updates the fee analysis table*/

create procedure RedoFeeAnal (@HID_ID int, @GLO_ADMIN tinyint)
/*==================================================*/
/*Start Date 20/01/08*/
/*Author Lee Norman*/
/*==================================================*/
as 
begin

declare @Now datetime
declare @SD int
declare @ED int
declare @CLI_ID int
declare @RID_ID int
declare @BED varchar(3) 
declare @DateIn int
declare @Termination int
declare @DIN int 
declare @DOUT int
declare @MONTH int
declare @YEAR int

set @Now = getdate()
set @ED = dbo.DateToClarion(@Now)

if (@GLO_ADMIN = 1)
begin
	set @SD = 0
end
else
begin
	select @MONTH = datepart(mm, @Now)
	select @YEAR = datepart(yy, @Now)
	select @SD = dbo.DateToClarion(dateadd(mm, - 2, dbo.mdate(@YEAR, @MONTH, 1)))
end

delete from CliFeeAnal where HID_ID = @HID_ID and Date >= @SD


declare cli_cursor cursor 
for
select HID_ID, CLI_ID, RID_ID, BED, DateIn, Termination 
from Client
where (DateIn <= @ED) and ((Termination >= @SD) or (Termination is NULL) or (Termination = 0)) and HID_ID = @HID_ID

open cli_cursor

fetch next from cli_cursor into @HID_ID, @CLI_ID, @RID_ID, @BED, @DateIn, @Termination

while (@@FETCH_STATUS <> - 1)
begin
	if (@@FETCH_STATUS <> - 2)
	begin
		
		/*cli_cursor loop start*/
		/*print ltrim('HID_ID = '+ cast(@HID_ID as varchar(3)) + 'CLI_ID = ' + cast(@CLI_ID as varchar(3)))*/
		if (
		(select count(*) from CliFeeHistory where HID_ID = @HID_ID and CLI_ID = @CLI_ID) >= 1
		)
		begin /*if the client has a fee history start*/
			
			declare cli_fee_hist_cursor cursor
			for
			select DateEffective, EndDate
			from dbo.getcliFeeHistDates(@HID_ID, @CLI_ID)
			order by DateEffective

			open cli_fee_hist_cursor
			
			fetch next from cli_fee_hist_cursor into @DIN, @DOUT

			while (@@FETCH_STATUS <> -1)
			begin
				if (@@FETCH_STATUS <> -2)
				begin
					
					execute ufa @CLI_ID, @HID_ID, @DIN, @DOUT
					
				fetch next from cli_fee_hist_cursor into @DIN, @DOUT
				end
			end
			close cli_fee_hist_cursor
			deallocate cli_fee_hist_cursor

		end /*if the client has a room history end*/
		else
		begin
			if ((@Termination is NULL) or (@Termination = 0))
			begin
				set @DOUT = @ED 
			end
			else
			begin
				set @DOUT = @Termination
			end

			execute ufa @CLI_ID, @HID_ID, @DateIn, @DOUT
		end
		/*cli_cursor loop end*/
		
	fetch next from cli_cursor into @HID_ID, @CLI_ID, @RID_ID, @BED, @DateIn, @Termination
	end
end
close cli_cursor
deallocate cli_cursor

end
/*==================================================*/
/*END OF RedoFeeAnal*/
/*==================================================*/

the following is the code for the procedure that generates the second error when I try to define a cli_cursor again.

Code: 
/*re do bed analysis proceedure - 
works as a cursor collecting client periods in beds and then
calling the uba procedure to alter the bed analysis table*/

create procedure ReDoBedAnalysis(@HID_ID int, @GLO_ADMIN tinyint)
/*==================================================*/
/*Start Date 16/01/08*/
/*Author Lee Norman*/
/*==================================================*/
as 
begin

declare @Now datetime
declare @SD int
declare @ED int
declare @CLI_ID int
declare @RID_ID int
declare @BED varchar(3) 
declare @DateIn int
declare @Termination int
declare @DIN int 
declare @DOUT int
declare @MONTH int
declare @YEAR int

set @Now = getdate()
set @ED = dbo.DateToClarion(@Now)

if (@GLO_ADMIN = 1)
begin
	set @SD = 0
end
else
begin
	select @MONTH = datepart(mm, @Now)
	select @YEAR = datepart(yy, @Now)
	select @SD = dbo.DateToClarion(dateadd(mm, - 2, dbo.mdate(@YEAR, @MONTH, 1)))
end

delete from BedAnal where HID_ID = @HID_ID and Date >= @SD


declare cli_cursor cursor 
for
select HID_ID, CLI_ID, RID_ID, BED, DateIn, Termination 
from Client
where (DateIn <= @ED) and ((Termination >= @SD) or (Termination is NULL) or (Termination = 0)) and HID_ID = @HID_ID

open cli_cursor

fetch next from cli_cursor into @HID_ID, @CLI_ID, @RID_ID, @BED, @DateIn, @Termination

while (@@FETCH_STATUS <> - 1)
begin
	if (@@FETCH_STATUS <> - 2)
	begin
		
		/*cli_cursor loop start*/
		/*print ltrim('HID_ID = '+ cast(@HID_ID as varchar(3)) + 'CLI_ID = ' + cast(@CLI_ID as varchar(3)))*/
		if (
		(select count(*) from CliRmHis where HID_ID = @HID_ID and CLI_ID = @CLI_ID) >= 1
		)
		begin /*if the client has a room history start*/
			
			declare cli_rm_hist_cursor cursor
			for
			select RID_ID, BED, DateIn, DateOut
			from CliRmHis
			where HID_ID = @HID_ID and CLI_ID = @CLI_ID and DateOut > @SD
			order by DateIn

			open cli_rm_hist_cursor
			
			fetch next from cli_rm_hist_cursor into @RID_ID, @BED, @DIN, @DOUT

			while (@@FETCH_STATUS <> -1)
			begin
				if (@@FETCH_STATUS <> -2)
				begin
					
					if ((@DIN = 0) or (@DIN is NULL))
					begin
						set @DIN = @DateIn
					end

					execute uba @CLI_ID, @HID_ID, @RID_ID, @BED, @DIN, @DOUT
					
				fetch next from cli_rm_hist_cursor into @RID_ID, @BED, @DIN, @DOUT
				end
			end
			close cli_rm_hist_cursor
			deallocate cli_rm_hist_cursor

		end /*if the client has a room history end*/
		else
		begin
			if ((@Termination is NULL) or (@Termination = 0))
			begin
				set @DOUT = @ED 
			end
			else
			begin
				set @DOUT = @Termination
			end
			
			execute uba @CLI_ID, @HID_ID, @RID_ID, @BED, @DateIn, @DOUT
		end
		/*cli_cursor loop end*/
		
	fetch next from cli_cursor into @HID_ID, @CLI_ID, @RID_ID, @BED, @DateIn, @Termination
	end
end
close cli_cursor
deallocate cli_cursor

end
/*==================================================*/
/*END OF ReDoBedAnalysis*/
/*==================================================*/

The 2 errors only occur when a client (cli) has records in the client fee history table, this is the point where the missing function is used - causing the first error which then in turn causes the second error - if their are no entries in the client fee history table then their is no error generated and the system appears to correctly process all records.

it is probably a good Idea for me to explain due to the legacy systems that use this database all dates and times are stored as integer values and therefore must be converted before date operations can be completed.


Thanks Again,

Norm.
 
Upvote 0 Downvote
OK, so where you end up executing a stored procedure/function for each row, there's probably not a lot you can do to remove the cursor (apart from checking if there is a set based approach you can use instead of the sp/function).

However, for sections like this:
Code: 
delete from GrpAnal

declare grp_cursor cursor
for
select Date as Date, sum(TotNights) as TotBeds, sum(Nights) as TotOcc, (sum(Nights) / sum(TotNights)) * 100 as Occ,
sum(Income) as Income, sum(Private) as Private, sum(LA) as LA, sum(DSS) as DSS, sum(TotRes) as TotRes,
(sum(Income) / sum(Nights)) * 7 as AveIncome from HomeAnal group by Date

open grp_cursor

fetch next from grp_cursor into @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome

while (@@FETCH_STATUS <> - 1)
begin
    if (@@FETCH_STATUS <> - 2)
    begin
        
        select @GRP_RecordID =    case when ((max(RecordID) = 0) or (max(RecordID) is NULL)) then
                                    1
                                else
                                    max(RecordID) + 1
                                end
        from GrpAnal

        insert GrpAnal
        select @GRP_RecordID, @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome
        
        fetch next from grp_cursor into @Date, @TotBeds, @TotOcc, @Occ, @Income, @Private, @LA, @DSS, @TotRes, @AveIncome
    end
end
close grp_cursor
deallocate grp_cursor
you should be able to remove the cursor and perform a set based insert. This should at least reduce some of the time it takes.


-------------------------------------------------------

Mark,
[URL unfurl="true"]http://aspnetlibrary.com[/url]
[URL unfurl="true"]http://mdssolutions.co.uk[/url] - Delivering professional ASP.NET solutions
[URL unfurl="true"]http://weblogs.asp.net/marksmith[/url]
 
Upvote 0 Downvote
Hi Mark,

I'm not sure that it would be possible to do a set based insert on this table for one reason.

The none auto incrementing primary key would make it difficult to insert multiple records directly into the table.

The solution(s) to this that we have found so far are to do it in the way I have in my code or to define a temporary table with an auto incrementing primary key (RecordID) and then inserting the data from the temp table into the main table while adding max(RecordID) to the temp table RecordID.

In your opinion is this the temporary table approach more efficient than the cursor method or not? could you suggest a more efficient method in these circumstances?


Thanks Mark

All help making this whole processs more efficient is greatly appreciated however can anyone give me a yes or no on my original question?

Normm
 
Upvote 0 Downvote
besides the fact that cursors are evil

>>The second error occurred during stored procedure 2 which also has a cursor named cli_cursor - the error stated that cli_cursor already existed.


have you tried naming the cursors unique for example cli_cursor1 in proc 1 and cli_cursor2 in proc 2?

Denis The SQL Menace
--------------------
SQL Server Code,Tips and Tricks, Performance Tuning
SQLBlog.com, Google Interview Questions
 
Upvote 0 Downvote
The solution(s) to this that we have found so far are to do it in the way I have in my code or to define a temporary table with an auto incrementing primary key (RecordID) and then inserting the data from the temp table into the main table while adding max(RecordID) to the temp table RecordID.

In your opinion is this the temporary table approach more efficient than the cursor method or not?
Click to expand...
A temporary table should be faster than a cursor, as if you define an IDENTITY field on it, then you can simply start the seed value at the current maximum. All records inserted after it will just increment their id by one, and then you can use the temp table to insert the records into the other table.

The problem I have in helping specifically with this problem as opposed to just offering general suggestions like I have so far) is the sheer volume of code and understanding of your structure that would be needed. If you could come up with a cut down example of the structure, along with sample data and expected results then we may be able to be more specific with the help. Otherwise, I think we'll be restricted to just giving you pointers as to what you could try.


-------------------------------------------------------

Mark,
[URL unfurl="true"]http://aspnetlibrary.com[/url]
[URL unfurl="true"]http://mdssolutions.co.uk[/url] - Delivering professional ASP.NET solutions
[URL unfurl="true"]http://weblogs.asp.net/marksmith[/url]
 
Upvote 0 Downvote
Dennis, I thought about this but decided that it would make no difference as each process is not mutually exclusive. Records created by proc1 are then used in proc2 so if the first part of the program falls over then the rest of the run is invalid.

 
Upvote 0 Downvote
Ok my general question is :-

If I have a cursor with 2 stored procedures in it each with an identically named cursor inside of them and an error occurs in the first procedure - is this cursor left open?

Secondly is the cursor left open after the end of the first stored procedure leading to an error generated by the second procedure when i try and define cli_cursor again?

Thanks

Normm

 
Upvote 0 Downvote
Thanks Denis

I will look into that.

Normm
 
Upvote 0 Downvote
Status
Not open for further replies.

Similar threads

leifoet
  • Locked
  • Question
Error in Select query
Replies
14
Views
156
Andrzejek
Andrzejek
Virendra Yaduvanshi
  • Locked
  • Question
SSRS - Subscription page error
Replies
0
Views
58
Virendra Yaduvanshi
Virendra Yaduvanshi
grnzbra
  • Locked
  • Question
Is &quot;IF EXISTS()...&quot; better than &quot;IF (SELECT COUNT...) &gt; 0&quot;
Replies
4
Views
728
gmmastros
gmmastros
ahmedsa2018
  • Locked
  • Question
the goal mfrom asking question is t
Replies
0
Views
50
ahmedsa2018
ahmedsa2018
peac3
  • Locked
  • Question
Gaps and islands
Replies
8
Views
132
Chris Miller
Chris Miller

Part and Inventory Search

Sponsor

Back
Top