I am trying to optimize my query for loading should be fast because once the data is 5000 thousand then
records is coming after 2 minute of time.
I am using two sps for my search for first SP by search Parameters getting candidate details and In second SP using for counts by candidate Id so sending candidate ids to second sp from first sp result. Later from Foreach result match candidate id
Below is my First SP query
declare @FirstName varchar(500)='',
@LastName varchar(500)='',
@Phone varchar(500)='',
@Email varchar(500)='',
@Specialty varchar(500)='',
@AdditionalLicensedStates varchar(500)='AL',
@DesiredLocation varchar(50)='',
@CompactState varchar(50)=''
SET NOCOUNT ON;
declare @query nvarchar(max)=' '
set @query=@query+
'SELECT
T.CandidateId,''''Ranking,T.[Status], t.FirstName,t.LastName, t.Phone, t.[Email],t.ID AS TroveId
,t.Desired,t.Licensed,t.Specialty,t.CallBack,t.LastContact
,t.LastContactNote,0 MatchingJobs ,t.AvailableDate ,T.DepartmentId
,0 DocumentId,''''Division,''''EMRSystem,''''Application,T.Compactstate
FROM (
select T.CandidateId,T.Status, C.FirstName,C.LastName, C.Phone, C.Email,T.ID,T.DepartmentId,j.DesiredStates as Desired,
j.Licensed,j.Specialty,T.Compactstate,j.AvailableDate,cb.CallbackDate CallBack,convert(date,n.DateCreated) LastContact,
n.Notes LastContactNote from T1 T(NOLOCK)
INNER JOIN Candidate C(NOLOCK) on T.CandidateId= C.Id and T.DepartmentId=1 AND T.Profile=''0''
INNER JOIN T2 P(NOLOCK) on T.CandidateId=P.TroveCandidateId
LEFT join T3(Nolock) j on j.CandidateId=T.CandidateId
Inner join T4 js on js.jackieId=j.Id
LEFT JOIN dbo.T5(Nolock) N ON N.ObjectID = T.CandidateId and n.IsLatest=1 and n.Category=''3'' and n.NoteType=''NormalNote''
LEFT JOIN T6(Nolock) cb on cb.CandidateId=t.CandidateId and cb.IsLatest=1 and cb.CallBackType=''Trove''
where
('''+@FirstName+''' ='''' or C.FirstName like ''%'+@FirstName+'%'') AND
('''+@LastName+''' ='''' or C.LastName like ''%'+@LastName+'%'') AND
('''+@Phone+''' ='''' or C.Phone ='''+@Phone+''') AND
('''+@Email+''' ='''' or C.Email like ''%'+@Email+'%'') AND
('''+@Specialty+''' ='''' or cast(Js.specialtyId as varchar(10)) = '''+@Specialty+''') AND
('''+@AdditionalLicensedStates+''' ='''' or J.Licensed like ''%'+@AdditionalLicensedStates+'%'') AND
('''+@DesiredLocation+''' ='''' or J.DesiredStates like ''%'+@DesiredLocation+'%'') AND
('''+@CompactState+''' ='''' or t.CompactState like ''%'+@CompactState+'%'')
) as t '
-- print @query
exec (@query)
Optimize quyer or code
2
Answers
There is a lot to unpack here. Firstly, let’s start with the dangerous:
Dangerous
Your code is wide open to injection attacks. This is incredibly dangerous. You are injecting your parameters into your statement, making it insecure. Your use of
EXEC(@SQL)
syntax doesn’t help the matter as it makes it impossible to parametrise your statement. Usesys.sp_executesql
to execute your statement and parametrise it.Deprecations
You are using
FROM <TableName> (<Hint>)
syntax, which is deprecated. It should beFROM <TableName> WITH (<Hint>)
, however, speaking of your table hints…(Ab)use of
NOLOCK
You’re (ab)using the
NOLOCK
hint here. Aaron Bertrand covers this subject in depth in Bad habits : Putting NOLOCK everywhere, however, you likely don’t need any of those hints. If you "must" (which I strongly doubt) use it change the isolation level, don’t spam the hint against every table.Non-SARGable clauses
Almost all your clauses aren’t SARGable. Queries with a leading wildcard can’t use indexes and almost all of your clauses do this. One of the only that doesn’t, against
specialtyId
CAST
s that column to avarchar
making it also non-SARGable. If you didn’t inject your value, perhaps you could parametrise it with a valid data type.Everything but the kitchen-sink
What you have here is known as a "Catch-all" or "Kitchen Sink" query, however, you don’t handle the problem well. You still check the value of every variable but if you’re using dynamic SQL then you should only pass the variables that have a non-
NULL
value (you are usingNULL
rather than''
for "no value, right?). As you’re using a dynamic statement, I continue down that route.Implicit INNER JOIN
Your
LEFT JOIN
toJackie
was an implicitINNER JOIN
as its columnid
had to have a non-NULL
value in theINNER JOIN
tojackieSpecialty
. Don’t useLEFT JOIN
s for tables that must have a value returned from them.Formatting
The formatting needed a lot of work too. Syntax like
''''Application
is very confusing (remember that’s in a dynamic statement). UseAS
or=
syntax such as'''' AS Application
orApplication = ''''
. Not to mention many of your statments were on a single line and are hard to read.Incorrect Alias
Your derived table was alaised as
tl
but your outerSELECT
referencest
.Solution
This is untested apart from to check that it creates a valid statement, but for some of your parameters should be faster (
@Specialty
and@Phone
). For others, it might be, as I make the query only pass the clauses for the variables that have a non-NULL
value (noteNULL
, not''
). As mentioned, leading wildcards aren’t SARGable, so if you need better performance, you should reconsider if you should really be using them here.This can’t be tested against your table, and the data types for variables are guessed. Use your best friend to help debug this if you encounter errors (I’ve done too much here in my opinion):
All the points made by Dale K are valid and important. I’m going to add that SQL in string literals is an unmaintainable aberration, whether you do it in C#, or in SQL as here.
You’re doing it so you can construct a where clause at runtime based on the parameters actually supplied. There is no need for this. Just use…
If the parameter is not supplied, all rows will match, everything will be returned. You will usually want to use this with NOCACHE, because each permutation of parameters requires a different query plan.
Absolutely the best way of avoiding string literals is QueryFirst. Your sql lives in .sql files in your app, syntax validated as you type. All the ADO stuff is generated for you, including real parameters, and creating SQL injection vulnerabilities becomes virtually impossible (and pointless, because the right way is easier).
Then, I’ve been staring at this for some time, and I can’t see for the life of me why you need an inner query. The WHERE clause appears to be on the inner query, thank goodness, but there’s absolutely no need I can see to have the two levels. SQL works best when you can specify what you want in "one hit", then leave the optimiser do its work.
If queries are easier to write, you can write more, smaller, more focussed queries. This query is not too scary but its starting to get up there: 7 tables, 8 parameters, 20 odd columns. If you pay attention to indexes, 5000 rows should take less than 10 seconds, guessing wildly, depends on your hardware. But who has the time to read through 5000 records? Should this be split into a small number of smaller queries?