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

Disable foreign keys during alter_table for sqlite3 adapter #32865

Merged
merged 1 commit into from
May 22, 2018

Conversation

yahonda
Copy link
Member

@yahonda 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
Copy link

r? @georgeclaghorn

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

@yahonda
Copy link
Member Author

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

what will happened if sqlite3 is below 3.8.2 version?

@yahonda
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

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

@yahonda
Copy link
Member Author

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
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
@@ -19,6 +19,52 @@ def test_foreign_keys
assert_equal "fk_name", fk.name unless current_adapter?(:SQLite3Adapter)
end
end

if current_adapter?(:SQLite3Adapter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you are right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yahonda yahonda force-pushed the another_31988 branch 3 times, most recently from 13f801d to d3a33d9 Compare May 21, 2018 14:16
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 rails#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 rails#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
@yahonda yahonda deleted the another_31988 branch May 22, 2018 18:49
@robinboening
Copy link
Contributor

@rafaelfranca Is there a reason this PR only made it into rails v6.0.0.beta1 release (https://github.com/rails/rails/releases/tag/v6.0.0.beta1 )? It was already merged one year ago and imo it fixes a broken behaviour which I jut experienced in rails 5.2.3.

Thanks
Robin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change_column_null tries to drop table [ActiveRecord][SQLite][Mac]
7 participants