diff options
Diffstat (limited to 'spec/lib/gitlab/database')
13 files changed, 556 insertions, 241 deletions
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index a763dc08b73..6b709cba5b3 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers do + include Database::TableSchemaHelpers + let(:model) do ActiveRecord::Migration.new.extend(described_class) end @@ -96,6 +98,131 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end + describe '#create_table_with_constraints' do + let(:table_name) { :test_table } + let(:column_attributes) do + [ + { name: 'id', sql_type: 'bigint', null: false, default: nil }, + { name: 'created_at', sql_type: 'timestamp with time zone', null: false, default: nil }, + { name: 'updated_at', sql_type: 'timestamp with time zone', null: false, default: nil }, + { name: 'some_id', sql_type: 'integer', null: false, default: nil }, + { name: 'active', sql_type: 'boolean', null: false, default: 'true' }, + { name: 'name', sql_type: 'text', null: true, default: nil } + ] + end + + before do + allow(model).to receive(:transaction_open?).and_return(true) + end + + context 'when no check constraints are defined' do + it 'creates the table as expected' do + model.create_table_with_constraints table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name + end + + expect_table_columns_to_match(column_attributes, table_name) + end + end + + context 'when check constraints are defined' do + context 'when the text_limit is explicity named' do + it 'creates the table as expected' do + model.create_table_with_constraints table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name + + t.text_limit :name, 255, name: 'check_name_length' + t.check_constraint :some_id_is_positive, 'some_id > 0' + end + + expect_table_columns_to_match(column_attributes, table_name) + + expect_check_constraint(table_name, 'check_name_length', 'char_length(name) <= 255') + expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0') + end + end + + context 'when the text_limit is not named' do + it 'creates the table as expected, naming the text limit' do + model.create_table_with_constraints table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name + + t.text_limit :name, 255 + t.check_constraint :some_id_is_positive, 'some_id > 0' + end + + expect_table_columns_to_match(column_attributes, table_name) + + expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 255') + expect_check_constraint(table_name, 'some_id_is_positive', 'some_id > 0') + end + end + + it 'runs the change within a with_lock_retries' do + expect(model).to receive(:with_lock_retries).ordered.and_yield + expect(model).to receive(:create_table).ordered.and_call_original + expect(model).to receive(:execute).with(<<~SQL).ordered + ALTER TABLE "#{table_name}"\nADD CONSTRAINT "check_cda6f69506" CHECK (char_length("name") <= 255) + SQL + + model.create_table_with_constraints table_name do |t| + t.text :name + t.text_limit :name, 255 + end + end + + context 'when constraints are given invalid names' do + let(:expected_max_length) { described_class::MAX_IDENTIFIER_NAME_LENGTH } + let(:expected_error_message) { "The maximum allowed constraint name is #{expected_max_length} characters" } + + context 'when the explicit text limit name is not valid' do + it 'raises an error' do + too_long_length = expected_max_length + 1 + + expect do + model.create_table_with_constraints table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name + + t.text_limit :name, 255, name: ('a' * too_long_length) + t.check_constraint :some_id_is_positive, 'some_id > 0' + end + end.to raise_error(expected_error_message) + end + end + + context 'when a check constraint name is not valid' do + it 'raises an error' do + too_long_length = expected_max_length + 1 + + expect do + model.create_table_with_constraints table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name + + t.text_limit :name, 255 + t.check_constraint ('a' * too_long_length), 'some_id > 0' + end + end.to raise_error(expected_error_message) + end + end + end + end + end + describe '#add_concurrent_index' do context 'outside a transaction' do before do @@ -1548,6 +1675,69 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end + describe '#initialize_conversion_of_integer_to_bigint' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:issue) { create(:issue, project: project) } + let!(:event) do + create(:event, :created, project: project, target: issue, author: user) + end + + context 'in a transaction' do + it 'raises RuntimeError' do + allow(model).to receive(:transaction_open?).and_return(true) + + expect { model.initialize_conversion_of_integer_to_bigint(:events, :id) } + .to raise_error(RuntimeError) + end + end + + context 'outside a transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + it 'creates a bigint column and starts backfilling it' do + expect(model) + .to receive(:add_column) + .with( + :events, + 'id_convert_to_bigint', + :bigint, + default: 0, + null: false + ) + + expect(model) + .to receive(:install_rename_triggers) + .with(:events, :id, 'id_convert_to_bigint') + + expect(model).to receive(:queue_background_migration_jobs_by_range_at_intervals).and_call_original + + expect(BackgroundMigrationWorker) + .to receive(:perform_in) + .ordered + .with( + 2.minutes, + 'CopyColumnUsingBackgroundMigrationJob', + [event.id, event.id, :events, :id, :id, 'id_convert_to_bigint', 100] + ) + + expect(Gitlab::BackgroundMigration) + .to receive(:steal) + .ordered + .with('CopyColumnUsingBackgroundMigrationJob') + + model.initialize_conversion_of_integer_to_bigint( + :events, + :id, + batch_size: 300, + sub_batch_size: 100 + ) + end + end + end + describe '#index_exists_by_name?' do it 'returns true if an index exists' do ActiveRecord::Base.connection.execute( diff --git a/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb b/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb index 56399941662..ec89f2ed61c 100644 --- a/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::PartitionCreator do - include PartitioningHelpers + include Database::PartitioningHelpers include ExclusiveLeaseHelpers describe '.register' do diff --git a/spec/lib/gitlab/database/partitioning/replace_table_spec.rb b/spec/lib/gitlab/database/partitioning/replace_table_spec.rb index d47666eeffd..8e27797208c 100644 --- a/spec/lib/gitlab/database/partitioning/replace_table_spec.rb +++ b/spec/lib/gitlab/database/partitioning/replace_table_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::ReplaceTable, '#perform' do - include TableSchemaHelpers + include Database::TableSchemaHelpers subject(:replace_table) { described_class.new(original_table, replacement_table, archived_table, 'id').perform } diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb index 7d88c17c9b3..93dbd9d7c30 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers do - include TriggerHelpers + include Database::TriggerHelpers let(:model) do ActiveRecord::Migration.new.extend(described_class) diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb index 7f61ff759fc..603f3dc41af 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do - include TableSchemaHelpers + include Database::TableSchemaHelpers let(:migration) do ActiveRecord::Migration.new.extend(described_class) diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb index f10ff704c17..b50e02c7043 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb @@ -3,25 +3,36 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers do - include PartitioningHelpers - include TriggerHelpers - include TableSchemaHelpers + include Database::PartitioningHelpers + include Database::TriggerHelpers + include Database::TableSchemaHelpers let(:migration) do ActiveRecord::Migration.new.extend(described_class) end let_it_be(:connection) { ActiveRecord::Base.connection } - let(:source_table) { :audit_events } + let(:source_table) { :_test_original_table } let(:partitioned_table) { '_test_migration_partitioned_table' } let(:function_name) { '_test_migration_function_name' } let(:trigger_name) { '_test_migration_trigger_name' } let(:partition_column) { 'created_at' } let(:min_date) { Date.new(2019, 12) } let(:max_date) { Date.new(2020, 3) } + let(:source_model) { Class.new(ActiveRecord::Base) } before do allow(migration).to receive(:puts) + + migration.create_table source_table do |t| + t.string :name, null: false + t.integer :age, null: false + t.datetime partition_column + t.datetime :updated_at + end + + source_model.table_name = source_table + allow(migration).to receive(:transaction_open?).and_return(false) allow(migration).to receive(:make_partitioned_table_name).and_return(partitioned_table) allow(migration).to receive(:make_sync_function_name).and_return(function_name) @@ -81,14 +92,11 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end context 'when the given table does not have a primary key' do - let(:source_table) { :_partitioning_migration_helper_test_table } - let(:partition_column) { :some_field } - it 'raises an error' do - migration.create_table source_table, id: false do |t| - t.integer :id - t.datetime partition_column - end + migration.execute(<<~SQL) + ALTER TABLE #{source_table} + DROP CONSTRAINT #{source_table}_pkey + SQL expect do migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date @@ -97,12 +105,12 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end context 'when an invalid partition column is given' do - let(:partition_column) { :_this_is_not_real } + let(:invalid_column) { :_this_is_not_real } it 'raises an error' do expect do - migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date - end.to raise_error(/partition column #{partition_column} does not exist/) + migration.partition_table_by_date source_table, invalid_column, min_date: min_date, max_date: max_date + end.to raise_error(/partition column #{invalid_column} does not exist/) end end @@ -126,19 +134,19 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe context 'with a non-integer primary key datatype' do before do - connection.create_table :another_example, id: false do |t| + connection.create_table non_int_table, id: false do |t| t.string :identifier, primary_key: true t.timestamp :created_at end end - let(:source_table) { :another_example } + let(:non_int_table) { :another_example } let(:old_primary_key) { 'identifier' } it 'does not change the primary key datatype' do - migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date + migration.partition_table_by_date non_int_table, partition_column, min_date: min_date, max_date: max_date - original_pk_column = connection.columns(source_table).find { |c| c.name == old_primary_key } + original_pk_column = connection.columns(non_int_table).find { |c| c.name == old_primary_key } pk_column = connection.columns(partitioned_table).find { |c| c.name == old_primary_key } expect(pk_column).not_to be_nil @@ -176,11 +184,9 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end context 'when min_date is not given' do - let(:source_table) { :todos } - context 'with records present already' do before do - create(:todo, created_at: Date.parse('2019-11-05')) + source_model.create!(name: 'Test', age: 10, created_at: Date.parse('2019-11-05')) end it 'creates a partition spanning over each month from the first record' do @@ -248,13 +254,12 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end describe 'keeping data in sync with the partitioned table' do - let(:source_table) { :todos } - let(:model) { Class.new(ActiveRecord::Base) } + let(:partitioned_model) { Class.new(ActiveRecord::Base) } let(:timestamp) { Time.utc(2019, 12, 1, 12).round } before do - model.primary_key = :id - model.table_name = partitioned_table + partitioned_model.primary_key = :id + partitioned_model.table_name = partitioned_table end it 'creates a trigger function on the original table' do @@ -270,50 +275,50 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe it 'syncs inserts to the partitioned tables' do migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date - expect(model.count).to eq(0) + expect(partitioned_model.count).to eq(0) - first_todo = create(:todo, created_at: timestamp, updated_at: timestamp) - second_todo = create(:todo, created_at: timestamp, updated_at: timestamp) + first_record = source_model.create!(name: 'Bob', age: 20, created_at: timestamp, updated_at: timestamp) + second_record = source_model.create!(name: 'Alice', age: 30, created_at: timestamp, updated_at: timestamp) - expect(model.count).to eq(2) - expect(model.find(first_todo.id).attributes).to eq(first_todo.attributes) - expect(model.find(second_todo.id).attributes).to eq(second_todo.attributes) + expect(partitioned_model.count).to eq(2) + expect(partitioned_model.find(first_record.id).attributes).to eq(first_record.attributes) + expect(partitioned_model.find(second_record.id).attributes).to eq(second_record.attributes) end it 'syncs updates to the partitioned tables' do migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date - first_todo = create(:todo, :pending, commit_id: nil, created_at: timestamp, updated_at: timestamp) - second_todo = create(:todo, created_at: timestamp, updated_at: timestamp) + first_record = source_model.create!(name: 'Bob', age: 20, created_at: timestamp, updated_at: timestamp) + second_record = source_model.create!(name: 'Alice', age: 30, created_at: timestamp, updated_at: timestamp) - expect(model.count).to eq(2) + expect(partitioned_model.count).to eq(2) - first_copy = model.find(first_todo.id) - second_copy = model.find(second_todo.id) + first_copy = partitioned_model.find(first_record.id) + second_copy = partitioned_model.find(second_record.id) - expect(first_copy.attributes).to eq(first_todo.attributes) - expect(second_copy.attributes).to eq(second_todo.attributes) + expect(first_copy.attributes).to eq(first_record.attributes) + expect(second_copy.attributes).to eq(second_record.attributes) - first_todo.update(state_event: 'done', commit_id: 'abc123', updated_at: timestamp + 1.second) + first_record.update!(age: 21, updated_at: timestamp + 1.hour) - expect(model.count).to eq(2) - expect(first_copy.reload.attributes).to eq(first_todo.attributes) - expect(second_copy.reload.attributes).to eq(second_todo.attributes) + expect(partitioned_model.count).to eq(2) + expect(first_copy.reload.attributes).to eq(first_record.attributes) + expect(second_copy.reload.attributes).to eq(second_record.attributes) end it 'syncs deletes to the partitioned tables' do migration.partition_table_by_date source_table, partition_column, min_date: min_date, max_date: max_date - first_todo = create(:todo, created_at: timestamp, updated_at: timestamp) - second_todo = create(:todo, created_at: timestamp, updated_at: timestamp) + first_record = source_model.create!(name: 'Bob', age: 20, created_at: timestamp, updated_at: timestamp) + second_record = source_model.create!(name: 'Alice', age: 30, created_at: timestamp, updated_at: timestamp) - expect(model.count).to eq(2) + expect(partitioned_model.count).to eq(2) - first_todo.destroy + first_record.destroy! - expect(model.count).to eq(1) - expect(model.find_by_id(first_todo.id)).to be_nil - expect(model.find(second_todo.id).attributes).to eq(second_todo.attributes) + expect(partitioned_model.count).to eq(1) + expect(partitioned_model.find_by_id(first_record.id)).to be_nil + expect(partitioned_model.find(second_record.id).attributes).to eq(second_record.attributes) end end end @@ -388,13 +393,12 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end context 'when records exist in the source table' do - let(:source_table) { 'todos' } let(:migration_class) { '::Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable' } let(:sub_batch_size) { described_class::SUB_BATCH_SIZE } let(:pause_seconds) { described_class::PAUSE_SECONDS } - let!(:first_id) { create(:todo).id } - let!(:second_id) { create(:todo).id } - let!(:third_id) { create(:todo).id } + let!(:first_id) { source_model.create!(name: 'Bob', age: 20).id } + let!(:second_id) { source_model.create!(name: 'Alice', age: 30).id } + let!(:third_id) { source_model.create!(name: 'Sam', age: 40).id } before do stub_const("#{described_class.name}::BATCH_SIZE", 2) @@ -410,10 +414,10 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe expect(BackgroundMigrationWorker.jobs.size).to eq(2) - first_job_arguments = [first_id, second_id, source_table, partitioned_table, 'id'] + first_job_arguments = [first_id, second_id, source_table.to_s, partitioned_table, 'id'] expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([migration_class, first_job_arguments]) - second_job_arguments = [third_id, third_id, source_table, partitioned_table, 'id'] + second_job_arguments = [third_id, third_id, source_table.to_s, partitioned_table, 'id'] expect(BackgroundMigrationWorker.jobs[1]['args']).to eq([migration_class, second_job_arguments]) end end @@ -482,7 +486,6 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe end describe '#finalize_backfilling_partitioned_table' do - let(:source_table) { 'todos' } let(:source_column) { 'id' } context 'when the table is not allowed' do @@ -536,27 +539,27 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe context 'when there is missed data' do let(:partitioned_model) { Class.new(ActiveRecord::Base) } let(:timestamp) { Time.utc(2019, 12, 1, 12).round } - let!(:todo1) { create(:todo, created_at: timestamp, updated_at: timestamp) } - let!(:todo2) { create(:todo, created_at: timestamp, updated_at: timestamp) } - let!(:todo3) { create(:todo, created_at: timestamp, updated_at: timestamp) } - let!(:todo4) { create(:todo, created_at: timestamp, updated_at: timestamp) } + let!(:record1) { source_model.create!(name: 'Bob', age: 20, created_at: timestamp, updated_at: timestamp) } + let!(:record2) { source_model.create!(name: 'Alice', age: 30, created_at: timestamp, updated_at: timestamp) } + let!(:record3) { source_model.create!(name: 'Sam', age: 40, created_at: timestamp, updated_at: timestamp) } + let!(:record4) { source_model.create!(name: 'Sue', age: 50, created_at: timestamp, updated_at: timestamp) } let!(:pending_job1) do create(:background_migration_job, class_name: described_class::MIGRATION_CLASS_NAME, - arguments: [todo1.id, todo2.id, source_table, partitioned_table, source_column]) + arguments: [record1.id, record2.id, source_table, partitioned_table, source_column]) end let!(:pending_job2) do create(:background_migration_job, class_name: described_class::MIGRATION_CLASS_NAME, - arguments: [todo3.id, todo3.id, source_table, partitioned_table, source_column]) + arguments: [record3.id, record3.id, source_table, partitioned_table, source_column]) end let!(:succeeded_job) do create(:background_migration_job, :succeeded, class_name: described_class::MIGRATION_CLASS_NAME, - arguments: [todo4.id, todo4.id, source_table, partitioned_table, source_column]) + arguments: [record4.id, record4.id, source_table, partitioned_table, source_column]) end before do @@ -575,17 +578,17 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe it 'idempotently cleans up after failed background migrations' do expect(partitioned_model.count).to eq(0) - partitioned_model.insert!(todo2.attributes) + partitioned_model.insert!(record2.attributes) expect_next_instance_of(Gitlab::Database::PartitioningMigrationHelpers::BackfillPartitionedTable) do |backfill| allow(backfill).to receive(:transaction_open?).and_return(false) expect(backfill).to receive(:perform) - .with(todo1.id, todo2.id, source_table, partitioned_table, source_column) + .with(record1.id, record2.id, source_table, partitioned_table, source_column) .and_call_original expect(backfill).to receive(:perform) - .with(todo3.id, todo3.id, source_table, partitioned_table, source_column) + .with(record3.id, record3.id, source_table, partitioned_table, source_column) .and_call_original end @@ -593,12 +596,12 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::TableManagementHe expect(partitioned_model.count).to eq(3) - [todo1, todo2, todo3].each do |original| + [record1, record2, record3].each do |original| copy = partitioned_model.find(original.id) expect(copy.attributes).to eq(original.attributes) end - expect(partitioned_model.find_by_id(todo4.id)).to be_nil + expect(partitioned_model.find_by_id(record4.id)).to be_nil [pending_job1, pending_job2].each do |job| expect(job.reload).to be_succeeded diff --git a/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb b/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb index 934e2274358..2c550f14a08 100644 --- a/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb +++ b/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb @@ -24,107 +24,48 @@ RSpec.describe Gitlab::Database::PostgresHll::BatchDistinctCounter do allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(in_transaction) end - context 'different distribution of relation records' do - [10, 100, 100_000].each do |spread| - context "records are spread within #{spread}" do - before do - ids = (1..spread).to_a.sample(10) - create_list(:issue, 10).each_with_index do |issue, i| - issue.id = ids[i] - end - end - - it 'counts table' do - expect(described_class.new(model).estimate_distinct_count).to be_within(error_rate).percent_of(10) - end - end - end - end - context 'unit test for different counting parameters' do before_all do create_list(:issue, 3, author: user) create_list(:issue, 2, author: another_user) end - describe '#estimate_distinct_count' do - it 'counts table' do - expect(described_class.new(model).estimate_distinct_count).to be_within(error_rate).percent_of(5) - end - - it 'counts with column field' do - expect(described_class.new(model, column).estimate_distinct_count).to be_within(error_rate).percent_of(2) - end - - it 'counts with :id field' do - expect(described_class.new(model, :id).estimate_distinct_count).to be_within(error_rate).percent_of(5) - end - - it 'counts with "id" field' do - expect(described_class.new(model, "id").estimate_distinct_count).to be_within(error_rate).percent_of(5) - end - - it 'counts with table.column field' do - expect(described_class.new(model, "#{model.table_name}.#{column}").estimate_distinct_count).to be_within(error_rate).percent_of(2) - end - - it 'counts with Arel column' do - expect(described_class.new(model, model.arel_table[column]).estimate_distinct_count).to be_within(error_rate).percent_of(2) - end - - it 'counts over joined relations' do - expect(described_class.new(model.joins(:author), "users.email").estimate_distinct_count).to be_within(error_rate).percent_of(2) - end - - it 'counts with :column field with batch_size of 50K' do - expect(described_class.new(model, column).estimate_distinct_count(batch_size: 50_000)).to be_within(error_rate).percent_of(2) - end - - it 'will not count table with a batch size less than allowed' do - expect(described_class.new(model, column).estimate_distinct_count(batch_size: small_batch_size)).to eq(fallback) - end - - it 'counts with different number of batches and aggregates total result' do - stub_const('Gitlab::Database::PostgresHll::BatchDistinctCounter::MIN_REQUIRED_BATCH_SIZE', 0) - - [1, 2, 4, 5, 6].each { |i| expect(described_class.new(model).estimate_distinct_count(batch_size: i)).to be_within(error_rate).percent_of(5) } - end - - it 'counts with a start and finish' do - expect(described_class.new(model, column).estimate_distinct_count(start: model.minimum(:id), finish: model.maximum(:id))).to be_within(error_rate).percent_of(2) + describe '#execute' do + it 'builds hll buckets' do + expect(described_class.new(model).execute).to be_an_instance_of(Gitlab::Database::PostgresHll::Buckets) end - it "defaults the batch size to #{Gitlab::Database::PostgresHll::BatchDistinctCounter::DEFAULT_BATCH_SIZE}" do + it "defaults batch size to #{Gitlab::Database::PostgresHll::BatchDistinctCounter::DEFAULT_BATCH_SIZE}" do min_id = model.minimum(:id) batch_end_id = min_id + calculate_batch_size(Gitlab::Database::PostgresHll::BatchDistinctCounter::DEFAULT_BATCH_SIZE) expect(model).to receive(:where).with("id" => min_id..batch_end_id).and_call_original - described_class.new(model).estimate_distinct_count + described_class.new(model).execute end context 'when a transaction is open' do let(:in_transaction) { true } it 'raises an error' do - expect { described_class.new(model, column).estimate_distinct_count }.to raise_error('BatchCount can not be run inside a transaction') + expect { described_class.new(model, column).execute }.to raise_error('BatchCount can not be run inside a transaction') end end context 'disallowed configurations' do let(:default_batch_size) { Gitlab::Database::PostgresHll::BatchDistinctCounter::DEFAULT_BATCH_SIZE } - it 'returns fallback if start is bigger than finish' do - expect(described_class.new(model, column).estimate_distinct_count(start: 1, finish: 0)).to eq(fallback) + it 'raises WRONG_CONFIGURATION_ERROR if start is bigger than finish' do + expect { described_class.new(model, column).execute(start: 1, finish: 0) }.to raise_error(described_class::WRONG_CONFIGURATION_ERROR) end - it 'returns fallback if data volume exceeds upper limit' do + it 'raises WRONG_CONFIGURATION_ERROR if data volume exceeds upper limit' do large_finish = Gitlab::Database::PostgresHll::BatchDistinctCounter::MAX_DATA_VOLUME + 1 - expect(described_class.new(model, column).estimate_distinct_count(start: 1, finish: large_finish)).to eq(fallback) + expect { described_class.new(model, column).execute(start: 1, finish: large_finish) }.to raise_error(described_class::WRONG_CONFIGURATION_ERROR) end - it 'returns fallback if batch size is less than min required' do - expect(described_class.new(model, column).estimate_distinct_count(batch_size: small_batch_size)).to eq(fallback) + it 'raises WRONG_CONFIGURATION_ERROR if batch size is less than min required' do + expect { described_class.new(model, column).execute(batch_size: small_batch_size) }.to raise_error(described_class::WRONG_CONFIGURATION_ERROR) end end end diff --git a/spec/lib/gitlab/database/postgres_hll/buckets_spec.rb b/spec/lib/gitlab/database/postgres_hll/buckets_spec.rb new file mode 100644 index 00000000000..b4d8fd4a449 --- /dev/null +++ b/spec/lib/gitlab/database/postgres_hll/buckets_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::PostgresHll::Buckets do + let(:error_rate) { Gitlab::Database::PostgresHll::BatchDistinctCounter::ERROR_RATE } # HyperLogLog is a probabilistic algorithm, which provides estimated data, with given error margin + let(:buckets_hash_5) { { 121 => 2, 126 => 1, 141 => 1, 383 => 1, 56 => 1 } } + let(:buckets_hash_2) { { 141 => 1, 56 => 1 } } + + describe '#estimated_distinct_count' do + it 'provides estimated cardinality', :aggregate_failures do + expect(described_class.new(buckets_hash_5).estimated_distinct_count).to be_within(error_rate).percent_of(5) + expect(described_class.new(buckets_hash_2).estimated_distinct_count).to be_within(error_rate).percent_of(2) + expect(described_class.new({}).estimated_distinct_count).to eq 0 + expect(described_class.new.estimated_distinct_count).to eq 0 + end + end + + describe '#merge_hash!' do + let(:hash_a) { { 1 => 1, 2 => 3 } } + let(:hash_b) { { 1 => 2, 2 => 1 } } + + it 'merges two hashes together into union of two sets' do + expect(described_class.new(hash_a).merge_hash!(hash_b).to_json).to eq described_class.new(1 => 2, 2 => 3).to_json + end + end + + describe '#to_json' do + it 'serialize HyperLogLog buckets as hash' do + expect(described_class.new(1 => 5).to_json).to eq '{"1":5}' + end + end +end diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index f45d959c0de..ae6362ba812 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -3,65 +3,79 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Reindexing::Coordinator do + include Database::DatabaseHelpers include ExclusiveLeaseHelpers describe '.perform' do - subject { described_class.new(indexes).perform } + subject { described_class.new(index, notifier).perform } - let(:indexes) { [instance_double(Gitlab::Database::PostgresIndex), instance_double(Gitlab::Database::PostgresIndex)] } - let(:reindexers) { [instance_double(Gitlab::Database::Reindexing::ConcurrentReindex), instance_double(Gitlab::Database::Reindexing::ConcurrentReindex)] } + before do + swapout_view_for_table(:postgres_indexes) + + allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer) + allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) + end + + let(:index) { create(:postgres_index) } + let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) } + let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ConcurrentReindex, perform: nil) } + let(:action) { create(:reindex_action, index: index) } let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) } let(:lease_key) { 'gitlab/database/reindexing/coordinator' } let(:lease_timeout) { 1.day } let(:uuid) { 'uuid' } - before do - allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).and_yield + context 'locking' do + it 'acquires a lock while reindexing' do + expect(lease).to receive(:try_obtain).ordered.and_return(uuid) - indexes.zip(reindexers).each do |index, reindexer| - allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer) - allow(reindexer).to receive(:perform) - end - end + expect(reindexer).to receive(:perform).ordered - it 'performs concurrent reindexing for each index' do - indexes.zip(reindexers).each do |index, reindexer| - expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).ordered.and_return(reindexer) - expect(reindexer).to receive(:perform) + expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) + + subject end - subject + it 'does not perform reindexing actions if lease is not granted' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new) + + subject + end end - it 'keeps track of actions and creates ReindexAction records' do - indexes.each do |index| - expect(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).with(index).and_yield + context 'notifications' do + it 'sends #notify_start before reindexing' do + expect(notifier).to receive(:notify_start).with(action).ordered + expect(reindexer).to receive(:perform).ordered + + subject end - subject + it 'sends #notify_end after reindexing and updating the action is done' do + expect(action).to receive(:finish).ordered + expect(notifier).to receive(:notify_end).with(action).ordered + + subject + end end - context 'locking' do - it 'acquires a lock while reindexing' do - indexes.each do |index| - expect(lease).to receive(:try_obtain).ordered.and_return(uuid) - action = instance_double(Gitlab::Database::Reindexing::ConcurrentReindex) - expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).ordered.with(index).and_return(action) - expect(action).to receive(:perform).ordered - expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) - end + context 'action tracking' do + it 'calls #finish on the action' do + expect(reindexer).to receive(:perform).ordered + expect(action).to receive(:finish).ordered subject end - it 'does does not perform reindexing actions if lease is not granted' do - indexes.each do |index| - expect(lease).to receive(:try_obtain).ordered.and_return(false) - expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new) - end + it 'upon error, it still calls finish and raises the error' do + expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong') + expect(action).to receive(:finish).ordered - subject + expect { subject }.to raise_error(/something went wrong/) + + expect(action).to be_failed end end end diff --git a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb new file mode 100644 index 00000000000..e76718fe48a --- /dev/null +++ b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do + include Database::DatabaseHelpers + + let(:api_key) { "foo" } + let(:api_url) { "http://bar"} + let(:additional_tag) { "some-tag" } + + let(:action) { create(:reindex_action) } + + before do + swapout_view_for_table(:postgres_indexes) + end + + let(:headers) do + { + 'Content-Type': 'application/json', + 'Authorization': "Bearer #{api_key}" + } + end + + let(:response) { double('response', success?: true) } + + def expect_api_call(payload) + expect(Gitlab::HTTP).to receive(:post).with("#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).and_return(response) + end + + shared_examples_for 'interacting with Grafana annotations API' do + it 'POSTs a JSON payload' do + expect_api_call(payload) + + expect(subject).to be_truthy + end + + context 'on error' do + it 'does not raise the error and returns false' do + allow(Gitlab::HTTP).to receive(:post).and_raise('something went wrong') + + expect(subject).to be_falsey + end + + context 'when request was not successful' do + it 'returns false' do + expect_api_call(payload) + allow(response).to receive(:success?).and_return(false) + + expect(subject).to be_falsey + end + end + end + + context 'without api_key' do + let(:api_key) { '' } + + it 'does not post anything' do + expect(Gitlab::HTTP).not_to receive(:post) + + expect(subject).to be_falsey + end + end + + context 'without api_url' do + let(:api_url) { '' } + + it 'does not post anything' do + expect(Gitlab::HTTP).not_to receive(:post) + + expect(subject).to be_falsey + end + end + end + + describe '#notify_start' do + context 'additional tag is nil' do + subject { described_class.new(api_key, api_url, nil).notify_start(action) } + + let(:payload) do + { + time: (action.action_start.utc.to_f * 1000).to_i, + tags: ['reindex', action.index.tablename, action.index.name], + text: "Started reindexing of #{action.index.name} on #{action.index.tablename}" + } + end + + it_behaves_like 'interacting with Grafana annotations API' + end + + context 'additional tag is not nil' do + subject { described_class.new(api_key, api_url, additional_tag).notify_start(action) } + + let(:payload) do + { + time: (action.action_start.utc.to_f * 1000).to_i, + tags: ['reindex', additional_tag, action.index.tablename, action.index.name], + text: "Started reindexing of #{action.index.name} on #{action.index.tablename}" + } + end + + it_behaves_like 'interacting with Grafana annotations API' + end + end + + describe '#notify_end' do + context 'additional tag is nil' do + subject { described_class.new(api_key, api_url, nil).notify_end(action) } + + let(:payload) do + { + time: (action.action_start.utc.to_f * 1000).to_i, + tags: ['reindex', action.index.tablename, action.index.name], + text: "Finished reindexing of #{action.index.name} on #{action.index.tablename} (#{action.state})", + timeEnd: (action.action_end.utc.to_f * 1000).to_i, + isRegion: true + } + end + + it_behaves_like 'interacting with Grafana annotations API' + end + + context 'additional tag is not nil' do + subject { described_class.new(api_key, api_url, additional_tag).notify_end(action) } + + let(:payload) do + { + time: (action.action_start.utc.to_f * 1000).to_i, + tags: ['reindex', additional_tag, action.index.tablename, action.index.name], + text: "Finished reindexing of #{action.index.name} on #{action.index.tablename} (#{action.state})", + timeEnd: (action.action_end.utc.to_f * 1000).to_i, + isRegion: true + } + end + + it_behaves_like 'interacting with Grafana annotations API' + end + end +end diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index a5e2f368f40..4466679a099 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Reindexing::IndexSelection do - include DatabaseHelpers + include Database::DatabaseHelpers subject { described_class.new(Gitlab::Database::PostgresIndex.all).to_a } diff --git a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb index 225f23d2135..a8f196d8f0e 100644 --- a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb +++ b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb @@ -2,91 +2,83 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::ReindexAction, '.keep_track_of' do - let(:index) { double('index', identifier: 'public.something', ondisk_size_bytes: 10240, reload: nil, bloat_size: 42) } - let(:size_after) { 512 } +RSpec.describe Gitlab::Database::Reindexing::ReindexAction do + include Database::DatabaseHelpers - it 'yields to the caller' do - expect { |b| described_class.keep_track_of(index, &b) }.to yield_control - end + let(:index) { create(:postgres_index) } - def find_record - described_class.find_by(index_identifier: index.identifier) + before_all do + swapout_view_for_table(:postgres_indexes) end - it 'creates the record with a start time and updates its end time' do - freeze_time do - described_class.keep_track_of(index) do - expect(find_record.action_start).to be_within(1.second).of(Time.zone.now) + describe '.create_for' do + subject { described_class.create_for(index) } - travel(10.seconds) - end + it 'creates a new record for the given index' do + freeze_time do + record = subject - duration = find_record.action_end - find_record.action_start + expect(record.index_identifier).to eq(index.identifier) + expect(record.action_start).to eq(Time.zone.now) + expect(record.ondisk_size_bytes_start).to eq(index.ondisk_size_bytes) + expect(subject.bloat_estimate_bytes_start).to eq(index.bloat_size) - expect(duration).to be_within(1.second).of(10.seconds) + expect(record).to be_persisted + end end end - it 'creates the record with its status set to :started and updates its state to :finished' do - described_class.keep_track_of(index) do - expect(find_record).to be_started - end + describe '#finish' do + subject { action.finish } - expect(find_record).to be_finished - end + let(:action) { build(:reindex_action, index: index) } - it 'creates the record with the indexes start size and updates its end size' do - described_class.keep_track_of(index) do - expect(find_record.ondisk_size_bytes_start).to eq(index.ondisk_size_bytes) + it 'sets #action_end' do + freeze_time do + subject - expect(index).to receive(:reload).once - allow(index).to receive(:ondisk_size_bytes).and_return(size_after) + expect(action.action_end).to eq(Time.zone.now) + end end - expect(find_record.ondisk_size_bytes_end).to eq(size_after) - end + it 'sets #ondisk_size_bytes_end after reloading the index record' do + new_size = 4711 + expect(action.index).to receive(:reload).ordered + expect(action.index).to receive(:ondisk_size_bytes).and_return(new_size).ordered + + subject - it 'creates the record with the indexes bloat estimate' do - described_class.keep_track_of(index) do - expect(find_record.bloat_estimate_bytes_start).to eq(index.bloat_size) + expect(action.ondisk_size_bytes_end).to eq(new_size) end - end - context 'in case of errors' do - it 'sets the state to failed' do - expect do - described_class.keep_track_of(index) do - raise 'something went wrong' - end - end.to raise_error(/something went wrong/) + context 'setting #state' do + it 'sets #state to finished if not given' do + action.state = nil - expect(find_record).to be_failed - end + subject - it 'records the end time' do - freeze_time do - expect do - described_class.keep_track_of(index) do - raise 'something went wrong' - end - end.to raise_error(/something went wrong/) + expect(action).to be_finished + end + + it 'sets #state to finished if not set to started' do + action.state = :started - expect(find_record.action_end).to be_within(1.second).of(Time.zone.now) + subject + + expect(action).to be_finished end - end - it 'records the resulting index size' do - expect(index).to receive(:reload).once - allow(index).to receive(:ondisk_size_bytes).and_return(size_after) + it 'does not change state if set to failed' do + action.state = :failed + + expect { subject }.not_to change { action.state } + end + end - expect do - described_class.keep_track_of(index) do - raise 'something went wrong' - end - end.to raise_error(/something went wrong/) + it 'saves the record' do + expect(action).to receive(:save!) - expect(find_record.ondisk_size_bytes_end).to eq(size_after) + subject end end end diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index eb78a5fe8ea..b2f038e8b62 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -11,13 +11,16 @@ RSpec.describe Gitlab::Database::Reindexing do let(:coordinator) { instance_double(Gitlab::Database::Reindexing::Coordinator) } let(:index_selection) { instance_double(Gitlab::Database::Reindexing::IndexSelection) } let(:candidate_indexes) { double } - let(:indexes) { double } + let(:indexes) { [double, double] } it 'delegates to Coordinator' do expect(Gitlab::Database::Reindexing::IndexSelection).to receive(:new).with(candidate_indexes).and_return(index_selection) expect(index_selection).to receive(:take).with(2).and_return(indexes) - expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).with(indexes).and_return(coordinator) - expect(coordinator).to receive(:perform) + + indexes.each do |index| + expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).with(index).and_return(coordinator) + expect(coordinator).to receive(:perform) + end subject end |