Commit 24f2dcd1 authored by Nick Thomas's avatar Nick Thomas

Merge branch 'sh-skip-sti-tables-reltuples' into 'master'

Fix counting of groups in admin dashboard

Closes gitlab-ee#7435

See merge request gitlab-org/gitlab-ce!26009
parents ccbbc100 b35a6880
---
title: Fix counting of groups in admin dashboard
merge_request: 26009
author:
type: fixed
...@@ -37,6 +37,22 @@ module Gitlab ...@@ -37,6 +37,22 @@ module Gitlab
private private
# Models using single-type inheritance (STI) don't work with
# reltuple count estimates. We just have to ignore them and
# use another strategy to compute them.
def non_sti_models
models.reject { |model| sti_model?(model) }
end
def non_sti_table_names
non_sti_models.map(&:table_name)
end
def sti_model?(model)
model.column_names.include?(model.inheritance_column) &&
model.base_class != model
end
def table_names def table_names
models.map(&:table_name) models.map(&:table_name)
end end
...@@ -47,7 +63,7 @@ module Gitlab ...@@ -47,7 +63,7 @@ module Gitlab
# Querying tuple stats only works on the primary. Due to load balancing, the # Querying tuple stats only works on the primary. Due to load balancing, the
# easiest way to do this is to start a transaction. # easiest way to do this is to start a transaction.
ActiveRecord::Base.transaction do ActiveRecord::Base.transaction do
get_statistics(table_names, check_statistics: check_statistics).each_with_object({}) do |row, data| get_statistics(non_sti_table_names, check_statistics: check_statistics).each_with_object({}) do |row, data|
model = table_to_model[row.table_name] model = table_to_model[row.table_name]
data[model] = row.estimate data[model] = row.estimate
end end
......
...@@ -48,12 +48,20 @@ module Gitlab ...@@ -48,12 +48,20 @@ module Gitlab
end end
end end
def where_clause(model)
return unless sti_model?(model)
"WHERE #{model.inheritance_column} = '#{model.name}'"
end
def tablesample_count(model, estimate) def tablesample_count(model, estimate)
portion = (TABLESAMPLE_ROW_TARGET.to_f / estimate).round(4) portion = (TABLESAMPLE_ROW_TARGET.to_f / estimate).round(4)
inverse = 1 / portion inverse = 1 / portion
query = <<~SQL query = <<~SQL
SELECT (COUNT(*)*#{inverse})::integer AS count SELECT (COUNT(*)*#{inverse})::integer AS count
FROM #{model.table_name} TABLESAMPLE SYSTEM (#{portion * 100}) FROM #{model.table_name}
TABLESAMPLE SYSTEM (#{portion * 100})
#{where_clause(model)}
SQL SQL
rows = ActiveRecord::Base.connection.select_all(query) rows = ActiveRecord::Base.connection.select_all(query)
......
...@@ -6,10 +6,11 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do ...@@ -6,10 +6,11 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do
create(:identity) create(:identity)
end end
let(:models) { [Project, Identity] }
subject { described_class.new(models).count } subject { described_class.new(models).count }
describe '#count', :postgresql do describe '#count', :postgresql do
let(:models) { [Project, Identity] }
context 'when reltuples is up to date' do context 'when reltuples is up to date' do
before do before do
ActiveRecord::Base.connection.execute('ANALYZE projects') ActiveRecord::Base.connection.execute('ANALYZE projects')
...@@ -23,6 +24,22 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do ...@@ -23,6 +24,22 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do
end end
end end
context 'when models using single-type inheritance are used' do
let(:models) { [Group, CiService, Namespace] }
before do
models.each do |model|
ActiveRecord::Base.connection.execute("ANALYZE #{model.table_name}")
end
end
it 'returns nil counts for inherited tables' do
models.each { |model| expect(model).not_to receive(:count) }
expect(subject).to eq({ Namespace => 3 })
end
end
context 'insufficient permissions' do context 'insufficient permissions' do
it 'returns an empty hash' do it 'returns an empty hash' do
allow(ActiveRecord::Base).to receive(:transaction).and_raise(PG::InsufficientPrivilege) allow(ActiveRecord::Base).to receive(:transaction).and_raise(PG::InsufficientPrivilege)
......
...@@ -4,15 +4,23 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do ...@@ -4,15 +4,23 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do
before do before do
create_list(:project, 3) create_list(:project, 3)
create(:identity) create(:identity)
create(:group)
end end
let(:models) { [Project, Identity] } let(:models) { [Project, Identity, Group, Namespace] }
let(:strategy) { described_class.new(models) } let(:strategy) { described_class.new(models) }
subject { strategy.count } subject { strategy.count }
describe '#count', :postgresql do describe '#count', :postgresql do
let(:estimates) { { Project => threshold + 1, Identity => threshold - 1 } } let(:estimates) do
{
Project => threshold + 1,
Identity => threshold - 1,
Group => threshold + 1,
Namespace => threshold + 1
}
end
let(:threshold) { Gitlab::Database::Count::TablesampleCountStrategy::EXACT_COUNT_THRESHOLD } let(:threshold) { Gitlab::Database::Count::TablesampleCountStrategy::EXACT_COUNT_THRESHOLD }
before do before do
...@@ -30,9 +38,13 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do ...@@ -30,9 +38,13 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do
context 'for tables with an estimated large size' do context 'for tables with an estimated large size' do
it 'performs a tablesample count' do it 'performs a tablesample count' do
expect(Project).not_to receive(:count) expect(Project).not_to receive(:count)
expect(Group).not_to receive(:count)
expect(Namespace).not_to receive(:count)
result = subject result = subject
expect(result[Project]).to eq(3) expect(result[Project]).to eq(3)
expect(result[Group]).to eq(1)
expect(result[Namespace]).to eq(4)
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