skip to Main Content

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


  1. 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. Use sys.sp_executesql to execute your statement and parametrise it.

    Deprecations

    You are using FROM <TableName> (<Hint>) syntax, which is deprecated. It should be FROM <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 CASTs that column to a varchar 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 using NULL rather than '' for "no value, right?). As you’re using a dynamic statement, I continue down that route.

    Implicit INNER JOIN

    Your LEFT JOIN to Jackie was an implicit INNER JOIN as its column id had to have a non-NULL value in the INNER JOIN to jackieSpecialty. Don’t use LEFT JOINs 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). Use AS or = syntax such as '''' AS Application or Application = ''''. 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 outer SELECT references t.

    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 (note NULL, 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 following datatypes are GUESSED
    DECLARE @FirstName nvarchar(50),
            @LastName nvarchar(50),
            @Phone varchar(15),
            @Email varchar(100),
            @Specialty int = 5,
            @AdditionalLicensedStates varchar(10),
            @DesiredLocation varchar(50),
            @CompactState varchar(5);
    
    DECLARE @query nvarchar(MAX),
            @CRLF nchar(2) = NCHAR(13) + NCHAR(10);
    SET @query= N'SELECT t.CandidateId,' + @CRLF +
                N'       '''' AS Ranking,' + @CRLF +
                N'       t.[Status],' + @CRLF +
                N'       t.FirstName,' + @CRLF +
                N'       t.LastName,' + @CRLF +
                N'       t.Phone,' + @CRLF +
                N'       t.[Email],' + @CRLF +
                N'       t.ID AS TroveId,' + @CRLF + 
                N'       t.Desired,' + @CRLF +
                N'       t.Licensed,' + @CRLF +
                N'       t.Specialty,' + @CRLF +
                N'       t.CallBack,' + @CRLF +
                N'       t.LastContact,' + @CRLF +
                N'       t.LastContactNote,' + @CRLF +
                N'       0 AS MatchingJobs,' + @CRLF +
                N'       t.AvailableDate,' + @CRLF +
                N'       t.DepartmentId,' + @CRLF +
                N'       0 AS  DocumentId,' + @CRLF +
                N'       '''' AS Division,' + @CRLF +
                N'       '''' AS EMRSystem,' + @CRLF +
                N'       '''' AS Application,' + @CRLF +
                N'       t.Compactstate ' + @CRLF +
                N'FROM (SELECT T.CandidateId,' + @CRLF +
                N'             T.Status,' + @CRLF +
                N'             C.FirstName,' + @CRLF +
                N'             C.LastName,' + @CRLF +
                N'             C.Phone,' + @CRLF +
                N'             C.Email,' + @CRLF +
                N'             T.ID,' + @CRLF +
                N'             T.DepartmentId,' + @CRLF +
                N'             j.DesiredStates AS Desired,' + @CRLF +
                N'             j.Licensed,' + @CRLF +
                N'             j.Specialty,' + @CRLF +
                N'             T.Compactstate,' + @CRLF +
                N'             j.AvailableDate,' + @CRLF +
                N'             cb.CallbackDate AS CallBack,' + @CRLF +
                N'             CONVERT(date,n.DateCreated) AS LastContact,' + @CRLF +
                N'             n.Notes AS LastContactNote' + @CRLF +
                N'      FROM dbo.TreasureTrove T' + @CRLF +
                N'           INNER JOIN dbo.Candidate C ON T.CandidateId = C.Id' + @CRLF +
                N'                                     AND T.DepartmentId = 1' + @CRLF +
                N'                                     AND T.Profile = ''0''' + @CRLF + --If this is 0, why not compare to a literal int?
                N'           INNER JOIN dbo.ProfileLinked P ON T.CandidateId = P.TroveCandidateId' + @CRLF +
                N'           INNER JOIN dbo.Jackie j ON j.CandidateId = T.CandidateId' + @CRLF + -- As j.Id must have a non-NULL value, changed to an INNER JOIN
                N'           INNER JOIN dbo.jackieSpecialty js ON js.jackieId = j.Id' + @CRLF +
                N'           LEFT JOIN dbo.Notes N ON N.ObjectID = T.CandidateId' + @CRLF +
                N'                                AND n.IsLatest = 1 ' + @CRLF +
                N'                                AND n.Category = ''3''' + @CRLF + --If this is a 3, why not compare to a literal int?
                N'                                AND n.NoteType = ''NormalNote''' + @CRLF +
                N'           LEFT JOIN dbo.CallBack cb ON cb.CandidateId = t.CandidateId' + @CRLF +
                N'                                     AND cb.IsLatest = 1' + @CRLF +
                N'                                     AND cb.CallBackType = ''Trove''' + @CRLF +
                --I'm suggest you pass NULL for your parameters, not '' when you don't need the, I assume they are NULL below
                NULLIF(N'WHERE ' + CONCAT_WS(@CRLF + N'  AND ',CASE WHEN @FirstName IS NOT NULL THEN N'C.FirstName LIKE ''%''+@FirstName+''%''' END,
                                                               CASE WHEN @LastName IS NOT NULL THEN N'C.LastName LIKE ''%''+@LastName+''%''' END,
                                                               CASE WHEN @Phone IS NOT NULL THEN N'C.Phone = @Phone' END,
                                                               CASE WHEN @Email IS NOT NULL THEN N'C.Email like ''%''+@Email+''%''' END,
                                                               CASE WHEN @Specialty IS NOT NULL THEN N'Js.specialtyId = @Specialty' END, --Don't CAST/CONVERT your columns. I guess the data type here later.
                                                               CASE WHEN @AdditionalLicensedStates IS NOT NULL THEN N'J.Licensed LIKE ''%''+@AdditionalLicensedStates+''%''' END,
                                                               CASE WHEN @DesiredLocation IS NOT NULL THEN N'J.DesiredStates LIKE ''%''+@DesiredLocation+''%''' END,
                                                               CASE WHEN @CompactState IS NOT NULL THEN N't.CompactState LIKE ''%''+@CompactState+''%''' END),N'WHERE ') + N') as t';    
    PRINT @query;-- YOur best friend
    
    EXEC sys.sp_executesql @query,
                           N'@FirstName nvarchar(50),@LastName nvarchar(50),@Phone varchar(15),@Email varchar(100),@Specialty int = 5,@AdditionalLicensedStates varchar(10),@DesiredLocation varchar(50),@CompactState varchar(5)', --Remember, these are all guessed
                           @FirstName,
                           @LastName,
                           @Phone,
                           @Email,
                           @Specialty,
                           @AdditionalLicensedStates,
                           @DesiredLocation,
                           @CompactState;
    
    Login or Signup to reply.
  2. 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…

    @FirstName is null or C.FirstName LIKE @Firstname + '%'
    

    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?

    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search