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

Temporal table migration regression from EF Core 8 to 9 #35108

Closed
Moerup opened this issue Nov 14, 2024 · 14 comments · Fixed by #35283 or #35289
Closed

Temporal table migration regression from EF Core 8 to 9 #35108

Moerup opened this issue Nov 14, 2024 · 14 comments · Fixed by #35283 or #35289
Assignees
Labels
area-migrations area-sqlserver area-temporal-tables closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression
Milestone

Comments

@Moerup
Copy link

Moerup commented Nov 14, 2024

Migration regression from EF Core 8 to 9

I'm in the process of upgrading an application from .NET 8 to 9.

But I'm running into problems when trying to run existing migrations that have been created with EF Core 8 and working for over a year now on multiple versions of EF Core (8.0.4 to 8.0.11)

I have a migration that throws exceptions and can't complete at all on v9.0.0, but if I revert to 8.0.11 it works fine.
It's a series of multiple pretty complex migrations and changes over time, so creating a small repro will take some time, so I hope that's not necessary. Let me know if it is or you need more information!

Include your code

I have Temporal tables enabled for a model: "TestObjective" like this:

public void Configure(EntityTypeBuilder<TestObjective> builder)
{
    builder.ToTable(nameof(MasterCatalogueDbContext.TestObjectives), schema: DatabaseConstants.TestCatalogueSchemaName);

    // Enable history
    builder.Metadata.SetIsTemporal(true);

   // Removed relations, property configurations and Includes for brevity.
}

At some point we renamed a relations table from "TestObjectivesProjectRoles" to "TestObjectivesOwners".
All this is auto generated with dotnet-ef migrations add
But the migration generated to alter the temporal columns:

migrationBuilder.AlterColumn<DateTime>(
    name: "PeriodStart",
    schema: "testCatalogue",
    table: "TestObjectivesOwners",
    type: "datetime2",
    nullable: false,
    oldClrType: typeof(DateTime),
    oldType: "datetime2")
    .Annotation("SqlServer:IsTemporal", true)
    .Annotation("SqlServer:TemporalHistoryTableName", "TestObjectivesOwnersHistory")
    .Annotation("SqlServer:TemporalHistoryTableSchema", "testCatalogue")
    .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart")
    .OldAnnotation("SqlServer:IsTemporal", true)
    .OldAnnotation("SqlServer:TemporalHistoryTableName", "TestObjectivesProjectRolesHistory")
    .OldAnnotation("SqlServer:TemporalHistoryTableSchema", "testCatalogue")
    .OldAnnotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
    .OldAnnotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

Results in this error:

2024-11-14T15:07:43 [15:07:43 INF] Applying migration '20240102112555_RenameTestCatalogueOwnerTables'.
2024-11-14T15:07:44 [15:07:44 ERR] Failed executing DbCommand (22ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
2024-11-14T15:07:44 DECLARE @var4 sysname;
2024-11-14T15:07:44 SELECT @var4 = [d].[name]
2024-11-14T15:07:44 FROM [sys].[default_constraints] [d]
2024-11-14T15:07:44 INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
2024-11-14T15:07:44 WHERE ([d].[parent_object_id] = OBJECT_ID(N'[testCatalogue].[TestObjectivesOwners]') AND [c].[name] = N'PeriodStart');
2024-11-14T15:07:44 IF @var4 IS NOT NULL EXEC(N'ALTER TABLE [testCatalogue].[TestObjectivesOwners] DROP CONSTRAINT [' + @var4 + '];');
2024-11-14T15:07:44 ALTER TABLE [testCatalogue].[TestObjectivesOwners] ALTER COLUMN [PeriodStart] datetime2 GENERATED ALWAYS AS ROW START HIDDEN NOT NULL;
2024-11-14T15:07:44 [15:07:44 ERR] Failed to migrate MasterCatalogue database!
2024-11-14T15:07:44 Microsoft.Data.SqlClient.SqlException (0x80131904): Period column 'PeriodStart' in a system-versioned temporal table cannot be altered.
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean isAsync, Int32 timeout, Boolean asyncWrite)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
2024-11-14T15:07:44    at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.Execute(IReadOnlyList`1 migrationCommands, IRelationalConnection connection, MigrationExecutionState executionState, Boolean beginTransaction, Boolean commitTransaction, Nullable`1 isolationLevel)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.<>c.<ExecuteNonQuery>b__3_1(DbContext _, ValueTuple`6 s)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IReadOnlyList`1 migrationCommands, IRelationalConnection connection, MigrationExecutionState executionState, Boolean commitTransaction, Nullable`1 isolationLevel)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.MigrateImplementation(DbContext context, String targetMigration, MigrationExecutionState state, Boolean useTransaction)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.<>c.<Migrate>b__20_1(DbContext c, ValueTuple`4 s)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
2024-11-14T15:07:44    at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.Migrate(DatabaseFacade databaseFacade)
2024-11-14T15:07:44    at RTDT.MasterCatalogue.Web.Program.MigrateAndSeed(WebApplication app) in C:\GitVestas\RTDT\src\Services\RTDT.MasterCatalogue\src\RTDT.MasterCatalogue.Web\Program.cs:line 171
2024-11-14T15:07:44 ClientConnectionId:2bd97670-656a-4168-b8b1-d65d6137ead2
2024-11-14T15:07:44 Error Number:13599,State:1,Class:16

Include provider and version information

EF Core version: 9.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 9.0
Operating system: Windows & Linux (Local & CICD)
IDE: Visual Studio Enterprise 2022 17.12.0

@bhp15973
Copy link

I have the same problem.
In our CICD we are always recreating db, so we execute always all migrations. I have seen that the difference is near PeriodStart/PeriodEnd
GENERATED ALWAYS AS ROW START HIDDEN is added in EF Core 9.
Our error says: cannot create generated always column when system_time period is not defined.

@roji roji changed the title Migration regression from EF Core 8 to 9 Temporal table migration regression from EF Core 8 to 9 Nov 15, 2024
@Moerup
Copy link
Author

Moerup commented Nov 18, 2024

@roji I can see you have changed the title to only indicate that it's temporal table related, but the second issue in my post does not seem related at all to temporal tables, but rowversion and it's default value.

Maybe I'm wrong tho and they are both related to temporal, just wanted to make sure that part doesn't get overlooked.

@roji
Copy link
Member

roji commented Nov 18, 2024

@Moerup I indeed missed the 2nd issue - but it would be better to have that as a separate issue, as it indeed seems unrelated.

@danroot
Copy link

danroot commented Nov 19, 2024

I get same error as @bhp15973
The generated migration is:
ALTER TABLE [InsuranceQuotes] ADD [PeriodEnd] datetime2 GENERATED ALWAYS AS ROW END HIDDEN NOT NULL DEFAULT '9999-12-31T23:59:59.9999999';

Resulting in this error from SQL:
Cannot create generated always column when SYSTEM_TIME period is not defined.

As far as I can tell this is a standard migration of a non-temporal table to a temporal table. This migration works in efcore 8.

I believe the issue is the ADD COLUMN and PERIOD FOR SYSTEM_TIME need to all happen at the same time:

ALTER TABLE dbo.YourTable
ADD SysStartTime datetime2 GENERATED ALWAYS AS ROW START NOT NULL,
SysEndTime datetime2 GENERATED ALWAYS AS ROW END NOT NULL,
PERIOD FOR SYSTEM_TIME (SysStartTime, SysEndTime);

in EF8 "GENERATED ALWAYS" is not specified.

@danroot
Copy link

danroot commented Nov 21, 2024

I have created a repo that presents a minimal reproduction of what I think is the same or similar issue as OP. @Moerup let me know if this is a different issue and I will create a new issue. This is a blocking issue for my team adopting EFCore 9, which we really want to do!

https://github.com/danroot/Spike.EFCore9TemporalTable

Steps to reproduce with this repo:

  • be sure the connection string is correct in OnConfiguring. Note the database will be dropped in the test.
  • run the test
  • observe test fails with "Cannot create generated always column when SYSTEM_TIME period is not defined"

Steps to reproduce from scratch:

  • Create a EF Core 8 db context
  • Add an initial migration
  • Update the tables to use temporal tables in OnModelCreating of the db context: modelBuilder.Entity().ToTable("Contacts", b => b.IsTemporal());
  • Add a migration for the db context
  • Update the Microsoft.EntityFrameworkCore.* nuget packages to to EF Core 9
  • Run the migration by calling context.Database.Migrate or using dotnet ef database update

@WolfspiritM
Copy link

WolfspiritM commented Nov 22, 2024

We're also having the exact same problem with a migration that converts a non temporal table into a temporal table.
"Cannot create generated always column when SYSTEM_TIME period is not defined"

As we apply migrations from code when developing, this causes issues if there is no database available yet.

The problem is that Aspire 9 is based on EFCore 9 so we can't upgrade Aspire until this is fixed but Aspire only supports the last version so until this is fixed we're using a version of Aspire that is out of support. So we hope this gets fixed soon.

@danroot
Copy link

danroot commented Nov 22, 2024

@WolfspiritM You can use Aspire with EFCore 8. In our main solution, because of this regression, we have Aspire running an ASP.NET Core 9 project using EFCore 8. The main thing I think might not work is health checks against the ef context. If you want a containerized SQL in development:

var sql = builder.AddSqlServer("sql")
    .WithLifetime(ContainerLifetime.Persistent)
    .AddDatabase("SomeProjectDevDb");

var api = builder.AddProject<Projects.SomeProject_Api>("someproject-api")
    .WithReference(sql,"DefaultConnection") // <--- makes ConnectionStrings__DefaultConnection get injected into config of api project

If you don't want containerized SQL Server, just inject the connectionstring:

var sql = builder.AddConnectionString("DefaultConnection");
var api = builder.AddProject<Projects.SomeProject_Api>("someproject-api")
    .WithReference(sql,"DefaultConnection");

In the api, the implication is that instead of this:

builder.AddSqlServerDbContext<MyDbContext>("myConnection");

We just continue to do this, same as always:

builder.Services.AddDbContext<MyDbContext>();

Of course we do want to get to EFCore 9, but hopefully this makes it less of a block.

In our case, the impact is that we can't run integration tests under EFCore 9. We have tests that create a blank database by running migrations, create a snapshot, then test and rollback snapshot after each test. In this scenario, the migrations to create the database fail. We also have code to let developers get a 'fresh' dev database from scratch. Again, the migrations fail in EFCore9, but not 8.

A nuclear option that might work depending on the use of the migrations is to reset migrations.

@WolfspiritM
Copy link

@WolfspiritM You can use Aspire with EFCore 8.

Thanks. My main issue was that the Aspire components for sqlserver were using efcore 9 packages.
But thanks to discord I figured out that Aspire 9 can be used with net8 and uses Efcore 8 packages then.
I forgot that EfCore is part of the "framework" 😄

So I will give Aspire 9 with net8 a try now until this issue is resolved.

@rhahmed1122
Copy link

getting same issue on migrating EF Core 8 to 9.
It's a potential blocker for my team to migrate from efcore 8 to 9.

we have some temporal tables and after updating efcore from 8 to 9. we are unable to update database through migration and getting following error.

Cannot create generated always column when SYSTEM_TIME period is not defined.

@GordonBeeming
Copy link

I'm running into the same issue at the moment, also moving from 8 to 9. Similar error messages to others

Column 'SysStartTime' in table 'MyDatabase.MySchema.MyTable' cannot be specified as 'GENERATED ALWAYS' in ALTER COLUMN statement.

maumar added a commit that referenced this issue Dec 6, 2024
In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables.
This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling.
Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model.
The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate.
Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.

Also fixes #35148 which was the same underlying issue.
@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 Dec 6, 2024
maumar added a commit that referenced this issue Dec 6, 2024
In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables.
This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling.
Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model.
The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate.
Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.

Fixes #35108
Also fixes #35148 which was the same underlying issue.
@maumar
Copy link
Contributor

maumar commented Dec 6, 2024

thanks @danroot for the great repros! It was very helpful and fix is on the way. Several people are hitting this so we will try to fit this into the next available patch release (likely 9.0.2)

In the meantime, here is the workaround. It's a bit of work and may not be realistic if affected migrations are large enough, but hopefully will unblock some folks.

For the affected migration code (that work on temporal tables or convert from regular to temporal) look for migrationBuilder.AlterColumn, and migrationBuilder.CreateColumn that contain these annotations (should be pretty much all of the operations):

.Annotation("SqlServer:IsTemporal", true)
.Annotation("SqlServer:TemporalHistoryTableName", "HistoryTable")
.Annotation("SqlServer:TemporalHistoryTableSchema", null)
.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

For AlterColumnOperation, remove all those annotations (as well as .OldAnnotations entries for those 5 values)

.OldAnnotation("SqlServer:IsTemporal", true)
.OldAnnotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
.OldAnnotation("SqlServer:TemporalHistoryTableSchema", null)
.OldAnnotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
.OldAnnotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

After removing those annotations, if there are no other changes in that operation (i.e. or of the "old" properties have the same value as the new ones) - remove the migrationBuilder.AlterColumn code entirely for that column.

For CreateColumn, also remove those 5 annotations, but additionally for the period start column add:

.Annotation("SqlServer:TemporalIsPeriodStartColumn", true)

and for period end column add:

.Annotation("SqlServer:TemporalIsPeriodEndColumn", true)

If the migration creates a temporal table right away (rather than converting regular table into temporal), you also need to fix annotations on the CreateTable columns:

this is how the operation should look like initially:

migrationBuilder.CreateTable(
	name: "Customers",
	columns: table => new
	{
		Id = table.Column<int>(type: "int", nullable: false)
			.Annotation("SqlServer:Identity", "1, 1")
			.Annotation("SqlServer:IsTemporal", true)
			.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
			.Annotation("SqlServer:TemporalHistoryTableSchema", null)
			.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
			.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart"),
		Name = table.Column<string>(type: "nvarchar(max)", nullable: false)
			.Annotation("SqlServer:IsTemporal", true)
			.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
			.Annotation("SqlServer:TemporalHistoryTableSchema", null)
			.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
			.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart"),
		PeriodEnd = table.Column<DateTime>(type: "datetime2", nullable: false)
			.Annotation("SqlServer:IsTemporal", true)
			.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
			.Annotation("SqlServer:TemporalHistoryTableSchema", null)
			.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
			.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart"),
		PeriodStart = table.Column<DateTime>(type: "datetime2", nullable: false)
			.Annotation("SqlServer:IsTemporal", true)
			.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
			.Annotation("SqlServer:TemporalHistoryTableSchema", null)
			.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
			.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart")
	},
	constraints: table =>
	{
		table.PrimaryKey("PK_Customers", x => x.Id);
	})
	.Annotation("SqlServer:IsTemporal", true)
	.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
	.Annotation("SqlServer:TemporalHistoryTableSchema", null!)
	.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
	.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

this is what it should be changed to:

migrationBuilder.CreateTable(
	name: "Customers",
	columns: table => new
	{
		Id = table.Column<int>(type: "int", nullable: false)
			.Annotation("SqlServer:Identity", "1, 1"),
		
		Name = table.Column<string>(type: "nvarchar(max)", nullable: false),
		
		PeriodEnd = table.Column<DateTime>(type: "datetime2", nullable: false)
			.Annotation("SqlServer:TemporalIsPeriodEndColumn", true),
		
		PeriodStart = table.Column<DateTime>(type: "datetime2", nullable: false)
			.Annotation("SqlServer:TemporalIsPeriodStartColumn", true)
	},
	constraints: table =>
	{
		table.PrimaryKey("PK_Customers", x => x.Id);
	})
	.Annotation("SqlServer:IsTemporal", true)
	.Annotation("SqlServer:TemporalHistoryTableName", "CustomersHistory")
	.Annotation("SqlServer:TemporalHistoryTableSchema", null!)
	.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
	.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

do not remove annotations on the CreateTable operation itself, just on the columns

Also note that period start and end columns each have an annotation added just like migrationBuilder.CreateColumn mentioned before.

@maumar
Copy link
Contributor

maumar commented Dec 6, 2024

as an example, Up method from @danroot 's repro migration in the file 20241121163049_Temporal.cs should look like this:

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.AlterTable(
                name: "Persons")
                .Annotation("SqlServer:IsTemporal", true)
                .Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
                .Annotation("SqlServer:TemporalHistoryTableSchema", null)
                .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            migrationBuilder.AlterTable(
                name: "Contacts")
                .Annotation("SqlServer:IsTemporal", true)
                .Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
                .Annotation("SqlServer:TemporalHistoryTableSchema", null)
                .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<string>(
            //    name: "LastName",
            //    table: "Persons",
            //    type: "nvarchar(100)",
            //    maxLength: 100,
            //    nullable: false,
            //    oldClrType: typeof(string),
            //    oldType: "nvarchar(100)",
            //    oldMaxLength: 100)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<string>(
            //    name: "FirstName",
            //    table: "Persons",
            //    type: "nvarchar(100)",
            //    maxLength: 100,
            //    nullable: false,
            //    oldClrType: typeof(string),
            //    oldType: "nvarchar(100)",
            //    oldMaxLength: 100)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<int>(
            //    name: "Id",
            //    table: "Persons",
            //    type: "int",
            //    nullable: false,
            //    oldClrType: typeof(int),
            //    oldType: "int")
            //    .Annotation("SqlServer:Identity", "1, 1")
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart")
            //    .OldAnnotation("SqlServer:Identity", "1, 1");

            migrationBuilder.AddColumn<DateTime>(
                name: "PeriodEnd",
                table: "Persons",
                type: "datetime2",
                nullable: false,
                defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
                //.Annotation("SqlServer:IsTemporal", true)
                //.Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
                //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
                //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
                .Annotation("SqlServer:TemporalIsPeriodEndColumn", true);

            migrationBuilder.AddColumn<DateTime>(
                name: "PeriodStart",
                table: "Persons",
                type: "datetime2",
                nullable: false,
                defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
                //.Annotation("SqlServer:IsTemporal", true)
                //.Annotation("SqlServer:TemporalHistoryTableName", "PersonsHistory")
                //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
                //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
                .Annotation("SqlServer:TemporalIsPeriodStartColumn", true);

            //migrationBuilder.AlterColumn<string>(
            //    name: "Phone",
            //    table: "Contacts",
            //    type: "nvarchar(20)",
            //    maxLength: 20,
            //    nullable: false,
            //    oldClrType: typeof(string),
            //    oldType: "nvarchar(20)",
            //    oldMaxLength: 20)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<int>(
            //    name: "PersonId",
            //    table: "Contacts",
            //    type: "int",
            //    nullable: true,
            //    oldClrType: typeof(int),
            //    oldType: "int",
            //    oldNullable: true)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<string>(
            //    name: "Name",
            //    table: "Contacts",
            //    type: "nvarchar(100)",
            //    maxLength: 100,
            //    nullable: false,
            //    oldClrType: typeof(string),
            //    oldType: "nvarchar(100)",
            //    oldMaxLength: 100)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<string>(
            //    name: "Email",
            //    table: "Contacts",
            //    type: "nvarchar(100)",
            //    maxLength: 100,
            //    nullable: false,
            //    oldClrType: typeof(string),
            //    oldType: "nvarchar(100)",
            //    oldMaxLength: 100)
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

            //migrationBuilder.AlterColumn<int>(
            //    name: "Id",
            //    table: "Contacts",
            //    type: "int",
            //    nullable: false,
            //    oldClrType: typeof(int),
            //    oldType: "int")
            //    .Annotation("SqlServer:Identity", "1, 1")
            //    //.Annotation("SqlServer:IsTemporal", true)
            //    //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
            //    //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
            //    //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
            //    //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart")
            //    .OldAnnotation("SqlServer:Identity", "1, 1");

            migrationBuilder.AddColumn<DateTime>(
                name: "PeriodEnd",
                table: "Contacts",
                type: "datetime2",
                nullable: false,
                defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
                //.Annotation("SqlServer:IsTemporal", true)
                //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
                //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
                //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
                .Annotation("SqlServer:TemporalIsPeriodEndColumn", true);

            migrationBuilder.AddColumn<DateTime>(
                name: "PeriodStart",
                table: "Contacts",
                type: "datetime2",
                nullable: false,
                defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified))
                //.Annotation("SqlServer:IsTemporal", true)
                //.Annotation("SqlServer:TemporalHistoryTableName", "ContactsHistory")
                //.Annotation("SqlServer:TemporalHistoryTableSchema", null)
                //.Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                //.Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");
                .Annotation("SqlServer:TemporalIsPeriodEndColumn", true);
        }

maumar added a commit that referenced this issue Dec 7, 2024
In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables.
This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling.
Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model.
The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate.
Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.

Fixes #35108
Also fixes #35148 which was the same underlying issue.
@maumar maumar closed this as completed in 8a918fc Dec 7, 2024
@maumar
Copy link
Contributor

maumar commented Dec 7, 2024

reopening for potential patch

maumar added a commit that referenced this issue Dec 7, 2024
In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables. This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling. Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model. The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate. Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.
maumar added a commit that referenced this issue Dec 10, 2024
… 9 (#35289)

In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables. This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling. Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model. The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate. Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.
@maumar maumar added this to the 9.0.2 milestone Dec 10, 2024
@maumar
Copy link
Contributor

maumar commented Dec 10, 2024

fixed for 9.0.2, closing

@maumar maumar closed this as completed Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment