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

Don't use Min/Max batch size values when generating migrations #12466

Closed
julielerman opened this issue Jun 25, 2018 · 8 comments · Fixed by #27903
Closed

Don't use Min/Max batch size values when generating migrations #12466

julielerman opened this issue Jun 25, 2018 · 8 comments · Fixed by #27903
Labels
area-migrations-seeding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-6.0 type-bug
Milestone

Comments

@julielerman
Copy link

I've noticed that sometimes when I'm inserting multiple entities of the same type with hasdata they will each get their own insertdata command. Other times I've seen it use one insertdata command and list multiple items in the objects parameter. I haven't been able to track down the pattern for what results in one or the other.

Here is a screenshot of a migration wher eyou can see that brewertypes are combined into one insertdata command but down below there are two separate commands for inserting 2 Units (the 2nd one is chopped off in the screenshot).
image

One difference between BrewerType and Unit is that BrewerType owns an another type via OwnsOne --which is supplying some of the values for each of those rows. Is that enough to make the difference?

Figured I would ask rather than continue to try and try.

Thanks!

Further technical details

EF Core version: 2.1.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2017 15.7.3

@smitpatel
Copy link
Member

I have seen this in past. AFAIK, the behavior is coming from MinBatchSize value. If the number of InsertData operations is higher than MinBatchSize then we get single operation, else they are put into individual operations.

@julielerman
Copy link
Author

that does make sense but given that (AFAIK) the default batching minimum is 4 but I've seen it at 3 (like above) and sometimes at 2 even though above the 2 were separated... I'm not sure if that's a solid pattern. It's not a huge deal. I just wanted to be able to explain it if someone asks because while I'm working on my 2.1 course I'm seeing this randomness. Thanks :)

@AndriySvyryd
Copy link
Member

The generated migration operations don't depend on the configured Min/Max batch size. If you could show an example where the data has been separated into two I could state the reason more accurately, but usually it's because the affected columns are different, i.e. one row uses the default value for a column while the other one specifies an explicit value.

@AndriySvyryd AndriySvyryd added this to the Discussions milestone Jun 28, 2018
@divega
Copy link
Contributor

divega commented Sep 10, 2018

Bringing back to triage. Here is a repro that shows that MinBatchSize actually affects whether the InsertData calls generated are separated or a single call.

You can simply create a project with the code blow and add a migration to see the effect.

I added TWEAK comments in places that can be modified to change the outcome.

Demos.DataSeeding.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <RootNamespace>Demos</RootNamespace>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="2.2.0-*" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="2.2.0-*" />
    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.2.0-*" />
  </ItemGroup>

</Project>

Program.cs:

using System;
using System.Drawing;
using Microsoft.EntityFrameworkCore;

namespace Demos
{
    public class Program
    {
        private static void Main()
        {
            using (var db = new BloggingContext())
            {
                foreach (var theme in db.Themes)
                {
                    Console.WriteLine(
                        $"Id = {theme.ThemeId}, Name = {theme.Name}, Color = {theme.TitleColor}");
                }
            }

            Console.Read();
        }
    }

    public class Blog
    {
        public int BlogId { get; set; }
        public string BlogUrl { get; set; }
        public Theme Theme { get; set; }
    }

    public class Theme
    {
        public int ThemeId { get; set; }
        public string Name { get; set; }
        public string TitleColor { get; set; }
    }

    public class BloggingContext : DbContext
    {
        public DbSet<Blog> Blogs { get; set; }
        public DbSet<Theme> Themes { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder
                .UseSqlServer(
                    @"Server=(localdb)\mssqllocaldb;Database=Demo.DataSeeding;Trusted_Connection=True;ConnectRetryCount=0",
                    options => options.MinBatchSize(4)); // TWEAK: try values other than the default
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            // Seed data
            modelBuilder
                .Entity<Theme>()
                .HasData( //TWEAK: removing two entities with default MinBatchSize causes split InsertData
                    new Theme { ThemeId = 1, Name = "MSDN", TitleColor = Color.Red.Name },
                    new Theme { ThemeId = 2, Name = "TechNet", TitleColor = Color.DarkCyan.Name },
                    new Theme { ThemeId = 3, Name = "Docs", TitleColor = Color.FloralWhite.Name },
                    new Theme { ThemeId = 4, Name = "VS Developer Community", TitleColor = Color.LightBlue.Name },
                    new Theme { ThemeId = 5, Name = "Personal", TitleColor = Color.LightGreen.Name });
        }
    }
}

@divega divega removed this from the Discussions milestone Sep 10, 2018
@divega
Copy link
Contributor

divega commented Sep 10, 2018

IMO it would be nice of the batch size wasn't considered and the InsertData call was generated always as a single call when various operations of the same type happen on the same type. E.g.:

            migrationBuilder.InsertData(
                table: "Themes",
                columns: new[] { "ThemeId", "Name", "TitleColor" },
                values: new object[,]
                {
                    { 1, "MSDN", "Red" },
                    { 4, "VS Developer Community", "LightBlue" },
                    { 5, "Personal", "LightGreen" }
                });

Rather than separate:

            migrationBuilder.InsertData(
                table: "Themes",
                columns: new[] { "ThemeId", "Name", "TitleColor" },
                values: new object[] { 1, "MSDN", "Red" });

            migrationBuilder.InsertData(
                table: "Themes",
                columns: new[] { "ThemeId", "Name", "TitleColor" },
                values: new object[] { 4, "VS Developer Community", "LightBlue" });

            migrationBuilder.InsertData(
                table: "Themes",
                columns: new[] { "ThemeId", "Name", "TitleColor" },
                values: new object[] { 5, "Personal", "LightGreen" });

@divega
Copy link
Contributor

divega commented Sep 10, 2018

Another thing that can be improved is the generation of DeleteData. It is shame because this one in particular could be very concise. It seems we always generate separate calls, but I didn't investigate deeply if the reason is similar. E.g. removing 4 of the seeded entities in a subsequent migration produces code like this:

            migrationBuilder.DeleteData(
                table: "Themes",
                keyColumn: "ThemeId",
                keyValue: 1);

            migrationBuilder.DeleteData(
                table: "Themes",
                keyColumn: "ThemeId",
                keyValue: 2);

            migrationBuilder.DeleteData(
                table: "Themes",
                keyColumn: "ThemeId",
                keyValue: 3);

            migrationBuilder.DeleteData(
                table: "Themes",
                keyColumn: "ThemeId",
                keyValue: 4);

@ajcvickers
Copy link
Member

Triage: putting this on the backlog to consider decoupling update-pipeline batching from generated code in the migration.

@ajcvickers ajcvickers added this to the Backlog milestone Sep 10, 2018
@bricelam bricelam removed their assignment Oct 12, 2018
@AndriySvyryd AndriySvyryd changed the title HasData sometimes combines like objects in InsertData and sometimes doesn't Don't use Min/Max batch size values when generating migrations Aug 22, 2019
@bricelam bricelam self-assigned this Nov 4, 2019
@AndriySvyryd AndriySvyryd self-assigned this Mar 20, 2020
@bricelam bricelam removed their assignment Aug 24, 2020
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Sep 1, 2020

This requires either #22063 or #20664

@AndriySvyryd AndriySvyryd modified the milestones: 6.0.0, Backlog Sep 15, 2021
@ajcvickers ajcvickers modified the milestones: Backlog, 7.0.0 Nov 10, 2021
@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 7.0.0 Apr 26, 2022
AndriySvyryd added a commit that referenced this issue Apr 28, 2022
Use row-based diffing for data seeding

Fixes #22063
Fixes #12466
Fixes #27575
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Apr 29, 2022
@AndriySvyryd AndriySvyryd removed their assignment Apr 29, 2022
AndriySvyryd added a commit that referenced this issue Apr 29, 2022
Use row-based diffing for data seeding

Part of #3170
Fixes #22063
Fixes #12466
Fixes #27575
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview5 May 4, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview5, 7.0.0 Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations-seeding closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-6.0 type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants