-
Notifications
You must be signed in to change notification settings - Fork 654
Improve profile query performance (and most likely performance elsewhere) #10500
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
base: dev
Are you sure you want to change the base?
Conversation
…ere) Addresses NuGet#10221 (partly) fixes NuGet#5877 fixes NuGet#5659 fixes NuGet#10025
Did some informal testing Baseline: 5078 ms With signers:
With signers, updated index:
No signers:
No signers, updated index:
|
) | ||
INCLUDE( | ||
[Key] | ||
,[Copyright] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be essentially copying the table to have a new clustered index (effectively). I think this is an option, however we need to maintain this list of columns along with the Packages
table itself. This is a maintenance challenge.
I am not sure how effectively SQL Server can add a nullable column to an existing index's INCLUDE
. I know adding a nullable column (or a column with a specific default value) to a table is fast. We have done this many times. Based on some web searches it looks like we'll need to recreate the index.
So there are two concerns here:
- How do we keep this list up to data as we add columns to the
Packages
table. I think we would need remember to do aCREATE INDEX
withONLINE
andDROP_EXISTING
. We would need to write this SQL each time. If we forget, I think the query engine will stop using the index because all of the needed columns are not present. EF does not help us here. - We don't have a good way to test the performance impact of the query rebuild and the impact on other endpoints. It is non-trivial to FF this index.
I am thinking more and more that we shouldn't be using SQL for this and instead use our Search Service to populate the Profile page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this index included all columns at the time it was created, and then slipped under the radar, causing it to never be used. I am reviving it. Agree on the up to date challenges, but I have no concerns regarding the performance impact. It can only get better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, the design of the Packages table is wrong, should have had the PackageRegistrationKey as part of the clustering key from day one.
I think we're getting killed by the download count sort and the fact that package registrations can have multiple owners. I don't know how much time you want to pour into getting SQL to serve use for this query pattern. Our search service would be able to do this very quickly, e.g. (internal search endpoint, not the same as the V3 search) |
@joelverhagen I implemented random idea 2 (I think?) 😄 256 ms!
|
@joelverhagen Let me do some testing without the "problematic" index changes |
Summary of the changes (in less than 80 characters):
Addresses #10221 (partly)
fixes #5877
fixes #5659
fixes #10025