-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sp_QuickieStore: Add the ability to sort by arbitrary orders, but particularly wait stats and "plan count by hashes". #459
sp_QuickieStore: Add the ability to sort by arbitrary orders, but particularly wait stats and "plan count by hashes". #459
Conversation
…hes per query hash
… is not used, and broke ties in new sort order.
@ReeceGoding thanks for this. I’m on vacation until the end of July, so please take your time with testing etc. I won’t be able to look at anything until then. |
@erikdarlingdata I've not tested what I'm mentioning in this comment yet, but I think that we've both screwed up by not updating the list of tables to truncate. I've missed |
Had some time to think about it. I'll put the wait stats bit in this PR as well. It needs the same architecture and doing two at once gives me a good excuse to make sure that it's tidy and easy to extend. |
27050b0
to
3e00059
Compare
I've added a wait stats part on. I intended to add in a sort order for total wait time, but it was so easy to add in What I find most remarkable about the sort orders that I've added today is that it just worked. I spent only a few minutes removing silly syntax errors and then the code simply worked. It's a good hint that the architecture that I've added in has done its job and we can now add whatever I'll test more, of course. Afterwards, I'll add on the promised sort order for total wait time. If my bragging in the previous paragraph is honest, then it will be very easy. |
… line in qsrs output's ORDER BY.
Added on an Regarding the question of if the 'total waits' sort order is useful, I've found it handy because it reveals big waits that I'd never think to specifically filter for. It's good when a tool tells you about problems you didn't know you had. The only downside is that the
It was even easy than I thought! It took less than half an hour and I don't recall needing to think at any point. I'm completely convinced that the architecture for adding arbitrary sort orders will be useful, particularly for #448. |
a7c1ac0
to
a4ffb42
Compare
…why this is needed.
a4ffb42
to
236737a
Compare
da82087
to
ff8273c
Compare
@erikdarlingdata I think it all works and my testing is done. |
@ReeceGoding I noticed you were still tweaking this a little today -- do you have anything left to do or is this good to merge into dev? I'd like to test it with the other changes combined. |
@erikdarlingdata I consider it finished. |
This commit is a small monster, but I was stunned by the results. It lets you very quickly find the query hashes that are generating the biggest number of distinct plans. This simultaneously tells you which ad-hoc queries are the most worth worrying about and it also tells you what queries (even non-ad-hoc stuff such as stored procedures) have changed their plans a lot. Even if you think the code is ugly (it is), run it at least once before dismissing it. I think it's going to be very useful. Erik's idea of using the hashes was a very good one.
The design of
sp_QuickieStore
makes it difficult to add new sort orders that don't come fromsys.query_store_runtime_stats
. This is the source of most of the ugliness in this PR and it required me adding to the existing architecture. However, I think that I may have solved the problem of adding arbitrary sort orders tosp_QuickieStore
. I suspect that this may be immediately helpful for #448 , but I don't know that yet.This closes most of #446 , but I still need to figure out how to sort by wait stats. It might be best if I used this branch as a base for that, but I don't know yet. Don't wait on merging this just for the sake of the wait stats stuff. It will probably be easier to do the wait stats stuff after this is merged.