-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Optimize remove_columns to use a single SQL statement when supported #42075
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good to me besides a couple nitpicks.
Would be worth for @kamipo to take a look as well.
columns = connection.columns("my_table").map(&:name) | ||
assert_equal ["id"], columns | ||
|
||
if current_adapter?(:Mysql2Adapter, :PostgreSQLAdapter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check supports_bulk_alter?
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before submitting the PR, I flip flopped on this choice a bit.
On one hand, using supports_bulk_alter?
kind of copies and reuses the code under test, so using an explicit list of adapters here shows exactly which ones this applies to and there is less confusion on the side effects of the change.
On the other hand, as adapters improve, this test could fail requiring some extra maintenance.
Ultimately, I'm happy to go either way, just explaining my thought process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in "Use supports_bulk_alter? in tests"
assert_equal ["id"], columns | ||
|
||
if current_adapter?(:Mysql2Adapter, :PostgreSQLAdapter) | ||
assert_equal 1, queries.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an assert_queries
that you should be able to use I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in "Use assert_queries"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a changelog entry in Active Record? There is also likely documentation that needs to be updated as well to include mentioning the slight change in behavior.
6303593
to
0969f69
Compare
Done in commit "Add CHANGELOG entry". I'm open to all feedback on wording. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you squash your commits please?
end | ||
|
||
expected_query_count = if current_adapter?(:SQLite3Adapter) | ||
24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit surprised by this myself. The SQLite3 adapter does a lot of work to drop a column:
PRAGMA foreign_keys
PRAGMA defer_foreign_keys
PRAGMA defer_foreign_keys = ON
PRAGMA foreign_keys = OFF
CREATE TEMPORARY TABLE "amy_table" ("id" integer NOT NULL PRIMARY KEY, "col_one" integer DEFAULT NULL, "col_two" integer DEFAULT NULL)
INSERT INTO "amy_table" ("id","col_one","col_two")
SELECT "id","col_one","col_two" FROM "my_table"
DROP TABLE "my_table"
CREATE TABLE "my_table" ("id" integer NOT NULL PRIMARY KEY, "col_two" integer DEFAULT NULL)
INSERT INTO "my_table" ("id","col_two")
SELECT "id","col_two" FROM "amy_table"
DROP TABLE "amy_table"
PRAGMA defer_foreign_keys = 0
PRAGMA foreign_keys = 1
PRAGMA foreign_keys
PRAGMA defer_foreign_keys
PRAGMA defer_foreign_keys = ON
PRAGMA foreign_keys = OFF
CREATE TEMPORARY TABLE "amy_table" ("id" integer NOT NULL PRIMARY KEY, "col_two" integer DEFAULT NULL)
INSERT INTO "amy_table" ("id","col_two")
SELECT "id","col_two" FROM "my_table"
DROP TABLE "my_table"
CREATE TABLE "my_table" ("id" integer NOT NULL PRIMARY KEY)
INSERT INTO "my_table" ("id")
SELECT "id" FROM "amy_table"
DROP TABLE "amy_table"
PRAGMA defer_foreign_keys = 0
PRAGMA foreign_keys = 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. Sqlite has no ALTER table -_-.
I fear this magic number might prove hard to maintain. Maybe just don't assert at all for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. Done in "Skip query count assert on SQLite3"
def remove_columns_for_alter(table_name, *column_names, **options) | ||
column_names.map { |column_name| remove_column_for_alter(table_name, column_name) } | ||
def remove_columns_for_alter(table_name, *column_names, type: nil, **options) | ||
column_names.map { |column_name| remove_column_for_alter(table_name, column_name, type, **options) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I suppose I was trying to unify these signature and pass all args through, but I agree that it is unnecessary to achieve the feature.
I reverted this line in "Revert method signature changes and extra args"
1c8055c
to
79c42c4
Compare
Done. |
@@ -629,8 +629,13 @@ def remove_columns(table_name, *column_names, type: nil, **options) | |||
raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") | |||
end | |||
|
|||
column_names.each do |column_name| | |||
remove_column(table_name, column_name, type, **options) | |||
if supports_bulk_alter? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition excludes the SQLite adapter, but for the wrong reason: supports_bulk_alter?
tells us whether the adapter knows how to apply multiple schema changes together when alter_table
is called with the bulk: true
option, and not whether the database has the ability to execute an ALTER statement that contains multiple changes.
It's a subtle distinction, but if bulk alter support was added to the SQLite adapter in future (e.g. via #31628), this condition would become true, and we'd try to execute a statement that SQLite still couldn't handle. Conversely, if a future version of SQLite added support for dropping multiple columns in a single statement, we couldn't change the adapter to return true from supports_bulk_alter?
.
I think we should change this method to always use a single statement, and override it in the SQLite adapter to remove each column separately, which is how we deal with its partial ALTER support for other operations:
rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
Lines 240 to 283 in 0184a01
def add_column(table_name, column_name, type, **options) #:nodoc: | |
if invalid_alter_table_type?(type, options) | |
alter_table(table_name) do |definition| | |
definition.column(column_name, type, **options) | |
end | |
else | |
super | |
end | |
end | |
def remove_column(table_name, column_name, type = nil, **options) #:nodoc: | |
alter_table(table_name) do |definition| | |
definition.remove_column column_name | |
definition.foreign_keys.delete_if do |_, fk_options| | |
fk_options[:column] == column_name.to_s | |
end | |
end | |
end | |
def change_column_default(table_name, column_name, default_or_changes) #:nodoc: | |
default = extract_new_default_value(default_or_changes) | |
alter_table(table_name) do |definition| | |
definition[column_name].default = default | |
end | |
end | |
def change_column_null(table_name, column_name, null, default = nil) #:nodoc: | |
unless null || default.nil? | |
exec_query("UPDATE #{quote_table_name(table_name)} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL") | |
end | |
alter_table(table_name) do |definition| | |
definition[column_name].null = null | |
end | |
end | |
def change_column(table_name, column_name, type, **options) #:nodoc: | |
alter_table(table_name) do |definition| | |
definition[column_name].instance_eval do | |
self.type = aliased_types(type.to_s, type) | |
self.options.merge!(options) | |
end | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eugeneius Thank you for that detailed explanation. That makes sense.
I've applied your suggestion in the latest revision.
Because this adds logic to the SQLite3 adapter, I've restored the query count assert in the test. I think it was helpful in showing me that the override was doing what I expected it to.
45736c7
to
dc0d94e
Compare
@@ -1627,10 +1625,6 @@ def add_timestamps_for_alter(table_name, **options) | |||
] | |||
end | |||
|
|||
def remove_timestamps_for_alter(table_name, **options) | |||
remove_columns_for_alter(table_name, :updated_at, :created_at) | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is still necessary, although evidently we're missing a test to demonstrate why.
Given this migration:
class RemoveTitleAndTimestampsFromPosts < ActiveRecord::Migration[7.0]
def change
change_table :posts, bulk: true do |t|
t.remove :title, type: :string
t.remove_timestamps
end
end
end
Before this change, the operations are combined into a single statement:
ALTER TABLE `posts` DROP COLUMN `title`, DROP COLUMN `updated_at`, DROP COLUMN `created_at`
Afterwards, that's no longer the case:
ALTER TABLE `posts` DROP COLUMN `title`
ALTER TABLE `posts` DROP COLUMN `updated_at`, DROP COLUMN `created_at`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the missing test coverage in 217f0e8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching and testing this!
Restored this method in the latest revision.
eb88a4b
to
8cda28e
Compare
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
Outdated
Show resolved
Hide resolved
Both MySQL and PostgreSQL support dropping multiple columns in a single SQL statement. For tables that are very large, dropping a column can be time consuming. When multiple columns are involved, dropping them all at once can reduce the total overhead when compared to dropping each column individually. SQLite3 does not support this feature so its adapter overrides the remove_columns method to workaround SQLite3's ALTER TABLE limitations. The already written method remove_columns_for_alter creates the ALTER TABLE SQL fragments to execute. The remove_timestamps method also happens to drop multiple columns, so it now uses the updated remove_columns to take advantage of the optimization.
7a151b2
to
bff3523
Compare
Thanks @eugeneius I applied your suggestions. |
Thank you @jdufresne for the contribution! ✌️ |
Follow rails/rails#42075. This PR supports single SQL statement syntax of Oracle for `remove_columns` and fixes the CI build error. ```console % bundle exec rake (snip) Failures: 1) OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table Failure/Error: expect(TestPost.columns_hash["title"]).to be_nil expected: nil got: #<ActiveRecord::ConnectionAdapters::OracleEnhanced::Column:0x0000000003f99680 @name="title", @sql_typ...on=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil> # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:102:in `block in <module:Support>' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:111:in `notify_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/fail_with.rb:35:in `fail_with' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:40:in `handle_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:56:in `block in handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:27:in `with_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:48:in `handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/expectation_target.rb:65:in `to' # ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:931:in `block (3 levels) in <top (required)>' (snip) 2) OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table Failure/Error: with_retry { @connection.exec(sql, *bindvars, &block) } ActiveRecord::StatementInvalid: OCIError: ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/cursor.rb:137:in `exec' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:271:in `exec_internal' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:262:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `block in exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:433:in `with_retry' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:97:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `block in execute' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:725:in `block (2 levels) in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:724:in `block in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/notifications/instrumenter.rb:44:in `instrument' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:715:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/dbms_output.rb:35:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `execute' (snip) # ------------------ # --- Caused by: --- # OCIError: # ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so Finished in 3 minutes 0.9 seconds (files took 0.54912 seconds to load) 391 examples, 2 failures, 5 pending Failed examples: rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:924 # OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:934 # OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table ``` https://github.com/rsim/oracle-enhanced/runs/2482234335
Follow rails/rails#42075. This PR supports single SQL statement syntax of Oracle for `remove_columns` and fixes the CI build error. ```console % bundle exec rake (snip) Failures: 1) OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table Failure/Error: expect(TestPost.columns_hash["title"]).to be_nil expected: nil got: #<ActiveRecord::ConnectionAdapters::OracleEnhanced::Column:0x0000000003f99680 @name="title", @sql_typ...on=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil> # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:102:in `block in <module:Support>' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:111:in `notify_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/fail_with.rb:35:in `fail_with' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:40:in `handle_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:56:in `block in handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:27:in `with_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:48:in `handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/expectation_target.rb:65:in `to' # ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:931:in `block (3 levels) in <top (required)>' (snip) 2) OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table Failure/Error: with_retry { @connection.exec(sql, *bindvars, &block) } ActiveRecord::StatementInvalid: OCIError: ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/cursor.rb:137:in `exec' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:271:in `exec_internal' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:262:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `block in exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:433:in `with_retry' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:97:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `block in execute' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:725:in `block (2 levels) in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:724:in `block in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/notifications/instrumenter.rb:44:in `instrument' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:715:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/dbms_output.rb:35:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `execute' (snip) # ------------------ # --- Caused by: --- # OCIError: # ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so Finished in 3 minutes 0.9 seconds (files took 0.54912 seconds to load) 391 examples, 2 failures, 5 pending Failed examples: rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:924 # OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:934 # OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table ``` https://github.com/rsim/oracle-enhanced/runs/2482234335
Both MySQL and PostgreSQL support dropping multiple columns in a single
SQL statement.
For tables that are very large, dropping a column can be time consuming.
When multiple columns are involved, dropping them all at once can reduce
the total overhead when compared to dropping each column individually.
SQLite3 does not support this feature so its adapter overrides the
remove_columns method to workaround SQLite3's ALTER TABLE limitations.
The already written, but previously unused method
remove_columns_for_alter creates the ALTER TABLE SQL fragments to
execute.
The remove_timestamps method also happens to drop multiple columns, so
it now uses the updated remove_columns to take advantage of the
optimization.