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

Disable foreign keys during `alter_table` for sqlite3 adapter #32865

Merged
merged 1 commit into from May 22, 2018

Conversation

Projects
None yet
6 participants
@yahonda
Contributor

yahonda commented May 10, 2018

Summary

Unlike other databases, changing SQLite3 table definitions need to create a temporary table.
While changing table operations, the original table needs dropped which caused
SQLite3::ConstraintException: FOREIGN KEY constraint failed if the table is referenced by foreign keys. This pull request disables foreign keys by disable_referential_integrity.

Also disable_referential_integrity method needs to execute defer_foreign_keys = ON
to defer re-enabling foreign keys until the transaction is committed.

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

Fixes #31988

Other Information

  • This pull request has been validated with sqlite3 3.8.2 on Ubuntu 14.04 LTS and sqlite3 3.22.0 on Ubuntu 18.04 LTS.
$ sqlite3 --version
3.8.2 2013-12-06 14:53:30 27392118af4c38c5203a04b8013e1afdb1cebd0d
$ sqlite3 --version
3.22.0 2018-01-22 18:45:57 0c55d179733b46d8d0ba4d88e01a25e10677046ee3da1d5b1581e86726f2alt1

This feature has been introduced since
SQLite Release 3.8.0 On 2013-08-26 . Also according to git blame at cloned SQLite repository, defer_foreign_keys pragma has been implemented
since version-3.8.0 mackyle/sqlite@8ebb3ba

  • .reset_column_information added to address ActiveModel::UnknownAttributeError
Error:
ActiveRecord::Migration::ForeignKeyChangeColumnTest#test_change_column_of_parent_table:
ActiveModel::UnknownAttributeError: unknown attribute 'name' for ActiveRecord::Migration::ForeignKeyChangeColumnTest::Post.
@rails-bot

This comment has been minimized.

rails-bot commented May 10, 2018

r? @georgeclaghorn

(@rails-bot has picked a reviewer for you, use r? to override)

@yahonda

This comment has been minimized.

Contributor

yahonda commented May 10, 2018

I have validated this pull request works with SQLite version 3.8.2 on Ubuntu 14.04.5 LTS (GNU/Linux 4.4.0-31-generic x86_64)

Then executed PRAGMA defer_foreign_keys = ON; manually from sqlite3 command line tool. It looks like this command is a no-op for SQLite 3.8.2 which does not support defer_foreign_keys.

$ sqlite3
SQLite version 3.8.2 2013-12-06 14:53:30
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> PRAGMA defer_foreign_keys;
0
sqlite> PRAGMA defer_foreign_keys = ON;
sqlite> PRAGMA defer_foreign_keys = 0;
sqlite> .quit
$
@saiqulhaq

This comment has been minimized.

saiqulhaq commented May 14, 2018

what will happened if sqlite3 is below 3.8.2 version?

@yahonda

This comment has been minimized.

Contributor

yahonda commented May 15, 2018

This pull request works with SQLite 3.8.2. I also have created a SQL script file.

$ sqlite3 --version
3.8.2 2013-12-06 14:53:30 27392118af4c38c5203a04b8013e1afdb1cebd0d
$ curl https://raw.githubusercontent.com/yahonda/diag32865/master/01_foreign_keys_and_defer_foreign_keys.sql | sqlite3
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1530  100  1530    0     0  41991      0 --:--:-- --:--:-- --:--:-- 42500
1
0
1|mypost
$
@yahonda

This comment has been minimized.

Contributor

yahonda commented May 15, 2018

According to git blame at cloned sqlite repository, defer_foreign_keys pragma has been implemented
since version-3.8.0 mackyle/sqlite@8ebb3ba

@yahonda

This comment has been minimized.

Contributor

yahonda commented May 15, 2018

At first, I misunderstood that defer_foreign_keys feature has been supported since SQLite Release 3.12.0, which is wrong. This feature has been implemented and documented since 3.8.0. 3.12.0 added support for disables RESTRICT actions on foreign key.

"https://www.sqlite.org/releaselog/3_12_0.html

The PRAGMA defer_foreign_keys=ON statement now also disables RESTRICT actions on foreign key.

https://www.sqlite.org/releaselog/3_8_0.html

Added the defer_foreign_keys pragma and the sqlite3_db_status(db, SQLITE_DBSTATUS_DEFERRED_FKS,...) C-language interface.

Thanks.

@yahonda

This comment has been minimized.

Contributor

yahonda commented May 17, 2018

I have made a proposal at Rails core mailing list to bump the minimum version of SQLite to 3.8 for Rails 6 https://groups.google.com/forum/#!topic/rubyonrails-core/OoHD65yjkvk

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented May 17, 2018

Yes. Let's raise the minimal version for Rails 6.

@yahonda

This comment has been minimized.

Contributor

yahonda commented May 17, 2018

Thanks for the suggestion. I have opened #32923 to bump the minimum version of SQLite to 3.8 for Rails 6.

yahonda added a commit to yahonda/rails that referenced this pull request May 21, 2018

Bump minimum SQLite version to 3.8
These OS versions have SQLite 3.8 or higher by default.

- macOS 10.10 (Yosemite) or higher
- Ubuntu 14.04 LTS or higher

Raising the minimum version of SQLite 3.8 introduces these changes:

- All of bundled adapters support `supports_multi_insert?`
- SQLite 3.8 always satisifies `supports_foreign_keys_in_create?` and `supports_partial_index?`
- sqlite adapter can support `alter_table` method for foreign key referenced tables by rails#32865
- Deprecated `supports_multi_insert?` method
activerecord/test/cases/migration/foreign_key_test.rb Outdated
@@ -19,6 +19,52 @@ def test_foreign_keys
assert_equal "fk_name", fk.name unless current_adapter?(:SQLite3Adapter)
end
end
if current_adapter?(:SQLite3Adapter)

This comment has been minimized.

@kamipo

kamipo May 21, 2018

Member

The condition is unnecessary since the test case should pass for all adapters which support foreign keys.

This comment has been minimized.

@yahonda

yahonda May 21, 2018

Contributor

Thanks for the comment. When tested with mysql2 adapter, it gets ActiveRecord::StatementInvalid: Mysql2::Error: SAVEPOINT active_record_1 does not exist: ROLLBACK TO SAVEPOINT active_record_1 exception, which has been addressed by adding self.use_transactional_tests = false. Now it is green for all bundled adapters.

This comment has been minimized.

@yahonda

yahonda May 21, 2018

Contributor

It got failures with all bundled adapters https://travis-ci.org/rails/rails/builds/381658874 then reverted change then now it is tested with sqlite3 adapters only.

This comment has been minimized.

@kamipo

kamipo May 21, 2018

Member

The reason which got failures is that dropping tables defined in https://github.com/rails/rails/blob/master/activerecord/test/schema/schema.rb after loaded.

This comment has been minimized.

@yahonda

yahonda May 21, 2018

Contributor

ah, you are right.

This comment has been minimized.

@yahonda

yahonda May 21, 2018

Contributor

Now using Rocket and Astronaut models not to conflict with other test files.

Disable foreign keys during `alter_table` for sqlite3 adapter
Unlike other databases, changing SQLite3 table definitions need to create a temporary table.
While changing table operations, the original table needs dropped which caused
`SQLite3::ConstraintException: FOREIGN KEY constraint failed` if the table is referenced by foreign keys.
This pull request disables foreign keys by `disable_referential_integrity`.

Also `disable_referential_integrity` method needs to execute `defer_foreign_keys = ON`
to defer re-enabling foreign keys until the transaction is committed.

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

Fixes #31988

- This `defer_foreign_keys = ON` has been supported since SQLite 3.8.0
https://www.sqlite.org/releaselog/3_8_0.html and Rails 6 requires SQLite 3.8 #32923 now

- <Models>.reset_column_information added to address `ActiveModel::UnknownAttributeError`

```
Error:
ActiveRecord::Migration::ForeignKeyChangeColumnTest#test_change_column_of_parent_table:
ActiveModel::UnknownAttributeError: unknown attribute 'name' for ActiveRecord::Migration::ForeignKeyChangeColumnTest::Post.
```

@rafaelfranca rafaelfranca merged commit 09f26ed into rails:master May 22, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yahonda yahonda deleted the yahonda:another_31988 branch May 22, 2018

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