Commit 4497aa1e authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch 'ab/reindexing-index-model' into 'master'

Reindexing: Automatic index selection

See merge request gitlab-org/gitlab!42967
parents 2c908cc1 38758bb9
---
title: 'Add database view for postgres indexes'
merge_request: 42967
author:
type: other
# frozen_string_literal: true
class AddPostgresIndexView < ActiveRecord::Migration[6.0]
DOWNTIME = false
def up
execute(<<~SQL)
CREATE VIEW postgres_indexes AS
SELECT
pg_namespace.nspname || '.' || pg_class.relname as identifier,
pg_index.indexrelid,
pg_namespace.nspname as schema,
pg_class.relname as name,
pg_index.indisunique as unique,
pg_index.indisvalid as valid_index,
pg_class.relispartition as partitioned,
pg_index.indisexclusion as exclusion,
pg_indexes.indexdef as definition,
pg_relation_size(pg_class.oid) as ondisk_size_bytes
FROM pg_index
INNER JOIN pg_class ON pg_class.oid = pg_index.indexrelid
INNER JOIN pg_namespace ON pg_class.relnamespace = pg_namespace.oid
INNER JOIN pg_indexes ON pg_class.relname = pg_indexes.indexname
WHERE pg_namespace.nspname <> 'pg_catalog'
SQL
end
def down
execute(<<~SQL)
DROP VIEW postgres_indexes
SQL
end
end
705d010620b1aa95e86c8fb5fb9175fe77778376d228003e9fd2c8d0bb20a347
\ No newline at end of file
......@@ -14411,6 +14411,23 @@ CREATE SEQUENCE pool_repositories_id_seq
ALTER SEQUENCE pool_repositories_id_seq OWNED BY pool_repositories.id;
CREATE VIEW postgres_indexes AS
SELECT (((pg_namespace.nspname)::text || '.'::text) || (pg_class.relname)::text) AS identifier,
pg_index.indexrelid,
pg_namespace.nspname AS schema,
pg_class.relname AS name,
pg_index.indisunique AS "unique",
pg_index.indisvalid AS valid_index,
pg_class.relispartition AS partitioned,
pg_index.indisexclusion AS exclusion,
pg_indexes.indexdef AS definition,
pg_relation_size((pg_class.oid)::regclass) AS ondisk_size_bytes
FROM (((pg_index
JOIN pg_class ON ((pg_class.oid = pg_index.indexrelid)))
JOIN pg_namespace ON ((pg_class.relnamespace = pg_namespace.oid)))
JOIN pg_indexes ON ((pg_class.relname = pg_indexes.indexname)))
WHERE (pg_namespace.nspname <> 'pg_catalog'::name);
CREATE TABLE programming_languages (
id integer NOT NULL,
name character varying NOT NULL,
......
# frozen_string_literal: true
module Gitlab
module Database
class PostgresIndex < ActiveRecord::Base
self.table_name = 'postgres_indexes'
self.primary_key = 'identifier'
scope :by_identifier, ->(identifier) do
raise ArgumentError, "Index name is not fully qualified with a schema: #{identifier}" unless identifier =~ /^\w+\.\w+$/
find(identifier)
end
# A 'regular' index is a non-unique index,
# that does not serve an exclusion constraint and
# is defined on a table that is not partitioned.
scope :regular, -> { where(unique: false, partitioned: false, exclusion: false)}
scope :random_few, ->(how_many) do
limit(how_many).order(Arel.sql('RANDOM()'))
end
def to_s
name
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Database
module Reindexing
def self.perform(index_selector)
Array.wrap(index_selector).each do |index|
ConcurrentReindex.new(index).perform
end
end
end
end
end
......@@ -22,6 +22,10 @@ module Gitlab
def perform
raise ReindexError, 'UNIQUE indexes are currently not supported' if index.unique?
raise ReindexError, 'partitioned indexes are currently not supported' if index.partitioned?
raise ReindexError, 'indexes serving an exclusion constraint are currently not supported' if index.exclusion?
logger.info "Starting reindex of #{index}"
with_rebuilt_index do |replacement_index|
swap_index(replacement_index)
......@@ -31,12 +35,13 @@ module Gitlab
private
def with_rebuilt_index
logger.debug("dropping dangling index from previous run (if it exists): #{replacement_index_name}")
remove_replacement_index
if Gitlab::Database::PostgresIndex.find_by(schema: index.schema, name: replacement_index_name)
logger.debug("dropping dangling index from previous run (if it exists): #{replacement_index_name}")
remove_index(index.schema, replacement_index_name)
end
create_replacement_index_statement = index.definition
.sub(/CREATE INDEX/, 'CREATE INDEX CONCURRENTLY')
.sub(/#{index.name}/, replacement_index_name)
.sub(/CREATE INDEX #{index.name}/, "CREATE INDEX CONCURRENTLY #{replacement_index_name}")
logger.info("creating replacement index #{replacement_index_name}")
logger.debug("replacement index definition: #{create_replacement_index_statement}")
......@@ -45,55 +50,57 @@ module Gitlab
connection.execute(create_replacement_index_statement)
end
replacement_index = Index.find_with_schema("#{index.schema}.#{replacement_index_name}")
replacement_index = Gitlab::Database::PostgresIndex.find_by(schema: index.schema, name: replacement_index_name)
unless replacement_index.valid?
unless replacement_index.valid_index?
message = 'replacement index was created as INVALID'
logger.error("#{message}, cleaning up")
raise ReindexError, "failed to reindex #{index}: #{message}"
end
yield replacement_index
rescue Gitlab::Database::WithLockRetries::AttemptsExhaustedError => e
logger.error('failed to obtain the required database locks to swap the indexes, cleaning up')
raise ReindexError, e.message
rescue ActiveRecord::ActiveRecordError, PG::Error => e
logger.error("database error while attempting reindex of #{index}: #{e.message}")
raise ReindexError, e.message
ensure
logger.info("dropping unneeded replacement index: #{replacement_index_name}")
remove_replacement_index
begin
remove_index(index.schema, replacement_index_name)
rescue => e
logger.error(e)
end
end
def swap_index(replacement_index)
replaced_index_name = constrained_index_name(REPLACED_INDEX_PREFIX)
logger.info("swapping replacement index #{replacement_index} with #{index}")
with_lock_retries do
rename_index(index.name, replaced_index_name)
rename_index(replacement_index.name, index.name)
rename_index(replaced_index_name, replacement_index.name)
rename_index(index.schema, index.name, replaced_index_name)
rename_index(replacement_index.schema, replacement_index.name, index.name)
rename_index(index.schema, replaced_index_name, replacement_index.name)
end
end
def rename_index(old_index_name, new_index_name)
connection.execute("ALTER INDEX #{old_index_name} RENAME TO #{new_index_name}")
def rename_index(schema, old_index_name, new_index_name)
connection.execute(<<~SQL)
ALTER INDEX #{quote_table_name(schema)}.#{quote_table_name(old_index_name)}
RENAME TO #{quote_table_name(new_index_name)}
SQL
end
def remove_replacement_index
def remove_index(schema, name)
logger.info("Removing index #{schema}.#{name}")
disable_statement_timeout do
connection.execute("DROP INDEX CONCURRENTLY IF EXISTS #{replacement_index_name}")
connection.execute(<<~SQL)
DROP INDEX CONCURRENTLY
IF EXISTS #{quote_table_name(schema)}.#{quote_table_name(name)}
SQL
end
end
def replacement_index_name
@replacement_index_name ||= constrained_index_name(TEMPORARY_INDEX_PREFIX)
@replacement_index_name ||= "#{TEMPORARY_INDEX_PREFIX}#{index.indexrelid}"
end
def constrained_index_name(prefix)
"#{prefix}#{index.name}".slice(0, PG_IDENTIFIER_LENGTH)
def replaced_index_name
@replaced_index_name ||= "#{REPLACED_INDEX_PREFIX}#{index.indexrelid}"
end
def with_lock_retries(&block)
......@@ -102,7 +109,7 @@ module Gitlab
Gitlab::Database::WithLockRetries.new(**arguments).run(raise_on_exhaustion: true, &block)
end
delegate :execute, to: :connection
delegate :execute, :quote_table_name, to: :connection
def connection
@connection ||= ActiveRecord::Base.connection
end
......
# frozen_string_literal: true
module Gitlab
module Database
module Reindexing
class Index
def self.find_with_schema(full_name)
raise ArgumentError, "Index name is not fully qualified with a schema: #{full_name}" unless full_name =~ /^\w+\.\w+$/
schema, index = full_name.split('.')
record = ActiveRecord::Base.connection.select_one(<<~SQL)
SELECT
pg_index.indisunique as is_unique,
pg_index.indisvalid as is_valid,
pg_indexes.indexdef as definition,
pg_namespace.nspname as schema,
pg_class.relname as name
FROM pg_index
INNER JOIN pg_class ON pg_class.oid = pg_index.indexrelid
INNER JOIN pg_namespace ON pg_class.relnamespace = pg_namespace.oid
INNER JOIN pg_indexes ON pg_class.relname = pg_indexes.indexname
WHERE pg_namespace.nspname = #{ActiveRecord::Base.connection.quote(schema)}
AND pg_class.relname = #{ActiveRecord::Base.connection.quote(index)}
SQL
return unless record
new(OpenStruct.new(record))
end
delegate :definition, :schema, :name, to: :@attrs
def initialize(attrs)
@attrs = attrs
end
def unique?
@attrs.is_unique
end
def valid?
@attrs.is_valid
end
def to_s
name
end
end
end
end
end
......@@ -174,14 +174,16 @@ namespace :gitlab do
exit
end
raise ArgumentError, 'must give the index name to reindex' unless args[:index_name]
index = Gitlab::Database::Reindexing::Index.find_with_schema(args[:index_name])
raise ArgumentError, "Given index does not exist: #{args[:index_name]}" unless index
puts "Rebuilding index #{index}".color(:green)
Gitlab::Database::Reindexing::ConcurrentReindex.new(index).perform
indexes = if args[:index_name]
Gitlab::Database::PostgresIndex.by_identifier(args[:index_name])
else
Gitlab::Database::PostgresIndex.regular.random_few(2)
end
Gitlab::Database::Reindexing.perform(indexes)
rescue => e
Gitlab::AppLogger.error(e)
raise
end
end
end
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Database::Reindexing::Index do
RSpec.describe Gitlab::Database::PostgresIndex do
before do
ActiveRecord::Base.connection.execute(<<~SQL)
CREATE INDEX foo_idx ON public.users (name);
......@@ -13,16 +13,16 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
end
def find(name)
described_class.find_with_schema(name)
described_class.by_identifier(name)
end
describe '.find_with_schema' do
it 'returns an instance of Gitlab::Database::Reindexing::Index when the index is present' do
expect(find('public.foo_idx')).to be_a(Gitlab::Database::Reindexing::Index)
describe '.by_identifier' do
it 'finds the index' do
expect(find('public.foo_idx')).to be_a(Gitlab::Database::PostgresIndex)
end
it 'returns nil if the index is not present' do
expect(find('public.idontexist')).to be_nil
it 'raises an error if not found' do
expect { find('public.idontexist') }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'raises ArgumentError if given a non-fully qualified index name' do
......@@ -30,6 +30,26 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
end
end
describe '#regular' do
it 'only non-unique indexes' do
expect(described_class.regular).to all(have_attributes(unique: false))
end
it 'only non partitioned indexes ' do
expect(described_class.regular).to all(have_attributes(partitioned: false))
end
it 'only indexes that dont serve an exclusion constraint' do
expect(described_class.regular).to all(have_attributes(exclusion: false))
end
end
describe '#random_few' do
it 'limits to two records by default' do
expect(described_class.random_few(2).size).to eq(2)
end
end
describe '#unique?' do
it 'returns true for a unique index' do
expect(find('public.bar_key')).to be_unique
......@@ -44,9 +64,9 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
end
end
describe '#valid?' do
it 'returns true if the index is valid' do
expect(find('public.foo_idx')).to be_valid
describe '#valid_index?' do
it 'returns true if the index is invalid' do
expect(find('public.foo_idx')).to be_valid_index
end
it 'returns false if the index is marked as invalid' do
......@@ -56,7 +76,7 @@ RSpec.describe Gitlab::Database::Reindexing::Index do
WHERE pg_class.relname = 'foo_idx' AND pg_index.indexrelid = pg_class.oid
SQL
expect(find('public.foo_idx')).not_to be_valid
expect(find('public.foo_idx')).not_to be_valid_index
end
end
......
......@@ -8,7 +8,7 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
let(:table_name) { '_test_reindex_table' }
let(:column_name) { '_test_column' }
let(:index_name) { '_test_reindex_index' }
let(:index) { double('index', name: index_name, schema: 'public', unique?: false, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
let(:index) { instance_double(Gitlab::Database::PostgresIndex, indexrelid: 42, name: index_name, schema: 'public', partitioned?: false, unique?: false, exclusion?: false, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
let(:logger) { double('logger', debug: nil, info: nil, error: nil ) }
let(:connection) { ActiveRecord::Base.connection }
......@@ -23,7 +23,9 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
end
context 'when the index is unique' do
let(:index) { double('index', name: index_name, unique?: true, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') }
before do
allow(index).to receive(:unique?).and_return(true)
end
it 'raises an error' do
expect do
......@@ -32,12 +34,41 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
end
end
context 'when the index is partitioned' do
before do
allow(index).to receive(:partitioned?).and_return(true)
end
it 'raises an error' do
expect do
subject.perform
end.to raise_error(described_class::ReindexError, /partitioned indexes are currently not supported/)
end
end
context 'when the index serves an exclusion constraint' do
before do
allow(index).to receive(:exclusion?).and_return(true)
end
it 'raises an error' do
expect do
subject.perform
end.to raise_error(described_class::ReindexError, /indexes serving an exclusion constraint are currently not supported/)
end
end
context 'replacing the original index with a rebuilt copy' do
let(:replacement_name) { 'tmp_reindex__test_reindex_index' }
let(:replaced_name) { 'old_reindex__test_reindex_index' }
let(:replacement_name) { 'tmp_reindex_42' }
let(:replaced_name) { 'old_reindex_42' }
let(:create_index) { "CREATE INDEX CONCURRENTLY #{replacement_name} ON public.#{table_name} USING btree (#{column_name})" }
let(:drop_index) { "DROP INDEX CONCURRENTLY IF EXISTS #{replacement_name}" }
let(:drop_index) do
<<~SQL
DROP INDEX CONCURRENTLY
IF EXISTS "public"."#{replacement_name}"
SQL
end
let!(:original_index) { find_index_create_statement }
......@@ -58,18 +89,17 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
end
it 'replaces the existing index with an identical index' do
expect(subject).to receive(:disable_statement_timeout).exactly(3).times.and_yield
expect(subject).to receive(:disable_statement_timeout).twice.and_yield
expect_to_execute_concurrently_in_order(drop_index)
expect_to_execute_concurrently_in_order(create_index)
expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
end
expect_to_execute_in_order("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
expect_to_execute_in_order("ALTER INDEX #{replacement_name} RENAME TO #{index.name}")
expect_to_execute_in_order("ALTER INDEX #{replaced_name} RENAME TO #{replacement_name}")
expect_index_rename(index.name, replaced_name)
expect_index_rename(replacement_name, index.name)
expect_index_rename(replaced_name, replacement_name)
expect_to_execute_concurrently_in_order(drop_index)
......@@ -93,9 +123,9 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
end
expect_to_execute_in_order("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
expect_to_execute_in_order("ALTER INDEX #{replacement_name} RENAME TO #{index.name}")
expect_to_execute_in_order("ALTER INDEX #{replaced_name} RENAME TO #{replacement_name}")
expect_index_rename(index.name, replaced_name)
expect_index_rename(replacement_name, index.name)
expect_index_rename(replaced_name, replacement_name)
expect_to_execute_concurrently_in_order(drop_index)
......@@ -107,14 +137,12 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
context 'when it fails to create the replacement index' do
it 'safely cleans up and signals the error' do
expect_to_execute_concurrently_in_order(drop_index)
expect(connection).to receive(:execute).with(create_index).ordered
.and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
expect_to_execute_concurrently_in_order(drop_index)
expect { subject.perform }.to raise_error(described_class::ReindexError, /connect timeout/)
expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/)
check_index_exists
end
......@@ -122,11 +150,10 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
context 'when the replacement index is not valid' do
it 'safely cleans up and signals the error' do
expect_to_execute_concurrently_in_order(drop_index)
expect_to_execute_concurrently_in_order(create_index)
replacement_index = double('replacement index', valid_index?: false)
allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index)
replacement_index = double('replacement index', valid?: false)
allow(Gitlab::Database::Reindexing::Index).to receive(:find_with_schema).with("public.#{replacement_name}").and_return(replacement_index)
expect_to_execute_concurrently_in_order(create_index)
expect_to_execute_concurrently_in_order(drop_index)
......@@ -138,20 +165,20 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
context 'when a database error occurs while swapping the indexes' do
it 'safely cleans up and signals the error' do
expect_to_execute_concurrently_in_order(drop_index)
replacement_index = double('replacement index', valid_index?: true)
allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index)
expect_to_execute_concurrently_in_order(create_index)
expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield
end
expect(connection).to receive(:execute).ordered
.with("ALTER INDEX #{index.name} RENAME TO #{replaced_name}")
.and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
expect_index_rename(index.name, replaced_name).and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout')
expect_to_execute_concurrently_in_order(drop_index)
expect { subject.perform }.to raise_error(described_class::ReindexError, /connect timeout/)
expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/)
check_index_exists
end
......@@ -159,7 +186,6 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
context 'when with_lock_retries fails to acquire the lock' do
it 'safely cleans up and signals the error' do
expect_to_execute_concurrently_in_order(drop_index)
expect_to_execute_concurrently_in_order(create_index)
expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance|
......@@ -169,7 +195,7 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
expect_to_execute_concurrently_in_order(drop_index)
expect { subject.perform }.to raise_error(described_class::ReindexError, /exhausted/)
expect { subject.perform }.to raise_error(::Gitlab::Database::WithLockRetries::AttemptsExhaustedError, /exhausted/)
check_index_exists
end
......@@ -185,8 +211,11 @@ RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do
end
end
def expect_to_execute_in_order(sql)
expect(connection).to receive(:execute).with(sql).ordered.and_call_original
def expect_index_rename(from, to)
expect(connection).to receive(:execute).with(<<~SQL).ordered
ALTER INDEX "public"."#{from}"
RENAME TO "#{to}"
SQL
end
def find_index_create_statement
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Database::Reindexing do
describe '.perform' do
context 'multiple indexes' do
let(:indexes) { [double, double] }
let(:reindexers) { [double, double] }
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)
end
described_class.perform(indexes)
end
end
context 'single index' do
let(:index) { double }
let(:reindexer) { double }
it 'performs concurrent reindexing for single index' do
expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer)
expect(reindexer).to receive(:perform)
described_class.perform(index)
end
end
end
end
......@@ -165,30 +165,32 @@ RSpec.describe 'gitlab:db namespace rake task' do
end
describe 'reindex' do
let(:reindex) { double('reindex') }
let(:indexes) { double('indexes') }
context 'when no index_name is given' do
it 'raises an error' do
expect do
run_rake_task('gitlab:db:reindex', '')
end.to raise_error(ArgumentError, /must give the index name/)
it 'rebuilds a random number of large indexes' do
expect(Gitlab::Database::PostgresIndex).to receive_message_chain('regular.random_few').and_return(indexes)
expect(Gitlab::Database::Reindexing).to receive(:perform).with(indexes)
run_rake_task('gitlab:db:reindex')
end
end
context 'with index name given' do
let(:index) { double('index') }
let(:reindex) { double('reindex') }
it 'calls the index rebuilder with the proper arguments' do
expect(Gitlab::Database::Reindexing::Index).to receive(:find_with_schema).with('public.foo_idx').and_return(index)
expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index, any_args).and_return(reindex)
expect(reindex).to receive(:perform)
expect(Gitlab::Database::PostgresIndex).to receive(:by_identifier).with('public.foo_idx').and_return(index)
expect(Gitlab::Database::Reindexing).to receive(:perform).with(index)
run_rake_task('gitlab:db:reindex', '[public.foo_idx]')
end
it 'raises an error if the index does not exist' do
expect(Gitlab::Database::Reindexing::Index).to receive(:find_with_schema).with('public.absent_index').and_return(nil)
expect(Gitlab::Database::PostgresIndex).to receive(:by_identifier).with('public.absent_index').and_raise(ActiveRecord::RecordNotFound)
expect { run_rake_task('gitlab:db:reindex', '[public.absent_index]') }.to raise_error(ArgumentError, /index does not exist/)
expect { run_rake_task('gitlab:db:reindex', '[public.absent_index]') }.to raise_error(ActiveRecord::RecordNotFound)
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