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

Take min/max batch size into account again for seeding #28955

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

roji
Copy link
Member

@roji roji commented Sep 1, 2022

Made MigrationsModelDiffer.GetDataOperations pass through our normal batching logic again, by splitting CommandBatchPreparer.BatchCommands to allow the logic to be called without IUpdateEntries.

On a side note, I was surprised to see we actually have seeding-specific logic for bulk insert... Seeding isn't supposed to be high-perf, and we specifically discourage having a large number of rows (e.g. huge context snapshots), which is where bulk inserting makes the most sense... I'd consider removing it altogether, to simplify the code and avoid bugs (like this one).

Fixes #28876

@AndriySvyryd
Copy link
Member

Weren't you able to split the batches in MigrationsSqlGenerator.GenerateModificationCommands? We do want the data operations to be in one "batch" in the migration for readability

@roji
Copy link
Member Author

roji commented Sep 1, 2022

Sorry, didn't have that in mind when implementing - I agree it's better (and doesn't bake the max batch size into the scaffolded migrations).

Though note that we don't "batch" updates/deletes in the migration for readability, though we could pretty easily do the same we're doing there for inserts. Let me know if you want me to do that.

@AndriySvyryd
Copy link
Member

Though note that we don't "batch" updates/deletes in the migration for readability, though we could pretty easily do the same we're doing there for inserts. Let me know if you want me to do that.

I'm not sure about updates, since they could be very different in shape, but we should definitely do it for deletes. However, it's better to file an issue to do it later since batch deletion would be a lot less common than batch insert in seeding.

@roji
Copy link
Member Author

roji commented Sep 1, 2022

However, it's better to file an issue to do it later since batch deletion would be a lot less common than batch insert in seeding.

Not to be a troll, but every batch insert in Up means batch delete in Down :trollface:

But of course it's true that Up actually gets used a lot more than Down.

#28956

@roji roji merged commit d453d3f into dotnet:release/7.0 Sep 7, 2022
@roji roji deleted the SeedingBatches branch September 7, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seeding no longer takes max batch size into account
2 participants