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

Sqlite Migrations: Table rebuilds #329

Closed
bricelam opened this issue Jun 17, 2014 · 101 comments · Fixed by #21575
Closed

Sqlite Migrations: Table rebuilds #329

bricelam opened this issue Jun 17, 2014 · 101 comments · Fixed by #21575
Assignees
Labels
area-migrations closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@bricelam
Copy link
Contributor

bricelam commented Jun 17, 2014

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
Copy link
Contributor

ErikEJ commented Jun 18, 2014

Yes, great RDBMS

@bricelam bricelam assigned ghost Jun 18, 2014
@ghost ghost modified the milestones: 1.0.0-alpha3, 1.0.0-alpha4 Aug 20, 2014
@ghost ghost added 3 - Done and removed type-enhancement labels Sep 12, 2014
@ghost ghost removed their assignment Sep 12, 2014
@bricelam
Copy link
Contributor Author

bricelam commented Jun 18, 2015

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

@bricelam bricelam reopened this Jun 18, 2015
@bricelam bricelam removed the 2 - Done label Jun 18, 2015
@bricelam bricelam removed this from the 7.0.0-alpha4 milestone Jun 18, 2015
@rowanmiller rowanmiller added this to the 7.0.0 milestone Jun 26, 2015
@natemcmaster
Copy link
Contributor

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
Copy link
Contributor

ErikEJ commented Jul 7, 2015

You are entering a World of Pain 😃

@natemcmaster
Copy link
Contributor

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

@bricelam
Copy link
Contributor Author

@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
Copy link
Contributor Author

Making this a pri1 issue given the new information.

@bricelam
Copy link
Contributor Author

bricelam commented Feb 4, 2020

Nothing really new here, just jotting down some notes since I started thinking about this again today:

// UNDONE: Not supported by SQLite
//migrationBuilder.AlterColumn<string>(
//    name: "Title",
//    table: "Posts",
//    nullable: false,
//    defaultValue: "",
//    oldClrType: typeof(string),
//    oldType: "TEXT",
//    oldNullable: true);

// Create a new table with the desired schema
migrationBuilder.CreateTable(
    name: "ef_temp_Posts",
    columns: table => new
    {
        Id = table.Column<int>(nullable: false)
            .Annotation("Sqlite:Autoincrement", true),
        Title = table.Column<string>(nullable: false)
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Posts", x => x.Id);
    });

// Create temporary UNIQUE indexes to enforce during INSERT
migrationBuilder.CreateIndex(
    name: "ef_temp_IX_Posts_Title",
    table: "ef_temp_Posts",
    column: "Title",
    unique: true);

// Copy data from the old table. Use IFNULL to specify a default value for newly
// required columns
migrationBuilder.Sql(@"
    INSERT INTO ef_temp_Posts (Id, Title)
    SELECT Id, IFNULL(Title, '')
    FROM Posts;
");

// Suspend foreign key enforcement during the swap
// NB: This commits the current transaction. We can't rollback the migration if
// anything after this fails. We can mitigate it by doing rebuilds as late as
// possible
migrationBuilder.Sql("PRAGMA foreign_keys = 0;", suppressTransaction: true);

// Swap in the new table
migrationBuilder.DropTable(
    name: "Posts");
migrationBuilder.RenameTable(
    name: "ef_temp_Posts",
    newName: "Posts");

// NB: This will cause an integrity check which may fail if foreign keys were
// not previously enforced and inconsistent data has crept in
migrationBuilder.Sql("PRAGMA foreign_keys = 1;", suppressTransaction: true);

// Drop temporary UNIQUE indexes
migrationBuilder.DropIndex(
    name: "ef_temp_IX_Posts_Title",
    table: "Posts",
    column: "Title");

// Rebuild any indexes
migrationBuilder.CreateIndex(
    name: "IX_Posts_Title",
    table: "Posts",
    column: "Title",
    unique: true);

@bricelam
Copy link
Contributor Author

bricelam commented Mar 4, 2020

I looked into using PRAGMA defer_foreign_keys to avoid suppressTransaction: true, but dropping the table sill cascade-deletes dependents.

We discussed the consequences of suppressTransaction: true in a design meeting today and decided that it is OK to do this by default on SQLite. We did this for certain scenarios in EF6 on SQL Server and never heard any negative feedback. An option to disable it (and thus table rebuilds) would, however, be nice to have.

We also discussed blindly turning foreign keys back on (when we don't know if they were enabled in the first place) and decided that it's OK. If you turn them off for performance reasons, your data should still be consistent otherwise additional things in EF will break. The default foreign key behavior will just be reset when the connection is closed and re-opened after migrating anyway.

Other points that came up in the discussion:

  • We should warn when we do a rebuild that any changes you've made outside of your EF model may be lost. (But this shouldn't really be an issue if you're using migraitons to manage your database schema)
  • There's no value in re-using parts of the query or update pipeline to generate the INSERT INTO..SELECT statements. Just use the usual mechanism for generating SQL in migrations
  • No need to support table rebuilds on SQL Server as part of this. Re-openeing SqlServer Migrations: Rebuild column when IDENTITY changes #2100 for doing column rebuilds when IDENTITY changes

@danielmeza
Copy link

From the SQLiteStudio there is a short sintax to clone the original table

CREATE TABLE sqlitestudio_temp_table AS SELECT *
                                          FROM Table;

this way we can reduce the complexity.

@bricelam bricelam changed the title Migrations: Table rebuilds Sqlite Migrations: Table rebuilds Apr 7, 2020
@ir2000
Copy link

ir2000 commented Apr 29, 2020

@danielmeza you would lose primary key and foreign keys

A table created using CREATE TABLE AS has no PRIMARY KEY and no constraints of any kind. The default value of each column is NULL. The default collation sequence for each column of the new table is BINARY.
https://www.sqlite.org/lang_createtable.html#createtabas

@InteXX
Copy link

InteXX commented Apr 29, 2020

@ir2000 @danielmeza

Yes, in that single statement data is lost. But it's a valuable technique for cloning the schema.

One could then clear out the data and repopulate, bringing all keys along this time.

@danielmeza
Copy link

@ir2000 yes but we not need that info in the temp table.

@danielmeza
Copy link

@bricelam we should cover in this version escenarios with an existing db? Example: code first from db escenarios.

@Damus765
Copy link

Damus765 commented Apr 30, 2020

@danielmeza I would say we need to create every constraint in the "temp table". Because what you call "temp table" is not really temporary. It is in fact the new table that will be kept once renamed. So I'm not sure CREATE TABLE AS is the best way to create the new table.

@danielmeza
Copy link

@Damus765 I was missing the rename process, I thought that the data will be copy to the new table so the process goes like this. Create the temp table -> delete the old table -> create the new table with the new schema -> copy data back from the temp table -> delete the temp table.

@Damus765
Copy link

Damus765 commented Apr 30, 2020

@danielmeza According to the SQLite documentation, the recommended steps are as follows:

  1. Create new table
  2. Copy data from old table into new table
  3. Drop old table
  4. Rename new into old

Only one data copy is required and you don't need a temp table.

@bricelam
Copy link
Contributor Author

@danielmeza Our strategy for using migrations with existing databases is tracked by #2167

@bricelam
Copy link
Contributor Author

bricelam commented May 29, 2020

More related issues for SQL Server: #12586, #16758, #18965 & #20943

@bricelam bricelam added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 9, 2020
@bricelam bricelam modified the milestones: 5.0.0, 5.0.0-preview8 Jul 16, 2020
@bricelam
Copy link
Contributor Author

bricelam commented Jul 16, 2020

it's been 84 years...

But seriously, my six-year-old wasn't even born when I filed this issue. Crazy.

May all your rebuilding dreams come true! Check out our daily builds if you're eager to give it a try. Let me know if you find any bugs. (I still have a bit more testing to do myself.)

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. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.