diff options
| -rw-r--r-- | app/models/project.rb | 5 | ||||
| -rw-r--r-- | app/models/project_feature.rb | 26 | ||||
| -rw-r--r-- | app/models/site_statistic.rb | 74 | ||||
| -rw-r--r-- | changelogs/unreleased/ce-6064-geo-sql-query-for-counting-projects-with-wikis-is-very-slow.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20180629153018_create_site_statistics.rb | 18 | ||||
| -rw-r--r-- | db/post_migrate/20180706223200_populate_site_statistics.rb | 25 | ||||
| -rw-r--r-- | db/schema.rb | 5 | ||||
| -rw-r--r-- | spec/factories/projects.rb | 2 | ||||
| -rw-r--r-- | spec/factories/site_statistics.rb | 7 | ||||
| -rw-r--r-- | spec/models/project_feature_spec.rb | 42 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 16 | ||||
| -rw-r--r-- | spec/models/site_statistic_spec.rb | 83 |
12 files changed, 307 insertions, 1 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index f880d728839..32315dfaa01 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -31,6 +31,7 @@ class Project < ActiveRecord::Base BoardLimitExceeded = Class.new(StandardError) + STATISTICS_ATTRIBUTE = 'repositories_count'.freeze NUMBER_OF_PERMITTED_BOARDS = 1 UNKNOWN_IMPORT_URL = 'http://unknown.git'.freeze # Hashed Storage versions handle rolling out new storage to project and dependents models: @@ -79,6 +80,10 @@ class Project < ActiveRecord::Base after_create :create_project_feature, unless: :project_feature + after_create -> { SiteStatistic.track(STATISTICS_ATTRIBUTE) } + before_destroy ->(project) { project.project_feature.untrack_statistics_for_deletion! } + after_destroy -> { SiteStatistic.untrack(STATISTICS_ATTRIBUTE) } + after_create :create_ci_cd_settings, unless: :ci_cd_settings, if: proc { ProjectCiCdSetting.available? } diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index bfb8d703ec9..9c768b13f78 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -19,6 +19,7 @@ class ProjectFeature < ActiveRecord::Base ENABLED = 20 FEATURES = %i(issues merge_requests wiki snippets builds repository).freeze + STATISTICS_ATTRIBUTE = 'wikis_count'.freeze class << self def access_level_attribute(feature) @@ -52,6 +53,9 @@ class ProjectFeature < ActiveRecord::Base default_value_for :wiki_access_level, value: ENABLED, allows_nil: false default_value_for :repository_access_level, value: ENABLED, allows_nil: false + after_create ->(model) { SiteStatistic.track(STATISTICS_ATTRIBUTE) if model.wiki_enabled? } + after_update :update_site_statistics + def feature_available?(feature, user) get_permission(user, access_level(feature)) end @@ -76,8 +80,30 @@ class ProjectFeature < ActiveRecord::Base issues_access_level > DISABLED end + # This is a workaround for the removal hooks not been triggered when removing a Project. + # + # ProjectFeature is removed using database cascade index rule. + # This method is called by Project model when deletion starts. + def untrack_statistics_for_deletion! + return unless wiki_enabled? + + SiteStatistic.untrack(STATISTICS_ATTRIBUTE) + end + private + def update_site_statistics + return unless wiki_access_level_changed? + + if self.wiki_access_level_was == DISABLED + # possible new states are PRIVATE / ENABLED, both should be tracked + SiteStatistic.track(STATISTICS_ATTRIBUTE) + elsif self.wiki_access_level == DISABLED + # old state was either PRIVATE / ENABLED, only untrack if new state is DISABLED + SiteStatistic.untrack(STATISTICS_ATTRIBUTE) + end + end + # Validates builds and merge requests access level # which cannot be higher than repository access level def repository_children_level diff --git a/app/models/site_statistic.rb b/app/models/site_statistic.rb new file mode 100644 index 00000000000..9c9c3172fe6 --- /dev/null +++ b/app/models/site_statistic.rb @@ -0,0 +1,74 @@ +class SiteStatistic < ActiveRecord::Base + # prevents the creation of multiple rows + default_value_for :id, 1 + + COUNTER_ATTRIBUTES = %w(repositories_count wikis_count).freeze + REQUIRED_SCHEMA_VERSION = 20180629153018 + + # Tracks specific attribute + # + # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES + def self.track(raw_attribute) + with_statistics_available(raw_attribute) do |attribute| + SiteStatistic.update_all(["#{attribute} = #{attribute}+1"]) + end + end + + # Untracks specific attribute + # + # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES + def self.untrack(raw_attribute) + with_statistics_available(raw_attribute) do |attribute| + SiteStatistic.update_all(["#{attribute} = #{attribute}-1 WHERE #{attribute} > 0"]) + end + end + + # Wrapper for track/untrack operations with basic validations and enforced requirements + # + # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES + # @yield [String] attribute quoted to be used inside SQL / Arel query + def self.with_statistics_available(raw_attribute) + unless raw_attribute.in?(COUNTER_ATTRIBUTES) + raise ArgumentError, "Invalid attribute: '#{raw_attribute}' to '#{caller_locations(1, 1)[0].label}' method. " \ + "Valid attributes are: #{COUNTER_ATTRIBUTES.join(', ')}" + end + + return unless available? + + self.fetch # make sure record exists + + attribute = self.connection.quote_column_name(raw_attribute) + + # will be running on its own transaction context + yield(attribute) + end + + # Returns a site statistic record with tracked information + # + # @return [SiteStatistic] record with tracked information + def self.fetch + SiteStatistic.transaction(requires_new: true) do + SiteStatistic.first_or_create! + end + rescue ActiveRecord::RecordNotUnique + retry + end + + # Return whether required schema change is available + # + # This is needed in order to degrade gracefully when testing schema migrations + # + # @return [Boolean] whether schema is available + def self.available? + @available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION + end + + # Resets cached column information + # + # This is called during schema migration specs, in order to reset internal cache state + def self.reset_column_information + @available_flag = nil + + super + end +end diff --git a/changelogs/unreleased/ce-6064-geo-sql-query-for-counting-projects-with-wikis-is-very-slow.yml b/changelogs/unreleased/ce-6064-geo-sql-query-for-counting-projects-with-wikis-is-very-slow.yml new file mode 100644 index 00000000000..b76437a8773 --- /dev/null +++ b/changelogs/unreleased/ce-6064-geo-sql-query-for-counting-projects-with-wikis-is-very-slow.yml @@ -0,0 +1,5 @@ +--- +title: Tracking the number of repositories and wikis with a cached counter for site-wide statistics +merge_request: 20413 +author: +type: performance diff --git a/db/migrate/20180629153018_create_site_statistics.rb b/db/migrate/20180629153018_create_site_statistics.rb new file mode 100644 index 00000000000..085ce1ba64b --- /dev/null +++ b/db/migrate/20180629153018_create_site_statistics.rb @@ -0,0 +1,18 @@ +class CreateSiteStatistics < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + create_table :site_statistics do |t| + t.integer :repositories_count, default: 0, null: false + t.integer :wikis_count, default: 0, null: false + end + + execute('INSERT INTO site_statistics (id) VALUES(1)') + end + + def down + drop_table :site_statistics + end +end diff --git a/db/post_migrate/20180706223200_populate_site_statistics.rb b/db/post_migrate/20180706223200_populate_site_statistics.rb new file mode 100644 index 00000000000..e78e9eb900a --- /dev/null +++ b/db/post_migrate/20180706223200_populate_site_statistics.rb @@ -0,0 +1,25 @@ +class PopulateSiteStatistics < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + transaction do + execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql? # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967 + + execute("UPDATE site_statistics SET repositories_count = (SELECT COUNT(*) FROM projects)") + end + + transaction do + execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql? # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967 + + execute("UPDATE site_statistics SET wikis_count = (SELECT COUNT(*) FROM project_features WHERE wiki_access_level != 0)") + end + end + + def down + # No downside in keeping the counter up-to-date + end +end diff --git a/db/schema.rb b/db/schema.rb index 3db11d8447e..8ae0197d1b4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1841,6 +1841,11 @@ ActiveRecord::Schema.define(version: 20180722103201) do add_index "services", ["project_id"], name: "index_services_on_project_id", using: :btree add_index "services", ["template"], name: "index_services_on_template", using: :btree + create_table "site_statistics", force: :cascade do |t| + t.integer "repositories_count", default: 0, null: false + t.integer "wikis_count", default: 0, null: false + end + create_table "snippets", force: :cascade do |t| t.string "title" t.text "content" diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index fec1bea2751..1215b04913e 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -34,7 +34,7 @@ FactoryBot.define do builds_access_level = [evaluator.builds_access_level, evaluator.repository_access_level].min merge_requests_access_level = [evaluator.merge_requests_access_level, evaluator.repository_access_level].min - project.project_feature.update_columns( + project.project_feature.update( wiki_access_level: evaluator.wiki_access_level, builds_access_level: builds_access_level, snippets_access_level: evaluator.snippets_access_level, diff --git a/spec/factories/site_statistics.rb b/spec/factories/site_statistics.rb new file mode 100644 index 00000000000..dd8c795515a --- /dev/null +++ b/spec/factories/site_statistics.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :site_statistics, class: 'SiteStatistic' do + id 1 + repositories_count 999 + wikis_count 555 + end +end diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index cd7f77024da..10617edec0f 100644 --- a/spec/models/project_feature_spec.rb +++ b/spec/models/project_feature_spec.rb @@ -119,4 +119,46 @@ describe ProjectFeature do end end end + + context 'Site Statistics' do + set(:project_with_wiki) { create(:project, :wiki_enabled) } + set(:project_without_wiki) { create(:project, :wiki_disabled) } + + context 'when creating a project' do + it 'tracks wiki availability when wikis are enabled by default' do + expect { create(:project) }.to change { SiteStatistic.fetch.wikis_count }.by(1) + end + + it 'does not track wiki availability when wikis are disabled by default' do + expect { create(:project, :wiki_disabled) }.not_to change { SiteStatistic.fetch.wikis_count } + end + end + + context 'when updating a project_feature' do + it 'untracks wiki availability when disabling wiki access' do + expect { project_with_wiki.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED) } + .to change { SiteStatistic.fetch.wikis_count }.by(-1) + end + + it 'tracks again wiki availability when re-enabling wiki access as public' do + expect { project_without_wiki.project_feature.update_attribute(:wiki_access_level, ProjectFeature::ENABLED) } + .to change { SiteStatistic.fetch.wikis_count }.by(1) + end + + it 'tracks again wiki availability when re-enabling wiki access as private' do + expect { project_without_wiki.project_feature.update_attribute(:wiki_access_level, ProjectFeature::PRIVATE) } + .to change { SiteStatistic.fetch.wikis_count }.by(1) + end + end + + context 'when removing a project' do + it 'untracks wiki availability when removing a project with previous wiki access' do + expect { project_with_wiki.destroy }.to change { SiteStatistic.fetch.wikis_count }.by(-1) + end + + it 'does not untrack wiki availability when removing a project without wiki access' do + expect { project_without_wiki.destroy }.not_to change { SiteStatistic.fetch.wikis_count } + end + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b0ec725bf70..88442247093 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -102,6 +102,22 @@ describe Project do end end + context 'Site Statistics' do + context 'when creating a new project' do + it 'tracks project in SiteStatistic' do + expect { create(:project) }.to change { SiteStatistic.fetch.repositories_count }.by(1) + end + end + + context 'when deleting a project' do + it 'untracks project in SiteStatistic' do + project = create(:project) + + expect { project.destroy }.to change { SiteStatistic.fetch.repositories_count }.by(-1) + end + end + end + context 'updating cd_cd_settings' do it 'does not raise an error' do project = create(:project) diff --git a/spec/models/site_statistic_spec.rb b/spec/models/site_statistic_spec.rb new file mode 100644 index 00000000000..9b056fbf332 --- /dev/null +++ b/spec/models/site_statistic_spec.rb @@ -0,0 +1,83 @@ +require 'spec_helper' + +describe SiteStatistic do + describe '.fetch' do + context 'existing record' do + it 'returns existing SiteStatistic model' do + statistics = create(:site_statistics) + + expect(described_class.fetch).to be_a(described_class) + expect(described_class.fetch).to eq(statistics) + end + end + + context 'non existing record' do + it 'creates a new SiteStatistic model' do + expect(described_class.first).to be_nil + expect(described_class.fetch).to be_a(described_class) + end + end + end + + describe '.track' do + context 'with allowed attributes' do + let(:statistics) { create(:site_statistics) } + + it 'increases the attribute counter' do + expect { described_class.track('repositories_count') }.to change { statistics.reload.repositories_count }.by(1) + expect { described_class.track('wikis_count') }.to change { statistics.reload.wikis_count }.by(1) + end + + it 'doesnt increase the attribute counter when an exception happens during transaction' do + expect do + begin + described_class.transaction do + described_class.track('repositories_count') + + raise StandardError + end + rescue StandardError + # no-op + end + end.not_to change { statistics.reload.repositories_count } + end + end + + context 'with not allowed attributes' do + it 'returns error' do + expect { described_class.track('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'track\' method/) + end + end + end + + describe '.untrack' do + context 'with allowed attributes' do + let(:statistics) { create(:site_statistics) } + + it 'decreases the attribute counter' do + expect { described_class.untrack('repositories_count') }.to change { statistics.reload.repositories_count }.by(-1) + expect { described_class.untrack('wikis_count') }.to change { statistics.reload.wikis_count }.by(-1) + end + + it 'doesnt decrease the attribute counter when an exception happens during transaction' do + expect do + begin + described_class.transaction do + described_class.track('repositories_count') + + raise StandardError + end + rescue StandardError + # no-op + end + end.not_to change { described_class.fetch.repositories_count } + end + end + + context 'with not allowed attributes' do + it 'returns error' do + expect { described_class.untrack('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'untrack\' method/) + end + end + end +end |