From 02b8aa60960a8cc49b419f34c9d24a09b310c6b6 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Mon, 17 Mar 2025 08:25:30 +0800 Subject: [PATCH] DEV: Protection for migrations that creates index concurrently take 2 (#31792) This is a follow up to 6820622467ab3613e824f0cb6219def2a575bc1d. This commit addresses migrations that uses `remove_index` with the `if_exists: true` option to drop an existing index before creating an index using the `concurrently` option. This commit also reruns two migration which may have caused indexes to be left in an `invalid` state. ### Reviewers Note Plugin tests are failing due to https://github.com/discourse/discourse-translator/pull/251 --- ...115143259_add_index_to_users_ip_address.rb | 9 --- ...ique_constraint_from_tag_users_indexes.rb} | 15 +++- ...313045010_add_index_to_users_ip_address.rb | 19 +++++ lib/migration/safe_migrate.rb | 71 +++++++++++++------ ...0230309014016_create_index_concurrently.rb | 4 -- .../20230309014016_some_migration.rb | 14 ++++ spec/lib/migration/safe_migrate_spec.rb | 53 ++++++++++++-- 7 files changed, 142 insertions(+), 43 deletions(-) delete mode 100644 db/migrate/20241115143259_add_index_to_users_ip_address.rb rename db/migrate/{20240829142639_remove_unique_constraint_from_tag_users_indexes.rb => 20250313044812_remove_unique_constraint_from_tag_users_indexes.rb} (57%) create mode 100644 db/migrate/20250313045010_add_index_to_users_ip_address.rb create mode 100644 spec/fixtures/db/migrate/create_index_concurrently_safe_activerecord/20230309014016_some_migration.rb diff --git a/db/migrate/20241115143259_add_index_to_users_ip_address.rb b/db/migrate/20241115143259_add_index_to_users_ip_address.rb deleted file mode 100644 index b3731df66b3..00000000000 --- a/db/migrate/20241115143259_add_index_to_users_ip_address.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -class AddIndexToUsersIpAddress < ActiveRecord::Migration[7.1] - disable_ddl_transaction! - - def change - add_index :users, :ip_address, algorithm: :concurrently, name: "idx_users_ip_address" - end -end diff --git a/db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb b/db/migrate/20250313044812_remove_unique_constraint_from_tag_users_indexes.rb similarity index 57% rename from db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb rename to db/migrate/20250313044812_remove_unique_constraint_from_tag_users_indexes.rb index 22795fc7d03..8b73e76b583 100644 --- a/db/migrate/20240829142639_remove_unique_constraint_from_tag_users_indexes.rb +++ b/db/migrate/20250313044812_remove_unique_constraint_from_tag_users_indexes.rb @@ -1,10 +1,21 @@ # frozen_string_literal: true + class RemoveUniqueConstraintFromTagUsersIndexes < ActiveRecord::Migration[7.1] disable_ddl_transaction! def up - remove_index :tag_users, name: :idx_tag_users_ix1, algorithm: :concurrently - remove_index :tag_users, name: :idx_tag_users_ix2, algorithm: :concurrently + remove_index :tag_users, name: :idx_tag_users_ix1, algorithm: :concurrently, if_exists: true + remove_index :tag_users, name: :idx_tag_users_ix2, algorithm: :concurrently, if_exists: true + + remove_index :tag_users, + %i[user_id tag_id notification_level], + algorithm: :concurrently, + if_exists: true + + remove_index :tag_users, + %i[tag_id user_id notification_level], + algorithm: :concurrently, + if_exists: true add_index :tag_users, %i[user_id tag_id notification_level], algorithm: :concurrently add_index :tag_users, %i[tag_id user_id notification_level], algorithm: :concurrently diff --git a/db/migrate/20250313045010_add_index_to_users_ip_address.rb b/db/migrate/20250313045010_add_index_to_users_ip_address.rb new file mode 100644 index 00000000000..33757f96d84 --- /dev/null +++ b/db/migrate/20250313045010_add_index_to_users_ip_address.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddIndexToUsersIpAddress < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def up + remove_index :users, + :ip_address, + algorithm: :concurrently, + name: "idx_users_ip_address", + if_exists: true + + add_index :users, :ip_address, algorithm: :concurrently, name: "idx_users_ip_address" + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/migration/safe_migrate.rb b/lib/migration/safe_migrate.rb index 9a97e6a285c..b85c9d89d1c 100644 --- a/lib/migration/safe_migrate.rb +++ b/lib/migration/safe_migrate.rb @@ -78,6 +78,20 @@ class Migration::SafeMigrate return if ENV["RAILS_ENV"] == "production" @@migration_sqls = [] + @@activerecord_remove_indexes = [] + + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.class_eval do + alias_method :original_remove_index, :remove_index + + def remove_index(table_name, column_name = nil, **options) + @@activerecord_remove_indexes << ( + options[:name] || index_name(table_name, options.merge(column: column_name)) + ) + + # Call the original method + original_remove_index(table_name, column_name, **options) + end + end PG::Connection.class_eval do alias_method :exec_migrator_unpatched, :exec @@ -99,7 +113,13 @@ class Migration::SafeMigrate return if !PG::Connection.method_defined?(:exec_migrator_unpatched) return if ENV["RAILS_ENV"] == "production" - @@migration_sqls = [] + @@migration_sqls.clear + @@activerecord_remove_indexes.clear + + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.class_eval do + alias_method :remove_index, :original_remove_index + remove_method :original_remove_index + end PG::Connection.class_eval do alias_method :exec, :exec_migrator_unpatched @@ -120,21 +140,6 @@ class Migration::SafeMigrate end end - UNSAFE_DROP_INDEX_CONCURRENTLY_WARNING = <<~TEXT - WARNING - ------------------------------------------------------------------------------------- - An attempt was made to create an index concurrently in a migration without first dropping the index. - - Per postgres documentation: - - If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, - the CREATE INDEX command will fail but leave behind an “invalid” index. This index will be ignored for querying - purposes because it might be incomplete; however it will still consume update overhead. The recommended recovery - method in such cases is to drop the index and try again to perform CREATE INDEX CONCURRENTLY . - - Please update the migration to first drop the index if it exists before creating it concurrently. - TEXT - def self.protect!(sql) @@migration_sqls << sql @@ -169,15 +174,37 @@ class Migration::SafeMigrate TEXT raise Discourse::InvalidMigration, "Attempt was made to rename or delete column" elsif sql =~ /\A\s*create\s+(?:unique\s+)?index\s+concurrently\s+/i + index_name = + sql.match(/\bINDEX\s+CONCURRENTLY\s+(?:IF\s+NOT\s+EXISTS\s+)?"?([a-zA-Z0-9_\.]+)"?/i)[1] + + return if @@activerecord_remove_indexes.include?(index_name) + match = sql.match(/\bON\s+(?:ONLY\s+)?(?:"([^"]+)"|([a-zA-Z0-9_\.]+))/i) table_name = match[1] || match[2] - if !@@migration_sqls.any? { |migration_sql| - migration_sql =~ /\A\s*drop\s+index\s+(?:concurrently\s+)?if\s+exists\s+/i && - migration_sql.include?(table_name) - } - $stdout.puts("", UNSAFE_DROP_INDEX_CONCURRENTLY_WARNING) - end + has_drop_index_statement = + @@migration_sqls.any? do |migration_sql| + migration_sql =~ /\A\s*drop\s+index/i && migration_sql.include?(table_name) && + migration_sql.include?(index_name) + end + + return if has_drop_index_statement + + raise(Discourse::InvalidMigration, <<~RAW) + WARNING + ------------------------------------------------------------------------------------- + An attempt was made to create an index concurrently in a migration without first dropping the index. + SQL used was: '#{sql}' + + Per postgres documentation: + + If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, + the CREATE INDEX command will fail but leave behind an “invalid” index. This index will be ignored for querying + purposes because it might be incomplete; however it will still consume update overhead. The recommended recovery + method in such cases is to drop the index and try again to perform CREATE INDEX CONCURRENTLY . + + Please update the migration to first drop the index if it exists before creating it concurrently. + RAW end end diff --git a/spec/fixtures/db/migrate/create_index_concurrently_safe/20230309014016_create_index_concurrently.rb b/spec/fixtures/db/migrate/create_index_concurrently_safe/20230309014016_create_index_concurrently.rb index 6ba2f22cf2a..ef3735bd74f 100644 --- a/spec/fixtures/db/migrate/create_index_concurrently_safe/20230309014016_create_index_concurrently.rb +++ b/spec/fixtures/db/migrate/create_index_concurrently_safe/20230309014016_create_index_concurrently.rb @@ -11,10 +11,6 @@ class CreateIndexConcurrently < ActiveRecord::Migration[7.2] execute <<~SQL CREATE INDEX CONCURRENTLY some_notifications_index ON notifications(user_id); SQL - - execute <<~SQL - DROP INDEX some_notifications_index; - SQL end def down diff --git a/spec/fixtures/db/migrate/create_index_concurrently_safe_activerecord/20230309014016_some_migration.rb b/spec/fixtures/db/migrate/create_index_concurrently_safe_activerecord/20230309014016_some_migration.rb new file mode 100644 index 00000000000..ef34b55572d --- /dev/null +++ b/spec/fixtures/db/migrate/create_index_concurrently_safe_activerecord/20230309014016_some_migration.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class SomeMigration < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def up + remove_index :notifications, %i[user_id id], if_exists: true + add_index :notifications, %i[user_id id], algorithm: :concurrently + end + + def down + raise "not tested" + end +end diff --git a/spec/lib/migration/safe_migrate_spec.rb b/spec/lib/migration/safe_migrate_spec.rb index eef2a1848cb..e5e7796eef2 100644 --- a/spec/lib/migration/safe_migrate_spec.rb +++ b/spec/lib/migration/safe_migrate_spec.rb @@ -71,11 +71,44 @@ RSpec.describe Migration::SafeMigrate do expect { User.first.username }.not_to raise_error end - it "allows running a migration that creates an index concurrently if it drops the index first" do - path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/create_index_concurrently_safe" - output = capture_stdout { expect do migrate_up(path) end.to raise_error(StandardError) } + it "allows running a migration that creates an index concurrently if it checks if the index exists first" do + Migration::SafeMigrate.enable! - expect(output).not_to include(described_class::UNSAFE_DROP_INDEX_CONCURRENTLY_WARNING) + path = + File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/create_index_concurrently_safe_activerecord" + + error = nil + + capture_stdout do + begin + migrate_up(path) + rescue StandardError => e + error = e + end + end + + expect(error.cause.cause.message).to include( + "CREATE INDEX CONCURRENTLY cannot run inside a transaction block", + ) + end + + it "allows running a migration that creates an index concurrently if it drops the index first" do + Migration::SafeMigrate.enable! + + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/create_index_concurrently_safe" + error = nil + + capture_stdout do + begin + migrate_up(path) + rescue StandardError => e + error = e + end + end + + expect(error.cause.cause.message).to include( + "CREATE INDEX CONCURRENTLY cannot run inside a transaction block", + ) end it "bans running a migration that creates an index concurrently without first dropping the index if it exists" do @@ -84,9 +117,17 @@ RSpec.describe Migration::SafeMigrate do path = File.expand_path("#{Rails.root}/spec/fixtures/db/migrate/create_index_concurrently_unsafe") - output = capture_stdout { expect do migrate_up(path) end.to raise_error(StandardError) } + error = nil - expect(output).to include(described_class::UNSAFE_DROP_INDEX_CONCURRENTLY_WARNING) + capture_stdout do + migrate_up(path) + rescue StandardError => e + error = e + end + + expect(error.message).to include( + "An attempt was made to create an index concurrently in a migration without first dropping the index.", + ) end it "allows dropping NOT NULL" do