Post reply

Debugging Stored Procedure with looping cursor

  • October 19, 2010 at 3:12 pm

    #241886

    Hello Friends,

    I am creating a stored procedure in MSSQL 2005. I have an Item table called IM_ITEM. This table lists the item numbers, descriptions, a field that I use as a flag (PROF_COD_1), and a numeric field (PROF_NO_1) that I am putting a value in. I also have an IM_INV table that I am pulling the data from, calculating an avg cost and then updating the PROF_NO_1 field in the IM_ITEM table with the avg cost. So basically I need to update every row of a table with different values for each row. I am only updating on column. Below is the query that I've created so far. I am using a cursor to pull the IM_ITEM table and use the ITEM_NO and PROF_COD_1 field in a while loop. It should be going through each row, checking to see if the PROF_COD_1 is set to anything except YES. If it is set to YES, it should end. If it is set to null or anything else it should process the update. The update is performing a calculation as it runs. However, when I execute the stored procedure it appears to be updating the first row in the IM_ITEM table, but nothing else and the stored procedure continues to run. Do I have something wrong in the way I have set this up? Does anyone have any ideas? Here is my query:

    SET QUOTED_IDENTIFIER ON

    GO

    /* This is a custom procedure that looks at the company wide average cost for an item and populates the PROF_NO_1 field with

    with the value. It get the company wide average cost by adding all of the locations GL values and all of the locations qty

    on hand values and then dividing the Total GL Values by the Total Qty on Hand values. Then before it performs the update

    it checkes the Code Profile to see if it is being overriden. If it is then it will not update this value.*/

    ALTER procedure [dbo].[UA_USP_IM_UPD_PROF_NO_1_CO_AVG_COST]

    as

    begin

    set nocount on

    -- Create cursor and load in all current item numbers and PROF_COD_1. This allows us to run the update

    -- statement row by row and enter a different value for the PROF_NO_1.

    declare curItemList cursor for

    select ITEM_NO, PROF_COD_1

    from IM_ITEM

    open curItemList

    -- establish our variables

    declare @ItemNo T_ITEM_NO, @ProfCOD1 T_COD

    -- we need to fetch the first line before we can use the @@Fetch_Status

    fetch next from curItemList into @ItemNo, @ProfCOD1

    -- When it reaches the end of the IM_ITEM table it will end

    while @@Fetch_Status = 0

    begin

    -- check to see if the PROF_NO_1 is going to manually overridden. If PROF_COD_1 is

    -- set to YES then our update will not run for this row and move onto the next therefore

    -- keeping the manually set value

    if (@ProfCOD1 <> 'YES')

    begin

    update IM_ITEM

    set

    -- This sets the value we need. This is a company wide avg cost. We are taking the sum of the

    -- GL Value of all locations for that item number and dividing it by the sum of the Qty on Hand of all

    -- locations for that item number.

    PROF_NO_1 = (select sum(GL_VAL) / sum(QTY_ON_HND)

    from IM_INV

    where ITEM_NO = @ItemNo)

    where ITEM_NO = @ItemNo

    -- move onto the next row

    fetch next from curItemList into @ItemNo, @ProfCOD1

    end

    end

    --need to close and deallocate the cursor when we are done.

    close curItemList

    deallocate curItemList

    end

    Thank you so much for your assistance! - Isaac

  • October 19, 2010 at 3:24 pm

    #1239383

    Option 1:

    move the FETCH part from the IF section to the outer section.

    So, instead of

    if (@ProfCOD1 <> 'YES')

    begin

    fetch next from curItemList into @ItemNo, @ProfCOD1

    end

    use

    if (@ProfCOD1 <> 'YES')

    begin

    your code

    end

    fetch next from curItemList into @ItemNo, @ProfCOD1

    Option 2:

    rewrite your *cough* c.u.r.s.o.r. to do it all in one path using a CTE to hold the result of sum(GL_VAL) / sum(QTY_ON_HND) and the ID (row_number??) of the first row per ITEM_NO

    that is "YES" to find the terminating clause.

    If you'd like to see acoded version please provide table def, ready to use sample data in the same ready to use format as you did with the *cough* "stuff" including your expected result. Depending on the number of rows and the indexes you're using you might be surprised how much faster it'll be... 😉


    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • October 19, 2010 at 3:35 pm

    #1239393

    Thank you very much! I can't believe I didn't notice that. It makes sense. If the fetch line is in the IF statement and the IF statement is false then it wouldn't run the fetch. Now I am getting a divided by zero error so I need to tweak my calculation a bit. As you wrote your response I get the impression that cursors are not the best way to go? I haven't been working with SQL queries very long so would you mind enlightening me why this isn't the best way to go and what was this other way you were describing? Thanks. - Isaac

  • October 19, 2010 at 4:03 pm

    #1239401

    From what I understand, there is some overhead for every transaction. With a set-based query, this overhead is performed once; with a cursor, this overhead is performed for every single row.

    It's actually fairly easy to rewrite this without a cursor based on what you've already provided.

    UPDATE IM_Item

    SET Prof_No_1 = (

    SELECT CASE WHEN Sum(Qty_On_Hnd) > 0 THEN Sum(GL_Val)/Sum(Qty_On_Hnd) ELSE NULL END

    -- Edited to handle your divide by zero error. You can replace the null with 0 or any other expression.

    FROM IM_Inv

    WHERE IM_Inv.Item_No = IM_Item.Item_No

    )

    WHERE Prof_Cod_1 <> 'Yes'

    I did have a question about your cursor approach. Why don't you just eliminate those records where PROF_COD_1 = 'Yes' from your cursor so that you don't need to test later whether that field is different from 'Yes'?

    DECLARE curItemList CURSOR FOR

    SELECT Item_No

    FROM IM_Item

    WHERE Prof_Cod_1 <> 'Yes'

    That way, you don't need your IF statement.

    Drew

    J. Drew Allen
    Business Intelligence Analyst
    Philadelphia, PA

  • October 19, 2010 at 4:04 pm

    #1239402

    Well, when using a *cough* c.u.r.s.o.r. you're processing one row at a time (also called RBAR - row by agonizing row). SQL Server is optimized to deal with sets. A set in your case are all rows from IM_ITEM per ITEM_NO where PROF_COD_1 is <>'YES' for the whole set.

    At a second glance I noticed you didn't provide any sort order (ORDER BY clause) when you specified your c.u.r.s.o.r..

    This might lead to situation where rows are updated that you didn't expect. Without an ORDER BY specified it's up to SQL Server in what order to return the rows.

    Example:

    You have the following sequence:

    ID PROF_COD_1 PROF_NO_1

    1 'NO',0

    2 NULL,0

    3 'MAYBE',0

    4 'YES',0

    5 'WHO CARES',0

    6 'I don''t know',0

    If you'd use ORDER BY ID then all rows with ID<4 would be updated. If you'd specify ORDER BY ID DESC only rows 5 and 6 would be changed. But without any specific orde it might be any combination of rows from 1 to 6 (excluding row 4)


    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • October 19, 2010 at 4:20 pm

    #1239406

    I'm not sure if the c.u.r.s.o.r. will provide the results you're looking for...

    It would really help a lot if you could provide some more details (table def, sample data and expected result).

    I guess Drew is on the right track. But when dealing with data, we should not rely on guessing. We should know.


    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • October 19, 2010 at 4:29 pm

    #1239409

    Thank you Lutz and Drew. Drew, I will adjust my query accordingly. That makes sense. I also figured out that when tries to calculate my average in the update statement it is trying to divide by zero cause sometimes the Qty_on_hand and gl_val are zero. So I am thinking that I can narrow down my cursor statement some more by selecting only those rows where the QTY_ON_HND <> 0 or NULL. In the meantime, like Lutz has mentioned, there might be a better way to do this. Lutz, what is the best way to post the information you request? Are there help files in this forum about posting that kind of data? - Isaac

  • October 19, 2010 at 4:40 pm

    #1239413

    sqwasi (10/19/2010)

    Thank you Lutz and Drew. Drew, I will adjust my query accordingly. That makes sense. I also figured out that when tries to calculate my average in the update statement it is trying to divide by zero cause sometimes the Qty_on_hand and gl_val are zero. So I am thinking that I can narrow down my cursor statement some more by selecting only those rows where the QTY_ON_HND <> 0 or NULL. In the meantime, like Lutz has mentioned, there might be a better way to do this. Lutz, what is the best way to post the information you request? Are there help files in this forum about posting that kind of data? - Isaac

    Please have a look at the first link in my signature.

    As a side note: Thank you for your constructive replies and your effort in trying to learn a more efficient way rather than just the way that might just work. As long as people like you are out there I'm going to continue trying to help. People like you make it worth the effort. 🙂


    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • October 20, 2010 at 4:12 pm

    #1239990

    Lutz, thank you! I will read through that link. Today I was able to modify my code and I got it to work. I ran it and it only took 6 seconds. So maybe this time I wont worry about not using a cursor cause it seemed to accomplish what I needed to and without any problems. I will consider your advise next time and look at other options. I had to modify my calculations quite a bit cause once I got it running I realized I wasn't accounting for 0's and nulls. So here is how it ended up. Maybe it might help someone in the future. Thanks - Isaac

    USE [UA]

    GO

    /****** Object: StoredProcedure [dbo].[UA_USP_IM_UPD_PROF_NO_1_CO_AVG_COST] Script Date: 10/19/2010 14:06:53 ******/

    SET ANSI_NULLS ON

    GO

    SET QUOTED_IDENTIFIER ON

    GO

    /* Created by Isaac

    This is a custom procedure that looks at the company wide average cost for an item and populates the PROF_NO_1 field with

    with the value. It get the company wide average cost by adding all of the locations GL values and all of the locations qty

    on hand values and then dividing the Total GL Values by the Total Qty on Hand values. Then before it performs the update

    it checkes the Code Profile to see if it is being overriden. If it is then it will not update this value.*/

    CREATE procedure [dbo].[UA_USP_IM_UPD_PROF_NO_1_CO_AVG_COST]

    as

    begin

    set nocount on

    -- Create cursor and load in all current item numbers and PROF_COD_1. This allows us to run the update

    -- statement row by row and enter a different value for the PROF_NO_1. It only brings accross the item

    -- numbers that are not being set to be overriden.

    declare curItemList cursor for

    select ITEM_NO, PROF_COD_1

    from IM_ITEM

    where PROF_COD_1 is NULL or

    PROF_COD_1 <> 'YES'

    order by ITEM_NO

    open curItemList

    -- establish our variables

    declare @ItemNo T_ITEM_NO, @ProfCOD1 T_COD

    -- we need to fetch the first line before we can use the @@Fetch_Status

    fetch next from curItemList into @ItemNo, @ProfCOD1

    -- When it reaches the end of the IM_ITEM table it will end

    while @@Fetch_Status = 0

    begin

    update IM_ITEM

    set

    -- This sets the value we need. This is a company wide avg cost. We are taking the sum of the

    -- GL Value of all locations for that item number and dividing it by the sum of the Qty on Hand of all

    -- locations for that item number. If it is zero then we take the LST_AVG_COST or LST_COST

    PROF_NO_1 = (select

    case

    when sum(QTY_ON_HND) > 0 and sum(GL_VAL) >= 0

    then sum(GL_VAL) / sum(QTY_ON_HND)

    else

    case

    when avg(LST_AVG_COST) > 0

    then avg(LST_AVG_COST)

    else avg(LST_COST)

    end

    end

    from IM_INV

    where ITEM_NO = @ItemNo)

    where ITEM_NO = @ItemNo

    -- move onto the next row

    fetch next from curItemList into @ItemNo, @ProfCOD1

    end

    --need to close and deallocate the cursor when we are done.

    close curItemList

    deallocate curItemList

    end

  • October 20, 2010 at 5:00 pm

    #1240000

    You might want to compare the performance of your loop with the following code.

    I can't tell you if it'll return the same results since I have nothing to test against, but I think I covered everything from the loop... Give it a try and let us know... 😉

    ;WITH curItemList AS

    (

    SELECT ITEM_NO, PROF_COD_1

    FROM IM_ITEM

    WHERE PROF_COD_1 IS NULL OR

    PROF_COD_1 <> 'YES'

    ) , subqry AS

    (

    SELECT

    curItemList.ItemNo,

    CASE

    WHEN SUM(QTY_ON_HND) > 0 AND SUM(GL_VAL) >= 0

    THEN SUM(GL_VAL) / SUM(QTY_ON_HND)

    ELSE

    CASE

    WHEN AVG(LST_AVG_COST) > 0

    THEN AVG(LST_AVG_COST)

    ELSE AVG(LST_COST)

    END

    END as subqry_result

    FROM IM_INV

    INNER JOIN curItemList

    ON IM_INV.ITEM_NO = curItemList.ItemNo

    GROUP BY curItemList.ItemNo

    )

    UPDATE IM_ITEM

    SET

    PROF_NO_1 = subqry_result

    FROM IM_ITEM

    INNER JOIN subqry

    on IM_ITEM.ITEM_NO = subqry.ItemNo


    Lutz
    A pessimist is an optimist with experience.

    How to get fast answers to your question[/url]
    How to post performance related questions[/url]
    Links for Tally Table [/url] , Cross Tabs [/url] and Dynamic Cross Tabs [/url], Delimited Split Function[/url]

  • October 20, 2010 at 8:51 pm

    #1240035

    sqwasi (10/20/2010)

    Lutz, thank you! I will read through that link. Today I was able to modify my code and I got it to work. I ran it and it only took 6 seconds. So maybe this time I wont worry about not using a cursor cause it seemed to accomplish what I needed to and without any problems.

    Hmm, a c.u.r.s.o.r. processing in 6 seconds - must not have been very many rows to process.

    What happens to this code if you were to create a large (say, 1 million rows) dataset to process? (You might be looking at 6 minutes...)

    On that 1 million rows, Lutz's method should run in less than 10 seconds (with proper indexes).

    Wayne
    Microsoft Certified Master: SQL Server 2008
    Author - SQL Server T-SQL Recipes

    If you can't explain to another person how the code that you're copying from the internet works, then DON'T USE IT on a production system! After all, you will be the one supporting it!
    Links:
    For better assistance in answering your questions
    Performance Problems
    Common date/time routines
    Understanding and Using APPLY Part 1 & Part 2

Viewing 11 posts - 1 through 11 (of 11 total)

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

Cookies on SQLServerCentral

This website stores cookies on your computer.

These cookies are used to improve your website experience and provide more personalized services to you, both on this website and through other media.

To find out more about the cookies we use, see our Privacy Policy