Commit a82dde8a authored by Andreas Brandl's avatar Andreas Brandl

Assert outside of transaction scope where possible

Using with_lock_retries outside of an existing transaction scope is safe
because it doesn't require us opening a subtransaction. Hence add
assertions to make sure we're outside of a transaction scope.
parent afe5cae4
...@@ -6,6 +6,8 @@ module Gitlab ...@@ -6,6 +6,8 @@ module Gitlab
module ForeignKeyHelpers module ForeignKeyHelpers
include ::Gitlab::Database::SchemaHelpers include ::Gitlab::Database::SchemaHelpers
ERROR_SCOPE = 'foreign keys'
# Adds a foreign key with only minimal locking on the tables involved. # Adds a foreign key with only minimal locking on the tables involved.
# #
# In concept it works similarly to add_concurrent_foreign_key, but we have # In concept it works similarly to add_concurrent_foreign_key, but we have
...@@ -32,6 +34,8 @@ module Gitlab ...@@ -32,6 +34,8 @@ module Gitlab
# name - The name of the foreign key. # name - The name of the foreign key.
# #
def add_concurrent_partitioned_foreign_key(source, target, column:, on_delete: :cascade, name: nil) def add_concurrent_partitioned_foreign_key(source, target, column:, on_delete: :cascade, name: nil)
assert_not_in_transaction_block(scope: ERROR_SCOPE)
partition_options = { partition_options = {
column: column, column: column,
on_delete: on_delete, on_delete: on_delete,
......
...@@ -7,6 +7,8 @@ module Gitlab ...@@ -7,6 +7,8 @@ module Gitlab
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
include Gitlab::Database::SchemaHelpers include Gitlab::Database::SchemaHelpers
ERROR_SCOPE = 'index'
# Concurrently creates a new index on a partitioned table. In concept this works similarly to # Concurrently creates a new index on a partitioned table. In concept this works similarly to
# `add_concurrent_index`, and won't block reads or writes on the table while the index is being built. # `add_concurrent_index`, and won't block reads or writes on the table while the index is being built.
# #
...@@ -21,6 +23,8 @@ module Gitlab ...@@ -21,6 +23,8 @@ module Gitlab
# #
# See Rails' `add_index` for more info on the available arguments. # See Rails' `add_index` for more info on the available arguments.
def add_concurrent_partitioned_index(table_name, column_names, options = {}) def add_concurrent_partitioned_index(table_name, column_names, options = {})
assert_not_in_transaction_block(scope: ERROR_SCOPE)
raise ArgumentError, 'A name is required for indexes added to partitioned tables' unless options[:name] raise ArgumentError, 'A name is required for indexes added to partitioned tables' unless options[:name]
partitioned_table = find_partitioned_table(table_name) partitioned_table = find_partitioned_table(table_name)
...@@ -57,6 +61,8 @@ module Gitlab ...@@ -57,6 +61,8 @@ module Gitlab
# #
# remove_concurrent_partitioned_index_by_name :users, 'index_name_goes_here' # remove_concurrent_partitioned_index_by_name :users, 'index_name_goes_here'
def remove_concurrent_partitioned_index_by_name(table_name, index_name) def remove_concurrent_partitioned_index_by_name(table_name, index_name)
assert_not_in_transaction_block(scope: ERROR_SCOPE)
find_partitioned_table(table_name) find_partitioned_table(table_name)
unless index_name_exists?(table_name, index_name) unless index_name_exists?(table_name, index_name)
......
...@@ -27,6 +27,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers ...@@ -27,6 +27,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers
before do before do
allow(migration).to receive(:puts) allow(migration).to receive(:puts)
allow(migration).to receive(:transaction_open?).and_return(false)
connection.execute(<<~SQL) connection.execute(<<~SQL)
CREATE TABLE #{target_table_name} ( CREATE TABLE #{target_table_name} (
...@@ -141,5 +142,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers ...@@ -141,5 +142,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers
.with(source_table_name, target_table_name, options) .with(source_table_name, target_table_name, options)
end end
end end
context 'when run inside a transaction block' do
it 'raises an error' do
expect(migration).to receive(:transaction_open?).and_return(true)
expect do
migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, column: column_name)
end.to raise_error(/can not be run inside a transaction/)
end
end
end end
end end
...@@ -20,6 +20,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do ...@@ -20,6 +20,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do
before do before do
allow(migration).to receive(:puts) allow(migration).to receive(:puts)
allow(migration).to receive(:transaction_open?).and_return(false)
connection.execute(<<~SQL) connection.execute(<<~SQL)
CREATE TABLE #{table_name} ( CREATE TABLE #{table_name} (
...@@ -127,6 +128,16 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do ...@@ -127,6 +128,16 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do
end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/) end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/)
end end
end end
context 'when run inside a transaction block' do
it 'raises an error' do
expect(migration).to receive(:transaction_open?).and_return(true)
expect do
migration.add_concurrent_partitioned_index(table_name, column_name)
end.to raise_error(/can not be run inside a transaction/)
end
end
end end
describe '#remove_concurrent_partitioned_index_by_name' do describe '#remove_concurrent_partitioned_index_by_name' do
...@@ -182,5 +193,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do ...@@ -182,5 +193,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do
end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/) end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/)
end end
end end
context 'when run inside a transaction block' do
it 'raises an error' do
expect(migration).to receive(:transaction_open?).and_return(true)
expect do
migration.remove_concurrent_partitioned_index_by_name(table_name, index_name)
end.to raise_error(/can not be run inside a transaction/)
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment