June 14, 2003 at 8:46 am
Can you help us evaluate this Stored Procedure?
Needless to say we want this SP to run as fast as possible. All the INNER JOINS are either Primary or Foreign keys. appt.apptdate is a clustered key. We have around 500,000 records in the appt table.
@Mode = ‘FD_DAILYREGISTER’ will normally bring back less than 1000 records.
@Mode = ‘FD_APPTREGISTER’ will normally bring back less than 3000 records.
@Mode = ‘FD_OPENCHARTS’ will normally bring back less than 3000 records.
@Mode = ‘FD_OPENTICKETS’ will normally bring back less than 3000 records.
The rest of the appt table records are not needed in this SP, but are need in other SPs.
Thanks for your help.
Phil
CREATE PROCEDURE GetFrontDeskData
(
@PracticeID bigint,
@LocationID bigint,
@Mode nvarchar(25)
)
AS
SET DATEFORMAT mdy
DECLARE @DateHolder AS DATETIME
SET @DateHolder = (CONVERT(char(25), GETDATE(), 101) + '23:59:59')
DECLARE @tdate AS DATETIME
SET @tdate = GETDATE()
SET CONCAT_NULL_YIELDS_NULL OFF
/* Get Appt Records */
SELECT
appt.apptid,
appt.statusid,
appt.roomid,
appt.stateid,
appt.problemid,
appt.patientid,
appt.userid,
appt.pcdrid,
appt.stypeid,
appt.providerid,
appt.locationid,
appt.pcdrid,
appt.paymentid1,
appt.paymentid2,
appt.paymentid3,
appt.paymentid4,
appt.paymentid5,
p1.sign,
p1.responsibility,
p2.sign as 'p2sign',
p2.responsibility as 'p2responsibility',
p3.sign as 'p3sign',
p3.responsibility as 'p3responsibility',
p4.sign as 'p4sign',
p4.responsibility as 'p4responsibility',
p5.sign as 'p5sign',
p5.responsibility as 'p5responsibility',
stype.color as 'bcolor',
alert.color as 'acolor',
CONVERT(char(25), appt.apptdate, 101) as 'Date',
CONVERT(char(25), appt.apptdate, 100) as 'Time',
stype.code as 'Type',
' ' as 'Color',
appt.confirmed as 'C',
appt.waitlist as 'W',
LEFT(CONVERT(char(1), patient.hipaa), 1) as 'H',
(patient.lastname + ', ' + patient.firstname) as 'Name',
status.code as 'Status',
/* timer.duration as 'Duration', */
'00:00' as 'Duration',
room.code as 'Room',
provider.code as 'Provider',
location.code as 'Location',
appt.reason as 'Reason',
pcdr.code as 'Procedure',
p1.code as 'Payment',
appt.amount1 as 'Amount',
appt.check_claim1 as 'Check Claim',
appt.notes1 as 'Note',
p2.code as 'Payment2',
appt.amount2 as 'Amount2',
appt.check_claim2 as 'Check Claim2',
appt.notes2 as 'Note2',
p3.code as 'Payment3',
appt.amount3 as 'Amount3',
appt.check_claim3 as 'Check Claim3',
appt.notes3 as 'Note3',
p4.code as 'Payment4',
appt.amount4 as 'Amount4',
appt.check_claim4 as 'Check Claim4',
appt.notes4 as 'Note4',
p5.code as 'Payment5',
appt.amount5 as 'Amount5',
appt.check_claim5 as 'Check Claim5',
appt.notes5 as 'Note5',
(RTRIM(CONVERT(char(5), problem.problem)) + ' - ' + problem.description) as 'Problem',
patient.pid as 'PID',
CONVERT(char(25), appt.created, 101) as 'Created',
CONVERT(char(25), appt.updated, 101) as 'Updated',
userx.code as 'User'
FROM appt
INNER JOIN patient ON appt.patientid = patient.patientid
INNER JOIN status ON appt.statusid = status.statusid
INNER JOIN room ON appt.roomid = room.roomid
INNER JOIN problem ON appt.problemid = problem.problemid
INNER JOIN location ON appt.locationid = location.locationid
INNER JOIN provider ON appt.providerid = provider.providerid
INNER JOIN pcdr ON appt.pcdrid = pcdr.pcdrid
INNER JOIN stype ON appt.stypeid = stype.typeid
INNER JOIN userx ON appt.userid = userx.userid
INNER JOIN payment AS p1 ON appt.paymentid1 = p1.paymentid
INNER JOIN payment AS p2 ON appt.paymentid2 = p2.paymentid
INNER JOIN payment AS p3 ON appt.paymentid3 = p3.paymentid
INNER JOIN payment AS p4 ON appt.paymentid4 = p4.paymentid
INNER JOIN payment AS p5 ON appt.paymentid5 = p5.paymentid
INNER JOIN alert ON patient.alertid = alert.alertid
WHERE
(@Mode = 'FD_DAILYREGISTER' and appt.type = 'A' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and (status.statusid = -1 or status.daily_register = 1) and appt.open_ticket = 'O' and appt.apptdate <= @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL))
OR
(@Mode = 'FD_APPTREGISTER' and appt.type = 'A' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and (status.statusid = -1 or status.appt_register = 1) and appt.apptdate > @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL))
OR
(@Mode = 'FD_OPENCHARTS' and appt.type = 'A' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and (status.statusid = -1 or status.open_charts = 1) and appt.apptdate <= @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL))
OR
(@Mode = 'FD_OPENTICKETS' and (appt.type = 'A' or appt.type = 'O') and substring(stype.code,1,1) <> '*' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and /*(status.statusid <> -1 and status.open_tickets = 1)*/ status.open_tickets = 1 and appt.open_ticket = 'O' and appt.apptdate <= @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL))
OR
(@Mode = 'FD_OPENCHARGETICKETS' and (appt.type = 'A' or appt.type = 'O') and substring(stype.code,1,1) <> '*' and appt.practiceid = @PracticeID and /*(status.statusid <> -1 and status.open_tickets = 1)*/ status.open_tickets = 1 and appt.open_ticket = 'O' and appt.apptdate <= @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL))
ORDER BY appt.apptdate ASC
GO
June 14, 2003 at 11:20 am
Some comments:
1) You're bringing back a LOT of data. Compare the run times just bringing back a pkey or whatever, you'll see the difference. If you have to have it, you have to have it, but I'd look for ideas to trim some.
2) Qualify the tables with the owner - ala dbo.tablename. You'll gain a bit. Not a lot, but every bit helps.
3) Consider using either NOLOCK or setting isolation mode to read uncommitted if you can stand it.
4) Placement of the clustered index seems ok at first glance.
Andy
June 14, 2003 at 1:09 pm
Andy,
Thanks for your input.
Unfortunately I do need all the data. In your opinion could the WHERE be better constructed? Would it make a difference if I put (...) around the entire WHERE?
Phil
June 14, 2003 at 1:25 pm
Andy,
Another point.
I have tried inserting in the front of this a temp table filtering out by apptdate which then passed only the 1000 - 3000 records to the INNER JOINS but basically found that the overhead of the temp table didn't buy me anything.
Would it help if I broke it down into 4 if then else statements with the if being each of the WHERE individually?
What is the best coding practice in this example?
Again thanks,
Phil
June 14, 2003 at 3:07 pm
Typically you only add additional parens to make the logic correct. Don't know there is a best practice here. I work to solve the problem - write a query that returns the data correctly - then look to see if performance is adequate. The ugly part of query tuning is you cant really optimize bits and pieces, you have to do it holistically. In other words, you have to make a change and see what the query planner does with it.
A temp table might still work, did you index it to match what you have on the real table? A different way to approach the same thing is to replact the appt table with a subquery that does the initial limiting.
After that, do you have every join column indexed on both sides?
If you post the text of the query plan, we can look to see if something seems bad, such as a table scan.
Andy
June 15, 2003 at 3:04 pm
Try
Where Case When @Mode = 'FD_DAILYREGISTER'
Then Case When appt.type = 'A' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and (status.statusid = -1 or status.daily_register = 1) and appt.open_ticket = 'O' and appt.apptdate <= @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL)) Then '1' Else '0' End
When @Mode = 'FD_APPTREGISTER'
Then Case When appt.type = 'A' and appt.practiceid = @PracticeID and appt.locationid = @LocationID and (status.statusid = -1 or status.appt_register = 1) and appt.apptdate > @DateHolder and (appt.deleted <> 'Y' or appt.deleted IS NULL)) Then '1' Else '0' End
Else '0' End='1'
Can (appt.deleted <> 'Y' or appt.deleted IS NULL)) be replaced with IsNull(appt.deleted,'N')='N' to get rid of the OR?
June 16, 2003 at 11:33 pm
It took some time to get an accurate results from the CASE test. It took twice as long with the CASE.
Phil
June 17, 2003 at 4:40 am
Personally I would consider doing like so.
Replace all occurrances of @DateHolder with
DATEADD(ss,-1,DATEADD(d,DATEDIFF(d,0,GETDATE()+1),0))
in your query code and get rid of the variable declaration.
Replace @tdate with
GETDATE()
and get rid of variable declaration.
Not really sure you need SET DATEFORMAT so say good-bye to it.
Then try this.
CREATE PROCEDURE GetFrontDeskData;1
@PracticeID bigint,
@LocationID bigint,
@Mode nvarchar(25)
AS
SET NOCOUNT ON -- Does save some time and data.
/* Note here, I prefer to group my SPs which is what the ;n represents 1 is always first and is called by default when you call EXEC SPNAME without the ;n. You could give each their own name and if you plan to use on other screens may be easier to keep up with. */
if @Mode = 'FD_DAILYREGISTER'
exec GetFrontDeskData;2 @PracticeID, @LocationID
else if @Mode = 'FD_APPTREGISTER'
exec GetFrontDeskData;3 @PracticeID, @LocationID
else if @Mode = 'FD_OPENCHARTS'
exec GetFrontDeskData;4 @PracticeID, @LocationID
else if @Mode = 'FD_OPENTICKETS'
exec GetFrontDeskData;5 @PracticeID, @LocationID
else if @Mode = 'FD_OPENCHARGETICKETS'
exec GetFrontDeskData;6 @PracticeID /* Note I didn't carefully check all, but don't create and pass unneeded variables. Saves some minor memory overhead. */
GO
/* Note: Always format your code for best readability, saves time in the future when you need to review. */
/* This is the procdure that handles FD_DAILYREGISTER. */
CREATE PROCEDURE GetFrontDeskData;1
@PracticeID bigint,
@LocationID bigint
AS
SET NOCOUNT ON
SELECT
/* Note your columns here. */
FROM
appt
INNER JOIN
patient
INNER JOIN /* It is easier to read and understand realtionships if you do like so when related. May also have performance impact on occasion. */
alert
ON
patient.alertid = alert.alertid
ON
appt.patientid = patient.patientid
INNER JOIN
status
ON
appt.statusid = status.statusid
INNER JOIN
room
ON
appt.roomid = room.roomid
INNER JOIN
problem
ON
appt.problemid = problem.problemid
INNER JOIN
location
ON
appt.locationid = location.locationid
INNER JOIN
provider
ON
appt.providerid = provider.providerid
INNER JOIN
pcdr
ON
appt.pcdrid = pcdr.pcdrid
INNER JOIN
stype
ON
appt.stypeid = stype.typeid
INNER JOIN
userx
ON
appt.userid = userx.userid
INNER JOIN
payment AS p1
ON
appt.paymentid1 = p1.paymentid
INNER JOIN
payment AS p2
ON
appt.paymentid2 = p2.paymentid
INNER JOIN
payment AS p3
ON
appt.paymentid3 = p3.paymentid
INNER JOIN
payment AS p4
ON
appt.paymentid4 = p4.paymentid
INNER JOIN
payment AS p5
ON
appt.paymentid5 = p5.paymentid
WHERE -- The where cluase related to FD_DAILYREGISTER
appt.type = 'A' and
appt.practiceid = @PracticeID and
appt.locationid = @LocationID and
(
status.statusid = -1 or
status.daily_register = 1
) and
appt.open_ticket = 'O' and
appt.apptdate <= DATEADD(ss,-1,DATEADD(d,DATEDIFF(d,0,GETDATE()+1),0)) and
(
appt.deleted <> 'Y' or
appt.deleted IS NULL
)
GO
/* This is the procdure that handles FD_APPTREGISTER. */
CREATE PROCEDURE GetFrontDeskData;3
@PracticeID bigint,
@LocationID bigint
AS
SET NOCOUNT ON
/*Same query as before only where clause changes. In this instance based on @mode = FD_APPTREGISTER items.*/
GO
/* And so on....*/
/* This is the procdure that handles FD_OPENCHARGETICKETS. */
CREATE PROCEDURE GetFrontDeskData;6
@PracticeID bigint
/* Note @LocationID not needed in query so I don't even create parameter position for. */
AS
SET NOCOUNT ON
/*Same query as before only where clause changes. In this instance based on @mode = FD_OPENCHARGETICKETS items.*/
WHERE
appt.type IN ('A','O') and /* <<<<<<< Note here I use IN to combine OR statements, IN is more optimized than individual ORs when related as was. */
substring(stype.code,1,1) <> '*' and
appt.practiceid = @PracticeID and
status.open_tickets = 1 and
appt.open_ticket = 'O' and
appt.apptdate <= DATEADD(ss,-1,DATEADD(d,DATEDIFF(d,0,GETDATE()+1),0)) and
(
appt.deleted <> 'Y' or
appt.deleted IS NULL
)
GO
I know you stated you needed all fields. If that was so you could combine work and individual modes do not need all fields do consider dropping unneeded columns and handle on the client side dealing without them on a combined screen.
You should see a performance gain with seperate SPs (even grouped they are still considered to the engine as seperate) because each will have a more optimal query plan that can be properly utilized when calling.
June 17, 2003 at 4:42 am
Oops, keep in mind the code listed in the previous is example based with notes and not fully completed. Make sure you read the notes and explainations to build proper to your needs.
June 17, 2003 at 6:39 am
I would go with Antares approach: it's modular, maintainable, and has the best chance for a well-optimized plan based on the SARGs.
One minor thing, you could possible combine:
WHERE...
AND
(
appt.deleted <> 'Y'
OR
appt.deleted IS NULL
)
into
WHERE...
AND ISNULL(appt.deleted, 'N') <> 'Y'
Since an index on this column is not likely to be used (only 3 values? no selectivity...), you might see a performance gain from putting an expression on the left side of the SARG and getting rid of the OR expression...
Every little bit helps, right...
jay
June 17, 2003 at 6:49 am
Good additonal point Jay. Always helps when more than one person looks over code. After all this then it boils down to testing indexes and execution plans.
June 17, 2003 at 7:21 am
Thanks for all your help. I get your idea on the multiple creates. I will give it a shot and let you know. I previous tried doing it in a
IF @Mode = 'FD_DAILYREGISTER'
BEGIN
...
END
ELSE IF @MODE = 'FD_APPTREGISTER'
BEGIN
...
END
and that did not really help either.
Are the multiple creates faster than the IF ELSE IF approach?
FYI - I do need the same columns passed with each @Mode.
Again thanks for your help.
Phil
June 17, 2003 at 7:36 am
quote:
Are the multiple creates faster than the IF ELSE IF approach?
Yes, for each PROCEDURE in the the procedure group, SQL Server will compile an optimized query plan.
June 17, 2003 at 12:20 pm
Just a thought after I left this but here
quote:
AND ISNULL(appt.deleted, 'N') <> 'Y'
Are we really only talking NULL, N or Y. If so I suggest setting all to N that are NULL and changing the column to default NON-NULL. Then it would be better to do
AND appt.deleted = 'N'
equals generally outperforms != if not the use the != instead of <> as it is more with the current standard.
June 17, 2003 at 12:24 pm
I've been outdone once again. 🙂
Viewing 15 posts - 1 through 15 (of 17 total)
You must be logged in to reply to this topic. Login to reply