microsoft / SqlNexus

SQL Nexus is a tool that helps you identify the root cause of SQL Server performance issues. It loads and analyzes performance data collected by SQL LogScout, SQLDiag or PSSDiag. It can dramatically reduce the amount of time you spend manually analyzing data.
MIT License
345 stars 95 forks source link

Different values between Huge Memory Grant Alert and actual Memory Grant Report #262

Closed hacitandogan closed 6 months ago

hacitandogan commented 8 months ago

Difference reported between Best Practices & Analysis Summary report rule and actual Memory Grant Report.

Huge Memory Grant found There are 313 queries with memory grants using > 1 GB of memory. Average grant size = 24.57 GB. The largest grant size found = 28.54 GB. Consider tuning queries to reduce memory usage.

vs.

image
hacitandogan commented 8 months ago

Rule is using ; tbl_dm_exec_query_memory_grants "-- dm_exec_query_memory_grants --"

PerfStats file , running a select against sys.dm_exec_query_memory_grants directly , no filters.

Report is using ; tbl_Query_Execution_Memory "-- query execution memory --"

PerfStats file , running a select with some joins , getting same memory grant values from sys.dm_exec_query_memory_grants but doing a cross apply for sys.dm_exec_sql_text , losing any lines there ? Also joining with sys.dm_exec_query_resource_semaphores only on resource_semaphore_id ,which is causing duplicate values as resource_semaphore_id is not unique (it is per pool_id)

mg.pool_id = rs.pool_id fixing the duplicate row issue. Changing Cross Apply to Outer Apply to not lose any Rows ? (may need to handle NULL q.text values properly ) Merge queries into one and not use other rowset ?

This should be fixed in the log capture side (Logscout / pssdiag).

Or I can simply change the report definitions to use tbl_dm_exec_query_memory_grants (as those values seems more accurate)

@PiJoCoder any comment on this one ?

PiJoCoder commented 8 months ago

Rule is using ; tbl_dm_exec_query_memory_grants "-- dm_exec_query_memory_grants --"

PerfStats file , running a select against sys.dm_exec_query_memory_grants directly , no filters.

Report is using ; tbl_Query_Execution_Memory "-- query execution memory --"

.... This should be fixed in the log capture side (Logscout / pssdiag).

Or I can simply change the report definitions to use tbl_dm_exec_query_memory_grants (as those values seems more accurate)

@PiJoCoder any comment on this one ?

I am not biased to either solution because I have worked on both sectionс of code, though I have not analyzed tbl_Query_Execution_Memory for accuracy. I do prefer the simpler solution of using tbl_dm_exec_query_memory_grants. But I also think we should update the code in SQL LogScout (PSSDIAG is a minimum maintenance mode). If you have analyzed the code for tbl_Query_Execution_Memory and have reocmmendations, you can make the change yourself. Or if you prefer I can work on it.

hacitandogan commented 7 months ago

@PiJoCoder , I don't want to just replace this with tbl_dm_exec_query_memory_grants as it does not really show text for all queries (mainly if it is ad-hoc ) , yet we have the query text on tbl_Query_Execution_Memory for the same line. I will work on the capture script on LogScout to make sure these two datasets represent same and accurate data. Should we keep this one until we complete that ?

PiJoCoder commented 7 months ago

@PiJoCoder , I don't want to just replace this with tbl_dm_exec_query_memory_grants as it does not really show text for all queries (mainly if it is ad-hoc ) , yet we have the query text on tbl_Query_Execution_Memory for the same line. I will work on the capture script on LogScout to make sure these two datasets represent same and accurate data. Should we keep this one until we complete that ?

Thank you for the clarification. One thing that comes to mind is could we somehow support backward compatibility? Probably not, once we change the report Data set, but just something to think about. Yeah, we can keep this item active for now until the SQL LogScout coding is complete. No rush. We can schedule for next wave if that's where it ends up over time.

hacitandogan commented 6 months ago

Closing this to move a step forward in wave3, discussed modifications implemented in LogScout in item 561 and will be available in next release.