Commit b35a6880 authored by Stan Hu's avatar Stan Hu

Fix counting of groups in admin dashboard

1. Ignore tables that use STI in reltuples count strategy.

   Models that use Rails' single-type inheritance, such as `Group` and
   `CiService`, need an additional WHERE clause to count the total
   properly, which isn't supported by the reltuples strategy.  For now,
   we just omit these from the statistics sampling and rely on the other
   strategies to get this data.

2. Fix tablesample count strategy not counting groups properly.

   Models such as `Group` needs a WHERE clause to distinguish it from
   namespaces. We now add in the WHERE clause if STI is in use.

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/7435
parent 121de6dc
---
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