Closed joakimriedel closed 11 months ago
/cc @bricelam @maumar
this is still a problem in the current bits. We should disable period before nullable -> non-nullable alter column operations, like we do for dropcolumn. There are also some other operations that require it, listed in the remarks linked in the bug report. Also we should test our migrations with data (at least in some cases) - this bug would have slipped through the cracks, even if we had this exact scenario covered in our tests. It only repros if there is a row with a null value for the column that is to be converted to non-nullable.
@joakimriedel do you see the exception for rename case as well, or just noting the fact that we don't disable period for the rename case (but there is no exception as is)?
@maumar the rename case was a bit different, I did not get any exception running the migration locally on mssqllocaldb , but the idempotent script blew up running on Azure SQL complaining about the history table not being renamed or something. I had to manually disable versioning to run the rename on both tables to upgrade our production servers.
@joakimriedel thanks for the info, I will look into the rename case bit deeper then. If you still have the exception message that Azure SQL gave you, can you provide it?
@maumar unfortunately that migration was a few months back and I didn't file an issue since it thought it was a temporary issue. I could try to see if I can recreate it.
@joakimriedel that would definitely help us pin point the problem. Alternatively, we could just blanket disable versioning for all the alter/rename operations, but I'd really like to avoid that, because this could cause failures in other scenarios. Temporal tables migrations is really messy because of the shared state between operations.
@maumar OK so I restored the database back to before the migration with renamed column and tried it again, which refreshed my memory.
Renaming the column does not error, in fact, it does not do anything at all.
I can call sp_rename
how many times I want, it just won't rename and it does not throw any errors. The error I got previously was due to the next step in the migration using the new column name (while the db still had the old name).
To actually make sp_rename
rename the column, versioning has to be disabled and you have to call sp_rename
on both the temporary table and the history table and then enable versioning again.
@joakimriedel thanks a lot for the extra info!
@maumar two questions, 1) will the rename case also be patched 2) will the fix be backported to 8.0 as well?
@joakimriedel rename issue looks like a bug in Azure SQL, rather than EF Core. However, I am unable to reproduce it on my end. The simple scenario of rename property on a temporal table in Azure SQL works just fine in my test environment. I was about to reach out to you, if you could help us with pin-pointing the problem. Would you be willing to provide the migration code (or the migration sql) and/or the table / EF model that the name change was applied on? Could be publicly or privately, if that works better - my email is my github alias at microsoft dot com.
When it comes to patching, unfortunately we caught the bug about 2 weeks too late. At this point we only accept critical level bugs into EF8. This issue doesn't meet the bar for patch at the moment, given that it's relatively uncommon scenario and there is a reasonable workaround. If we get more customers hitting the problem, we will reconsider patching it into 8, but for now the answer is sadly: no.
@maumar today I finally got some time to look at this again, and it seems as if it is just one particular table that won't work to rename columns on without disabling system versioning. The only thing that differs with this table and other system versioned I have in my database is that it also has a geography spatial column. I posted a report here, hopefully I can get some debugging help from the Azure SQL team.
File a bug
Altering a nullable column to not null of a temporal table generates invalid idempotent script. The script does not include necessary commands to disable system versioning and alter the history table data, which cause an error running the script due to null column values in history. (see remark)
Include your code
The migration will be generated similar to;
Generating the idempotent script would output something like;
But I would expect it to be something like;
I am also facing a similar issue when renaming a column;
instead of
even though it's marked temporal in migration code
DROPPING columns however, works perfectly and the necessary disabling of
SYSTEM_VERSIONING
gets written to the idempotent script.Exception
Trying to execute
dotnet ef database update
fails with the following exception.Include provider and version information
EF Core version: 7.0.11 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 7