dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.66k stars 3.16k forks source link

Sqlite Migrations: Table rebuilds #329

Closed bricelam closed 4 years ago

bricelam commented 10 years ago

In SQLite, the following operations will require rebuilding the table - https://docs.microsoft.com/en-us/ef/core/providers/sqlite/limitations.

Other databases also have a smaller set of limitations that require a table rebuild (i.e. changing Identity on a column in SQL Server).

ErikEJ commented 10 years ago

Yes, great RDBMS

bricelam commented 9 years ago

Re-opening. I've "regressed" this. (not sure if it ever actually worked since there weren't any tests)

natemcmaster commented 9 years ago

I've discovered a limitation that makes me wonder if the complication of rebuilds will exceed their value.

The limitation PRAGMA foreign_keys=on/off; does not work within a transaction.

Consider the following setup:

Table "Parent" - no FK's Table "Children" - references "Parent" via a FK

  1. Scenario 1 - renaming "Parent" to "NewTableName". If the alter table statement executes with PRAGMA foreign_keys=ON;, then SQLite will handle updating the "Children" reference automatically. Otherwise, it will break the FK relationship.
  2. Scenario 2 - rebuilding "Parent" ( to imitate drop column or some other operation not natively available). This must be executed with PRAGMA foreign_keys=OFF; otherwise the rebuild looks to SQLite like Scenario 1 and "Children" will end up reference a table which is deleted in the rebuild.

Scenario 1 and 2 could be resolved by just executing the proper PRAGMA foreign_keys before renaming/rebuilding if not for this limitation:

This pragma is a no-op within a transaction; foreign key constraint enforcement may only be enabled or disabled when there is no pending BEGIN or SAVEPOINT.

https://www.sqlite.org/pragma.html#pragma_foreign_keys

This means we cannot execute an entire migration within one transaction, but need to break SQLite migrations into multiple transactions. Reverting to the beginning of a migration becomes impossible.

ErikEJ commented 9 years ago

You are entering a World of Pain :smiley:

natemcmaster commented 9 years ago

Moving to backlog to work on high-priority items. Consider again after #1141 and #2558.

bricelam commented 9 years ago

@natemcmaster Did you try the approach recommended in the SQLite documentation: Making Other Kinds Of Table Schema Changes? Triggers will be lost. Views may be broken. Indexes (like columns) would be re-created from the model snapshot. But otherwise it looks like it'll work.

Note, we won't be able to use the simpler and faster procedure since SQLite doesn't support variables and Migrations is designed in a way that lets you generate SQL scripts.

bricelam commented 9 years ago

Making this a pri1 issue given the new information.

natemcmaster commented 9 years ago

The rebuilds in https://github.com/natemcmaster/EntityFramework/tree/table-rebuild follow a similar pattern. But yes, they can be improved to incorporate the steps mentioned in SQLite. This will require a migration that uses the ADO layer since we can't perform all of those steps in DDL only.

bricelam commented 9 years ago

Which part's can't be done? As I see it, the steps for us are:

(Turn off foreign keys during migrations #2592) (Migrations begins a transaction)

  1. Create new table as "new_X"
  2. Transfer content from X into new_X
  3. Drop the old table X
  4. Change the name of new_X to X
  5. Reconstruct indexes
  6. (Optional) Run PRAGMA foreign_key_check;

(Migrations commits transaction)

natemcmaster commented 9 years ago

We also need to recreate triggers and views. We would need to detect by querying 'sqlite_master' and storing the results for recreation. This may require altering the definition of the view to reflect the new table structure.

bricelam commented 9 years ago

@natemcmaster I think we're ok saying, "Anything you created outside of EF, you're responsible for maintaining." The same is more-or-less true today.

bricelam commented 9 years ago

Another scenario to consider when implementing this is changing IDENTITY on SQL Server columns. (See #2100)

10Dev commented 8 years ago

I think you are stuck in a "missing the forest for the trees" situation here. Windows 10 UWP should be a "big deal" for Microsoft yet the only EF Data Provider is for SQLite that completely misses out on the EF "Magic Unicorn" of Code First Migrations for any non-trivial data persistance on UWP. You should have seen this coming down the tracks 3 years ago and now in 2016 a solution for UWP should be the utmost priority.

The outside world sees just Microsoft, not your internal divisions, so you need to clobber some managers on the head to get ESENT on board, or port SQL Server CE to UWP or start at team to donate fixes to SQLite or point a dedicated team at the awkward "make new table on every migration" solution or whatever gets EF in full glory mode working on UWP.

And it is hard to think "roadmap" on this issue as something should have been in place by the Windows 10 launch and at this point it is just crazy there is no working EF UWP solution.

10Dev commented 8 years ago

I am hoping EF on UWP can become "real" in a rapid timeframe. I was excited to grab the nightly and experience the developer friendlyness of EF only to see it crash and burn when the CodeFirst entities become non-trivial. Then I poked around here and in the docs to discover this insane Catch-22 situation.

So taking my previous comments and translating it into an action list for you to consider from simple to perhaps impossible:

  1. Update the documentation again with a more obvious and realistic notice something like "Do not attempt to use EF with the SQLite provider for anything other than classroom demos and the BUILD conference. Real world scenarios will rapidly have you screaming at the EF Team and that will make us sad"
  2. Change the label on this Issue from "enhancement" to "bug of the century" and the highest priority and actually assign it to someone. It only looks like an "enhancement" if you see SQLite as the problem instead of seeing it from a customer perspective where the lack of a full EF solution on UWP is the problem.
  3. Give bricelam a suitable yeam of great developers and tell him to go crazy on this.
  4. investigate the SQL Server team to see if anyone feels competant enough to to the Async thing on CE or whatever embedable chunk of SQL Server goodness they can dream up for UWP.
  5. Find out if anyone at Microsoft maintains ESENT and if you can get them to pitch in with whoever is doing the Azure Document DB provider.
  6. Get somebody to test various DB's to see if you can find another alternate to SQLite. The BrightstarDB project looks like it might have some UWP potential for example.
  7. Throw money at the SQLite organization or start up a Microsoft team to fix their DB.
  8. Make up some posters and park yourselves outside Satya's office to point out that "Microsoft's recommended data access technology" needs more resources and separate management from ASP.NET to prioritize resources across all of the Microsoft platforms so that UWP is not just a minor afterthought on ASP.NET
10Dev commented 8 years ago

Rowan has provided excellent feeback on this issue in #4423 and based on that, my wacky solution #7 seems like the most technically feasible approach. With the almost bizarre situation of SQLite now being embedded into Windows 10, the idea of Microsoft donating large amounts of cash and/or other resources to sqlite.org to clean up the mess listed in bricelam's first post on this issue would seem like the ideal alternative to the ugly black hole quagmire that #329 represents.

Nothing is worse than data loss and this smells like a seed candidate for random impossible to duplicate edge cases of disaster.

But not having a workable UWP EF solution is also leading to the same mess. I don't know about you, but as a developer I poke around looking at user reviews in Windows Store Apps that would be likely to use data such as todo, schedules, etc. It's only anecdotal, but there seems to be an alarming number of users complaining about data loss and my theory on that (which could be tital hogwash of course) is that without EF on UWP (and also missing in action on Windows 8 as well) that most developers have defaulted to object serialaztion of various sorts to plain old files on a disk which as any experienced developer knows is an eventual recipe for data loss. There is a reason we put up with the hassile of ORM to a RDB to get reliable, journalled, transactional data storage. There is no better technology to do this (IMO) than EF. EF missing on UWP = sad face for developers, and possibly horrific disaster stories for users.

If anyone could make #329 work it would have to be the EF team, but that possibly herculean level of effort might just be better spent of fixing SQLite instead.

10Dev commented 8 years ago

Any update on this?

The "enhancement" tag still sits there looking very strange.

The EF Sqlite provider does not work in any real world migration scenario.

The proposed solution is a whole bunch of code that is NOT required for any other database provider and hence seems like some sort of enhancement. But the enhancement is to SQLite really, since it doesn't apply to other databases.

From the point of view of someone using EF, this is a plain and simple bug.

From a bigger picture, Microsoft only supports SQLite on UWP becomes a meta-bug if EF does not work right.

Is there any possible group inside whoever manages UWP who can be tasked with this effort? Or it can come out of their budget perhaps. Or maybe I'm picking the wrong box to try and think outside of?

Is Core 1.1 possible?

rowanmiller commented 8 years ago

@10Dev this isn't currently on our list for 1.1 as there are other higher priority features we want to get thru first.

flashfearless commented 7 years ago

Personally, I would think that MS would find it inexcusable to supply EF framework migrations without supporting this functionality. What is the delay in getting this supported?

johnnildo commented 7 years ago

@flashfearless Well at least it is in 2.0's backlog. :)

johnnildo commented 7 years ago

So I'm trying to add a migration that, by hand, deletes a column on a master table. I created an example of what I'm trying to accomplish by creating 2 model classes:

    class Master
    {
        public int Id { get; set; }
        public string ToBeDeleted { get; set;}
    }

    class Detail
    {
        public int Id { get; set; }
        public Master Master { get; set; }
    }

I generate the first migration, add data to master and detail. I then delete the ToBeDeleted property then generate the second migration, which creates the infamous migrationBuilder.DropColumn call that isn't supported.

So I replaced that generated script with the manually written one:

        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.Sql("PRAGMA foreign_keys=\"0\"");
            migrationBuilder.CreateTable(
                name: "NEW_Masters",
                columns: table => new
                {
                    Id = table.Column<int>(nullable: false)
                        .Annotation("Sqlite:Autoincrement", true),
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_Masters", x => x.Id);
                });
            migrationBuilder.Sql("INSERT INTO NEW_Masters SELECT Id FROM Masters;");
            migrationBuilder.DropTable("Masters");
            migrationBuilder.RenameTable("NEW_Masters", newName: "Masters");
            migrationBuilder.Sql("PRAGMA foreign_keys=\"1\"");
        }

The problem is that this throws an exception:

An unhandled exception of type 'Microsoft.Data.Sqlite.SqliteException' occurred in Microsoft.EntityFrameworkCore.Relational.dll Additional information: SQLite Error 19: 'FOREIGN KEY constraint failed'.

When I inspect the generated SQL using Migration-Script, it creates SQL that works if I run it manually under SQLiteBrowser, so the exception must be some foreign key constraint check performed by EF.

If I could get some help on a workaround for dropping columns, I would greatly appreciate it.

bricelam commented 7 years ago

@milasch Could you submit a new issue so we can investigate? That all looks correct to me. I wonder if EF is sending a PRAGMA foreign_keys = 1; when it shouldn't. It might also have something to do running in a transaction.

bricelam commented 7 years ago

Ah yep, as Nate said above:

PRAGMA foreign_keys=on/off; does not work within a transaction.

There is a way to suppress the transaction for an operation, but it's only exposed on the Sql method. We may need to put together some guidance on how to do this. (cc @rowanmiller)

johnnildo commented 7 years ago

@bricelam No point in submitting a new issue then, right? I'll try to see how I can suppress the transaction.

johnnildo commented 7 years ago

@bricelam The following worked:

    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.CreateTable(
            name: "NEW_Masters",
            columns: table => new
            {
                Id = table.Column<int>(nullable: false)
                    .Annotation("Sqlite:Autoincrement", true),
            },
            constraints: table =>
            {
                table.PrimaryKey("PK_Masters", x => x.Id);
            });
        migrationBuilder.Sql("INSERT INTO NEW_Masters SELECT Id FROM Masters;");
        migrationBuilder.Sql("PRAGMA foreign_keys=\"0\"", true);
        migrationBuilder.Sql("DROP TABLE Masters", true);
        migrationBuilder.Sql("ALTER TABLE NEW_Masters RENAME TO Masters", true);
        migrationBuilder.Sql("PRAGMA foreign_keys=\"1\"", true);
    }

Thanks for your help!

bricelam commented 7 years ago

A cheaper alternative to supporting DropForeignKey, RenameColumn, and some uses of AlterColumn on SQLite would be to use PRAGMA writable_schema. (See #7969)

kierenj commented 7 years ago

I just got this (System.InvalidOperationException: 'To change the IDENTITY property of a column, the column needs to be dropped and recreated.') when using SQL Server and trying to make an int Id into a ValueGeneratedOnAdd. Is this not a bug, rather than an enhancement? I got no warnings, the migration generated just fine. But it's invalid?

bricelam commented 7 years ago

The migration reflects the desired schema changes. When the migration is applied, it is translated to the appropriate DDL. Applying this schema change requires a table (or in this case, column) rebuild. When this is implemented, the same generated migration will start working because we'll be able to generate the appropriate DDL.

The line is always blurry between bugs and enhancements. As a rule of thumb, bugs tend to be isolated limitations while features are broader and typically involve a significant amount of work. The important thing is that we have an issue tracking the scenario.

leak commented 7 years ago

It is really disappointing that such an essential feature as simply renaming a column via migrations is still unsupported in 2.0

bricelam commented 7 years ago

@leak Could you be hitting #9344? Renaming columns in Migrations is supported in all providers (e.g. SQL Server, PostgreSQL, etc.) except SQLite which doesn't provide native support for it. Have you considered submitting a feature request to SQLite?

leak commented 7 years ago

Well I am hitting most of the things below here: https://github.com/aspnet/EntityFrameworkCore/blob/dev/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs#L150

Talking to the SQLite guys seems pointless, they have been evasive about that topic for years now. (see their mailing list archives)

I'm just wondering if SQLite is so unpopular in conjunction with EF, or does it have something to do with exactly those unsupported operations? Am I the only one ever changing table layouts..? I guess not, but why doesn't this issue here doesn't get more traction then? Questions over questions..

The SQLite guys outlined what needs to be done for doing proper table rebuilds here: https://sqlite.org/lang_altertable.html

It would really be nice to see a lot of these NotSupportedExceptions disappear and the SQLite EF provider catching up with the other providers.

bricelam commented 7 years ago

I agree this is a critical feature for SQLite. I too would like to see it done as soon as possible. Unfortunately, with limited resources, it comes down to a matter of priorities, and we still have some pretty big priorities on our backlog:

...just to name a few.

ErikEJ commented 7 years ago

I you need an embedded database on Windows desktop, you could use the SQLCE provider

leak commented 7 years ago

@ErikEJ Last time i checked that provider had similar limitations, e.g. not being able to rename columns, or did that change?

ErikEJ commented 7 years ago

Yes, rename column is the only Migration limitation in the SQL CE provider, compared to 10 limitations in the SQLite provider - you can work around that with AddColumn and DropColumn

https://docs.microsoft.com/en-us/ef/core/providers/sqlite/limitations

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/wiki/Limitations

julielerman commented 7 years ago

Yuck, just hit this today when trying to run a migration that had renamecolumn in it. The renamecolumn method was added by dotnet ef migrations add somechange but can't actually run this migration (rename column not supported).

bricelam commented 6 years ago

One less papercut: With #9905, SQLite will now rebuild renamed indexes.

leak commented 6 years ago

So I gave this a whirl and from a rough bit of testing it seems to work. https://github.com/leak/EntityFrameworkCore/commit/f442290cf8ee6d8130e7cf777a2500179af1b8fa

The transaction/PRAGMA issue discussed above turned out to be a non-issue: new SqlOperation { Sql = "PRAGMA foreign_keys=OFF;", SuppressTransaction = true } seems to do the trick.

I'm not very familiar with EF code and especially the dependency injection part seems very backwards to me (why not just pass the IoC container around and just let every class grab what it needs..?). Also it is based on the rel/2.0.0 tag rather than current HEAD.

dazinator commented 6 years ago

@bricelam - perhaps worth updating the docs here to reflect rename index support: https://docs.microsoft.com/en-us/ef/core/providers/sqlite/limitations

johnnildo commented 6 years ago

@leak Although I don't understand the use of dependency injection in EF core, passing an IoC container around characterizes a service locator pattern, deemed by many an anti-pattern. But we digress...

dazinator commented 6 years ago

@bricelam - perhaps worth updating the docs here to reflect rename index support: https://docs.microsoft.com/en-us/ef/core/providers/sqlite/limitations

Have submitted a PR for the docs now.

leak commented 6 years ago

@milasch Sure, maybe it is just me, but classes like these creep me out big time... https://github.com/aspnet/EntityFrameworkCore/blob/dev/src/EFCore.Relational/Migrations/HistoryRepositoryDependencies.cs

WilliamABradley commented 6 years ago

@leak If this fixes all of the issues with SQLite migrations, why don't you submit a PR for it?

leak commented 6 years ago

@WilliamABradley I have not received any feedback if this code is even pull-request worthy. Additionally I do not know if this pull request is subject to a CLA signature, which I'm not willing to give.

bricelam commented 6 years ago

I have not received any feedback if this code is even pull-request worthy.

You'll need to sign a CLA before we can review it...

bricelam commented 6 years ago

Also, anyone actually interested in submitting a PR shouldn't look at your code since they must agree they...

...have sole ownership of the intellectual property rights...

nbarbettini commented 6 years ago

Adding for consideration: I use the dotnet new mvc template (and SQLite) in my open-source ASP.NET Core book. This limitation means that some messy workarounds are required.

ajcvickers commented 6 years ago

Make sure to test case in #11832

ajcvickers commented 6 years ago

Note: consider case described in #12196

bricelam commented 6 years ago

Hey cool, SQLite 3.25.0 supports renaming columns. Filed #13341 to implement it on our side.

julielerman commented 5 years ago

Before I dig too deeply I want to see if this is related. This is something I did not experience in 2.0 but am in 2.1.4. I have an existing migration. Then I add in a many to many entity between two entities and add the key mapping for that join entity. When I attempt to add a new migration, it fails. If I delete all of the migrations, I'm able to create a migration with the new mapping. So I believe the attempt of the migration API to create a change constraints is triggering the problem. Is that related to this issue already?