SEO

I am begginner at best so I hope someone that is better can help.
I have a stored procedure that updates a view that I wrote using 2
cursors.(Kind of a Inner Loop) I wrote it this way Because I couldn't
do it using reqular transact SQL.

The problem is that this procedure is taking longer and longer to run.
Up to 5 hours now! It is anaylizing about 30,000 records. I think
partly because we add new records every month.

The procedure works like this.
The first Cursor stores a unique account and duedate combination from
the view.
It then finds all the accts in the view that have that account duedate
combo and loads them into Cursor 2 this groups them together for data
manipulation. The accounts have to be grouped this way because a
account can have different due dates and multiple records within each
account due date combo and they need to be looked at this way as
little singular groups.

Here is my procedure I hope someone can shead some light on this. My
boss is giving me heck about it. (I think he thinks Girls cant code!)
I got this far I hope someone can help me optimize it further.




CREATE PROCEDURE dbo.sp_PromiseStatus
AS

BEGIN
SET NOCOUNT ON

/* Global variables */
DECLARE @tot_pay money
DECLARE @rec_upd VARCHAR(1)
DECLARE @todays_date varchar(12)
DECLARE @mActivityDate2_temp datetime
DECLARE @tot_paydate datetime
/* variables for cursor ACT_CUR1*/
DECLARE @mAcct_Num1 BIGINT
DECLARE @mDueDate1 datetime

/* variables for ACT_CUR2 */
DECLARE @mAcct_Num2 BIGINT
DECLARE @mActivity_Date2 datetime
DECLARE @mPromise_Amt_1 money
DECLARE @mPromise_Status varchar(3)
DECLARE @mCurrent_Due_Amt money
DECLARE @mDPD int
DECLARE @mPromise_Date datetime

SELECT @todays_date =''+CAST(DATEPART(mm,getdate()) AS varchar(2))
+'/'+CAST(DATEPART(dd,getdate()) AS varchar(2))
+'/'+CAST(DATEPART(yyyy,getdate()) AS varchar(4))+''

DECLARE ACT_CUR1 CURSOR FOR
SELECT DISTINCT
A.ACCT_NUM,
A.DUE_DATE
FROM VWAPPLICABLEPROMISEACTIVITYRECORDS A
OPEN ACT_CUR1
FETCH NEXT FROM ACT_CUR1 INTO @mAcct_Num1 , @mDueDate1

WHILE (@@FETCH_STATUS = 0)
BEGIN
SELECT @rec_upd = 'N '

DECLARE ACT_CUR2 CURSOR FOR
SELECT
B.ACCT_NUM,
B.ACTIVITY_DATE,
B.PROMISE_AMT_1,
B.PROMISE_STATUS,
B.CURRENT_DUE_AMT,
B.DAYS_DELINQUENT_NUM,
B.PROMISE_DATE_1
FROM VWAPPLICABLEPROMISEACTIVITYRECORDS B (UPDLOCK)
WHERE B.ACCT_NUM = @mAcct_Num1
AND B.DUE_DATE = @mDueDate1
ORDER BY B.ACCT_NUM,B.DUE_DATE,B.ACTIVITY_DATE,CASE
B.Time_Obtained
WHEN 0 THEN 0
ELSE 1
END Desc, B.Time_Obtained

OPEN ACT_CUR2
FETCH NEXT FROM ACT_CUR2
INTO @mAcct_Num2 ,@mActivity_Date2,@mPromise_Amt_1,@mPromise_Status ,@mCurrent_Due_Amt,@mDPD,@mPromise_Date


WHILE (@@FETCH_STATUS = 0)
BEGIN

----CHECK----------------------------------------------------------------------
--DECLARE @PrintVariable2 VARCHAR (8000)
--SELECT @PrintVariable2 = CAST(@MACCT_NUM2 AS VARCHAR)+'
'+CAST(@MACTIVITY_DATE2 AS VARCHAR)+' '+CAST(@MPROMISE_AMT_1 AS
VARCHAR)+' '+CAST(@MPROMISE_STATUS AS VARCHAR)+'
'+CAST(@mCurrent_Due_Amt AS VARCHAR)+' '+CAST(@mDPD AS VARCHAR)+'
'+CAST(@mPromise_Date AS VARCHAR)
--PRINT @PrintVariable2
----END
CHECK------------------------------------------------------------

IF @mDPD >= 30
BEGIN

SELECT @tot_pay = SUM(CONVERT(FLOAT, C.PAY_AMT))
FROM vwAplicablePayments C
WHERE C.ACCT_NUM = @mAcct_Num2
AND C.ACTIVITY_DATE >= @mActivity_Date2
AND C.ACTIVITY_DATE < @mActivity_Date2 + 15
----CHECK----------------------------------------------------------------------
--DECLARE @PrintVariable3 VARCHAR (8000)
--SELECT @PrintVariable3 ='Greater=30 DOLLARS COLLECTED'
--PRINT @PrintVariable3
----END
CHECK------------------------------------------------------------
END
ELSE IF @mDPD < 30
BEGIN

SELECT @tot_pay = SUM(CONVERT(FLOAT, C.PAY_AMT))
FROM vwAplicablePayments C
WHERE C.ACCT_NUM = @mAcct_Num2
AND C.ACTIVITY_DATE >= @mActivity_Date2
AND C.ACTIVITY_DATE BETWEEN @mActivity_Date2 AND
@mPromise_Date + 5
----CHECK--------------------------------------------------------------------
--DECLARE @PrintVariable4 VARCHAR (8000)
--SELECT @PrintVariable4 ='Less 30 DOLLARS COLLECTED'
--PRINT @PrintVariable4
----END CHECK------------------------------------------------------------
END


----------------------------------------MY REVISED
LOGIC-------------------------------------------------------
IF @rec_upd = 'N'
BEGIN

IF @mDPD >= 30
BEGIN

SELECT @mActivityDate2_temp = @mActivity_Date2 + 15

--DECLARE @PrintVariable5 VARCHAR (8000)
--SELECT @PrintVariable5 =' GREATER= 30 USING ACTVITY_DATE+15'
--PRINT @PrintVariable5



END
ELSE IF @mDPD < 30
BEGIN
SELECT @mActivityDate2_temp = @mPromise_Date + 5

--DECLARE @PrintVariable6 VARCHAR (8000)
--SELECT @PrintVariable6 =' LESS 30 USING PROMISE_DATE+5'
--PRINT @PrintVariable6

END


IF @tot_pay >= 0.9 * @mCurrent_Due_Amt--used to be promise amt
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET PROMISE_STATUS = 'PK',
TOTAL_DOLLARS_COLL = @tot_pay
WHERE CURRENT OF ACT_CUR2


--This statement updates the time that the status was placed
into PK.
IF @mPromise_Status IN ('PTP','OP')
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET Status_Date = @todays_date
WHERE CURRENT OF ACT_CUR2
END



SELECT @rec_upd = 'Y '
END
IF ((@tot_pay < 0.9 * @mCurrent_Due_Amt) OR @tot_pay IS NULL)
AND( @mActivityDate2_temp > @todays_date )--need to put 1day
of month here for snapshot9/01/2004
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET PROMISE_STATUS = 'OP'
WHERE CURRENT OF ACT_CUR2

--This statement updates the time that the status was placed
into OP which is the original Activity Date.
--The record will hold this date until it goes into PK,PB,or
IP.
IF @mPromise_Status IN ('PTP','OP')
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET Status_Date = @mActivity_Date2
WHERE CURRENT OF ACT_CUR2
END



END
ELSE IF ((@tot_pay < 0.9 * @mCurrent_Due_Amt) OR @tot_pay IS
NULL)
AND( @mActivityDate2_temp <= @todays_date )--need to put 1day
of month here for snapshot 9/01/2004
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET PROMISE_STATUS = 'PB',
TOTAL_DOLLARS_COLL = case when @tot_pay is null
then 0 else @tot_pay end
WHERE CURRENT OF ACT_CUR2


--This statement updates the time that the status was placed
into PB.
IF @mPromise_Status IN ('PTP','OP')
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET Status_Date = @todays_date
WHERE CURRENT OF ACT_CUR2
END


END
END
ELSE IF @rec_upd = 'Y'
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET PROMISE_STATUS = 'IP',
TOTAL_DOLLARS_COLL = 0
WHERE CURRENT OF ACT_CUR2


--This statement updates the time that the status was placed
into IP.
IF @mPromise_Status NOT IN ('IP')
BEGIN
UPDATE VWAPPLICABLEPROMISEACTIVITYRECORDS
SET Status_Date = @todays_date
WHERE CURRENT OF ACT_CUR2
END



END


FETCH NEXT FROM ACT_CUR2 INTO @mAcct_Num2
,@mActivity_Date2,@mPromise_Amt_1,@mPromise_Status ,@mCurrent_Due_Amt,@mDPD,@mPromise_Date
END
CLOSE ACT_CUR2
DEALLOCATE ACT_CUR2

FETCH NEXT FROM ACT_CUR1 INTO @mAcct_Num1 , @mDueDate1
END
CLOSE ACT_CUR1
DEALLOCATE ACT_CUR1

SET NOCOUNT OFF

END
GO

Without getting into code, I think you should consider using faster type
of cursor if possible. FORWARD ONLY for example

If you are a beginner then avoid cursors. They are rarely a necessary or
good solution to a problem in SQL. You will learn bad practices if you mess
with cursors before you can write good set-based SQL.

It doesn't look like you are doing anything in your code for which a cursor
makes sense so I recommend you start again from scratch. If you need help,
try to pare the problem down to it's essential features and then post again
with more information (DDL, some sample data, required results). This
article explains how best to ask for help:
http://www.aspfaq.com/etiquette.asp?id=5006

--
David Portas
SQL Server MVP
--

>> I am beginner at best so I hope someone that is better can help. <<

Virutally everything you have done is wrong.

1) The data element names violate ISO-11179 standards. What does a
VARCHAR(1) mean?

2) You should never write a cursor in an applications. Thye are for
utility programs.

3) You are using FLOAT and MONEY for currency amounts. And you got
the CAST() syntax wrong.

4) Unlike the 3GL languages, SQL has temporal data types. Why did you
avoid them in favor of COBOL-style string processing? You even load
the current timestamp into a string!

>> The problem is that this procedure is taking longer and longer to
run. Up to 5 hours now! It is anaylizing about 30,000 records. I think
partly because we add new records [sic] every month. <<

No. The basic algorithm you have written is a COBOL tape file merge.
30K rows is tiny.

>> The first Cursor [magnetic tape file] stores a unique account and
duedate combination from the view. <<

The view name has a silly "vw_" prefix in violation of ISO-11179. And
a table has no records!!! Rows are not records! Until you understand
that you will never write good SQL. A table is a set, and in SQL we
process data in whole sets, not single records.

>> It then finds all the accts in the view that have that account
duedate combo .. <<

No, you write a query that finds the set of (account_nbr, duedate)

>> .. and loads them into Cursor 2 this groups them together for data
manipulation. The accounts have to be grouped this way because an
account can have different due dates and multiple records within each
account due date combo and they need to be looked at this way as
little singular groups. <<

Think about the phrase "single group" for a minute A GROUP BY is
used to build grouped tables.

What you seem to be trying to do is update a table of promises. That
should be one UPDATE statement. The skeleton for the statement would
be something like this:

CREATE PROCEDURE PromiseStatus (..)
AS
UPDATE ApplicablePromises
SET status_date = CURRENT_TIMESTAMP,
promise_status = CASE WHEN ..END,
total_dollars_coll
= (SELECT SUM(P.pay_amt),
FROM ApplicablePromises AS A, Applicablepayments AS P
WHERE P.acct_num = A.acct_num
AND ApplicablePromises.acct_num = A.account_num
AND ApplicablePromises.due_date = A.due_date
AND P.activity_date BETWEEN ...
AND GROUP BY A.acct_num, A.due_date)
WHERE ... ;

You put all the IF_THEN_ELSE logic into a CASE expression. You put
the total into a scalar subquery. You use temporal functions to get
the current timestamp.

Quit thinking about one column at a time. That is how you process a
field in a record. A table is made of rows, and you think of the row
as the unit of work.

Philip Mette (philipdm@msn.com) writes:
> I am begginner at best so I hope someone that is better can help.
> I have a stored procedure that updates a view that I wrote using 2
> cursors.(Kind of a Inner Loop) I wrote it this way Because I couldn't
> do it using reqular transact SQL.
>
> The problem is that this procedure is taking longer and longer to run.
> Up to 5 hours now! It is anaylizing about 30,000 records. I think
> partly because we add new records every month.

While cursor-based solutions sometimes may be defendible and even be
the only solution, the cost for a cursor can be pretty severe. The
corresponding set-based solution is often several magnitudes faster.
Processing 30000 rows with a cursor is not going to be anywhere near
fast.

Just like David I would recommend you to start with a clean paper. I
started to look at your code, but, frankly, all the uppercase and the
poor formatting is quite deterring. And not knowing the tables and not
having sample data is not making things any easier.

In the short run, you may make some improvements by making the cursors
INSENSITIVE by adding this keyword before CURSOR, and by replacing
WHERE CURRENT OF with the actual key values.



--
Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

Ok well I have been yelled at worse No I see what you are saying. I
should have used Char variable not Varchar(1).That was a adjustment
later on that frankly wasnt changed properly. I used Cursors based on
a recommendation by a "SQL Guru" Obviously this is not a good
description of him. I am going to try this using the skeleton you gave
me.It is pointing me in a right direction I think.
Again please understand I am a beginner Query writer when it comes to
SQL and I am very good with VB ,.Net and Java(Used to do front end
work and analysis not DB.)I quess I was thinking of this like code
language,ie the use of arrays etc. Thanks for your 'direct' honesty
and help.




jcelko212@earthlink.net (--CELKO--) wrote in message news:<18c7b3c2.0411242243.2c96bbf0@posting.google. com>...
> >> I am beginner at best so I hope someone that is better can help. <<
>
> Virutally everything you have done is wrong.
>
> 1) The data element names violate ISO-11179 standards. What does a
> VARCHAR(1) mean?
>
> 2) You should never write a cursor in an applications. Thye are for
> utility programs.
>
> 3) You are using FLOAT and MONEY for currency amounts. And you got
> the CAST() syntax wrong.
>
> 4) Unlike the 3GL languages, SQL has temporal data types. Why did you
> avoid them in favor of COBOL-style string processing? You even load
> the current timestamp into a string!
>
> >> The problem is that this procedure is taking longer and longer to
> run. Up to 5 hours now! It is anaylizing about 30,000 records. I think
> partly because we add new records [sic] every month. <<
>
> No. The basic algorithm you have written is a COBOL tape file merge.
> 30K rows is tiny.
>
> >> The first Cursor [magnetic tape file] stores a unique account and
> duedate combination from the view. <<
>
> The view name has a silly "vw_" prefix in violation of ISO-11179. And
> a table has no records!!! Rows are not records! Until you understand
> that you will never write good SQL. A table is a set, and in SQL we
> process data in whole sets, not single records.
>
> >> It then finds all the accts in the view that have that account
> duedate combo .. <<
>
> No, you write a query that finds the set of (account_nbr, duedate)
>
> >> .. and loads them into Cursor 2 this groups them together for data
> manipulation. The accounts have to be grouped this way because an
> account can have different due dates and multiple records within each
> account due date combo and they need to be looked at this way as
> little singular groups. <<
>
> Think about the phrase "single group" for a minute A GROUP BY is
> used to build grouped tables.
>
> What you seem to be trying to do is update a table of promises. That
> should be one UPDATE statement. The skeleton for the statement would
> be something like this:
>
> CREATE PROCEDURE PromiseStatus (..)
> AS
> UPDATE ApplicablePromises
> SET status_date = CURRENT_TIMESTAMP,
> promise_status = CASE WHEN ..END,
> total_dollars_coll
> = (SELECT SUM(P.pay_amt),
> FROM ApplicablePromises AS A, Applicablepayments AS P
> WHERE P.acct_num = A.acct_num
> AND ApplicablePromises.acct_num = A.account_num
> AND ApplicablePromises.due_date = A.due_date
> AND P.activity_date BETWEEN ...
> AND GROUP BY A.acct_num, A.due_date)
> WHERE ... ;
>
> You put all the IF_THEN_ELSE logic into a CASE expression. You put
> the total into a scalar subquery. You use temporal functions to get
> the current timestamp.
>
> Quit thinking about one column at a time. That is how you process a
> field in a record. A table is made of rows, and you think of the row
> as the unit of work.

Thanks Erland.
A few Questions:
1.What does insensitive do?
2.Instead of Where Current Of
If I used Activity_Record_Num(The Primary Key)
which is distinctive would that work?
3.I use a statement called (UPDLOCK) in this part of the code:
DECLARE ACT_CUR2 CURSOR FOR
SELECT
B.ACCT_NUM,
B.ACTIVITY_DATE,
B.PROMISE_AMT_1,
B.PROMISE_STATUS,
B.CURRENT_DUE_AMT,
B.DAYS_DELINQUENT_NUM,
B.PROMISE_DATE_1
FROM VWPROMISEACTIVITYRECORDS B (UPDLOCK)


Does that effect performance speed as well? This is the only process
that runs against this table so I am wondering if it is needed.

Just so you know I am working on a rewrite like you suggested. In the
short, I would like to add these suggestions that you made to the
cursor based SP.




Erland Sommarskog <esquel@sommarskog.se> wrote in message news:<Xns95ACF2C23D6DBYazorman@127.0.0.1>...
> Philip Mette (philipdm@msn.com) writes:
> > I am begginner at best so I hope someone that is better can help.
> > I have a stored procedure that updates a view that I wrote using 2
> > cursors.(Kind of a Inner Loop) I wrote it this way Because I couldn't
> > do it using reqular transact SQL.
> >
> > The problem is that this procedure is taking longer and longer to run.
> > Up to 5 hours now! It is anaylizing about 30,000 records. I think
> > partly because we add new records every month.
>
> While cursor-based solutions sometimes may be defendible and even be
> the only solution, the cost for a cursor can be pretty severe. The
> corresponding set-based solution is often several magnitudes faster.
> Processing 30000 rows with a cursor is not going to be anywhere near
> fast.
>
> Just like David I would recommend you to start with a clean paper. I
> started to look at your code, but, frankly, all the uppercase and the
> poor formatting is quite deterring. And not knowing the tables and not
> having sample data is not making things any easier.
>
> In the short run, you may make some improvements by making the cursors
> INSENSITIVE by adding this keyword before CURSOR, and by replacing
> WHERE CURRENT OF with the actual key values.

Philip Mette (philipdm@msn.com) writes:
> Thanks Erland.
> A few Questions:
> 1.What does insensitive do?

The meaning of INSENSITIVE is that the dataset does not change
once the cursor has been created. SQL Server copies the data to a
worktable. This may sound like some overhead, and it probably is, but
it's a small overhead in comparison with the iterative processing.

The default cursor type is keyset. I think it means that the keys are
stable, but updates in non-key columns will be reflected. But I have
never fully grasped keyset cursors so I could be wrong. What I have
seen is some really horrible query plans to set up a keyset-driven
cursor. Sure, it was in 6.5, but I see no reason to try it again.

> 2.Instead of Where Current Of
> If I used Activity_Record_Num(The Primary Key)
> which is distinctive would that work?

I didn't look to close on your code, but you should pick data from
the cursor which identifies the current row(s) being processed. (Note
that you could run a cursor over an aggregate, and thus update several
rows at same time. There are situations when this is a possible solution.

> 3.I use a statement called (UPDLOCK) in this part of the code:
>...
> Does that effect performance speed as well?

Not very likely. Since you don't seem to have an active transaction, the
effect of the UPDLOCK is none anyway.


--
Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se

Books Online for SQL Server SP3 at
http://www.microsoft.com/sql/techinf...2000/books.asp

Erland,

I got the process run time to 2:00 minutes! The solution was to create
a composite index on Account and Due_date in the tables, which is what
is in my where clauses in my select statements. That and changing the
cursors to Insensitive, made this query fast. This amount of time is
more than acceptable. Thanks for pointing me in the right direction. I
am going to continue to look into a transact SQL solution in the
future.
Thanks Again,

Jill
(Using Phil's Login Name. Dont have a google acct.)




Erland Sommarskog <esquel@sommarskog.se> wrote in message news:<Xns95ADDFEC37F0BYazorman@127.0.0.1>...
> Philip Mette (philipdm@msn.com) writes:
> > Thanks Erland.
> > A few Questions:
> > 1.What does insensitive do?
>
> The meaning of INSENSITIVE is that the dataset does not change
> once the cursor has been created. SQL Server copies the data to a
> worktable. This may sound like some overhead, and it probably is, but
> it's a small overhead in comparison with the iterative processing.
>
> The default cursor type is keyset. I think it means that the keys are
> stable, but updates in non-key columns will be reflected. But I have
> never fully grasped keyset cursors so I could be wrong. What I have
> seen is some really horrible query plans to set up a keyset-driven
> cursor. Sure, it was in 6.5, but I see no reason to try it again.
>
> > 2.Instead of Where Current Of
> > If I used Activity_Record_Num(The Primary Key)
> > which is distinctive would that work?
>
> I didn't look to close on your code, but you should pick data from
> the cursor which identifies the current row(s) being processed. (Note
> that you could run a cursor over an aggregate, and thus update several
> rows at same time. There are situations when this is a possible solution.
>
> > 3.I use a statement called (UPDLOCK) in this part of the code:
> >...
> > Does that effect performance speed as well?
>
> Not very likely. Since you don't seem to have an active transaction, the
> effect of the UPDLOCK is none anyway.

Hello guys,

I have may be similar problem.
I need to recalculate a progressive balance based on a table with the
following structure:

EcrID varchar(15)
AccountNumber varchar(15)
EcrDate DateTime
EcrAmount float
ProgBalance float

The program has to calculate the progbalance which is for each row :
progbalance=progbalance + EcrAmount
Rows are sorted by AccountNumber and EcrDate.

Is it possible to that by a single update statement or do I have to
use cursors?

(Sorry for my english)

Many thanks for your help.