Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added index on ([SemVerLevelKey], [IsLatest], [Deleted]) on Packages table #3934

Merged
merged 1 commit into from
May 10, 2017
Merged

Added index on ([SemVerLevelKey], [IsLatest], [Deleted]) on Packages table #3934

merged 1 commit into from
May 10, 2017

Conversation

agr
Copy link
Contributor

@agr agr commented May 10, 2017

The

(@p__linq__0 int)SELECT 
    [GroupBy1].[A1] AS [C1]
    FROM ( SELECT 
        COUNT(1) AS [A1]
        FROM [dbo].[Packages] AS [Extent1]
        WHERE ([Extent1].[Deleted] <> 1) AND (([Extent1].[SemVerLevelKey] = @p__linq__0) OR (([Extent1].[SemVerLevelKey] IS NULL) AND (@p__linq__0 IS NULL))) AND ([Extent1].[IsLatest] = 1)
    )  AS [GroupBy1]

query now tops as the worst performing query, following the Azure's performance recommendation, creating an index for that query.

@agr agr merged commit def1907 into dev May 10, 2017
@joelverhagen
Copy link
Member

Please make sure we add a step the deployment issue about running this migration.

@agr agr deleted the dev-agr-idx branch May 10, 2017 22:27
agr added a commit that referenced this pull request May 11, 2017
@agr
Revert "Added index on ([SemVerLevelKey], [IsLatest], [Deleted]) on P…
c4297a9 
…ackages table. (#3934)"

This reverts commit def1907.
xavierdecoster
xavierdecoster reviewed May 11, 2017
View reviewed changes
Copy link
Member

@xavierdecoster xavierdecoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✋ this PR is duplicate of #3867, which already exists in the feature/semver2 branch.

Why not cherry pick that change and make our lives easier by avoiding merge conflicts? Can you rollback this change on dev please?

xavierdecoster
xavierdecoster reviewed May 11, 2017
View reviewed changes
src/NuGetGallery/Migrations/201705102016311_AddIndexSemVerLevelKeyDeletedIsLatest.cs
public override void Up()
{
Sql($"IF NOT EXISTS(SELECT * FROM sys.indexes WHERE name = '{IndexName}' AND object_id = OBJECT_ID('Packages')) " +
$"CREATE NONCLUSTERED INDEX [{IndexName}] ON [dbo].[Packages]([SemVerLevelKey], [IsLatest], [Deleted]) ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misses WITH (ONLINE = ON). You don't want the query to time-out when creating this index on PROD, and instead want to create this index online on the db server.

If you revert this change and cherry pick from feature/semver2, you'll get this for free ;)
https://github.com/NuGet/NuGetGallery/pull/3867/files#diff-a713b5aadf42ca59c0c90263d6d72100R10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WITH(ONLINE=ON) does not work with SQL Express Local DB...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, which is why, in the past, we always removed that part after applying this migration to PROD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xavierdecoster xavierdecoster xavierdecoster left review comments

@skofman1 skofman1 skofman1 approved these changes

@joelverhagen joelverhagen joelverhagen approved these changes

@cristinamanum cristinamanum Awaiting requested review from cristinamanum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agr @joelverhagen @xavierdecoster @skofman1 @dnfclas