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.

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

Add support for DATA_COMPRESSION and SORT_IN_TEMPDB SQL Server index options #30831

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

hmajerus
Copy link
Contributor

@hmajerus hmajerus commented May 5, 2023

Closes #30408

Please check whether the PR fulfills these requirements

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a (loosely) detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@hmajerus
Copy link
Contributor Author

hmajerus commented May 5, 2023

@dotnet-policy-service agree company="Buildertrend"

@hmajerus
Copy link
Contributor Author

@ajcvickers and/or @bricelam Sorry for the ping, but let me know if there is anything I can provide or do to make this PR more manageable to review. I have the time to account for any recommendations you may have.

Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

This design looks good to me.

I want @AndriySvyryd to review the metadata changes.

You're also missing the logic to put this in the migrations model snapshot. Without it, you'll keep getting drop-creates every migration. I think that logic lives here:

protected override MethodCallCodeFragment? GenerateFluentApi(IIndex index, IAnnotation annotation)
=> annotation.Name switch
{
SqlServerAnnotationNames.Clustered => (bool)annotation.Value! == false
? new MethodCallCodeFragment(IndexIsClusteredMethodInfo, false)
: new MethodCallCodeFragment(IndexIsClusteredMethodInfo),
SqlServerAnnotationNames.Include => new MethodCallCodeFragment(IndexIncludePropertiesMethodInfo, annotation.Value),
SqlServerAnnotationNames.FillFactor => new MethodCallCodeFragment(IndexHasFillFactorMethodInfo, annotation.Value),
_ => null
};

@bricelam bricelam requested a review from AndriySvyryd August 24, 2023 16:58
@bricelam bricelam changed the base branch from main to release/8.0 August 24, 2023 16:58
@hmajerus
Copy link
Contributor Author

Cool, I'm busy here at the end of the week, but will get to fixing that next week.

@bricelam
Copy link
Contributor

No rush, sorry it took us so long to get to

…erIndexBuilderExtensions. Add tests to verify snapshot for IsSortedInTempDb, UseDataCompression, and FillFactor.
@hmajerus
Copy link
Contributor Author

Updated to use SqlServerIndexBuilderExtensions for DataCompression and SortInTempDb in snapshots.

Want to point out that without that update the snapshots would still contain something that looks like this:

b.HasIndex("Id").HasAnnotation("SqlServer:DataCompression", DataCompressionType.Row);

Which will prevent those drop-creates. This is currently the case for the IsCreatedOnline Index option.

This update makes the snapshot look like the following, which I imagine is preferred to the one above anyway.

SqlServerIndexBuilderExtensions.UseDataCompression(b.HasIndex("Id"), DataCompressionType.Row);

@bricelam bricelam merged commit be31c61 into dotnet:release/8.0 Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server Index options SortInTempDB and DataCompression
4 participants