2

I have a SQL Server 2017 Enterprise Edition instance where a stored procedure was taking approx. five minutes to execute. After reviewing the stored procedure code, I could see that there was an inline scalar UDF referenced several times in the SELECT list and also in the predicate WHERE clause of the stored procedure body.

I advised the application team owning the code that they should refactor their stored proc not to use an inline UDF which they took onboard and replaced with a TVF. Whilst they were doing this, I noticed that the application database still had database compatibility level 100 so I raised this to the latest level of 140 after running the database through the Data Migration Assistant to check for deprecated features and breaking changes.

Following the replacement of the UDF for a TVF and raising the database compatibility level from 100 to 140, the performance has increased greatly and the stored proc now executes in under a minute but performance is still not where I'd like to it be. I'm hoping someone might be able to advise of anything obvious I'm missing or point me in the right direction of anything else I can do to further optimise the code or get this to perform better? The execution plan is here: https://www.brentozar.com/pastetheplan/?id=ByrsEdRpr

The code for the stored proc and function are as below and the stored procedure is called by the application as such: "EXEC dbo.CAOT_GetApplicationQueue;1"

/****** Object: StoredProcedure [dbo].[CAOT_GetApplicationQueue] ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

CREATE PROCEDURE [dbo].[CAOT_GetApplicationQueue]
(@userID VARCHAR(50)='', @showComplete CHAR(1)='N', @JustMyQueue BIT=0, @ChannelId VARCHAR(10) = NULL )
AS
BEGIN
SELECT App.pkApplication ,
COALESCE(ApplicationReference, AlternateApplicationReference) AS ApplicationReference ,
ApplicationDate ,
Name ,
Telephone ,
[Address] ,
Email ,
CIN ,
Dob ,
CreatedDate ,
BusinessPhone ,
PostCode ,
MobilePhone ,
[Action] ,
ActionStatus ,
branchNumber ,
AccountNumber ,
AccountType ,
act.accountDescription,
IsNull( appstatus.DESCRIPTION ,'-- CREATED --') As LastStatus,
IsNull(appstatus.DAYS,'0') DaysSinceLastStatus ,
DATEDIFF(d,ApplicationDate, GETDATE()) DaysCreated,
InitialUserID,
IsNull(appstatus.STATUS,'-- MADE --') APPLICATIONSTATUS
FROM dbo.CAOT_Application (NOLOCK) app
LEFT OUTER JOIN dbo.CAOT_AccountType (NOLOCK) act
ON app.AccountType = act.AccountTypecode
LEFT OUTER JOIN [CAOT_GetAllApplicationStatus]() appstatus
ON app.pkApplication = appstatus.[PKAPPLICATION]
WHERE (IsNull(appstatus.STATUSCODE,'MADE') NOT IN ('CANCELLED','DECLINED','COMPLETE','EXPIRED')
OR @showComplete='Y') AND
(@JustMyQueue = 0 OR InitialUserID = @userID) AND
(@ChannelId IS NULL OR ChannelID = @ChannelId OR (@ChannelId = 'CBU' AND ChannelID IS NULL AND isCAO='N'))
ORDER BY CASE WHEN InitialUserID = @userid THEN 10 ELSE 900 END, ApplicationDate DESC
END
GO

/****** Object: UserDefinedFunction [dbo].[CAOT_GetAllApplicationStatus] ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

CREATE FUNCTION [dbo].[CAOT_GetAllApplicationStatus]() RETURNS
@return TABLE
(
[PKAPPLICATION] [int] NOT NULL,
[PKAPPLICATIONEVENT] INT,
[EVENTCREATEDDATE] [DATETIME] NULL,
[KEY] VARCHAR(12) NULL,
[DESCRIPTION] VARCHAR(200) NULL,
[CODE] VARCHAR(20) NULL,
[DAYS] VARCHAR(20) NULL,
[STATUS] VARCHAR(200) NULL,
[STATUSCODE] VARCHAR(50) NULL
)

AS
BEGIN

Declare @AppStatus table
(
[PKAPPLICATION] [int] NOT NULL,
[PKAPPLICATIONEVENT] INT,
[EVENTCREATEDDATE] [DATETIME] NULL,
[KEY] VARCHAR(12) NULL,
[DESCRIPTION] VARCHAR(200) NULL,
[CODE] VARCHAR(20) NULL,
[DAYS] VARCHAR(20) NULL,
[STATUS] VARCHAR(200) NULL,
[STATUSCODE] VARCHAR(50) NULL
)

INSERT INTO @AppStatus

SELECT
fkApplication,
ev.pkApplicationEvent As pkApplicationEvent,
ev.CreateDate As 'EventCreatedDate',
CONVERT(VARCHAR(12), evt.fkApplicationStatus) As 'KEY',
evt.EventDescription As 'DESCRIPTION',
evt.EventCode As 'CODE' ,
CONVERT(VARCHAR(20), DATEDIFF(d, ev.createdate, GETDATE()) ) As 'DAYS',
apps.StatusDescription As 'STATUS' ,
apps.StatusCode As 'STATUSCODE'
FROM dbo.CAOT_ApplicationEvent (NOLOCK) ev
INNER JOIN dbo.CAOT_EventType (NOLOCK) evt ON ev.fkEventType = evt.pkEventType
INNER JOIN dbo.CAOT_ApplicationStatus (NOLOCK) apps ON evt.fkApplicationStatus = apps.pkApplicationStatus

ORDER BY ev.CreateDate DESC, ev.pkApplicationEvent DESC

INSERT INTO @return
Select * from @AppStatus AllStatus
Where AllStatus.EVENTCREATEDDATE = ( Select Max(LatestAppStatus.EVENTCREATEDDATE) from @AppStatus LatestAppStatus where LatestAppStatus.PKAPPLICATION =AllStatus.PKAPPLICATION ) --Z On X.PKAPPLICATION = Z.PKAPPLICATION

RETURN
END

GO
  • 3
    Have you been able to isolate the slow parts of the new code? E.g., how fast is the sproc if you comment out the function? Also, it looks like the TVF could be replaced with a view. That can often be faster, as the parser can discard unused parts of the view. – Jon of All Trades Dec 13 '19 at 15:11
  • If you include relevant table and sample data DDLs then people can reproduce and help you troubleshoot a lot easier. – LowlyDBA - John McCall Dec 13 '19 at 18:56
  • @LowlyDBA ah good idea, I didn't consider that. I've uploaded the code to codepile.net/pile/P0LlrJ0L – Fza Dec 14 '19 at 10:54
  • @JonofAllTrades thanks for your comment and that's a good shout. If I comment out the references to the function the stored procedure executes immediately in less than a second which would indicate that the TVF is the problem. T-SQL in't my strong point, do you have any pointers on how you'd go about refactoring a TVF to a view? – Fza Dec 14 '19 at 10:55
  • 1
    Try adding OPTION(RECOMPILE) to the proc query. I think you'll get a plan that is more optimal for the actual parameter values, especially with the OR constructs in the WHERE clause. – Dan Guzman Dec 14 '19 at 16:15
2
+50

You can replace your TVF with a view (or keep the TVF, but use the view for your performance-critical sproc):

CREATE VIEW CAOT_AllApplicationStatuses AS
  SELECT
    fkApplication,
    ev.pkApplicationEvent AS pkApplicationEvent,
    ev.CreateDate AS EventCreatedDate,
    CONVERT(VARCHAR(12), evt.fkApplicationStatus) As 'KEY',
    evt.EventDescription AS 'DESCRIPTION',
    evt.EventCode AS 'CODE',
    CONVERT(VARCHAR(20), DATEDIFF(d, ev.createdate, GETDATE())) AS 'DAYS',
    apps.StatusDescription AS 'STATUS',
    apps.StatusCode AS 'STATUSCODE'
  FROM
    dbo.CAOT_ApplicationEvent (NOLOCK) ev
    INNER JOIN dbo.CAOT_EventType (NOLOCK) evt ON ev.fkEventType = evt.pkEventType
    INNER JOIN dbo.CAOT_ApplicationStatus (NOLOCK) apps ON evt.fkApplicationStatus = apps.pkApplicationStatus
  WHERE
    NOT EXISTS
      (
      SELECT * FROM dbo.CAOT_ApplicationEvent AS LaterEvent WHERE EV.pkApplication = LaterEvent.pkApplication AND LaterEvent.pkApplication.CreateDate > EV.CreateDate
      )
  ORDER BY
    ev.CreateDate DESC, ev.pkApplicationEvent DESC

This is simply the content of the TVF's main SELECT query, with the WHERE clause from the second SELECT incorporated as a NOT EXISTS. I'm trusting that all records in CAOT_ApplicationEvent have records in CAOT_EventType and CAOT_ApplicationStatus; if that's not the case, you'll need to add those joins in the NOT EXISTS query.

Just using a view rather than a TVF may help, as the parser will incorporate the view into the final query, and discard unused parts; those calls to CONVERT(), for example, are likely to be relatively expensive, but they appear to be unused. However, the complex predicates in your top-level sproc may necessitate a table scan. Let's give this a shot and see if it needs more work!

| improve this answer | |
  • 1
    JonOfAllTrades thanks for that. I had to modify the view definition you provided and comment out the ORDER BY clause and also change the references of pkApplication in the NOT EXISTS clause to fkApplication to get it to work. The exact code of the view and updated stored proc is here: codepile.net/pile/rLzZ1GLW In my test environment, the stored proc + view completes in 13 seconds as opposed to the stored proc + tvf which takes 21 seconds so a definite improvement. The query plan with the view is here: brentozar.com/pastetheplan/?id=SkErPimCH – Fza Dec 15 '19 at 12:50
  • JonOfAllTrades in the new query execution plan there's a missing index warning which I tried creating but didn't make much/any difference really. There's a warning in the query execution plan due to the use of CONVERT in the VIEW definition, is there another type conversion that's more optimal or anything else you can think of that we could try to squeeze some more performance out of this? Thanks again for all your help in getting me this far! – Fza Dec 15 '19 at 12:56
  • Missing index warnings are worth acting on perhaps one time in five, in my experience. They are worth considering, but no more. Regarding CAST vs CONVERT, I'd expect little difference in performance (and someone tested this). You can probably just eliminate them; is there any reason to change integers to text for this query? – Jon of All Trades Dec 15 '19 at 18:03
  • JonOfAllTrades I tried eliminating the CONVERT(s) from the view definition and they made no difference to the performance as expected. Two other users have suggested breaking up the predicates in the stored procedure body using UNION ALL, do you think there's merit to that and worth trying? – Fza Dec 16 '19 at 16:43
  • If there are any fields you don't need, I'd start with trimming those. However, it's worth trying the UNION trick; I've seen it help quite a bit, occasionally. In principle, the query planner should recognize that the plans are logically equivalent, and implement one or the other as it sees fit, but sometimes a hint helps. – Jon of All Trades Dec 16 '19 at 17:26
0

After reading http://sommarskog.se/dyn-search.html

you could split the query into multiple parts, something like:

create procedure [dbo].[CAOT_GetApplicationQueue] ( @userID varchar(50) = '', @showComplete char(1) = 'N', @JustMyQueue bit = 0, @ChannelId varchar(10) = null )
as begin
    select case when InitialUserID = @userID then 10 else 900 end as SortCol, app.pkApplication, coalesce(ApplicationReference, AlternateApplicationReference) as ApplicationReference, ApplicationDate, Name, Telephone, [Address], Email, CIN, Dob, CreatedDate, BusinessPhone, PostCode, MobilePhone, [Action], ActionStatus, branchNumber, AccountNumber, AccountType
         , act.accountDescription, isnull(appstatus.DESCRIPTION, '-- CREATED --') as LastStatus, isnull(appstatus.DAYS, '0') DaysSinceLastStatus, datediff(d, ApplicationDate, getdate()) DaysCreated, InitialUserID, isnull(appstatus.STATUS, '-- MADE --') APPLICATIONSTATUS
    from dbo.CAOT_Application  app
    left outer join dbo.CAOT_AccountType  act on app.AccountType = act.AccountTypecode
    left outer join [CAOT_GetAllApplicationStatus]() appstatus on app.pkApplication = appstatus.[PKAPPLICATION]
    where ( isnull(appstatus.STATUSCODE, 'MADE') not in ( 'CANCELLED', 'DECLINED', 'COMPLETE', 'EXPIRED' ) or @showComplete = 'Y' )
    and   ( @JustMyQueue = 0 /* or InitialUserID = @userID */)
    and   ( @ChannelId is null or ChannelID = @ChannelId or ( @ChannelId = 'CBU' and ChannelID is null and isCAO = 'N' ))
    union all 
    select 10 as SortCol, app.pkApplication, coalesce(ApplicationReference, AlternateApplicationReference) as ApplicationReference, ApplicationDate, Name, Telephone, [Address], Email, CIN, Dob, CreatedDate, BusinessPhone, PostCode, MobilePhone, [Action], ActionStatus, branchNumber, AccountNumber, AccountType
         , act.accountDescription, isnull(appstatus.DESCRIPTION, '-- CREATED --') as LastStatus, isnull(appstatus.DAYS, '0') DaysSinceLastStatus, datediff(d, ApplicationDate, getdate()) DaysCreated, InitialUserID, isnull(appstatus.STATUS, '-- MADE --') APPLICATIONSTATUS
    from dbo.CAOT_Application  app
    left outer join dbo.CAOT_AccountType  act on app.AccountType = act.AccountTypecode
    left outer join [CAOT_GetAllApplicationStatus]() appstatus on app.pkApplication = appstatus.[PKAPPLICATION]
    where ( isnull(appstatus.STATUSCODE, 'MADE') not in ( 'CANCELLED', 'DECLINED', 'COMPLETE', 'EXPIRED' ) or @showComplete = 'Y' )
    and   ( @JustMyQueue <> 0)
    and   ( InitialUserID = @userID)
    and   ( @ChannelId is null or ChannelID = @ChannelId or ( @ChannelId = 'CBU' and ChannelID is null and isCAO = 'N' ))
    order by SortCol, ApplicationDate desc
    option (recompile)
end;

That may searching fewer rows of your CAOT_Application table.

| improve this answer | |
  • Henrik, thanks for your suggestion. Your stored procedure actually takens longer to execute (29 seconds) than the original stored proc (21 seconds). If I modify your stored proc to reference the view suggested by JonOfAllTrades then it completes in 13 seconds which is the same as the original stored proc when updated to reference the view. The execution plan generated is almost identical too: brentozar.com/pastetheplan/?id=HJrV1EB0H – Fza Dec 16 '19 at 16:32
  • @Fza; well, there are many more items you can try, once you've read Sommaskog, for instance on ChannelID. Then look at adding non-clustered indexes. You must certainly also need to follow the suggestions by Jon of All Trades and KumarHarsh – Henrik Staun Poulsen Dec 17 '19 at 7:18
0

Main area of concern

  1. Missing SET NOCOUNT ON in proc.
  2. Do not use MTVF :Your UDF is Multi statement TVF(MTVF) Currently it unnecessarily fetch all rows.It give wrong estimate to optimizer.
  3. MTVF is not require. Create Inline TVF(ITVF) or use Outer Apply.If you use Inline TVF then use them inside OUTER APPLY instead of LEFT JOIN.Pass pkApplicationStatus and @showComplete in Inline TVF as parameter.This will guarantee return only require number of rows.Also ITVF t will execute less than MTVF.
  4. Alternatively you write ITVF logic in temp table.It will also work ok.
  5. Condition using too much OR condition are bad perform-ant.(@JustMyQueue = 0 OR InitialUserID = @userID). Try to convert them in UNION ALL or Put them in #temp table and JOIN.

Example,

create table #Status(Status varchar(15))
if(@showComplete='Y')
begin
insert into #Status
end
else
begin
insert into #Status
end

Now simply join #Status

Edit 1 :

You must have notice the performance improvement after passing Parameter in your UDF. It can be further improve. If possible reduce RETURNS VARCHAR(100) to VARCHAR(50) or so.

ALTER FUNCTION [dbo].[CAOT_GetApplicationStatus]
  (@pkApplication INT, @returnType varchar(20))
  RETURNS VARCHAR(100)

AS
--DECLARE @status VARCHAR(100)
RETURN(
SELECT TOP 1

    CASE @returntype
      WHEN 'KEY' THEN CONVERT(VARCHAR(12), isnull(evt.fkApplicationStatus,'1'))
      WHEN 'DESCRIPTION' THEN isnull(evt.EventDescription,'-- CREATED --')
      WHEN 'CODE' THEN isnull(evt.EventCode,'CREATE')
      WHEN 'DAYS' THEN CONVERT(VARCHAR(20), isnull(DATEDIFF(d, ev.createdate, GETDATE()),'0') )
      WHEN 'STATUS' THEN isnull(apps.StatusDescription,'-- MADE --')
      WHEN 'STATUSCODE' THEN isnull(apps.StatusCode,'MADE')
    ELSE isnull(evt.EventDescription,'')
  END
FROM dbo.CAOT_ApplicationEvent (NOLOCK) ev
    INNER JOIN dbo.CAOT_EventType (NOLOCK) evt
        ON ev.fkEventType = evt.pkEventType
    INNER JOIN dbo.CAOT_ApplicationStatus (NOLOCK) apps
        ON evt.fkApplicationStatus = apps.pkApplicationStatus
WHERE fkApplication = @pkApplication
ORDER BY ev.CreateDate DESC, ev.pkApplicationEvent DESC
)

GO

Don't forget the #Status join trick. You should update your script and query plan.

| improve this answer | |
  • Kumar, thanks for the input. I tried including SET NOCOUNT ON in the stored procedure but it didn't make any difference to performance, will keep that in though since it would appear to be a best practice. The code for the original scalar UDF is here: codepile.net/pile/jzy1ZMzG But like I said, T-SQL is not my strong point, how would you go about re-factoring this as a temp table or ITVF as you've suggested? – Fza Dec 16 '19 at 16:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.