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

EF Core IsTemporal() creates huge migration #29536

Closed
HellHunterMax opened this issue Nov 11, 2022 · 3 comments · Fixed by #32239
Closed

EF Core IsTemporal() creates huge migration #29536

HellHunterMax opened this issue Nov 11, 2022 · 3 comments · Fixed by #32239
Assignees
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement

Comments

@HellHunterMax
Copy link

When using EF core to make a table temporal after creation of the table, EF core creates Alter column commands for each field.
This should not be necessary because Temporal is table based, not column based.
Now in the test project you can see there are not that many fields on the entity, but imagine an entity with 30 fields.
This creates a migration that is 1000+ lines of code.

In the old EF core 6 it would create a small migration, but it would recreate the same migration every time you add-migration (this has been fixed here in version 7 but introduced this "bug?")

Project to reproduce bug

EF Core version: 7.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: NET 6.0
Operating system: windows 10
IDE: Visual Studio 2022 17.4

@ajcvickers
Copy link
Member

/cc @maumar

@HellHunterMax
Copy link
Author

Not Just this. But adding properties and adding Temporal at the same time does not work.
image

@pvidal-hdr
Copy link

seeing this as well and it makes checking/validating migrations very difficult. out of curiosity, is the annotation on the columns needed? from my use, it hasn't seemed to be required, but want to confirm before i make it my standard to remove the extra annotation calls to "clean up" the migration. thanks!

maumar added a commit that referenced this issue Nov 6, 2023
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 added a commit that referenced this issue Nov 13, 2023
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 added a commit that referenced this issue Nov 13, 2023
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 added a commit that referenced this issue Nov 13, 2023
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 maumar modified the milestones: Backlog, 9.0.0 Nov 13, 2023
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 13, 2023
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants