2
\$\begingroup\$

The below code is run on a routine rather than say login time but being new to using FETCH NEXT I can't help think maybe there is a trick here to merge at least one of the SET lines.

The end results should give me the total unique users, how many completed a module and how many passed.

Fields are indexed on the table at this time but all additional thoughts welcome.

 USE [DB]
    GO
    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO
    ALTER PROCEDURE [dbo].[spModuleStatisticsUnique2]
    -- Add the parameters for the stored procedure here
    AS

    DECLARE @i INT = 0;
DECLARE @i INT = 0;
DECLARE @Started   int;
DECLARE @Completed  int;
DECLARE @Passed  int;
DECLARE @ModuleID  int;
    DECLARE merge_cursor CURSOR FAST_FORWARD FOR SELECT [ModuleID] FROM dbo.[TblModules] 
    --ORDER BY [ModuleID]
    OPEN merge_cursor

    FETCH NEXT FROM merge_cursor INTO @ModuleID

    WHILE @@FETCH_STATUS = 0
    BEGIN 

        SET @Started =  (Select Count(ID) FROM [TblResults] WHERE [ModuleID] = @ModuleID) 
        SET @Completed = (Select Count(ID) FROM [TblResults] WHERE [ModuleID] = @ModuleID and ModuleDatecomplete <> '')
        SET @Passed = (Select Count(ID) FROM [TblResults] WHERE [ModuleID] = @ModuleID and Pass = 'Yes')

        UPDATE [TblModules]
        SET 
        [Started] = @Started
        ,[Completed] = @Completed
        ,[Passed] = @Passed
        ,[PassedPercent] = ((@Passed * 100.0) / @Started)
        WHERE 
        [ModuleID] = @ModuleID 

    FETCH NEXT FROM merge_cursor INTO @ModuleID 
    END

    CLOSE merge_cursor
    DEALLOCATE merge_cursor
\$\endgroup\$
2
\$\begingroup\$

I don't think you need a cursor, looks like a simple update:

 WITH cte AS
  ( SELECT -- do all the counts using conditional aggregation
       ModuleID 
       ,Count(DISTINCT ID) AS Started
       ,Count(DISTINCT CASE WHEN ModuleDatecomplete <> '' THEN ID END) AS Completed
       ,Count(DISTINCT CASE WHEN Pass = 'Yes' THEN ID END) AS Passed
    FROM TblResults
    GROUP BY ModuleID
  )
 UPDATE TblModules
 FROM cte 
 SET 
   Started = cte.Started
   ,Completed = cte.Completed
   ,Passed = cte.Passed
   ,PassedPercent = ((cte.Passed * 100.0) / cte.Started)
 WHERE 
    ModuleID = cte.ModuleID 

Of course this is fully untested, but the syntax should be ok as-is.

Edit:

In 99+% rewriting a cursor with set-based SQL improves performance.
In your case you run three Selects and one Update per row, e.g. for 100 rows returned by the cursor Select the DBMS needs to execute 400 statements sequentially, 300 Selects and 100 Updates. Combining the three SETs into one using conditional aggregation would reduce the amount of work to 100 Selects and 100 Updates and result in approx. half the runtime. But a single Update might run 10 or 100 or 1000 times faster.

\$\endgroup\$
  • \$\begingroup\$ I know this is an old answer, but could you please add a little more explanation why this works as good or better? \$\endgroup\$ – Malachi Jul 4 '18 at 18:21
  • \$\begingroup\$ Hey, thank you for giving more detail on this. \$\endgroup\$ – Malachi Jul 5 '18 at 14:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.