-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Temporal table migrations refactor. #32239
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maumar
commented
Nov 6, 2023
maumar
commented
Nov 7, 2023
src/EFCore.SqlServer/Design/Internal/SqlServerCSharpRuntimeAnnotationCodeGenerator.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
reviewed
Nov 7, 2023
AndriySvyryd
reviewed
Nov 7, 2023
src/EFCore.SqlServer/Migrations/Internal/SqlServerMigrationsAnnotationProvider.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
reviewed
Nov 7, 2023
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
reviewed
Nov 7, 2023
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
reviewed
Nov 7, 2023
test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
Show resolved
Hide resolved
AndriySvyryd
approved these changes
Nov 7, 2023
bricelam
approved these changes
Nov 8, 2023
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.
LGTM. Just need to test scenarios with existing migrations.
src/EFCore.SqlServer/Migrations/Internal/SqlServerMigrationsAnnotationProvider.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/Internal/SqlServerMigrationsAnnotationProvider.cs
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
maumar
force-pushed
the
temporal_migration_fixes_doneish
branch
from
November 13, 2023 08:53
6e04733
to
b4c3667
Compare
Before we used to put temporal annotations on temporal tables and all their columns, so that it's easier to process. Problem was that this would generate very noisy migrations when converting from regular table to temporal and vice versa. Every column would have an AlterColumn operation (which we would ignore during processing, but they were nonetheless generated in migration files). Also, we were using relatively simple logic to track state of our temporal tables. Some operations require temporary disabling of the versioning/period, and we need to keep track of that so that we don't try to disable period twice, or forget to enable it later. The way we did it could lead to invalid SQL in some non-trivial scenarios (e.g. converting table to temporal and adding a new column at the same time) The new approach is to only put temporal annotations on the table itself, and the period columns. Regular columns of the temporal table don't have any temporal annotations on them anymore and we reason about temporal information based on other table-based migration operation in the batch and, if need be, on the relational model. We also keep track of the actual temporal information for every operation (rather than keeping global dictionaries of period/version), so that complex migrations, involving multiple operations are more robust. To achieve that we compute the initial (temporal) state of all the tables involved in the migration. We scan all the table operations, and if some info is missing we get it from relational model. Then we do the proper processing of the migration operations - when we encounter table operation, we update the temporal information for that table (since table operations contain relevant temporal annotations). For all other operations we extract the current temporal state for the table involved, and reason based on that info. Fixes #27459 - SQL Server Migrations: Review temporal table annotations Fixes #29536 - EF Core IsTemporal() creates huge migration Fixes #29799 - EF7 SqlServer Migration is trying to update columns on History table before creating the History table if any new columns are added in the same migration
maumar
force-pushed
the
temporal_migration_fixes_doneish
branch
from
November 13, 2023 09:04
b4c3667
to
b01ea75
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before we used to put temporal annotations on temporal tables and all their columns, so that it's easier to process. Problem was that this would generate very noisy migrations when converting from regular table to temporal and vice versa. Every column would have an AlterColumn operation (which we would ignore during processing, but they were nonetheless generated in migration files). Also, we were using relatively simple logic to track state of our temporal tables. Some operations require temporary disabling of the versioning/period, and we need to keep track of that so that we don't try to disable period twice, or forget to enable it later. The way we did it could lead to invalid SQL in some non-trivial scenarios (e.g. converting table to temporal and adding a new column at the same time)
The new approach is to only put temporal annotations on the table itself, and the period columns. Regular columns of the temporal table don't have any temporal annotations on them anymore and we reason about temporal information based on other table-based migration operation in the batch and, if need be, on the relational model. We also keep track of the actual temporal information for every operation (rather than keeping global dictionaries of period/version), so that complex migrations, involving multiple operations are more robust. To achieve that we compute the initial (temporal) state of all the tables involved in the migration. We scan all the table operations, and if some info is missing we get it from relational model. Then we do the proper processing of the migration operations - when we encounter table operation, we update the temporal information for that table (since table operations contain relevant temporal annotations). For all other operations we extract the current temporal state for the table involved, and reason based on that info.
Fixes #27459 - SQL Server Migrations: Review temporal table annotations
Fixes #29536 - EF Core IsTemporal() creates huge migration
Fixes #29799 - EF7 SqlServer Migration is trying to update columns on History table before creating the History table if any new columns are added in the same migration