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

5 files changed, 63 insertions, 5 deletions

diff --git a/changelogs/unreleased/sh-skip-sti-tables-reltuples.yml b/changelogs/unreleased/sh-skip-sti-tables-reltuples.yml
new file mode 100644
index 00000000000..5bf0ccf3e9d
--- /dev/null
+++ b/changelogs/unreleased/sh-skip-sti-tables-reltuples.yml
@@ -0,0 +1,5 @@
+---
+title: Fix counting of groups in admin dashboard
+merge_request: 26009
+author:
+type: fixed
diff --git a/lib/gitlab/database/count/reltuples_count_strategy.rb b/lib/gitlab/database/count/reltuples_count_strategy.rb
index c3a674aeb7e..695f6fa766e 100644
--- a/lib/gitlab/database/count/reltuples_count_strategy.rb
+++ b/lib/gitlab/database/count/reltuples_count_strategy.rb
@@ -37,6 +37,22 @@ module Gitlab
 
         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
           models.map(&:table_name)
         end
@@ -47,7 +63,7 @@ module Gitlab
           # Querying tuple stats only works on the primary. Due to load balancing, the
           # easiest way to do this is to start a transaction.
           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]
               data[model] = row.estimate
             end
diff --git a/lib/gitlab/database/count/tablesample_count_strategy.rb b/lib/gitlab/database/count/tablesample_count_strategy.rb
index fedf6ca4fe1..4fcd68b297d 100644
--- a/lib/gitlab/database/count/tablesample_count_strategy.rb
+++ b/lib/gitlab/database/count/tablesample_count_strategy.rb
@@ -48,12 +48,20 @@ module Gitlab
           end
         end
 
+        def where_clause(model)
+          return unless sti_model?(model)
+
+          "WHERE #{model.inheritance_column} = '#{model.name}'"
+        end
+
         def tablesample_count(model, estimate)
           portion = (TABLESAMPLE_ROW_TARGET.to_f / estimate).round(4)
           inverse = 1 / portion
           query = <<~SQL
             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
 
           rows = ActiveRecord::Base.connection.select_all(query)
diff --git a/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb b/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb
index b44e8c5a110..bd3c66d0548 100644
--- a/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb
+++ b/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb
@@ -6,10 +6,11 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do
     create(:identity)
   end
 
-  let(:models) { [Project, Identity] }
   subject { described_class.new(models).count }
 
   describe '#count', :postgresql do
+    let(:models) { [Project, Identity] }
+
     context 'when reltuples is up to date' do
       before do
         ActiveRecord::Base.connection.execute('ANALYZE projects')
@@ -23,6 +24,22 @@ describe Gitlab::Database::Count::ReltuplesCountStrategy do
       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
       it 'returns an empty hash' do
         allow(ActiveRecord::Base).to receive(:transaction).and_raise(PG::InsufficientPrivilege)
diff --git a/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb b/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb
index 203f9344a41..40d810b195b 100644
--- a/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb
+++ b/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb
@@ -4,15 +4,23 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do
   before do
     create_list(:project, 3)
     create(:identity)
+    create(:group)
   end
 
-  let(:models) { [Project, Identity] }
+  let(:models) { [Project, Identity, Group, Namespace] }
   let(:strategy) { described_class.new(models) }
 
   subject { strategy.count }
 
   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 }
 
     before do
@@ -30,9 +38,13 @@ describe Gitlab::Database::Count::TablesampleCountStrategy do
     context 'for tables with an estimated large size' do
       it 'performs a tablesample count' do
         expect(Project).not_to receive(:count)
+        expect(Group).not_to receive(:count)
+        expect(Namespace).not_to receive(:count)
 
         result = subject
         expect(result[Project]).to eq(3)
+        expect(result[Group]).to eq(1)
+        expect(result[Namespace]).to eq(4)
       end
     end