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
asked Dec 1, 2016 at 10:57
\$\endgroup\$
0

1 Answer 1

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.

answered Dec 7, 2016 at 16:52
\$\endgroup\$
1
  • \$\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\$ Commented Jul 4, 2018 at 18:21

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.