March 24, 2021 at 6:49 am
Im breaking my head on this. It runs in 2 ms but just has high reads n cpu sometimes. What can be done to improve or optimize
--select top 10 * from firm_master order by 1 desc
--select top 10 * from BOOKING order by 1 desc
declare
@Firmid varchar(20),
@Bookingid nvarchar(max),
@StartDate nvarchar(30),
@EndDate nvarchar(30),
@Userid nvarchar(max),
@Taskid nvarchar(max),
@Roleid nvarchar(30) = Null
set
@Firmid = 1621
set @Bookingid = 0
set @StartDate = '2021-04-09'
set @EndDate = '2021-04-10'
set @Userid = 95471
set @Taskid = 0
set @Roleid = 6
Set Nocount On
Declare @GetQuery nvarchar(max)
--declare @firmid_Snf varchar(20) = @Firmid
Set @GetQuery='
Select [bookingId] as id
,[bookingTitle] as [text]
, bk.[startDate] as start
,[endDate] as [end]
,[userId] as [resource]
,[roleId]
,[jobCode]
,[bookingType]
,[billableType]
,[taskId]
,[minutesDay]
,[minutesPerDay]
,[totalBucketMinutesAllocation]
,[percentAllocation]
,[includeSaturday]
,[includeSunday]
,[includeHoliday]
,[details]
,[firmid]
,[NoOFHours]
,[bookingJson]
,tm.client_id as clientId,
cm.XCMClientFullName AS ClientName,
cm.account_number as AccountNumber,
ft.task_code as tasktype,
convert(varchar,tm.period_enddate,101) as PE,
'''' as jobcode_name,
tm.task_desc,
(select cUserFullName from DBO.um where firm_id=bk.firmid and user_id=bk.[userId]) as username
From
dbo.Booking bk
Inner join dbo.tm on tm.firm_id = bk.firmid and bk.taskId = tm.task_id
Inner join dbo.cm on cm.firm_id = tm.firm_id and cm.client_id = tm.client_id
Inner join dbo.ft on ft.firm_id = tm.firm_id and ft.tasktype_id = tm.tasktype_id
inner join dbo.fsc on fsc.firm_id = ft.firm_id and fsc.Category_id = ft.category_id AND fsc.is_active=1
Where firmID= '+@firmid+' and isdeleted=''N'''
If(@startDate!='0')
Begin
Set @GetQuery= @GetQuery + ' and
(CONVERT(DATE,bk.startDate) between convert(datetime,'''+@StartDate+''') and convert(datetime,'''+@endDate+''') or
CONVERT(DATE,bk.enddate) between convert(datetime,'''+@StartDate+''') and convert(datetime,'''+@endDate+''') or
(CONVERT(DATE,bk.startDate)<convert(datetime,'''+@StartDate+''') and CONVERT(DATE,bk.endDate)>convert(datetime,'''+@endDate+''')))'
End
If(@Bookingid!='0')
Begin
Set @GetQuery= @GetQuery + ' and bk.bookingId in ('+@Bookingid+')'
End
If(@taskid!='0')
Begin
Set @GetQuery= @GetQuery + ' and bk.taskid in ('+@taskid+')'
End
If(@userid!='0')
Begin
Set @GetQuery= @GetQuery + ' and bk.userId in ('+@userid+')'
End
If(@Roleid!='0')
Begin
Set @GetQuery= @GetQuery + ' and bk.roleId in ('+@Roleid+')'
End
--Exec sp_executesql @GetQuery
--select @GetQuery
Declare @GetQueryNonTask nvarchar(max)
Set @GetQueryNonTask=' Union All
select [bookingId] as id
,[bookingTitle] as [text]
, bk.[startDate] as start
,[endDate] as [end]
,[userId] as [resource]
,[roleId]
,[jobCode]
,[bookingType]
,[billableType]
,[taskId]
,[minutesDay]
,[minutesPerDay]
,[totalBucketMinutesAllocation]
,[percentAllocation]
,[includeSaturday]
,[includeSunday]
,[includeHoliday]
,[details]
,[firmid]
,[NoOFHours]
,[bookingJson]
,NULL as clientId
,NULL as clientName
,NULL as AccountNumber
,NULL as tasktype
,NULL as PE
,'''' as jobcode_name
,NULL as task_desc
,(select cUserFullName from DBO.um where firm_id=bk.firmid and user_id=bk.[userId]) as username
From
dbo.bk
Where bk.taskid= 0 and bk.isdeleted=''N'' and bk.firmID= '+@firmid+' '
If(@startDate!='0')
Begin
Set @GetQueryNonTask= @GetQueryNonTask + ' and
(CONVERT(DATE,bk.startDate) between convert(datetime,'''+@StartDate+''') and convert(datetime,'''+@endDate+''') or
CONVERT(DATE,bk.enddate) between convert(datetime,'''+@StartDate+''') and convert(datetime,'''+@endDate+''') or
(CONVERT(DATE,bk.startDate)<convert(datetime,'''+@StartDate+''') and CONVERT(DATE,bk.endDate)>convert(datetime,'''+@endDate+''')))'
End
If(@Bookingid!='0')
Begin
Set @GetQueryNonTask= @GetQueryNonTask + ' and bk.bookingId in ('+@Bookingid+')'
End
If(@taskid!='0')
Begin
Set @GetQueryNonTask= @GetQueryNonTask + ' and bk.taskid in ('+@taskid+')'
End
If(@userid!='0')
Begin
Set @GetQueryNonTask= @GetQueryNonTask + ' and bk.userId in ('+@userid+')'
End
set @GetQuery = @GetQuery + @GetQueryNonTask
--select @GetQuery
Exec sp_executesql @GetQuery
March 24, 2021 at 10:44 am
Whoa!
You are literally begging for a SQL Injection attack with this code. I'd strongly suggest you read up on what that is and how to prevent it.
Next, you're trying to create a single query to cover every possible use case. This is a design flaw. SQL queries work best when they're kept clear and direct. Trying to do every possible permutation of every possible search criteria through a single piece of code leads to what you're looking at here. Please read these articles by Gail Shaw on Catch-All Queries. They will outline a bunch of ways to possibly improve what you have here.
However, one issue, and I think I've pointed this out before, is that you're performing functions on columns and parameters (or variables) in this code:
(CONVERT(DATE,bk.startDate) between convert(datetime,'''+@StartDate+''') and convert(datetime,'''+@endDate+''') or
That's going to prevent good statistic use and will preclude index seeks. You're going to see nothing but scans. That's a lot more I/O on the disk and in memory, and ultimately managing both increases CPU. So, instead, ensure that your bk.startDate is a DATE or DATETIME data type, that @StartDate is a DATE or DATETIME data type, that @EndDate is a DATE or DATETIME data type, that, in fact, everything in the database is defined as the appropriate data type. Conversions will lead to scans. Avoid them.
However, fix the Injection problems and the Catch-All query problems first.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
March 24, 2021 at 3:10 pm
To add to what Grant said, is there a reason you NEED that to be dynamic SQL? Quick look through it I don't see any reason for that to be dynamic SQL. I mean, it may make for a very lengthy WHERE clause, but I would rather have a long WHERE than have dynamic SQL. 9/10 times I find that dynamic SQL is the wrong approach. And the 1/10 times where it is the right approach, I find that it is usually for an administrative task and not for data lookups.
I know where I work we have some of these "catch all" queries and they become a pain in the butt to support long term. Today you have those 5 filter requirements and then in the future you get 6 and then 7 and then a value the end user expects to be there isn't because it is filtered out and you now need to make special cases and the query just gets messy.
You may also get a performance boost by changing your in-line select to a join (the last column) or you may not.
Depending on the size of the data coming back too, that could be your performance hit. If you are pulling back 1 GB of data (for example), reducing the data set or data size may help.
Also, is 2 ms worth optimizing further? Even if you get a 50% increase in performance, that is going from 2 ms to 1 ms. Having a spike in CPU for 2 ms probably won't even get caught by most monitoring tools. I think the most important fix is the SQL injection problem.
Lastly, check the execution plan. You may benefit from indexes (we don't know if you have any). You may benefit from updating your statistics if they are out of date.
The above is all just my opinion on what you should do.
As with all advice you find on a random internet forum - you shouldn't blindly follow it. Always test on a test server to see if there is negative side effects before making changes to live!
I recommend you NEVER run "random code" you found online on any system you care about UNLESS you understand and can verify the code OR you don't care if the code trashes your system.
March 24, 2021 at 3:22 pm
I totally agree with Grant on searching against a function leading to table scans and the potential for an injection attack. Even though you are using sp_ExecuteSQL, you are still building strings from your parameters as if you were just using EXEC to run your dynamic SQL. If you read the documentation and study the examples closely, sp_ExecuteSQL allows you to pass values in as parameters to avoid the possibility of SQL injection attacks.
That said, I don't see anything wrong in principle with using dynamic SQL to create queries based on the values of parameters as long as the resulting queries are themselves clear and direct. Catch-all queries usually involve OR conditions to which force table scans. There are no OR conditions in your example.
In your example, the resulting WHERE clause would not filter against bk.bookingID, bk.tastID, or bk.userID unless their associated parameters had nonzero values. This would not be a problem if you were passing single values in the parameters. The problem is that you are using NVARCHAR(MAX) strings to pass a set of values to filter on and including these strings in the query. That is what creates the possibility of a classic SQL injection attack.
The proper way to handle this is by passing table valued parameters and referencing them with your "IN" conditions. Google "Sp_ExecuteSQL table valued parameters" for examples.
Finally, fix those lines based on @StartDate and @EndDate. If bk.StartDate and bk.EndDate are not date/datetime datatypes, they should be. Using the parameter-passing functionality of sp_ExecuteSQL to pass in the values for @StartDate and @EndDate, the proper syntax should be this simple
bk.startDate between @StartDate and @endDate -- etc
Good luck.
__________________________________________________
Against stupidity the gods themselves contend in vain. -- Friedrich Schiller
Stop, children, what's that sound? Everybody look what's going down. -- Stephen Stills
March 24, 2021 at 3:46 pm
although. 2 ms for a dynamic sql? how often is this executed during the day? unless it is being executed hundreds of times per minute I would not bother too much with it.
(note that the code related to bk.startdate should be changed regardless)
March 25, 2021 at 3:57 am
This was removed by the editor as SPAM
March 25, 2021 at 10:08 am
Also, is 2 ms worth optimizing further? Even if you get a 50% increase in performance, that is going from 2 ms to 1 ms. Having a spike in CPU for 2 ms probably won't even get caught by most monitoring tools. I think the most important fix is the SQL injection problem.
Total agreement with all your post. Just a quick story. We once had a query that was called so often that getting it from 3ms to 2ms was a giant win. Sometimes, you do stuff that seems silly.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
March 25, 2021 at 2:12 pm
That is a good point Grant and Fredrico. Optimizing a 2 ms query that is run once per year is wasted effort. Optimizing it when it is run 100's of times per minute could save a ton of time. In an 8 hour day, if it is run 1,000,000 times per hour, shaving off 1 ms means you are shaving off 8,000,000 ms per day, or roughly 2 hours per day of "work". Which is a pretty big win.
I still think this is tricky to help optimize with what is given as there are so many unknowns. Is the estimated plan close enough to the actual? Is there tempdb spill? Missing indexes? Duplicate indexes (Pinal Dave had a fun video showing that having duplicate indexes can cause a performance hit)? etc....
Another thought about optimizing the query, if the application is calling the query multiple times to filter the data differently (for example you run it once to get all the data, then you run it a second time giving it a specific booking ID then run it a third time giving it the booking ID and task ID, then run it a forth time giving it a booking ID, task ID, and User ID as an example), you may benefit from filtering the data on the application side. Run the query once to get all of the data and then filter the data on the application side. Performance will vary depending on client hardware and the amount of data, but if you are looking at tuning the performance, it never hurts to look at tuning things on the client side.
The above is all just my opinion on what you should do.
As with all advice you find on a random internet forum - you shouldn't blindly follow it. Always test on a test server to see if there is negative side effects before making changes to live!
I recommend you NEVER run "random code" you found online on any system you care about UNLESS you understand and can verify the code OR you don't care if the code trashes your system.
March 25, 2021 at 2:26 pm
If you want the SQL optimised you would be better off giving us the actual SQL that is generated along with the execution plan.
March 25, 2021 at 5:23 pm
Just a point to note here: The way that dynamic SQL works, you are going to get MULTIPLE queries and execution plans from the various combinations of parameters. Some of them may be well-supported by indexes and some may be problem children that are scanning millions of rows or requiring a ton of key lookups. This would account for the "high reads n CPU sometimes."
To diagnose this, you can print out the dynamic SQL string variable you are executing. If a query is performing poorly with a given set of parameters, look at the query generated by those parameters, and look at it's execution plan. Only then will you know if you are missing a much-needed index, etc.
But again, like Grant said: You have to fix the code that tests a date range.
__________________________________________________
Against stupidity the gods themselves contend in vain. -- Friedrich Schiller
Stop, children, what's that sound? Everybody look what's going down. -- Stephen Stills
Viewing 10 posts - 1 through 9 (of 9 total)
You must be logged in to reply to this topic. Login to reply