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

Allow `truncate` for SQLite3 adapter and add `rails db:seed:replant` #34779

Merged
merged 7 commits into from Mar 4, 2019

Conversation

Projects
None yet
6 participants
@bogdanvlviv
Copy link
Contributor

bogdanvlviv commented Dec 23, 2018

  • Add ActiveRecord::Base.connection.truncate for SQLite3 adapter.
    SQLite doesn't support TRUNCATE TABLE, but SQLite3 adapter can support
    ActiveRecord::Base.connection.truncate by using DELETE FROM.
    DELETE without WHERE uses "The Truncate Optimization",
    see https://www.sqlite.org/lang_delete.html.

  • Add rails db:seed:replant that truncates tables of each database for current environment and loads the seeds.
    Closes #34765

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 2 times, most recently from 1abb52e to b71e651 Dec 23, 2018

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Dec 23, 2018

Awesome, @bogdanvlviv 🙏. I'll let someone else review the implementation, but this is great.

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Dec 27, 2018

What about to use also CASCADE to not crash on foreign key constraints?

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Dec 27, 2018

What about to use also CASCADE to not crash on foreign key constraints?

ActiveRecord::Base.connection.truncate doesn't allow to set "CASCADE", so I've tried to use ActiveRecord::Base.connection.disable_referential_integrity. It solved this issue for MySQL2, but for PostgreSQL it is still issue 🤔.

ActiveRecord::Base.connection.disable_referential_integrity do
  table_names.without(*internal_table_names).each do |table_name|
    ActiveRecord::Base.connection.truncate(table_name)
  end
end

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 2 times, most recently from e8f17e9 to a1f9b9b Dec 28, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Dec 28, 2018

On PostgreSQL, this issue (test failure) could be solved by applying CASCADE. We could change the truncate method to allow pass additional options like ActiveRecord::Base.connection.truncate(table_name, nil, cascade: true), see diff:

Diff
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index 2299fc0214..deac8e9e5b 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -135,7 +135,7 @@ def exec_delete(sql, name = nil, binds = [])
       end

       # Executes the truncate statement.
-      def truncate(table_name, name = nil)
+      def truncate(table_name, name = nil, options = {})
         raise NotImplementedError
       end

diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index dbc6614b93..ad13432aaa 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -271,7 +271,7 @@ def collation
         show_variable "collation_database"
       end

-      def truncate(table_name, name = nil)
+      def truncate(table_name, name = nil, options = {})
         execute "TRUNCATE TABLE #{quote_table_name(table_name)}", name
       end

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 381d5ab29b..7ed0c45c17 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -258,8 +258,11 @@ def clear_cache!
         end
       end

-      def truncate(table_name, name = nil)
-        exec_query "TRUNCATE TABLE #{quote_table_name(table_name)}", name, []
+      def truncate(table_name, name = nil, options = {})
+        cascade = if options[:cascade]
+          " CASCADE"
+        end
+        exec_query "TRUNCATE TABLE #{quote_table_name(table_name)}#{cascade}", name, []
       end

       # Is this connection alive and ready for queries?
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index 94754e5b86..144d1548b9 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -161,7 +161,7 @@ def clear_cache!
         @statements.clear
       end

-      def truncate(table_name, name = nil)
+      def truncate(table_name, name = nil, options = {})
         execute "DELETE FROM #{quote_table_name(table_name)}", name
       end

diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb
index 81a2b84e39..f852ebf328 100644
--- a/activerecord/lib/active_record/tasks/database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/database_tasks.rb
@@ -191,7 +191,7 @@ def truncate_tables

         ActiveRecord::Base.connection.disable_referential_integrity do
           table_names.without(*internal_table_names).each do |table_name|
-            ActiveRecord::Base.connection.truncate(table_name)
+            ActiveRecord::Base.connection.truncate(table_name, nil, cascade: true)
           end
         end
       end

But I'm not completly sure that it is right solution since it changes public API(truncate method).
Do you have any suggestion about solving this?

Possible related to 2812694

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 3 times, most recently from c48bd5e to 1a0bd9f Dec 30, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Dec 30, 2018

rails db:seed:replant should work with multi DB as well

Since version 6.0 Rails will support mult DB, so user will be
able to write in db/seeds.rb something like:

ActiveRecord::Base.connected_to(database: :primary) do
  User.create!(name: "Bogdan")
end

ActiveRecord::Base.connected_to(database: :animals) do
  Dog.create!(name: "Bosyi")
end

rails db:seed:replant should truncate tables in both databases,
and replant seeds.

Since not every DB backend has TRUNCATE, or if it has then
it can work in a different way than on any another DB backend.

For instance, in order to truncate a table and not crash on foreign key constraints
on MySQL we should execute something like:

SET FOREIGN_KEY_CHECKS = 0;
TRUNCATE TABLE `users`;

on PostgreSQL:

TRUNCATE TABLE "users", ...;

on SQLite:

PRAGMA defer_foreign_keys = ON;
PRAGMA foreign_keys = OFF;
DELETE FROM "users";

Because of that, we should also provide strategy by which other DB
adapters, like oracle-enhanced, or activerecord-sqlserver-adapter,
will we able to make rails db:seed:replant work.

Since we can't just rely on ActiveRecord::Base.connection.truncate
because of the inconsistency of DB backends, I want to recommend the next:
Implement "ActiveRecord::Tasks::#{adapter_name}DatabaseTasks#truncate_tables(*table_names)"
to an adapter that would like to support this rake task.
This method just receives an array of table names. Your implementation
should ensure that it truncates all of those tables. Also, be sure
that it does not crash on foreign key constraints!

/cc @dhh

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 30, 2018

Since we can't just rely on ActiveRecord::Base.connection.truncate

Yes, we can: to be compatible with a new release, adapters need to support whatever methods we decide on. (And our downstreams are super accomodating and responsive about that.)

I haven't looked at this, but we already have "empty out all the tables" behaviour for populating fixtures: unless I'm missing something, that should both prove that we have the technology, and suggest some scope for code sharing.

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Dec 30, 2018

Yes, we can: to be compatible with a new release, adapters need to support whatever methods we decide >on. (And our downstreams are super accomodating and responsive about that.)

That is right, I meant that ActiveRecord::Base.connection.truncate could behave on each DB backend differently and I'm not sure whether complementing of truncate (as I suggested in #34779 (comment)) is a good idea since TRUNCATE on backends could have different API as well.

I haven't looked at this, but we already have "empty out all the tables" behavior for populating fixtures: unless I'm missing something, that should both prove that we have the technology, and suggest some scope for code sharing.

I've only found this

def insert_fixtures_set(fixture_set, tables_to_delete = [])
fixture_inserts = fixture_set.map do |table_name, fixtures|
next if fixtures.empty?
build_fixture_sql(fixtures, table_name)
end.compact
table_deletes = tables_to_delete.map { |table| +"DELETE FROM #{quote_table_name table}" }
total_sql = Array.wrap(combine_multi_statements(table_deletes + fixture_inserts))
disable_referential_integrity do
transaction(requires_new: true) do
total_sql.each do |sql|
execute sql, "Fixtures Load"
yield if block_given?
end
end
end
end

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Dec 30, 2018

I've only found this

Yeah, that looks right: it disables referential integrity, then empties out all the tables.

We could consider switching that to truncate, if its additional constraints are okay, but I think we can treat that as a separate question.

(As a side note, CASCADE is not a solution here: by definition it means something's going to get cleared out that we're not expecting, which would be unhelpful -- in contrast, listing all the known tables in a single TRUNCATE statement might be useful)

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 2 times, most recently from 01213c8 to b1a22c8 Dec 30, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Dec 30, 2018

(As a side note, CASCADE is not a solution here: by definition it means something's going to get cleared out that we're not expecting, which would be unhelpful -- in contrast, listing all the known tables in a single TRUNCATE statement might be useful)

👍 Thanks for pointing this out, I changed the implementation for PostgreSQL to use TRUNCATE statement with a listing all the known tables.

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch from b1a22c8 to f5b96a8 Dec 30, 2018

table_names = ActiveRecord::Base.connection.tables
internal_table_names = [
ActiveRecord::Base.schema_migrations_table_name,
ActiveRecord::Base.internal_metadata_table_name

This comment has been minimized.

@kaspth

kaspth Jan 3, 2019

Member

Seems like this might have a need for something similar to what we're doing here: https://github.com/rails/rails/pull/33985/files#diff-18d566ee525de376b1a3741881545631R147

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 3, 2019

Author Contributor

There is one more place that possible should be refactored

[ActiveRecord::Base.schema_migrations_table_name, ActiveRecord::Base.internal_metadata_table_name, ignore_tables].flatten.any? do |ignored|
. I'm not sure that we can use something like ActiveRecord::Base.descendants.select { |model| !model._internal? }.map(&:table_name).compact here (It ignores join tables, ...) since It isn't the same as ActiveRecord::Base.connection.tables.

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch from f5b96a8 to d8feb78 Jan 13, 2019

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Jan 20, 2019

@bogdanvlviv Is this ready for another review?

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch from d8feb78 to c98a382 Jan 20, 2019

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Jan 20, 2019

Yes. I just rebased with the master to resolve the conflicts.

@dhh dhh requested a review from matthewd Jan 20, 2019

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Jan 20, 2019

@kaspth and @matthewd, can you guys do a final review?

@kaspth
Copy link
Member

kaspth left a comment

Got one question, then I'll pass this on to @matthewd or whoever else wants to take this home.

ActiveRecord::Base.configurations.configs_for(env_name: environment).each do |db_config|
truncate_tables db_config.config
end
ActiveRecord::Base.establish_connection(environment.to_sym)

This comment has been minimized.

@kaspth

kaspth Jan 24, 2019

Member

Isn't establish connection deprecated now that we have connect_to?

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 25, 2019

Author Contributor

ActiveRecord::Base.establish_connection isn't deprecated.
But yeah, we can give a chance ActiveRecord::Base.connected_to to be used here, see 8df9776.

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Jan 24, 2019

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 2 times, most recently from fe05b0f to 8df9776 Jan 25, 2019

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch from 8df9776 to d26952c Feb 7, 2019

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Feb 7, 2019

I just rebased this branch one more time to resolve the conflicts. Let me if you have any concerns about these changes.

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Feb 10, 2019

@kaspth @matthewd Any other implementation notes?

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch 4 times, most recently from c8c4386 to 2148716 Feb 25, 2019

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Mar 3, 2019

@bogdanvlviv You're happy with the state of things and ready to merge?

bogdanvlviv added some commits Dec 23, 2018

Add `ActiveRecord::Base.connection.truncate` for SQLite3 adapter.
SQLite doesn't support `TRUNCATE TABLE`, but SQLite3 adapter can support
`ActiveRecord::Base.connection.truncate` by using `DELETE FROM`.

`DELETE` without `WHERE` uses "The Truncate Optimization",
see https://www.sqlite.org/lang_delete.html.
Add `ActiveRecord::Tasks::DatabaseTasks.truncate_tables`
Extract code from the rake task. It will be easier to test this code and reuse.
`rails db:seed:replant` should work with multi DB
Since version 6.0 Rails will support mult DB, so user will be
able to write in `db/seeds.rb` something like:

```ruby
ActiveRecord::Base.connected_to(database: :primary) do
  User.create!(name: "Bogdan")
end

ActiveRecord::Base.connected_to(database: :animals) do
  Dog.create!(name: "Bosyi")
end
```

`rails db:seed:replant` should truncate tables in both databases,
and replant seeds.

</hr>

Since not every DB backend has `TRUNCATE`, or if it has then
it can work in a different way than on any another DB backend.

For instance, in order to truncate a table and not crash on foreign key constraints
on MySQL we should execute something like:

```sql
SET FOREIGN_KEY_CHECKS = 0;
TRUNCATE TABLE `users`;
```

on PostgreSQL:

```sql
TRUNCATE TABLE "users", ... ;
```

on SQLite:

```sql
PRAGMA defer_foreign_keys = ON;
PRAGMA foreign_keys = OFF;
DELETE FROM "users";
```

Because of that, we should also provide strategy by which other DB
adapters, like `oracle-enhanced`, or `activerecord-sqlserver-adapter`,
will we able to make `rails db:seed:replant` work.

Since we can't just rely on `ActiveRecord::Base.connection.truncate`
because of the inconsistency of DB backends, I want to recommend the next:
Implement "ActiveRecord::Tasks::#{adapter_name}DatabaseTasks#truncate_tables(*table_names)"
to an adapter that would like to support this rake task.
This method just receives an array of table names. Your implementation
should ensure that it truncates all of those tables. Also, be sure
that it does not crash on foreign key constraints!

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34765 branch from 2148716 to 1dad72e Mar 4, 2019

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Mar 4, 2019

Unless there are any new implementation notes, it should be good to go.
One thing that concerns me is CI builds. Seems like there are some flaky tests (I just run tests on my VM and haven't got those test failures, run the second time and have got them).
https://buildkite.com/rails/rails/builds/59209,
https://travis-ci.org/rails/rails/builds/501434510

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Mar 4, 2019

CI is terribly flaky at the moment 👎

@dhh dhh merged commit a8c0ebc into rails:master Mar 4, 2019

0 of 3 checks passed

buildkite/rails Build #59222 scheduled
Details
codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:issue-34765 branch Mar 4, 2019

kamipo added a commit that referenced this pull request Mar 5, 2019

Reset `connection_handlers` to default when any test dirties that
Most existing tests expects `connection_handlers` has only one default
handler, but the test added at #34779 dirties that.

We need to reset `connection_handlers` to default in that case.

Closes #35471.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.