Stored Procedure Review

  • I have never written a stored procedure before and need someone to tell me that I am not doing something completely wrong before I blow up our database. We use a ERP system called Vision and I want to call this procedure from Vision as a Scheduled Workflow each morning to update our Projects table with billing information. This procedure is written to loop through the projects by number, where wbs1 is the project number field (Varchar type). The procedure will copy the sum of the billed amounts from prsummarymain and place them into projectcustomtabfields. Does this make sense?

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

    ALTER PROCEDURE [dbo].[usp_updateProjectInfoTab]

    AS

    BEGIN TRANSACTION

    declare @TotalBilled int, @projnum int

    set @projnum=1

    while @projnum < 199999

    begin

    set @TotalBilled = 0

    set @TotalBilled = (select sum(billed) from prsummarymain

    where wbs1=CAST(@projnum AS char(6))

    and wbs1 not like '%?%'

    and wbs1 not LIKE '[A-Z]%'

    and billed > 0 )

    while @TotalBilled is not null

    begin

    update projectcustomtabfields

    set custJTDBilled =@TotalBilled

    where wbs1 = CAST(@projnum AS char(6)) and wbs2=''

    end

    set @projnum = @projnum+1

    end

    COMMIT

  • I am pretty sure that this is not working correctly as I executed it and it kept going for ten minutes. Meanwhile it generated 10 million lines of "1 row affected" while there are only 17,000 records in the database. I must be way off.

  • david.sims (7/23/2008)


    I have never written a stored procedure before and need someone to tell me that I am not doing something completely wrong before I blow up our database. We use a ERP system called Vision and I want to call this procedure from Vision as a Scheduled Workflow each morning to update our Projects table with billing information. This procedure is written to loop through the projects by number, where wbs1 is the project number field (Varchar type). The procedure will copy the sum of the billed amounts from prsummarymain and place them into projectcustomtabfields. Does this make sense?

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

    ALTER PROCEDURE [dbo].[usp_updateProjectInfoTab]

    AS

    BEGIN TRANSACTION

    declare @TotalBilled int, @projnum int

    set @projnum=1

    while @projnum < 199999

    begin

    set @TotalBilled = 0

    set @TotalBilled = (select sum(billed) from prsummarymain

    where wbs1=CAST(@projnum AS char(6))

    and wbs1 not like '%?%'

    and wbs1 not LIKE '[A-Z]%'

    and billed > 0 )

    while @TotalBilled is not null

    begin

    update projectcustomtabfields

    set custJTDBilled =@TotalBilled

    where wbs1 = CAST(@projnum AS char(6)) and wbs2=''

    end

    set @projnum = @projnum+1

    end

    COMMIT

    I don't think you need to use a cursor here at all. Before setting this as an update, write the select statement that returns the desired results. Once you have that - it is pretty easy to turn that into an update statement.

    I would start with:

    SELECT wbs1

    ,SUM(billed) As TotalBilled

    FROM prsummarymain

    WHERE wbs1 not like '%?%'

    AND wbs1 not LIKE '[A-Z]%'

    AND billed > 0

    Once you have this working, then the update statement would be:

    /*

    create a temporary copy for testing (don't want to update the production table until we

    know this works correctly, so we are going to update a temporary table

    */

    IF OBJECT_ID('tempdb..#temp_projectcustomtabfields') IS NOT NULL

    BEGIN;

    DROP TABLE #temp_projectcustomtabfields;

    END;

    SELECT * INTO #temp_projectcustomtabfields FROM projectcustomtabfields;

    /* the above is for testing only - once you have it working correctly

    change the table in the update statement to your actual table

    */

    UPDATE #temp_projectcustomtabfields

    SET custJTDBilled = (SELECT SUM(billed)

    FROM prsummarymain m

    WHERE m.wbs1 = projectcustomtabfields.wbs1)

    WHERE wbs2 = '';

    You probably will need to modify the WHERE clause on the update statement, but I hope this gets you a lot closer.

    Jeffrey Williams
    Problems are opportunities brilliantly disguised as insurmountable obstacles.

    How to post questions to get better answers faster
    Managing Transaction Logs

  • david.sims (7/23/2008)


    I am pretty sure that this is not working correctly as I executed it and it kept going for ten minutes. Meanwhile it generated 10 million lines of "1 row affected" while there are only 17,000 records in the database. I must be way off.

    10 sounds about right for a loop. Jeffrey has given you a good base to start from. Remember that SQL is about working with SETS of data not single rows and you'll get it.

    Jack Corbett
    Consultant - Straight Path Solutions
    Check out these links on how to get faster and more accurate answers:
    Forum Etiquette: How to post data/code on a forum to get the best help
    Need an Answer? Actually, No ... You Need a Question

  • david.sims (7/23/2008)


    ...

    ...

    while @TotalBilled is not null

    begin

    update projectcustomtabfields

    set custJTDBilled =@TotalBilled

    where wbs1 = CAST(@projnum AS char(6)) and wbs2=''

    end

    ...

    This loop is never going to quit - the value for @TotalBilled never changes inside the WHILE loop, so it can never become NULL and end the loop. Still, Jeffrey and Jack have it right - try writing it as a set-based update.

  • Simon Facer

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

    This loop is never going to quit - the value for @TotalBilled never changes inside the WHILE loop, so it can never become NULL and end the loop. Still, Jeffrey and Jack have it right - try writing it as a set-based update.

    Simon,

    but how you identify this?....;)

    Cheers!

    Sandy.

    --

  • Sandy (7/23/2008)


    Simon,

    but how you identify this?....;)

    To get out of a while loop, there must be a statement within the loop that sets the variable involved in the while condition to a value where the condition is no longer true.

    The format for a while loop (and this isn't only SQL) is

    While (Some condition is true)

    Do something

    If, within the block of Do something, you never change the conditions checked in the condition, the condition will never bocome false and the look will never exit.

    Compare these two SQL ones

    -- This loop will never exit because the value of @i is not changed within the loop

    DECLARE @i int

    SET @i = 0

    WHILE (@i<10)

    BEGIN

    SELECT @i

    END

    -- This loop will exit after 10 itterations

    DECLARE @i int

    SET @i = 0

    WHILE (@i<10)

    BEGIN

    SELECT @i

    SET @i = @i+1 -- change the variable involved in the loop condition

    END

    Gail Shaw
    Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
    SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

    We walk in the dark places no others will enter
    We stand on the bridge and no one may pass
  • This is originally how I set it up. The problem arises when you have a project that has not been billed. Because it has no billing activity, the sum(billed) is null.

    I get this error message

    Cannot insert the value NULL into column 'custJTDBilled' ... column does not allow nulls. UPDATE fails.

    I rewrote the query and still get an error message.

    declare @totalbilled int

    set @totalbilled = (SELECT SUM(billed)

    FROM prsummarymain m

    WHERE m.wbs1 = projectcustomtabfields.wbs1 )

    UPDATE projectcustomtabfields

    SET custJTDBilled = case

    when @totalbilled > 0 then @totalbilled

    else 0

    end

    WHERE wbs2 = '';

    Msg 4104, Level 16, State 1, Line 2

    The multi-part identifier "projectcustomtabfields.wbs1" could not be bound.

  • david.sims (7/24/2008)


    This is originally how I set it up. The problem arises when you have a project that has not been billed. Because it has no billing activity, the sum(billed) is null.

    I get this error message

    Cannot insert the value NULL into column 'custJTDBilled' ... column does not allow nulls. UPDATE fails.

    I rewrote the query and still get an error message.

    declare @totalbilled int

    set @totalbilled = (SELECT SUM(billed)

    FROM prsummarymain m

    WHERE m.wbs1 = projectcustomtabfields.wbs1 )

    UPDATE projectcustomtabfields

    SET custJTDBilled = case

    when @totalbilled > 0 then @totalbilled

    else 0

    end

    WHERE wbs2 = '';

    Msg 4104, Level 16, State 1, Line 2

    The multi-part identifier "projectcustomtabfields.wbs1" could not be bound.

    The problem is that you have not referenced your projectcustomtabfields table anywhere.

    try writing this using an inner join

    set @totalbilled = (SELECT SUM(billed)

    FROM prsummarymain m inner join projectcustomtabfields p on m.wbs1=p.wbs1)

  • That's still a row-by-row scenario. Look at Jeffrey's suggestion. A set-based single update is the optimal way of doing things.

    Gail Shaw
    Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
    SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

    We walk in the dark places no others will enter
    We stand on the bridge and no one may pass
  • Thanks for everyone's help. I think I stumbled into the solution. I was getting null values for some of the sum(billed) because not every project had been billed out. so, here is what I ended up with.

    create table #temptable (wbs1 varchar(15), totalbilled money)

    Insert into #temptable (wbs1, totalbilled)

    SELECT wbs1, convert(money, SUM(billed) )

    FROM prsummarymain m

    group by wbs1

    UPDATE projectcustomtabfields

    SET projectcustomtabfields.custJTDTotalBilled =totalbilled

    from #temptable t

    inner join projectcustomtabfields p on t.wbs1 = p.wbs1

    WHERE t.wbs1 = p.wbs1 and p.wbs2=''

    drop table #temptable

  • GilaMonster (7/24/2008)


    Sandy (7/23/2008)


    Simon,

    but how you identify this?....;)

    To get out of a while loop, there must be a statement within the loop that sets the variable involved in the while condition to a value where the condition is no longer true.

    ...

    Couldn't have said it better, thanks Gail.

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

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