search

erikdarlingdata / DarlingData

Open source SQL Server nonsense: sp_PressureDetector, sp_QuickieStore, sp_HumanEvents, etc.
https://www.erikdarling.com/
MIT License
476 stars 135 forks source link

sp_QuickieStore: Inconsistency in blocking access to msdb #485

Closed ReeceGoding closed 2 hours ago

ReeceGoding commented 2 hours ago

Pretty minor bug.

Version of the script Current dev branch.

What is the current behavior? If I set @get_all_databases = 1, then sp_QuickieStore always skips msdb. This behaviour is hard-coded by the d.database_id > 4 line here. If I instead specify @database_name = 'msdb', then I can hit it with sp_QuickieStore. This is inconsistent and leads a @get_all_databases = 1 user to the incorrect conclusion that nothing hungry has been running in msdb.

If the current behavior is a bug, please provide the steps to reproduce. See above.

What is the expected behavior? Either of the following:

  1. Explicitly forbid @database_name = 'msdb'.
  2. Change the d.database_id > 4 line to d.database_id > 3, thus allowing access to msdb.

We can argue over the "why are you enabling Query Store in msdb?" question, but that's not relevant here. Whatever opinion you hold on that, it's still inconsistent to forbid hitting it with @get_all_databases = 1 despite allowing it with @database_name = 'msdb'.

Which versions of SQL Server and which OS are affected by this issue? Did this work in previous versions of our procedures? Probably all.

IMPORTANT: If you're going to contribute code, please read the contributing guide first. https://github.com/erikdarlingdata/DarlingData/blob/main/CONTRIBUTING.md I'm happy to contribute the code, but I need Erik to pick either option 1 or 2 above.

Would you believe that I discovered this while trying to contribute code? I was too lazy to make a new databases so I was running queries against msdb so I'd have two databases of Query Store data to check with @get_all_databases = 1 .

erikdarlingdata commented 2 hours ago

I really don’t want this checking msdb for get all databases. If it won’t check for msdb when you specify it, you can make a modification there.

ReeceGoding commented 2 hours ago

@erikdarlingdata Can I ask why? I'd presume that anyone who enables Query Store on msdb had a good reason.

erikdarlingdata commented 2 hours ago

@ReeceGoding yes, it should focus on user databases. Activity worth investigating in msdb is exceedingly rare.

ReeceGoding commented 2 hours ago

@erikdarlingdata Thanks Erik. That's good enough for me.

Final question on this topic: Should we document this? The readme says that @get_all_databases

looks for query store enabled databases and returns combined results from all of them

but what we've both said here confirm that this is neither true nor the intention. What it actually does is check for all query store enabled user databases.

erikdarlingdata commented 2 hours ago

@ReeceGoding sure, it probably just needs the word “user” added, which is an easy enough change. Thanks.