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 6.0 temporal table migration when altering computed column not generating correct script #27844

Closed
siby-george opened this issue Apr 20, 2022 · 6 comments · Fixed by #32398
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 type-bug

Comments

@siby-george
Copy link

File a bug

Step 1 Make table temporal
Step 2 Add migration for that
Step 3 Add new computed column to the entity
step 4 Add Migration for altered column
setp 5 Generate script for migration

Include your code

added new computed column to existing Temporal Table

entity.Property(x => x.Level).HasComputedColumnSql("Id.GetLevel() PERSISTED");

Generate migration

dotnet ef migrations add addinglevel

Generate migration script

dotnet ef migrations script 20210624210145_AddTemporalSupport 20220419235406_addinglevel
Build started...
Build succeeded.
BEGIN TRANSACTION;
GO

ALTER TABLE [HierarchyNode] ADD [Level] AS Id.GetLevel() PERSISTED;
GO

INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
VALUES (N'20220419235406_addinglevel', N'6.0.4');
GO

COMMIT;
GO

When running migration in SQL Studio

Msg 13724, Level 16, State 1, Line 1
System-versioned table schema modification failed because adding computed column while system-versioning is ON is not supported.

Expected

Migration script to turn of versioning and alter both table and turn versioning on

EF Core version: 6.0.4
Database provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer)
Target framework: (e.g. .NET 6.0)
Operating system: Windows
IDE: (e.g. Visual Studio 2022 17.2)

@omarfq
Copy link

omarfq commented Sep 23, 2022

Had a similar issue while adding a new migration after altering a column name or adding a new column.
Steps are similar to @siby-george's:

  1. Make temporal table.
  2. Add new migration for that temporal table.
  3. Alter any column name or add new one to the existing entity.
  4. Add migration for altered/added column.
  5. Generate migration script.

Code

Original entity looked like this:

public class ExonerationDetermination : AuditEntity
  {
      public long Id { get; set; }
      public string Determination { get; set; }
      public string Reason { get; set; }
      public string Comments { get; set; }
      public virtual User CreatedByUser { get; set; }
  }

After modification:

public class ExonerationDetermination : AuditEntity
  {
      public long Id { get; set; }
      public string Determination { get; set; }
      public string Reason { get; set; }
      public string AdditionalComments { get; set; } // Modified
      public virtual User CreatedByUser { get; set; }
      public string PreviousStatus { get; set; } // Added
      public string NewStatus { get; set; } // Added
  }

Generate migration with command:

dotnet ef migrations add ExonerationRequests

When building and starting the project, the following error is displayed on the output window:

Setting SYSTEM_VERSIONING to ON failed because table 'PEJC.exoneration.Determination' has 14 columns and table 'PEJC.exoneration.DeterminationHistory' has 11 columns.

The three columns that are missing from the table 'PEJC.exoneration.DeterminationHistory' are precisely the ones we added/modified. We verified this in the Down method in the migration file generated by EF Core.

Expected

Migration script should be able to turn off versioning, alter/add columns as specified on the entity, then turn versioning back on.

Additional Info

EF Core version: 6.0.9
Database provider: SQL Server
Target framework: .NET 6
Operating system: Windows
IDE: Visual Studio 2022 V17.3.4

@James-Jackson-South
Copy link

Could someone post an example of what the correct migration script should be for such a situation (Add/Remove columns)? It would be useful as a workaround example for people waiting for a fix.

@ajcvickers ajcvickers removed this from the Backlog milestone Oct 17, 2022
@ajcvickers
Copy link
Member

/cc @maumar

@ajcvickers
Copy link
Member

@James-Jackson-South The correct script in this case would need to make the table not system-versioned, add the computed column, then make it system-versioned again. However, after discussion we're not sure if this will work either. It may not be possible to make the table system-versioned if it has a computed column, and even if it is, it's not clear how that computed column would work when looking at historical data. What is the computed column definition that you want to use?

@ajcvickers
Copy link
Member

EF Team Triage: Closing this issue as the requested additional details have not been provided and we have been unable to reproduce it.

BTW this is a canned response and may have info or details that do not directly apply to this particular issue. While we'd like to spend the time to uniquely address every incoming issue, we get a lot traffic on the EF projects and that is not practical. To ensure we maximize the time we have to work on fixing bugs, implementing new features, etc. we use canned responses for common triage decisions.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2022
@ajcvickers ajcvickers reopened this Nov 2, 2022
@ajcvickers ajcvickers added this to the Backlog milestone Nov 2, 2022
@James-Jackson-South
Copy link

@James-Jackson-South The correct script in this case would need to make the table not system-versioned, add the computed column, then make it system-versioned again. However, after discussion we're not sure if this will work either. It may not be possible to make the table system-versioned if it has a computed column, and even if it is, it's not clear how that computed column would work when looking at historical data. What is the computed column definition that you want to use?

Sorry @ajcvickers Just saw this. I wasn't using a computed column in my case but changing the column type. The workaround you suggested is the approach I eventually figured out, but it took me a rather long while to do so.

maumar added a commit that referenced this issue Nov 23, 2023
…mputed column not generating correct script

Working with computed column on a temporal table requires special processing - we need to disable the versioning, add the computed column to regular table and a column with the same name/type/position to the history table, but as a regular column.
After all is done we re-enable versioning. Historical information on the computed column will be copied to the counterpart column in history table without issue.

One thing that we can't do is modifying computed column SQL when table is temporal. What happens in case of CCSQL modification, we drop the column and create a new one with new CCSQL. This is not an issue for regular table, because the value is computed anyway, so old value is useless. But when we drop-create column, that column gets created at the last position in the table, and so the table no longer matches it's history table exactly (positions of columns may differ). We would have to drop+recreate column on a history table, but that results in losing historical data of that column. And that is valuable data, unlike for regular table. We could enable that scenario once/if we support table rebuilds.

Fixes #27844
@maumar maumar modified the milestones: Backlog, 9.0.0 Nov 23, 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 23, 2023
maumar added a commit that referenced this issue Nov 23, 2023
…mputed column not generating correct script

Working with computed column on a temporal table requires special processing - we need to disable the versioning, add the computed column to regular table and a column with the same name/type/position to the history table, but as a regular column.
After all is done we re-enable versioning. Historical information on the computed column will be copied to the counterpart column in history table without issue.

One thing that we can't do is modifying computed column SQL when table is temporal. What happens in case of CCSQL modification, we drop the column and create a new one with new CCSQL. This is not an issue for regular table, because the value is computed anyway, so old value is useless. But when we drop-create column, that column gets created at the last position in the table, and so the table no longer matches it's history table exactly (positions of columns may differ). We would have to drop+recreate column on a history table, but that results in losing historical data of that column. And that is valuable data, unlike for regular table. We could enable that scenario once/if we support table rebuilds.

Fixes #27844
maumar added a commit that referenced this issue Nov 28, 2023
…mputed column not generating correct script

Working with computed column on a temporal table requires special processing - we need to disable the versioning, add the computed column to regular table and a column with the same name/type/position to the history table, but as a regular column.
After all is done we re-enable versioning. Historical information on the computed column will be copied to the counterpart column in history table without issue.

One thing that we can't do is modifying computed column SQL when table is temporal. What happens in case of CCSQL modification, we drop the column and create a new one with new CCSQL. This is not an issue for regular table, because the value is computed anyway, so old value is useless. But when we drop-create column, that column gets created at the last position in the table, and so the table no longer matches it's history table exactly (positions of columns may differ). We would have to drop+recreate column on a history table, but that results in losing historical data of that column. And that is valuable data, unlike for regular table. We could enable that scenario once/if we support table rebuilds.

Fixes #27844
maumar added a commit that referenced this issue Nov 29, 2023
…mputed column not generating correct script (#32398)

Working with computed column on a temporal table requires special processing - we need to disable the versioning, add the computed column to regular table and a column with the same name/type/position to the history table, but as a regular column.
After all is done we re-enable versioning. Historical information on the computed column will be copied to the counterpart column in history table without issue.

One thing that we can't do is modifying computed column SQL when table is temporal. What happens in case of CCSQL modification, we drop the column and create a new one with new CCSQL. This is not an issue for regular table, because the value is computed anyway, so old value is useless. But when we drop-create column, that column gets created at the last position in the table, and so the table no longer matches it's history table exactly (positions of columns may differ). We would have to drop+recreate column on a history table, but that results in losing historical data of that column. And that is valuable data, unlike for regular table. We could enable that scenario once/if we support table rebuilds.

Fixes #27844
@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 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 type-bug
Projects
None yet
5 participants