Stored procedure inserting the same record repeatedly instead of looping through list from SELECT

Question

I am fairly new at writing procedures (beyond the basics)

I am trying to write a stored procedure that inserts into a table (dbo.billing_batch) based on a select statement that loops through the list of results (@DealerID FROM dbo.vehicle_info).

The SELECT DISTINCT... statement on its own works perfectly and returns a list of 54 records.

The result of the SELECT statement is dynamic and will change from week to week, so I cannot count on 54 records each time.

I am trying to use WHILE @DealerID IS NOT NULL to loop through the INSERT routine.

The loop is supposed to update dbo.billing_batch, however it is inserting the same 1st record (BillingBatchRosterID, DealerID) over and over and over to infinity.

I know I must be doing something wrong (I have never written a stored procedure that loops).

Any help would be greatly appreciated!

Here is the stored procedure code:

ALTER PROCEDURE [dbo].[sp_billing_batch_set]
    @varBillingBatchRosterID int
AS 
    SET NOCOUNT ON;
BEGIN
    DECLARE @DealerID int

    SELECT DISTINCT @DealerID = vi.DealerID
    FROM dbo.vehicle_info vi
    LEFT JOIN dbo.dealer_info di ON di.DealerID = vi.DealerID
    WHERE di.DealerActive = 1 
      AND (vi.ItemStatusID < 4 OR vi.ItemStatusID = 5 OR vi.ItemStatusID = 8)
END
 
WHILE @DealerID IS NOT NULL
BEGIN TRY
    INSERT INTO dbo.billing_batch (BillingBatchRosterID, DealerID)
    VALUES(@varBillingBatchRosterID,    -- BillingBatchRosterID - int
           @DealerID)                   -- DealerID - int
END TRY
BEGIN CATCH
    SELECT  ' There was an error: '  + error_message() AS ErrorDescription
END CATCH

1. Why are you using a `WHILE` at all here? 2. Where is the rest of your Procedure? There's no `END` to your `WHILE`. 3. Why would you expect anything else, you don't appear to assign the value of `@DealerID` again, so it'll infinately loop on the same value. — Thom A, Oct 06 '20 at 14:42
Side note: you should **not** use the `sp_` prefix for your stored procedures. Microsoft has [reserved that prefix for its own use (see *Naming Stored Procedures*)](http://msdn.microsoft.com/en-us/library/ms190669%28v=sql.105%29.aspx), and you do run the risk of a name clash sometime in the future. [It's also bad for your stored procedure performance](http://www.sqlperformance.com/2012/10/t-sql-queries/sp_prefix). It's best to just simply avoid `sp_` and use something else as a prefix - or no prefix at all! — marc_s, Oct 06 '20 at 14:45
Do not **eat the error** nor "handle it" within the procedure. Let the code that executes the procedure do that in whatever fashion it deems appropriate. Error messages should not be returned as a resultset. — SMor, Oct 06 '20 at 15:52

seanb · Accepted Answer · 2020-10-06T15:01:17.100

You have the same problems as another recent post here: Iterate over a table with a non-int id value

Why do a loop? Just do it as a single SQL statement
If you must use a loop, you will need to update your @Dealer value at each run (e.g., to the next DealerId) otherwise it will just infinitely loop with the same DealerID value
Don't do a loop.

Here's an example not needing a loop.

ALTER PROCEDURE [dbo].[P_billing_batch_set]
    @varBillingBatchRosterID int
AS 
BEGIN
SET NOCOUNT ON;

BEGIN TRY

INSERT INTO dbo.billing_batch (DealerID, BillingBatchRosterID)
    SELECT DISTINCT vi.DealerID, @varBillingBatchRosterID
    FROM dbo.vehicle_info vi
        INNER JOIN dbo.dealer_info di ON di.DealerID = vi.DealerID
    WHERE di.DealerActive = 1 
        AND (vi.ItemStatusID < 4 
            OR vi.ItemStatusID = 5
            OR vi.ItemStatusID = 8
            );

END TRY
BEGIN CATCH
    SELECT  ' There was an error: '  + error_message() AS ErrorDescription;
END CATCH;

END;

Note I

Changed the LEFT JOIN to an INNER JOIN as your WHERE clause needs the record to exist in the dealer_info table
Moved the SET NOCOUNT ON; to be within the BEGIN-END section
Moved the END to the end
Renamed your stored procedure as per the excellent comment from @marc_s (on the question itself)

I believe the value for the second column `BillingBatchRosterID` would come from the **parameter** of the stored procedure, `@varBillingBatchRosterID` - not from the table being selected from — marc_s, Oct 06 '20 at 14:47
Sorry, I wrote that before I saw the updated question from OP. Will update because of several other issues too. — seanb, Oct 06 '20 at 14:50

Stored procedure inserting the same record repeatedly instead of looping through list from SELECT

1 Answers1