none
SP design RRS feed

  • Question

  • We are web designers and were asked to speed up an SQL stored procedure.  It is getting a list of Patients and show counts of  unverified meals and snacks.  I see 2 sub selects to get counts.  I was wondering if anyone had an idea how to help me speed up this query.  I have also included the table schema if that helps.

    CREATE TABLE [dbo].[tblPatients](
    	[PatientID] [int] IDENTITY(1,1) NOT NULL,
    	[FirstName] [nvarchar](20) NULL,
    	[MiddleName] [nvarchar](20) NULL,
    	[LastName] [nvarchar](30) NULL,
    	[DOB] [smalldatetime] NOT NULL,
    	[StatusID] [smallint] NOT NULL,
    	[DietID] [int] NOT NULL,
    	[StartDiet] [smalldatetime] NULL,
    	[EndDiet] [smalldatetime] NULL,
    	[Email] [nvarchar](100) NULL,
    	[OrgID] [int] NOT NULL,
    	[Gender] [char](1) NULL,
    	[CaregiverLogin] [nvarchar](20) NULL,
    	[CaregiverPW] [nvarchar](20) NULL,
    	[LastLogin] [smalldatetime] NULL,
     CONSTRAINT [PK_tblPatients] PRIMARY KEY NONCLUSTERED 
    (
    	[PatientID] ASC
    )WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, FILLFACTOR = 90) ON [PRIMARY]
    ) ON [PRIMARY]
    GO
    
    ALTER TABLE [dbo].[tblPatients] ADD  CONSTRAINT [DF_tblPatients_StatusID]  DEFAULT (1) FOR [StatusID]
    GO
    
    ALTER TABLE [dbo].[tblPatients] ADD  CONSTRAINT [DF_tblPatients_DietID]  DEFAULT (0) FOR [DietID]
    GO
    
    ALTER TABLE [dbo].[tblPatients] ADD  CONSTRAINT [DF_tblPatients_UserID]  DEFAULT (0) FOR [OrgID]
    GO

    Below is the sp:

    ALTER PROCEDURE [dbo].[kd_selPatientsOneOrg]
    	@OrgID		int,
    	@ActiveOnly	bit = 1
    
    AS
    BEGIN
    	-- SET NOCOUNT ON added to prevent extra result sets from
    	-- interfering with SELECT statements.
    	SET NOCOUNT ON;
    
    	SELECT PatientID, 
    		FirstName, 
    		MiddleName, 
    		LastName, 
    		CASE WHEN MiddleName IS NULL THEN LastName + ', ' + FirstName
    			 ELSE LastName + ', ' + FirstName + ' ' + MiddleName 
    			 END AS LastFirst,
    		DOB, 
    		1 AS Age,
    		(SELECT DATEDIFF(month,[DOB],getdate()) % 12) AS AgeMonths,
    		StatusID, 
    		DietID, 
    		StartDiet, 
    		EndDiet, 
    		Email, 
    		OrgID, 
    		Gender, 
    		CaregiverLogin, 
    		CaregiverPW,
    		(SELECT COUNT(MealID)
    		   FROM dbo.tblActualMeals
    		  WHERE (Verified = 0)
    		  GROUP BY PatientID
    		 HAVING (PatientID = P.PatientID)
    		) As UnverifiedMeals,
    		(SELECT COUNT(SnackID)
    		   FROM dbo.tblActualSnacks
    		  WHERE (Verified = 0)
    		  GROUP BY PatientID
    		 HAVING (PatientID = P.PatientID)
    		) As UnverifiedSnacks
    	FROM dbo.tblPatients AS P
    	WHERE (P.OrgID = @OrgID) 
    	  AND (CASE WHEN @ActiveOnly = 1 AND P.StatusID = 1 THEN 'T'
    				WHEN @ActiveOnly = 0 AND P.StatusID <> 1 THEN 'T'
    				ELSE 'F'
    				END = 'T')
    	ORDER BY LastFirst;

    Friday, April 13, 2018 7:57 PM

Answers

  • Hi David,

    How about this stored procedure?

    ALTER PROCEDURE [dbo].[kd_selPatientsOneOrg]
    	@OrgID		int,
    	@ActiveOnly	bit = 1
    AS
    BEGIN
    	-- SET NOCOUNT ON added to prevent extra result sets from
    	-- interfering with SELECT statements.
    	SET NOCOUNT ON;
    
    	SELECT 
    	PatientID, 
    	FirstName, 
    	MiddleName, 
    	LastName, 
    	CASE WHEN MiddleName IS NULL THEN LastName + ', ' + FirstName ELSE LastName + ', ' + FirstName + ' ' + MiddleName END AS LastFirst,
    	DOB, 
    	1 AS Age,
    	DATEDIFF(month,[DOB],getdate()) % 12 AS AgeMonths,
      --(SELECT DATEDIFF(month,[DOB],getdate()) % 12) AS AgeMonths,
    	StatusID, 
    	DietID, 
    	StartDiet, 
    	EndDiet, 
    	Email, 
    	OrgID, 
    	Gender, 
    	CaregiverLogin, 
    	CaregiverPW,
    		--(SELECT COUNT(MealID)
    		--   FROM dbo.tblActualMeals
    		--  WHERE (Verified = 0)
    		--  GROUP BY PatientID
    		-- HAVING (PatientID = P.PatientID)
    		--) As UnverifiedMeals,
    		P1.UnverifiedMeals,
    		--(SELECT COUNT(SnackID)
    		--   FROM dbo.tblActualSnacks
    		--  WHERE (Verified = 0)
    		--  GROUP BY PatientID
    		-- HAVING (PatientID = P.PatientID)
    		--) As UnverifiedSnacks
    		P2.UnverifiedSnacks
    	FROM dbo.tblPatients AS P
    	CROSS APPLY ( SELECT COUNT(MealID) AS UnverifiedMeals FROM dbo.tblActualMeals
    		          WHERE Verified = 0 AND PatientID = P.PatientID
    		        ) P1
        CROSS APPLY (SELECT COUNT(SnackID) As UnverifiedSnacks FROM dbo.tblActualSnacks
    		  WHERE Verified = 0 AND PatientID = P.PatientID
    	            )P2
    	WHERE P.OrgID = @OrgID
    	  --AND CASE WHEN @ActiveOnly = 1 AND P.StatusID = 1 THEN 'T'
    			--   WHEN @ActiveOnly = 0 AND P.StatusID <> 1 THEN 'T'
    			--   ELSE 'F' END = 'T'
    			 AND ((@ActiveOnly = 1 AND P.StatusID = 1) OR (@ActiveOnly = 0 AND P.StatusID <> 1))		   
    	ORDER BY LastFirst;

    Best Regards,

    Will


    MSDN Community Support
    Please remember to click "Mark as Answer" the responses that resolved your issue, and to click "Unmark as Answer" if not. This can be beneficial to other community members reading this thread. If you have any compliments or complaints to MSDN Support, feel free to contact MSDNFSF@microsoft.com.

    • Marked as answer by David Chase89 Thursday, April 19, 2018 2:15 PM
    Monday, April 16, 2018 6:14 AM
    Moderator

All replies

  • For a start:

    WHERE (P.OrgID = @OrgID) 
      AND ((@ActiveOnly = 1 AND P.StatusID = 1)
        OR (@ActiveOnly = 0 AND P.StatusID <> 1))
    


    Russel Loski, MCSE Data Platform/Business Intelligence Twitter: @sqlmovers; blog: www.sqlmovers.com

    Friday, April 13, 2018 8:35 PM
  • First create this indexs and try with this version of the SP

    CREATE NONCLUSTERED INDEX [IX_tblPatients] ON [dbo].[tblPatients] ( [LastName] ASC, [FirstName] ASC )

    CREATE NONCLUSTERED INDEX [IX_tblPatients_1] ON [dbo].[tblPatients]
    (
     [StatusID] ASC
    )
    CREATE NONCLUSTERED INDEX [IX_tblPatients_2] ON [dbo].[tblPatients]
    (
     [OrgID] ASC
    )

    ALTER PROCEDURE [dbo].[kd_selPatientsOneOrg]
                    @OrgID      int,
                    @ActiveOnly bit = 1
    AS
        BEGIN
            SET NOCOUNT ON;
    
            SELECT
                   [PatientID]
                  ,[FirstName]
                  ,[MiddleName]
                  ,[LastName]
                  ,[LastFirst] = [LastName]+', '+[FirstName]+iif([MiddleName] IS NULL,'',' '+[MiddleName])
                  ,[DOB]
                  ,1 AS [Age]
                  ,[AgeMonths]=
                   (
                       SELECT
                              datediff(month,[DOB],getdate()) % 12
                   ) 
                  ,[StatusID]
                  ,[DietID]
                  ,[StartDiet]
                  ,[EndDiet]
                  ,[Email]
                  ,[OrgID]
                  ,[Gender]
                  ,[CaregiverLogin]
                  ,[CaregiverPW]
                  ,[UnverifiedMeals] =
                   (
                       SELECT
                              count([MealID])
                       FROM
                            [dbo].[tblActualMeals]
                       WHERE  [Verified] = 0
                              AND
                              [PatientID] = [P].[PatientID]
                   )
                  ,[UnverifiedSnacks] =
                   (
                       SELECT
                              count([SnackID])
                       FROM
                            [dbo].[tblActualSnacks]
                       WHERE  [Verified] = 0
                              AND
                              [PatientID] = [P].[PatientID]
                   )
            FROM
                 [dbo].[tblPatients] AS [P]
            WHERE
                   [P].[OrgID] = @OrgID
                   AND 
    			   (@ActiveOnly = 1
                       AND
                   [P].[StatusID] = 1)
                   OR 
    			   (@ActiveOnly = 0
                      AND
                   [P].[StatusID] <> 1)
            ORDER BY
                  [LastName]
                  ,[FirstName]
        END;


    Norman



    Friday, April 13, 2018 8:58 PM
  • i would try replacing the subqueries with joins as well

    ALTER PROCEDURE [dbo].[kd_selPatientsOneOrg]
    	@OrgID		int,
    	@ActiveOnly	bit = 1
    
    AS
    BEGIN
    	-- SET NOCOUNT ON added to prevent extra result sets from
    	-- interfering with SELECT statements.
    	SET NOCOUNT ON;
    
    	SELECT PatientID, 
    		FirstName, 
    		MiddleName, 
    		LastName, 
    		CONCAT(LastName + ', ',FirstName + ' ', MiddleName )			 END AS LastFirst,
    		DOB, 
    		1 AS Age,
    		(SELECT DATEDIFF(month,[DOB],getdate()) % 12) AS AgeMonths,
    		StatusID, 
    		DietID, 
    		StartDiet, 
    		EndDiet, 
    		Email, 
    		OrgID, 
    		Gender, 
    		CaregiverLogin, 
    		CaregiverPW,
    		M.MealCount As UnverifiedMeals,
    		S.SnackCount As UnverifiedSnacks
    	FROM dbo.tblPatients AS P
            LEFT JOIN (SELECT PatientID,COUNT(MealID) AS MealCount
    		   FROM dbo.tblActualMeals
    		  WHERE (Verified = 0)
    		  GROUP BY PatientID
    		) M
            ON M.PatientID = P.PatientID
           LEFT JOIN ((SELECT PatientID,
                       COUNT(SnackID) AS SnackCount
    		   FROM dbo.tblActualSnacks
    		  WHERE (Verified = 0)
    		  GROUP BY PatientID
    		)S
            ON S.PatientID = P.PatientID
    	WHERE (P.OrgID = @OrgID) 
    	  AND 
    ((@ActiveOnly = 1 AND P.StatusID = 1)
    OR (@ActiveOnly = 0 AND P.StatusID <> 1))
    	ORDER BY LastFirst;


    Please Mark This As Answer if it solved your issue
    Please Vote This As Helpful if it helps to solve your issue
    Visakh
    ----------------------------
    My Wiki User Page
    My MSDN Page
    My Personal Blog
    My Facebook Page

    Saturday, April 14, 2018 9:22 AM
  • Hi David,

    How about this stored procedure?

    ALTER PROCEDURE [dbo].[kd_selPatientsOneOrg]
    	@OrgID		int,
    	@ActiveOnly	bit = 1
    AS
    BEGIN
    	-- SET NOCOUNT ON added to prevent extra result sets from
    	-- interfering with SELECT statements.
    	SET NOCOUNT ON;
    
    	SELECT 
    	PatientID, 
    	FirstName, 
    	MiddleName, 
    	LastName, 
    	CASE WHEN MiddleName IS NULL THEN LastName + ', ' + FirstName ELSE LastName + ', ' + FirstName + ' ' + MiddleName END AS LastFirst,
    	DOB, 
    	1 AS Age,
    	DATEDIFF(month,[DOB],getdate()) % 12 AS AgeMonths,
      --(SELECT DATEDIFF(month,[DOB],getdate()) % 12) AS AgeMonths,
    	StatusID, 
    	DietID, 
    	StartDiet, 
    	EndDiet, 
    	Email, 
    	OrgID, 
    	Gender, 
    	CaregiverLogin, 
    	CaregiverPW,
    		--(SELECT COUNT(MealID)
    		--   FROM dbo.tblActualMeals
    		--  WHERE (Verified = 0)
    		--  GROUP BY PatientID
    		-- HAVING (PatientID = P.PatientID)
    		--) As UnverifiedMeals,
    		P1.UnverifiedMeals,
    		--(SELECT COUNT(SnackID)
    		--   FROM dbo.tblActualSnacks
    		--  WHERE (Verified = 0)
    		--  GROUP BY PatientID
    		-- HAVING (PatientID = P.PatientID)
    		--) As UnverifiedSnacks
    		P2.UnverifiedSnacks
    	FROM dbo.tblPatients AS P
    	CROSS APPLY ( SELECT COUNT(MealID) AS UnverifiedMeals FROM dbo.tblActualMeals
    		          WHERE Verified = 0 AND PatientID = P.PatientID
    		        ) P1
        CROSS APPLY (SELECT COUNT(SnackID) As UnverifiedSnacks FROM dbo.tblActualSnacks
    		  WHERE Verified = 0 AND PatientID = P.PatientID
    	            )P2
    	WHERE P.OrgID = @OrgID
    	  --AND CASE WHEN @ActiveOnly = 1 AND P.StatusID = 1 THEN 'T'
    			--   WHEN @ActiveOnly = 0 AND P.StatusID <> 1 THEN 'T'
    			--   ELSE 'F' END = 'T'
    			 AND ((@ActiveOnly = 1 AND P.StatusID = 1) OR (@ActiveOnly = 0 AND P.StatusID <> 1))		   
    	ORDER BY LastFirst;

    Best Regards,

    Will


    MSDN Community Support
    Please remember to click "Mark as Answer" the responses that resolved your issue, and to click "Unmark as Answer" if not. This can be beneficial to other community members reading this thread. If you have any compliments or complaints to MSDN Support, feel free to contact MSDNFSF@microsoft.com.

    • Marked as answer by David Chase89 Thursday, April 19, 2018 2:15 PM
    Monday, April 16, 2018 6:14 AM
    Moderator
  • All of these solutions get correct results so how do I know which one has the best performance?
    Monday, April 16, 2018 11:11 PM
  • How long it takes to run? If it is less than a minute then you are ok.
    Tuesday, April 17, 2018 1:27 AM
  • All of these solutions get correct results so how do I know which one has the best performance?

    Hi David,

    You could do like this.

    DBCC DROPCLEANBUFFERS; 
    DBCC FREEPROCCACHE;
    SET STATISTICS TIME ON;
    
    EXEC [Your Procedure Name]
    
    SET STATISTICS TIME OFF;

    For more information, please see:

    SET STATISTICS TIME (Transact-SQL)

    Best Regards,

    Will


    MSDN Community Support
    Please remember to click "Mark as Answer" the responses that resolved your issue, and to click "Unmark as Answer" if not. This can be beneficial to other community members reading this thread. If you have any compliments or complaints to MSDN Support, feel free to contact MSDNFSF@microsoft.com.

    Tuesday, April 17, 2018 2:22 AM
    Moderator
  • Thanks, I'll try that.
    Tuesday, April 17, 2018 1:06 PM
  • >> I have also included the table schema if that helps. <<

    YES! Bless you! Most people who post never bother to think about DDL, or appreciate how important it is.

    But it is clear that you are not DB people. Your schema is crap. It has no key, the data element names volate ISO-11179 rules and you have more NULLs in one table than I would have in an RDBMS for a major corporation. 

    For example, the use of the prefix "tbl" is a design flaw. It's so bad it has a name; it's called a Tibble. It mixes data and metadata in the name. I hope you know that the identity table property (it is not a column!) Counts the physical insertion attempts to a physical table on one particular machine, on one particular disk disk drive. It has nothing whatsoever to do with a valid logical model. Even worse, you it's an INTEGEReger. Keys are usually identifiers, and therefore cannot be numerics because you don't do any computations with them.

    You also have no idea what the R in RDBMS stands for. A diet is not in an innate part or characteristic of the patient. It is a relationship the patient has with the diet. In short, this awful schema is simply not normalized! You’ve crammed two or three tables INTEGERo one horrible non-table.

    There is also no such crap as a “status_id”; it has to be a some kind of status, because things like status and identifier are what ISO calls attribute properties. Essentially, if this was English, you’ve just written a string of adjectives without a noun. And it really looks that bad.

    Finally, a personal gripe of mine, we don’t use BIT flags in RDBMS. That was assembly language programming back when I started over 50 years ago. In SQL. We discovered the state of something, not by looking at a flag that was set in the previous pass through the machine, but by the current status of the data.

    I would give you a C or D if you were in one of my database classes. Frankly, I rather flunk you but after teaching this for over 30 years, my expectations have been lowered :(

    Now let’s try to do a fix on the skeleton. let’s start with the patient’s and the things we know that are attributes of a patient;

    CREATE TABLE Patients
    (patient_id CHAR(10) NOT NULL PRIMARY KEY,
     first_name NVARCHAR(20) DEFAULT ‘’ NOT NULL,
     last_name NVARCHAR(30) DEFAULT ‘’ NOT NULL,
    CHECK (first_name || last_name <> ‘’),-- has to have a name
     birth_date DATE NOT NULL,
     sex_code CHAR(1) NOT NULL
     CHECK(sex_code in (‘0’,’1’,’2’,’9’), --- iso standard
     email VARCHAR(256);

    while it is rare, it is actually possible to have an insanely long email address. If you don’t allow for you will get screwed; this is one of Mother Celko’s laws about data. The postfix “_date” is one of the ISO attribute properties, so while DOB is a common abbreviation it’s not technically correct. I’m also going to assume that you have searched carefully and have good reason for the lengths of Unicode characters you allow for names. So many people don’t bother with that or even look up the postal union rules for the length of an address line. That length is 35 characters, and if shorten names to that inside your database, you will be following Brent’s rule – “store data the way you use it, and use data the way you store it” 

    I’m not sure what to do with the next data element. There is no such thing in RDBMS generic status, it has to be the status of something in particular – employment, educational level, marriage and whatever. You run INTEGERo the comedian George Carlin? One of his routines is to give baseball scores, but without any team names. That’s what a status without an attribute is :-(

    foobar_status SMALLINTEGER NOT NULL,
     
    another little problem is that a status is what we call a nominal scale in data modeling. It has to be a list of discrete values

     CREATE TABLE Diets
    (diet_id CHAR(5) NOT NULL PRIMARY KEY,
     diet_start_date DATE DEFAULT CURRENT_TIMESTAMP NOT NULL,
     diet_end_date DATE NOT NULL,
     CHECK (diet_start_date <= diet_end_date)

    now we need to establish a relationship between the patients and their diets; the name menu seems reasonable. It might look something like this, but since I’m having to do a complete design without any specs. I don’t know. Like I said, this is just a skeleton to get you started.

    CREATE TABLE Menu
    (diet_id CHAR(5) NOT NULL
     REFERENCES Diets
     ON UPDATE CASCADE
     ON DELETE CASCADE,
     patient_id CHAR(10) NOT NULL
     REFERENCES Patients(patient_id
     ON UPDATE CASCADE
     ON DELETE CASCADE,
    PRIMARY KEY (diet_id, patient_id)
    );

    I have no idea an order organizational ID is or why you think a caregiver login should be part of a patient. I would think of the caregiver and the patient have a relationship, which means another table. The caregivers last login is not an attribute of a patient! It’s another relationship and it’s not even a relationship with a patient. It’s a relationship the caregiver has with himself.

    We can the body of your procedure, It is also a disaster. SQL is a tiered architecture declarative language; we spent a lot of time on the committee making sure. You actually assemble a name piece by piece, using string values. You’re not supposed to do that in a tiered architecture your passive components to the next tier (report, display, whatever) and do it at that tier. Never, never at the database level. This is how we did it with monolithic COBOL programs and you’re still writing code is it was the 1960s. I’ve already commented on how non-relational using BIT flag’s is in RDBMS.

    You also don’t seem to understand that a caregiver is an entity in and of itself, not an attribute of a patient. This means your schema should have had at least two tables showing the relationship of a caregiver to a patient as well as the caregivers as separate entities. Here’s a quick skeleton

    CREATE TABLE Caregivers
    (caregiver_id CHAR(??) NOT NULL PRIMARY KEY, 
     caregiver_password VARCHAR(??) NOT NULL,
    ..);
    CREATE TABLE Caregiver_Assignments
    (caregiver_id CHAR(??) NOT NULL
      REFERENCES Caregivers,
     patient_id CHAR(10) NOT NULL
      ON DELETE CASCADE, 
    ..);

    I’m trying to figure out why anybody would ever build a table called “Actual_Snacks” when a snack seems to be a kind of meal. Frankly, it looks like nobody has any idea how to normalize schema, or follow any data modeling rules. This looks like somebody copied a paper form or report, field for field, into a meaningless table. I’m not going to spend hours and days giving you free consulting, but you really need to sit down and spend some time correcting this. 

    --CELKO-- Books in Celko Series for Morgan-Kaufmann Publishing: Analytics and OLAP in SQL / Data and Databases: Concepts in Practice Data / Measurements and Standards in SQL SQL for Smarties / SQL Programming Style / SQL Puzzles and Answers / Thinking in Sets / Trees and Hierarchies in SQL

    Tuesday, April 17, 2018 4:16 PM