July 3, 2014 at 10:27 pm
So I've been charged with rewriting a very horribly written Stored procedure which is being used to search the database (basically across every table).
Over time, it gets slower and slower (up to 2 min). If I run the below every now and then it seems to improve speed (as quick as 10sec) but if I leave this in the stored proc it seems to get slower a lot quicker than if I leave it out and just run it on the odd ocation:
SET ARITHABORT ON;
SET ansi_warnings OFF
The database has been terribly designed, and I've tried to improve it over time since taking over, but I don't have time to do a full over hall. I've added indexes and stats that have been recommended by the sql tools which has helped a bit, but not a lot. (Foreign Keys are very rarely used, I've only been added when I get time).
Any Help, pointers, suggestions etc people could make on the below will be greatly appreciated.
Currently the SP uses an IF/ELSEIF to brake it into 4 different possible queries so that different parameters can be used. (total of 24 parameters). about 80%+ of the time the else part of the query is used which has all 24 parameters used in the Where clause.
I was thinking of rewriting the whole thing so that I generate a string based on what parameters are passes so that not all the parameters are used in the final query and then exec the string. (is this a good idea in general?)
basically This query will return all rows required if the user wanted to see all records (20,000 at the moment):
SELECT *
FROM dbo.sgleComplaint AS C
INNER JOIN dbo.tblModUsers AS mu ON mu.modIDNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmail AS e ON e.IdentificationNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS NextToAction ON NextToAction.fullUserID = e.NextToAction
INNER JOIN dbo.tblBIR AS B ON C.IdentificationNo = B.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS RE ON RE.eID = B.responsibleEmployee
LEFT OUTER JOIN dbo.tblSites AS s ON s.siteID = B.site
WHERE (e.eid IS null OR e.eid = (SELECT MAX(eID) AS mEID
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))
)
I have tired moving where as a table in the From and it seems to run as efficiently both ways, so not sure which is better long term.
Then the parameter where clause
WHERE (C.csType = @cstype or @csType='-1')AND (mu.status = @status OR @status = '0')
AND (C.ComplaintType = @ComplaintType OR @ComplaintType = '')
AND (C.IdentificationNo >= @startID OR @startID='')
AND (C.IdentificationNo <= @endID OR @endID='')
AND (C.ComplaintDate >= @LastUpdatedDateStart OR @LastUpdatedDateStart is null)
AND (C.ComplaintDate <= @LastUpdatedDateEnd+1 OR @LastUpdatedDateEnd is null)
AND (MU.modDate >= @ComplaintDateStart OR @ComplaintDateStart is null)
AND (MU.modDate <= @ComplaintDateEnd+1 OR @ComplaintDateEnd is null)
AND (mu.plannedCompDate >= @plannedCompDateStart OR @plannedCompDateStart is null)
AND (mu.plannedCompDate <= @plannedCompDateEnd OR @plannedCompDateEnd is null)
AND (B.subject like '%' + @subject + '%' OR @subject = '')
AND (C.BUInitiatingComplaint = @BUName OR @BUName = '')
AND (S.SiteID = @siteID OR @siteID = '')
AND (ea.userName = @Responsible OR @Responsible = '')
AND (e.NextToAction = @NextToAction OR @NextToAction = '')
AND (MU.modUser = @Initator OR @Initator = '')
AND (C.CustomerRef like '%' + @Reference + '%'OR @Reference = '')
AND (C.ProjectJobNo = @ProjectJobNo OR @ProjectJobNo = '')
AND (B.priority = @priority OR @priority = '')
AND (B.company like '%' + @company + '%' or @company = '')
AND (@General = '""' OR FREETEXT(B.History, @General) )
AND (S.SiteID = MyValue OR @region = '')
Thanks in advance for any suggestions anyone can make.
July 3, 2014 at 10:30 pm
The below is the current SP if that helps anyone help me.
CREATE PROCEDURE [dbo].[uspJobSearch_TEST]
@csType as varchar(20),
@status as varchar(20),
@startID as varchar(50),
@endID as varchar(50),
@complaintType as varchar(50),
@Subject as varchar(100),
@ComplaintDateStart as dateTime,
@ComplaintDateEnd as dateTime,
@plannedCompDateStart as dateTime,
@plannedCompDateEnd as dateTime,
@Responsible as varchar(50),
@Reference as varchar(50),
@ProjectJobNo as varchar(50),
@priority as varchar(50),
@NextToAction as varchar(50),
@Initator as varchar(50),
@siteID as int,
@BUName as varchar(50),
@userid as varchar(50),
@General as VARCHAR(200),
@company as varchar(200),
@LastUpdatedDateStart as dateTime,
@LastUpdatedDateEnd as dateTime,
@region as varchar(200)
AS
BEGIN
SET NOCOUNT ON;
--SET ARITHABORT ON;
--SET ansi_warnings OFF
IF ISNULL(@General,'') = '' SET @General = '""';
IF @Region<> ''
BEGIN
WITH DirectReports (RegionID, RegionName, Level)
AS
(
-- Anchor member definition
SELECT RegionID, RegionName, 0 AS Level
FROM dbo.tblRegion AS r
WHERE regionID =@region
UNION ALL
-- Recursive member definition
SELECT r.RegionID, r.RegionName, Level + 1
FROM dbo.tblRegion AS r
INNER JOIN DirectReports AS d
ON r.parentID = d.RegionID
)
SELECT @region = COALESCE(@region+',' ,'') + CAST(s.siteID AS VARCHAR(10))
FROM DirectReports LEFT OUTER JOIN dbo.tblSiteRegion AS s ON DirectReports.RegionID = s.regionID
ORDER BY SiteID
END
--SELECT @region
DECLARE @SQL AS VARCHAR(MAX)
SET @SQL = @SQL + ''
EXEC @SQL
IF @status='MyTasks'
SELECT TOP (100) PERCENT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status, d.domain,
ea.userID, ea.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate,
B.priority, e.eID, e.sDate, e.NextAction, userName.userName AS NextToAction, s.siteName,
(SELECT COUNT(*) AS Expr1
FROM dbo.tblAttachment
WHERE (IdentificationNo = C.IdentificationNo)) AS AttachmentCount
FROM dbo.tblModUsers AS mu RIGHT OUTER JOIN
(SELECT d.domain + '\' + ea.userID AS userID, ea.userName
FROM dbo.tblDomain AS d INNER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain) AS userName RIGHT OUTER JOIN
dbo.tblEmail AS e ON userName.userID = e.NextToAction RIGHT OUTER JOIN
dbo.sgleComplaint AS C ON e.IdentificationNo = C.IdentificationNo ON mu.modIDNo = C.IdentificationNo LEFT OUTER JOIN
dbo.tblDomain AS d RIGHT OUTER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain RIGHT OUTER JOIN
dbo.tblSites AS s RIGHT OUTER JOIN
dbo.tblBIR AS B ON s.siteID = B.site ON ea.eID = B.responsibleEmployee ON C.IdentificationNo = B.IdentificationNo
WHERE (C.csType = @cstype or @csType='-1')AND NOT (mu.status = 'Closed' OR mu.status= 'Canceled') AND ((e.eID =
(SELECT MAX(eID) AS Expr1
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))) OR e.eID is NULL)
AND ( (d.domain + '\' + ea.userID = @userid)
OR (e.NextToAction = @userid)
OR (MU.modUser = @userid))
ORDER BY C.IdentificationNo
ELSE IF @status = 'myOverdue'
SELECT TOP (100) PERCENT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status, d.domain,
ea.userID, ea.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate,
B.priority, e.eID, e.sDate, e.NextAction, userName.userName AS NextToAction, s.siteName,
(SELECT COUNT(*) AS Expr1
FROM dbo.tblAttachment
WHERE (IdentificationNo = C.IdentificationNo)) AS AttachmentCount
FROM dbo.tblModUsers AS mu RIGHT OUTER JOIN
(SELECT d.domain + '\' + ea.userID AS userID, ea.userName
FROM dbo.tblDomain AS d INNER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain) AS userName RIGHT OUTER JOIN
dbo.tblEmail AS e ON userName.userID = e.NextToAction RIGHT OUTER JOIN
dbo.sgleComplaint AS C ON e.IdentificationNo = C.IdentificationNo ON mu.modIDNo = C.IdentificationNo LEFT OUTER JOIN
dbo.tblDomain AS d RIGHT OUTER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain RIGHT OUTER JOIN
dbo.tblSites AS s RIGHT OUTER JOIN
dbo.tblBIR AS B ON s.siteID = B.site ON ea.eID = B.responsibleEmployee ON C.IdentificationNo = B.IdentificationNo
WHERE (C.csType = @cstype or @csType='-1')AND NOT (mu.status = 'Closed' OR mu.status= 'Canceled') AND ((e.eID =
(SELECT MAX(eID) AS Expr1
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))) OR e.eID is NULL)
AND ( (d.domain + '\' + ea.userID = @userid)
OR (e.NextToAction = @userid)
OR (MU.modUser = @userid))
AND (mu.plannedCompDate <= getDate())
ORDER BY C.IdentificationNo
ELSE IF @status = 'allOverdue'
SELECT TOP (100) PERCENT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status, d.domain,
ea.userID, ea.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate,
B.priority, e.eID, e.sDate, e.NextAction, userName.userName AS NextToAction, s.siteName,
(SELECT COUNT(*) AS Expr1
FROM dbo.tblAttachment
WHERE (IdentificationNo = C.IdentificationNo)) AS AttachmentCount
FROM dbo.tblModUsers AS mu RIGHT OUTER JOIN
(SELECT d.domain + '\' + ea.userID AS userID, ea.userName
FROM dbo.tblDomain AS d INNER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain) AS userName RIGHT OUTER JOIN
dbo.tblEmail AS e ON userName.userID = e.NextToAction RIGHT OUTER JOIN
dbo.sgleComplaint AS C ON e.IdentificationNo = C.IdentificationNo ON mu.modIDNo = C.IdentificationNo LEFT OUTER JOIN
dbo.tblDomain AS d RIGHT OUTER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain RIGHT OUTER JOIN
dbo.tblSites AS s RIGHT OUTER JOIN
dbo.tblBIR AS B ON s.siteID = B.site ON ea.eID = B.responsibleEmployee ON C.IdentificationNo = B.IdentificationNo
WHERE (C.csType = @cstype or @csType='-1')AND NOT (mu.status = 'Closed' OR mu.status= 'Canceled') AND ((e.eID =
(SELECT MAX(eID) AS Expr1
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))) OR e.eID is NULL)
AND (mu.plannedCompDate <= getDate())
ORDER BY C.IdentificationNo
ELSE IF @status = '-1'
SELECT TOP (100) PERCENT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status, d.domain, ea.userID,
ea.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate, B.priority, e.eID, e.sDate,
e.NextAction, userName.userName AS NextToAction, s.siteName, CASE WHEN AC.MyCount IS NULL THEN 0 ELSE AC.MyCount END AS AttachmentCount
FROM dbo.sgleComplaint AS C LEFT OUTER JOIN
(SELECT MAX(eID) AS eID, IdentificationNo
FROM dbo.tblEmail
GROUP BY IdentificationNo) AS myAction INNER JOIN
dbo.tblEmail AS e ON myAction.eID = e.eID LEFT OUTER JOIN
(SELECT d.domain + '\' + ea.userID AS userID, ea.userName
FROM dbo.tblDomain AS d INNER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain) AS userName ON e.NextToAction = userName.userID ON
myAction.IdentificationNo = C.IdentificationNo LEFT OUTER JOIN
dbo.tblModUsers AS mu ON C.IdentificationNo = mu.modIDNo LEFT OUTER JOIN
dbo.tblDomain AS d RIGHT OUTER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain RIGHT OUTER JOIN
dbo.tblSites AS s RIGHT OUTER JOIN
dbo.tblBIR AS B ON s.siteID = B.site ON ea.eID = B.responsibleEmployee ON C.IdentificationNo = B.IdentificationNo LEFT OUTER JOIN
(SELECT COUNT(*) AS MyCount, IdentificationNo
FROM dbo.tblAttachment
GROUP BY IdentificationNo) AS AC ON AC.IdentificationNo = C.IdentificationNo
LEFT OUTER JOIN (select cast(value as int) as MyValue from dbo.ParmsToList(@region )) AS PL ON S.SiteID = PL.MyValue
WHERE (C.csType = @cstype or @csType='-1')AND NOT (mu.status = 'Closed' OR mu.status= 'Canceled')
AND (C.ComplaintType = @ComplaintType OR @ComplaintType = '')
AND (C.IdentificationNo >= @startID OR @startID='')
AND (C.IdentificationNo <= @endID OR @endID='')
AND (C.ComplaintDate >= @LastUpdatedDateStart OR @LastUpdatedDateStart is null)
AND (C.ComplaintDate <= @LastUpdatedDateEnd+1 OR @LastUpdatedDateEnd is null)
AND (MU.modDate >= @ComplaintDateStart OR @ComplaintDateStart is null)
AND (MU.modDate <= @ComplaintDateEnd+1 OR @ComplaintDateEnd is null)
AND (mu.plannedCompDate >= @plannedCompDateStart OR @plannedCompDateStart is null)
AND (mu.plannedCompDate <= @plannedCompDateEnd OR @plannedCompDateEnd is null)
AND (B.subject like '%' + @subject + '%' OR @subject = '')
AND (C.BUInitiatingComplaint = @BUName OR @BUName = '')
AND (S.SiteID = @siteID OR @siteID = '')
AND (ea.userName = @Responsible OR @Responsible = '')
AND (e.NextToAction = @NextToAction OR @NextToAction = '')
AND (MU.modUser = @Initator OR @Initator = '')
AND (C.CustomerRef like '%' + @Reference + '%'OR @Reference = '')
AND (C.ProjectJobNo = @ProjectJobNo OR @ProjectJobNo = '')
AND (B.priority = @priority OR @priority = '')
AND (B.company like '%' + @company + '%' or @company = '')
AND (B.History like '%'+@General + '%' or @General = '""')
AND (S.SiteID = MyValue OR @region = '')--IN ( select cast(value as int) from dbo.ParmsToList(@region )) or @region = '')
ORDER BY C.IdentificationNo
ElSE
SELECT TOP (100) PERCENT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status, d.domain, ea.userID,
ea.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate, B.priority, e.eID, e.sDate,
e.NextAction, userName.userName AS NextToAction, s.siteName, CASE WHEN AC.MyCount IS NULL THEN 0 ELSE AC.MyCount END AS AttachmentCount
FROM dbo.sgleComplaint AS C LEFT OUTER JOIN
(SELECT MAX(eID) AS eID, IdentificationNo
FROM dbo.tblEmail
GROUP BY IdentificationNo) AS myAction INNER JOIN
dbo.tblEmail AS e ON myAction.eID = e.eID LEFT OUTER JOIN
(SELECT d.domain + '\' + ea.userID AS userID, ea.userName
FROM dbo.tblDomain AS d INNER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain) AS userName ON e.NextToAction = userName.userID ON
myAction.IdentificationNo = C.IdentificationNo LEFT OUTER JOIN
dbo.tblModUsers AS mu ON C.IdentificationNo = mu.modIDNo LEFT OUTER JOIN
dbo.tblDomain AS d RIGHT OUTER JOIN
dbo.tblEmailAddress AS ea ON d.domainID = ea.domain RIGHT OUTER JOIN
dbo.tblSites AS s RIGHT OUTER JOIN
dbo.tblBIR AS B ON s.siteID = B.site ON ea.eID = B.responsibleEmployee ON C.IdentificationNo = B.IdentificationNo LEFT OUTER JOIN
(SELECT COUNT(*) AS MyCount, IdentificationNo
FROM dbo.tblAttachment
GROUP BY IdentificationNo) AS AC ON AC.IdentificationNo = C.IdentificationNo
--LEFT JOIN CONTAINSTABLE(dbo.tblbir,history, @General) AS bf ON b.IdentificationNo = bf.
LEFT OUTER JOIN (select cast(value as int) as MyValue from dbo.ParmsToList(@region )) AS PL ON S.SiteID = PL.MyValue
WHERE (C.csType = @cstype or @csType='-1')AND (mu.status = @status OR @status = '0')
AND (C.ComplaintType = @ComplaintType OR @ComplaintType = '')
AND (C.IdentificationNo >= @startID OR @startID='')
AND (C.IdentificationNo <= @endID OR @endID='')
AND (C.ComplaintDate >= @LastUpdatedDateStart OR @LastUpdatedDateStart is null)
AND (C.ComplaintDate <= @LastUpdatedDateEnd+1 OR @LastUpdatedDateEnd is null)
AND (MU.modDate >= @ComplaintDateStart OR @ComplaintDateStart is null)
AND (MU.modDate <= @ComplaintDateEnd+1 OR @ComplaintDateEnd is null)
AND (mu.plannedCompDate >= @plannedCompDateStart OR @plannedCompDateStart is null)
AND (mu.plannedCompDate <= @plannedCompDateEnd OR @plannedCompDateEnd is null)
AND (B.subject like '%' + @subject + '%' OR @subject = '')
AND (C.BUInitiatingComplaint = @BUName OR @BUName = '')
AND (S.SiteID = @siteID OR @siteID = '')
AND (ea.userName = @Responsible OR @Responsible = '')
AND (e.NextToAction = @NextToAction OR @NextToAction = '')
AND (MU.modUser = @Initator OR @Initator = '')
AND (C.CustomerRef like '%' + @Reference + '%'OR @Reference = '')
AND (C.ProjectJobNo = @ProjectJobNo OR @ProjectJobNo = '')
AND (B.priority = @priority OR @priority = '')
AND (B.company like '%' + @company + '%' or @company = '')
AND (@General = '""' OR FREETEXT(B.History, @General) )--)
--AND bf. is not NULL
AND (S.SiteID = MyValue OR @region = '')
ORDER BY C.IdentificationNo
END
July 4, 2014 at 2:32 am
Oh boy. Where to start with this one.
First off, the use of multiple right joins in the queries in the stored procedure suggest that the queries were generated using a pointy-clicky thing rather than being designed and written. Many of those where-clause filters will convert outer joins into inner joins. In short, this is coding by accident. The end result has been obtained by trial and error.
Second, SQL Server cannot generate a plan which will work effectively with all of your many use cases; the optimal plan for returning a handful of rows is unlikely to bear any resemblance to the optimal plan for returning all possible rows. There are various methods to get around this problem - this is a catch-all query[/url], by the way - and the easiest to implement during development is to recompile at the statement level.
I recommend you rewrite this from scratch. Don't be put off by the amount of code in there, quite a lot of it is repeated - as you've already stated. Also, you have a standard to code against. Give yourself a day or so to work through the query which is most often chosen and rewrite it by hand. Avoid using right outer joins, most of us find it difficult to analyse a query using them without looking at the code in a mirror whilst reciting the Lord's Prayer backwards. Check your results regularly against your known - working - standard. Good luck.
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
July 4, 2014 at 4:10 am
Hi Chris,
Thanks for the feedback.
So today I figured I'd start by building a whole new query which uses a string building method.
I've only tested 2 of the most basic scenarios so far and already it appears to run about 5-10 times faster.
Any Comments ? Thanks Again.
Regards
Byron
ALTER PROCEDURE [dbo].[uspJobSearch_TEST]
@csType as varchar(20),
@status as varchar(20),
@startID as varchar(50),
@endID as varchar(50),
@complaintType as varchar(50),
@Subject as varchar(100),
@ComplaintDateStart as dateTime,
@ComplaintDateEnd as dateTime,
@plannedCompDateStart as dateTime,
@plannedCompDateEnd as dateTime,
@Responsible as varchar(50),
@Reference as varchar(50),
@ProjectJobNo as varchar(50),
@priority as varchar(50),
@NextToAction as varchar(50),
@Initator as varchar(50),
@siteID as int,
@BUName as varchar(50),
@userid as varchar(50),
@General as VARCHAR(200),
@company as varchar(200),
@LastUpdatedDateStart as dateTime,
@LastUpdatedDateEnd as dateTime,
@region as varchar(200)
AS
BEGIN
SET NOCOUNT ON;
--SET ARITHABORT ON;
--SET ansi_warnings OFF
IF ISNULL(@General,'') = '' SET @General = '""';
IF @Region<> ''
BEGIN
WITH DirectReports (RegionID, RegionName, Level)
AS
(
-- Anchor member definition
SELECT RegionID, RegionName, 0 AS Level
FROM dbo.tblRegion AS r
WHERE regionID =@region
UNION ALL
-- Recursive member definition
SELECT r.RegionID, r.RegionName, Level + 1
FROM dbo.tblRegion AS r
INNER JOIN DirectReports AS d
ON r.parentID = d.RegionID
)
SELECT @region = COALESCE(@region+',' ,'') + CAST(s.siteID AS VARCHAR(10))
FROM DirectReports LEFT OUTER JOIN dbo.tblSiteRegion AS s ON DirectReports.RegionID = s.regionID
ORDER BY SiteID
END
--SELECT @region
DECLARE @SQL AS VARCHAR(MAX)
SET @SQL = 'SELECT *,CASE WHEN AC.MyCount IS NULL THEN 0 ELSE AC.MyCount END AS AttachmentCount
FROM dbo.sgleComplaint AS C
INNER JOIN dbo.tblModUsers AS mu ON mu.modIDNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmail AS e ON e.IdentificationNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS NextToAction ON NextToAction.fullUserID = e.NextToAction
INNER JOIN dbo.tblBIR AS B ON C.IdentificationNo = B.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS RE ON RE.eID = B.responsibleEmployee
LEFT OUTER JOIN dbo.tblSites AS s ON s.siteID = B.site
LEFT OUTER JOIN (select cast(value as int) as MyValue from dbo.ParmsToList('''+@region +''')) AS PL ON S.SiteID = PL.MyValue
LEFT OUTER JOIN (SELECT COUNT(*) AS MyCount, IdentificationNo
FROM dbo.tblAttachment
GROUP BY IdentificationNo) AS AC ON AC.IdentificationNo = C.IdentificationNo
WHERE (e.eid IS null OR e.eid = (SELECT MAX(eID) AS mEID
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))
)'
IF @csType<>'-1' SET @SQL = @SQL + ' AND C.csType =''' + @cstype + ''''
IF @status='MyTasks'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND ( ( RE.fullUserID = ''' + @userid + ''')'
SET @SQL = @SQL + 'OR (e.NextToAction =''' + @userid+ ''')'
SET @SQL = @SQL + 'OR (MU.modUser = ''' + @userid+ '''))'
END
ELSE IF @status = 'myOverdue'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND ( ( RE.fullUserID = ''' + @userid + ''')'
SET @SQL = @SQL + 'OR (e.NextToAction =''' + @userid+ ''')'
SET @SQL = @SQL + 'OR (MU.modUser = ''' + @userid+ '''))'
SET @SQL = @SQL + 'AND (mu.plannedCompDate <= ''' + convert(varchar(10),getdate(),102) + ''')'
END
ELSE IF @status = 'allOverdue'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND (mu.plannedCompDate <= ''' + convert(varchar(10),getdate(),102) + ''')'
END
ELSE IF @status='-1'
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
ELSE
IF @status <> '0' SET @SQL = @SQL + ' AND mu.status = ''' + @status + ''''
IF @ComplaintType <> '' SET @SQL = @SQL + ' AND C.ComplaintType = ''' + @ComplaintType + ''''
IF NOT(@startID='') SET @SQL = @SQL + ' AND C.IdentificationNo >= ' + @startID
IF NOT(@endID='') SET @SQL = @SQL + ' AND C.IdentificationNo <= ' + @endID
IF NOT(@LastUpdatedDateStart is null) SET @SQL = @SQL + ' AND C.ComplaintDate >= ''' + convert(varchar(10),@LastUpdatedDateStart,102) + ''' '
IF NOT(@LastUpdatedDateEnd is null) SET @SQL = @SQL + ' AND C.ComplaintDate <= ''' + convert(varchar(10),@LastUpdatedDateEnd+1,102) + ''' '
IF NOT(@ComplaintDateStart is null) SET @SQL = @SQL + ' AND MU.modDate >= ''' + convert(varchar(10),@ComplaintDateStart,102) + ''' '
IF NOT(@ComplaintDateEnd is null) SET @SQL = @SQL + ' AND MU.modDate <= ''' + convert(varchar(10),@ComplaintDateEnd+1 ,102) + ''' '
IF NOT(@plannedCompDateStart is null) SET @SQL = @SQL + ' AND mu.plannedCompDate >= ''' + convert(varchar(10), @plannedCompDateStart,102) + ''' '
IF NOT(@plannedCompDateEnd is null) SET @SQL = @SQL + ' AND mu.plannedCompDate <= ''' + convert(varchar(10),@plannedCompDateEnd+1 ,102) + ''' '
IF @subject <> '' SET @SQL = @SQL + ' AND B.subject like ''%' + @subject + '%'''
IF @BUName <> '' SET @SQL = @SQL + ' AND C.BUInitiatingComplaint = ''' + @BUName + ''''
IF @siteID <> '' SET @SQL = @SQL + ' AND S.SiteID = ' + CAST(@siteID AS VARCHAR)
IF @Responsible <> '' SET @SQL = @SQL + ' AND RE.userName = ''' + @Responsible + ''''
IF @NextToAction <> '' SET @SQL = @SQL + ' AND e.NextToAction= ''' + @NextToAction + ''''
IF @Initator <> '' SET @SQL = @SQL + ' AND MU.modUser = ''' + @Initator + ''''
IF @Reference <> '' SET @SQL = @SQL + ' AND C.CustomerRef like ''%' + @Reference + '%'''
IF @ProjectJobNo <> '' SET @SQL = @SQL + ' AND C.ProjectJobNo = ''' + @ProjectJobNo + ''''
IF @priority <> '' SET @SQL = @SQL + ' AND B.priority = ''' + @priority + ''''
IF @company <> '' SET @SQL = @SQL + ' AND B.company like ''%' + @company + '%'''
IF @General <> '""' SET @SQL = @SQL + ' AND FREETEXT(B.History, '''+ @General + ''')'
IF @region <> '' SET @SQL = @SQL + ' AND S.SiteID = MyValue'
--SELECT @SQL
EXEC (@SQL )
END
July 4, 2014 at 4:19 am
Oh also, thanks for that link to the Catch-all post. That was highly informative.
July 4, 2014 at 4:56 am
Byzza (7/4/2014)
Any Comments ?
You have security vulnerabilities in that. Specifically SQL Injection. DO NOT concatenate user input into a string which will be executed. It is perfectly possible to do that dynamic search query in a secure way. See my blog post which Chris was nice enough to post.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
July 4, 2014 at 5:29 am
GilaMonster (7/4/2014)
Byzza (7/4/2014)
Any Comments ?You have security vulnerabilities in that. Specifically SQL Injection. DO NOT concatenate user input into a string which will be executed. It is perfectly possible to do that dynamic search query in a secure way. See my blog post which Chris was nice enough to post.
Thanks for the catch, Gail. I hadn't got as far as the dynamic sql.
Back to the OP.
Here’s your query extracted from the dynamic sql block with one or two formatting changes:
SELECT
*,
ISNULL(AC.MyCount,0) AS AttachmentCount
FROM dbo.sgleComplaint AS C
INNER JOIN dbo.tblModUsers AS mu
ON mu.modIDNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmail AS e
ON e.IdentificationNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS NextToAction
ON NextToAction.fullUserID = e.NextToAction
INNER JOIN dbo.tblBIR AS B
ON C.IdentificationNo = B.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS RE
ON RE.eID = B.responsibleEmployee
LEFT OUTER JOIN dbo.tblSites AS s
ON s.siteID = B.[site]
LEFT OUTER JOIN (
SELECT cast(value as int) as MyValue FROM dbo.ParmsToList('''+@region +''')
) AS PL
ON S.SiteID = PL.MyValue
LEFT OUTER JOIN (
SELECT COUNT(*) AS MyCount, IdentificationNo
FROM dbo.tblAttachment
GROUP BY IdentificationNo
) AS AC
ON AC.IdentificationNo = C.IdentificationNo
WHERE e.eid IS null OR e.eid = (SELECT MAX(ei.eID) AS mEID FROM dbo.tblEmail AS ei WHERE ei.IdentificationNo = C.IdentificationNo)
1)Change SELECT * to an explicit column list, you will increase the likelihood of SQL server using an index and not having to perform key lookups to retrieve columns which are probably not going to be used.
2)replace dbo.ParmsToList with the ssc string-splitter which you will find here[/url]. Perform the split separately from the main query and dump the results into a #temp table, then left-join the temp table into your query.
3)Change the WHERE clause, because it too is a catch-all:
WHERE e.eid IS null OR e.eid = (SELECT MAX(ei.eID) AS mEID FROM dbo.tblEmail AS ei WHERE ei.IdentificationNo = C.IdentificationNo)
4)You have a number of outer joins which will be converted into inner joins when they are referenced in the WHERE clause. If you really want to filter an outer-joined table, put the filter into the ON clause. You might want to do some testing with this to determine the correct behaviour for your query.
Taking points 2 and 3 into account, your query should look something like this:
SELECT CAST(value as int) as MyValue
INTO #ParmsList
FROM dbo.DelimitedSplit8K(@region,',')
SELECT
<<column list here>>,
ac.MyCount AS AttachmentCount
FROM dbo.sgleComplaint AS C
INNER JOIN dbo.tblModUsers AS mu
ON mu.modIDNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmail AS e
ON e.IdentificationNo = C.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS NextToAction
ON NextToAction.fullUserID = e.NextToAction
INNER JOIN dbo.tblBIR AS B
ON C.IdentificationNo = B.IdentificationNo
LEFT OUTER JOIN dbo.tblEmailAddress AS RE
ON RE.eID = B.responsibleEmployee
LEFT OUTER JOIN dbo.tblSites AS s
ON s.siteID = B.[site]
LEFT OUTER JOIN #ParmsList AS PL
ON S.SiteID = PL.MyValue
CROSS APPLY (
SELECT [MyCount] = COUNT(*)
FROM dbo.tblAttachment a
WHERE a.IdentificationNo = C.IdentificationNo
) ac
CROSS APPLY (
SELECT [mEID] = MAX(ei.eID)
FROM dbo.tblEmail ei
WHERE ei.IdentificationNo = C.IdentificationNo
) y
-- convert this expression and add to your dynamic sql
WHERE e.eid IS null OR e.eid = mEID
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
July 6, 2014 at 10:37 pm
Hi Gail, Chris,
Thank you both for your help.
I think I might have a stable secure procedure now, which does appear to be running substantially faster than previously and under all scenarios I tested 😀
after a lot of reviewing and refining I did realise the FROM dbo.ParmsToList('''+@region +''') wasn't needed at all. Wow was this badly designed. so the way it was originally working was a region ID got passed through. it would then generate a table with all Sites associated to that region. That table then got converted to a comma delimited string, and then finally that comma delimated string got turned back into a table (paramstolist) so it could be used as a table. I got rid of all the excess and just use the first table generated 🙂
I'm also not 100% sure on why to use Cross apply instead of inner joins? doesn't this just produce more records which you have to do extra filters on? At the moment I've left as either Inner Or Left outer joins. During my tests the speed seemed to be the same and produced the same expected results doing it either way.
ALTER PROCEDURE [dbo].[uspJobSearch_TEST]
@csType as nvarchar(20),
@status as nvarchar(20),
@startID as nvarchar(50),
@endID as nvarchar(50),
@complaintType as nvarchar(50),
@Subject as nvarchar(100),
@ComplaintDateStart as DATETIME,
@ComplaintDateEnd as dateTime,
@plannedCompDateStart as DATETIME,
@plannedCompDateEnd as dateTime,
@Responsible as nvarchar(50),
@Reference as nvarchar(50),
@ProjectJobNo as nvarchar(50),
@priority as nvarchar(50),
@NextToAction as nvarchar(50),
@Initator as nvarchar(50),
@siteID as int,
@BUName as nvarchar(50),
@userid as nvarchar(50),
@General as nVARCHAR(200),
@company as nvarchar(200),
@LastUpdatedDateStart as dateTime,
@LastUpdatedDateEnd as dateTime,
@region as nvarchar(200)
AS
BEGIN
SET NOCOUNT ON;
IF @Region<> ''
BEGIN
WITH DirectReports (RegionID, RegionName, Level)
AS
(
-- Anchor member definition
SELECT RegionID, RegionName, 0 AS Level
FROM dbo.tblRegion AS r
WHERE regionID =@region
UNION ALL
-- Recursive member definition
SELECT r.RegionID, r.RegionName, Level + 1
FROM dbo.tblRegion AS r
INNER JOIN DirectReports AS d
ON r.parentID = d.RegionID
)
--SELECT @region = COALESCE(@region+',' ,'') + CAST(s.siteID AS VARCHAR(10))
SELECT siteID
INTO #RegionSite
FROM DirectReports LEFT OUTER JOIN dbo.tblSiteRegion AS s ON DirectReports.RegionID = s.regionID
ORDER BY SiteID
END
DECLARE @SQL AS nVARCHAR(4000)
SET @SQL = 'SELECT C.IdentificationNo, C.ComplaintType, C.csType, B.subject, C.ComplaintDate, C.BUInitiatingComplaint, mu.status,
RE.userID, Re.userName, C.CustomerRef, C.ProjectJobNo, C.dateReceived, mu.modUser, mu.modDate, mu.actionCompleted, mu.plannedCompDate,
B.priority, e.eID, e.sDate, e.NextAction, NextToAction.userName AS NextToAction, s.siteName
,CASE WHEN AC.MyCount IS NULL THEN 0 ELSE AC.MyCount END AS AttachmentCount
FROM dbo.sgleComplaint AS C
INNER JOIN dbo.tblModUsers AS mu ON mu.modIDNo = C.IdentificationNo '
IF @status IN ('MyTasks', 'myOverDue') OR @NextToAction <> ''
SET @SQL = @SQL + 'INNER JOIN dbo.tblEmail AS e ON e.IdentificationNo = C.IdentificationNo '
ELSE
SET @SQL = @SQL + 'LEFT OUTER JOIN dbo.tblEmail AS e ON e.IdentificationNo = C.IdentificationNo '
SET @SQL = @SQL + 'LEFT OUTER JOIN dbo.tblEmailAddress AS NextToAction ON NextToAction.fullUserID = e.NextToAction
INNER JOIN dbo.tblBIR AS B ON C.IdentificationNo = B.IdentificationNo'
IF @status IN ('MyTasks', 'myOverDue') OR @Responsible <>''
SET @SQL = @SQL + ' INNER JOIN dbo.tblEmailAddress AS RE ON RE.eID = B.responsibleEmployee'
ELSE
SET @SQL = @SQL + ' LEFT OUTER JOIN dbo.tblEmailAddress AS RE ON RE.eID = B.responsibleEmployee'
IF @siteID <> ''
SET @SQL = @SQL + ' INNER JOIN dbo.tblSites AS s ON s.siteID = B.site'
ELSE
SET @SQL = @SQL + ' LEFT OUTER JOIN dbo.tblSites AS s ON s.siteID = B.site'
IF @region <> '' SET @SQL = @SQL + ' INNER JOIN #RegionSite R on S.siteID = R.SiteID'
SET @SQL = @SQL + ' LEFT OUTER JOIN (SELECT IdentificationNo, COUNT(*) AS MyCount
FROM dbo.tblAttachment
GROUP BY IdentificationNo) AS AC ON AC.IdentificationNo = C.IdentificationNo
WHERE (e.eid IS null OR e.eid = (SELECT MAX(eID) AS mEID
FROM dbo.tblEmail AS tblEmail_1
WHERE (IdentificationNo = C.IdentificationNo))
)'
IF @csType<>'-1' SET @SQL = @SQL + ' AND C.csType = @_cstype '
IF @status='MyTasks'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND ( ( RE.fullUserID = @_userID )'
SET @SQL = @SQL + 'OR (e.NextToAction = @_userID)'
SET @SQL = @SQL + 'OR (MU.modUser = @_userID))'
END
ELSE IF @status = 'myOverdue'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND ( ( RE.fullUserID = @_userID )'
SET @SQL = @SQL + 'OR (e.NextToAction = @_userID)'
SET @SQL = @SQL + 'OR (MU.modUser = @_userID))'
SET @SQL = @SQL + 'AND (mu.plannedCompDate <= ''' + convert(varchar(10),getdate(),102) + ''')'
END
ELSE IF @status = 'allOverdue'
BEGIN
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
SET @SQL = @SQL + 'AND (mu.plannedCompDate <= ''' + convert(varchar(10),getdate(),102) + ''')'
END
ELSE IF @status='-1'
SET @SQL = @SQL + ' AND NOT (mu.status = ''Closed'' OR mu.status= ''Canceled'') '
ELSE
IF @status <> '0' SET @SQL = @SQL + ' AND mu.status = @status '
IF @ComplaintType <> '' SET @SQL = @SQL + ' AND C.ComplaintType = @_ComplaintType '
IF NOT(@startID='') SET @SQL = @SQL + ' AND C.IdentificationNo >= @_startID '
IF NOT(@endID='') SET @SQL = @SQL + ' AND C.IdentificationNo <= @_endID '
IF NOT(@LastUpdatedDateStart is null) SET @SQL = @SQL + ' AND C.ComplaintDate >= convert(varchar(10),@_LastUpdatedDateStart,102) '
IF NOT(@LastUpdatedDateEnd is null) SET @SQL = @SQL + ' AND C.ComplaintDate <= convert(varchar(10),@_LastUpdatedDateEnd+1,102) '
IF NOT(@ComplaintDateStart is null) SET @SQL = @SQL + ' AND MU.modDate >= convert(varchar(10),@_ComplaintDateStart,102) '
IF NOT(@ComplaintDateEnd is null) SET @SQL = @SQL + ' AND MU.modDate <= convert(varchar(10),@_ComplaintDateEnd+1 ,102) '
IF NOT(@plannedCompDateStart is null) SET @SQL = @SQL + ' AND mu.plannedCompDate >= convert(varchar(10), @_plannedCompDateStart,102) '
IF NOT(@plannedCompDateEnd is null) SET @SQL = @SQL + ' AND mu.plannedCompDate <= convert(varchar(10),@_plannedCompDateEnd+1 ,102) '
IF @subject <> '' SET @SQL = @SQL + ' AND B.subject like ''%'' + @_subject + ''%'''
IF @BUName <> '' SET @SQL = @SQL + ' AND C.BUInitiatingComplaint = @_BUName '
IF @siteID <> '' SET @SQL = @SQL + ' AND S.SiteID = CAST(@_siteID AS VARCHAR)'
IF @Responsible <> '' SET @SQL = @SQL + ' AND RE.userName = @_Responsible '
IF @NextToAction <> '' SET @SQL = @SQL + ' AND e.NextToAction= @_NextToAction '
IF @Initator <> '' SET @SQL = @SQL + ' AND MU.modUser = @_Initator '
IF @Reference <> '' SET @SQL = @SQL + ' AND C.CustomerRef like ''%'' + @_Reference + ''%'''
IF @ProjectJobNo <> '' SET @SQL = @SQL + ' AND C.ProjectJobNo = @_ProjectJobNo '
IF @priority <> '' SET @SQL = @SQL + ' AND B.priority = @_priority '
IF @company <> '' SET @SQL = @SQL + ' AND B.company like ''%'' + @_company + ''%'''
IF @General <> '' SET @SQL = @SQL + ' AND FREETEXT(B.History, @_General)'
--IF @region <> '' SET @SQL = @SQL + ' AND S.SiteID = R.SiteID'
--SELECT @SQL
EXEC sp_executesql @SQL,
N'@_cstype nvarchar(20), @_userID nvarchar(50), @_status nvarchar(20), @_ComplaintType nvarchar(50), @_startID int, @_endID int
, @_LastUpdatedDateStart dateTime, @_LastUpdatedDateEnd dateTime, @_ComplaintDateStart dateTime, @_ComplaintDateEnd dateTime, @_plannedCompDateStart dateTime
, @_plannedCompDateEnd dateTime, @_subject nvarchar(100), @_BUName nvarchar(50),@_siteID int, @_Responsible nvarchar(50), @_NextToAction nvarchar(50)
, @_Initator nvarchar(50), @_Reference nvarchar(50), @_ProjectJobNo nvarchar(50), @_priority nvarchar(50), @_company nvarchar(200), @_General nVARCHAR(200)'
, @_cstype=@cstype, @_userID=@userID, @_status=@status, @_ComplaintType=@ComplaintType, @_startID=@startID, @_endID=@endID
,@_LastUpdatedDateStart=@LastUpdatedDateStart, @_LastUpdatedDateEnd=@LastUpdatedDateEnd, @_ComplaintDateStart=@ComplaintDateStart, @_ComplaintDateEnd=@ComplaintDateEnd, @_plannedCompDateStart=@plannedCompDateStart
,@_plannedCompDateEnd=@plannedCompDateEnd, @_subject=@subject, @_BUName=@BUName, @_siteID=@siteID, @_Responsible=@Responsible, @_NextToAction=@NextToAction
, @_Initator=@Initator, @_Reference=@Reference, @_ProjectJobNo=@ProjectJobNo, @_priority=@priority, @_company=@company, @_General=@General
END
Viewing 8 posts - 1 through 7 (of 7 total)
You must be logged in to reply to this topic. Login to reply