Optimize remove_columns to use a single SQL statement when supported

[jdufresne]

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.

[byroot]

Mostly looks good to me besides a couple nitpicks.

Would be worth for @kamipo to take a look as well.

[byroot]

Maybe check supports_bulk_alter? instead?

[jdufresne]

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.

[jdufresne]

Fixed in "Use supports_bulk_alter? in tests"

[byroot]

There's an assert_queries that you should be able to use I think.

[jdufresne]

Fixed in "Use assert_queries"

[eileencodes]

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.

[jdufresne]

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.

Done in commit "Add CHANGELOG entry". I'm open to all feedback on wording.

[byroot]

Could you squash your commits please?

[byroot]

24?

[jdufresne]

I was a bit surprised by this myself. The SQLite3 adapter does a lot of work to drop a column:

https://github.com/rails/rails/blob/main/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L250-L257

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.

[byroot]

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?

[jdufresne]

Sure thing. Done in "Skip query count assert on SQLite3"

[eugeneius]

This change seems unnecessary.

[jdufresne]

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"

[jdufresne]

Could you squash your commits please?

Done.

[eugeneius]

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

[jdufresne]

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

[eugeneius]

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`

[eugeneius]

I added the missing test coverage in 217f0e8.

[jdufresne]

Thanks for catching and testing this!

Restored this method in the latest revision.

[jdufresne]

Thanks @eugeneius I applied your suggestions.

[eugeneius]

Thank you @jdufresne for the contribution! ✌️