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

Migrations: Table rebuilds #329

Open
bricelam opened this Issue Jun 17, 2014 · 48 comments

Comments

Projects
None yet
@bricelam
Member

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

This comment has been minimized.

Show comment
Hide comment
@ErikEJ

ErikEJ Jun 18, 2014

Contributor

Yes, great RDBMS

Contributor

ErikEJ commented Jun 18, 2014

Yes, great RDBMS

@bricelam

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Jun 18, 2015

Member

Re-opening. I've regressed this.

Member

bricelam commented Jun 18, 2015

Re-opening. I've regressed this.

@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

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Jul 7, 2015

Member

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.

Member

natemcmaster commented Jul 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@ErikEJ

ErikEJ Jul 7, 2015

Contributor

You are entering a World of Pain 😃

Contributor

ErikEJ commented Jul 7, 2015

You are entering a World of Pain 😃

@natemcmaster

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Jul 8, 2015

Member

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

Member

natemcmaster commented Jul 8, 2015

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

@bricelam

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Jul 10, 2015

Member

@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.

Member

bricelam commented Jul 10, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Jul 10, 2015

Member

Making this a pri1 issue given the new information.

Member

bricelam commented Jul 10, 2015

Making this a pri1 issue given the new information.

@natemcmaster

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Jul 10, 2015

Member

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.

Member

natemcmaster commented Jul 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Jul 10, 2015

Member

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)

Member

bricelam commented Jul 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Jul 14, 2015

Member

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.

Member

natemcmaster commented Jul 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Jul 14, 2015

Member

@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.

Member

bricelam commented Jul 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Aug 17, 2017

Member

@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?

Member

bricelam commented Aug 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@leak

leak Aug 17, 2017

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.

leak commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Aug 17, 2017

Member

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:

  • GroupBy translation
  • Lazy loading
  • Update model from database
  • Stored procedure mapping
  • Improved view mapping
  • Data seeding

...just to name a few.

Member

bricelam commented Aug 17, 2017

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:

  • GroupBy translation
  • Lazy loading
  • Update model from database
  • Stored procedure mapping
  • Improved view mapping
  • Data seeding

...just to name a few.

@ErikEJ

This comment has been minimized.

Show comment
Hide comment
@ErikEJ

ErikEJ Aug 18, 2017

Contributor

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

Contributor

ErikEJ commented Aug 18, 2017

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

@leak

This comment has been minimized.

Show comment
Hide comment
@leak

leak Aug 18, 2017

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

leak commented Aug 18, 2017

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

@ErikEJ

This comment has been minimized.

Show comment
Hide comment
@ErikEJ

ErikEJ Aug 18, 2017

Contributor

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

Contributor

ErikEJ commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Sep 3, 2017

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).

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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Sep 26, 2017

Member

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

Member

bricelam commented Sep 26, 2017

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

@leak

This comment has been minimized.

Show comment
Hide comment
@leak

leak Nov 3, 2017

So I gave this a whirl and from a rough bit of testing it seems to work.
leak@f442290

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.

leak commented Nov 3, 2017

So I gave this a whirl and from a rough bit of testing it seems to work.
leak@f442290

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

This comment has been minimized.

Show comment
Hide comment
@dazinator

dazinator Jan 4, 2018

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

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

@milasch

This comment has been minimized.

Show comment
Hide comment
@milasch

milasch Jan 4, 2018

@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...

milasch commented Jan 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@dazinator

dazinator Jan 4, 2018

@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.

dazinator commented Jan 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@WilliamABradley

This comment has been minimized.

Show comment
Hide comment
@WilliamABradley

WilliamABradley Feb 4, 2018

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

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

@leak

This comment has been minimized.

Show comment
Hide comment
@leak

leak Feb 4, 2018

@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.

leak commented Feb 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Feb 5, 2018

Member

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...

Member

bricelam commented Feb 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Feb 5, 2018

Member

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...

Member

bricelam commented Feb 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@nbarbettini

nbarbettini Apr 19, 2018

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.

nbarbettini commented Apr 19, 2018

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

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers May 24, 2018

Member

Make sure to test case in #11832

Member

ajcvickers commented May 24, 2018

Make sure to test case in #11832

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jun 4, 2018

Member

Note: consider case described in #12196

Member

ajcvickers commented Jun 4, 2018

Note: consider case described in #12196

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment