Commit fec23138 authored by Matthias Käppler's avatar Matthias Käppler

Merge branch '346435-optimize-followed-users-queries' into 'master'

Optimize followed users queries

See merge request gitlab-org/gitlab!84856
parents 45f68c6d a93a42b8
......@@ -73,12 +73,22 @@ class UserRecentEventsFinder
return Event.none if users.empty?
if Feature.enabled?(:optimized_followed_users_queries, current_user)
query_builder_params = event_filter.in_operator_query_builder_params(users)
Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder
.new(**query_builder_params)
.execute
.limit(limit)
.offset(params[:offset] || 0)
else
if event_filter.filter == EventFilter::ALL
execute_optimized_multi(users)
else
event_filter.apply_filter(Event.where(author: users).limit_recent(limit, params[:offset] || 0))
end
end
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
......
......@@ -31,8 +31,9 @@ class Event < ApplicationRecord
private_constant :ACTIONS
WIKI_ACTIONS = [:created, :updated, :destroyed].freeze
DESIGN_ACTIONS = [:created, :updated, :destroyed].freeze
TEAM_ACTIONS = [:joined, :left, :expired].freeze
ISSUE_ACTIONS = [:created, :updated, :closed, :reopened].freeze
TARGET_TYPES = HashWithIndifferentAccess.new(
issue: Issue,
......
---
name: optimized_followed_users_queries
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84856
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/358649
milestone: '14.10'
type: development
group: group::optimize
default_enabled: false
# frozen_string_literal: true
class AddAsyncIndexForEventsFollowedUsers < Gitlab::Database::Migration[1.0]
INDEX_NAME = 'index_events_for_followed_users'
def up
prepare_async_index :events, %I[author_id target_type action id], name: INDEX_NAME
end
def down
unprepare_async_index :events, %I[author_id target_type action id], name: INDEX_NAME
end
end
7952024a6a8df98842fa23ca9a4c328b83816ded3071e7597dbab431a5561e1a
\ No newline at end of file
......@@ -13,6 +13,8 @@ module EE
scope :epics, -> { where(target_type: 'Epic') }
end
EPIC_ACTIONS = [:created, :closed, :reopened].freeze
override :capabilities
def capabilities
super.merge(read_epic: %i[epic? epic_note?])
......
......@@ -16,6 +16,20 @@ module EE
end
end
def in_operator_query_builder_params(user_ids)
case filter
when EPIC
in_operator_params(
array_scope_ids: user_ids,
scope: ::Event.epics,
in_column: :action,
in_values: ::Event.actions.values_at(*::Event::EPIC_ACTIONS)
)
else
super
end
end
private
override :filters
......
......@@ -5,28 +5,69 @@ require 'spec_helper'
RSpec.describe UserRecentEventsFinder do
describe '#execute' do
let_it_be(:current_user) { create(:user) }
let_it_be(:user) { create(:user) }
let_it_be(:user1) { create(:user) }
let_it_be(:user2) { create(:user) }
let_it_be(:epic) { create(:epic) }
let_it_be(:issue) { create(:issue) }
let_it_be(:private_epic) { create(:epic, group: create(:group, :private)) }
let_it_be(:note) { create(:note_on_epic, noteable: epic) }
let_it_be(:public_event) { create(:event, :commented, target: note, author: user, project: nil) }
let_it_be(:private_event) { create(:event, :closed, target: private_epic, author: user, project: nil) }
let_it_be(:public_event) { create(:event, :commented, target: note, author: user1, project: nil) }
let_it_be(:private_epic_event1) { create(:event, :closed, target: private_epic, author: user1, project: nil) }
subject { described_class.new(current_user, user, nil, {}).execute }
let(:event_filter) { nil }
let(:target_users) { user1 }
subject { described_class.new(current_user, target_users, event_filter, {}).execute }
shared_examples 'UserRecentEventsFinder examples' do
context 'epic related activities' do
context 'when profile is public' do
it { is_expected.to match_array([public_event, private_event]) }
it { is_expected.to match_array([public_event, private_epic_event1]) }
end
context 'when profile is private' do
before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, user).and_return(false)
allow(Ability).to receive(:allowed?).with(current_user, :read_user_profile, user1).and_return(false)
end
it { is_expected.to be_empty }
end
context 'wehen fetching events from multiple users' do
let_it_be(:private_issue_event) { create(:event, :created, target: issue, author: user1, project: nil) }
let_it_be(:private_epic_event2) do
create(:event, :created, target: private_epic, author: user2, project: nil)
end
let_it_be(:private_epic_event3) do
create(:event, :reopened, target: private_epic, author: user2, project: nil)
end
let(:event_filter) { EventFilter.new(EventFilter::EPIC) }
let(:target_users) { [user1, user2] }
context 'when filtering for epic events' do
it { is_expected.to eq([private_epic_event3, private_epic_event2, private_epic_event1]) }
end
end
end
end
context 'when the optimized_followed_users_queries FF is on' do
before do
stub_feature_flags(optimized_followed_users_queries: true)
end
it_behaves_like 'UserRecentEventsFinder examples'
end
context 'when the optimized_followed_users_queries FF is off' do
before do
stub_feature_flags(optimized_followed_users_queries: false)
end
it_behaves_like 'UserRecentEventsFinder examples'
end
end
end
# frozen_string_literal: true
# rubocop: disable CodeReuse/ActiveRecord
class EventFilter
include Gitlab::Utils::StrongMemoize
......@@ -24,7 +25,6 @@ class EventFilter
filter == key.to_s
end
# rubocop: disable CodeReuse/ActiveRecord
def apply_filter(events)
case filter
when PUSH
......@@ -34,9 +34,9 @@ class EventFilter
when COMMENTS
events.commented_action
when TEAM
events.where(action: [:joined, :left, :expired])
events.where(action: Event::TEAM_ACTIONS)
when ISSUE
events.where(action: [:created, :updated, :closed, :reopened], target_type: 'Issue')
events.where(action: Event::ISSUE_ACTIONS, target_type: 'Issue')
when WIKI
wiki_events(events)
when DESIGNS
......@@ -45,10 +45,157 @@ class EventFilter
events
end
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable Metrics/CyclomaticComplexity
# This method build specialized in-operator optimized queries based on different
# filter parameters. All queries will benefit from the index covering the following columns:
# author_id target_type action id
#
# More context: https://docs.gitlab.com/ee/development/database/efficient_in_operator_queries.html#the-inoperatoroptimization-module
def in_operator_query_builder_params(user_ids)
case filter
when ALL
in_operator_params(array_scope_ids: user_ids)
when PUSH
# Here we need to add an order hint column to force the correct index usage.
# Without the order hint, the following conditions will use the `index_events_on_author_id_and_id`
# index which is not as efficient as the `index_events_for_followed_users` index.
# > target_type IS NULL AND action = 5 AND author_id = X ORDER BY id DESC
#
# The order hint adds an extra order by column which doesn't affect the result but forces the planner
# to use the correct index:
# > target_type IS NULL AND action = 5 AND author_id = X ORDER BY target_type DESC, id DESC
in_operator_params(
array_scope_ids: user_ids,
scope: Event.where(target_type: nil).pushed_action,
order_hint_column: :target_type
)
when MERGED
in_operator_params(
array_scope_ids: user_ids,
scope: Event.where(target_type: MergeRequest.to_s).merged_action
)
when COMMENTS
in_operator_params(
array_scope_ids: user_ids,
scope: Event.commented_action,
in_column: :target_type,
in_values: [Note, *Note.descendants].map(&:name) # To make the query efficient we need to list all Note classes
)
when TEAM
in_operator_params(
array_scope_ids: user_ids,
scope: Event.where(target_type: nil),
order_hint_column: :target_type,
in_column: :action,
in_values: Event.actions.values_at(*Event::TEAM_ACTIONS)
)
when ISSUE
in_operator_params(
array_scope_ids: user_ids,
scope: Event.where(target_type: Issue.name),
in_column: :action,
in_values: Event.actions.values_at(*Event::ISSUE_ACTIONS)
)
when WIKI
in_operator_params(
array_scope_ids: user_ids,
scope: Event.for_wiki_page,
in_column: :action,
in_values: Event.actions.values_at(*Event::WIKI_ACTIONS)
)
when DESIGNS
in_operator_params(
array_scope_ids: user_ids,
scope: Event.for_design,
in_column: :action,
in_values: Event.actions.values_at(*Event::DESIGN_ACTIONS)
)
else
in_operator_params(array_scope_ids: user_ids)
end
end
# rubocop: enable Metrics/CyclomaticComplexity
private
def in_operator_params(array_scope_ids:, scope: nil, in_column: nil, in_values: nil, order_hint_column: nil)
base_scope = Event.all
base_scope = base_scope.merge(scope) if scope
order = { id: :desc }
finder_query = -> (id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) }
if order_hint_column.present?
order = Gitlab::Pagination::Keyset::Order.build([
Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
attribute_name: order_hint_column,
order_expression: Event.arel_table[order_hint_column].desc,
nullable: :nulls_last,
distinct: false
),
Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
attribute_name: :id,
order_expression: Event.arel_table[:id].desc
)
])
finder_query = -> (_order_hint, id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) }
end
base_scope = base_scope.reorder(order)
array_params = in_operator_array_params(
array_scope_ids: array_scope_ids,
scope: base_scope,
in_column: in_column,
in_values: in_values
)
array_params.merge(
scope: base_scope,
finder_query: finder_query
)
end
# This method builds the array_ parameters
# without in_column parameter: uses one IN filter: author_id
# with in_column: two IN filters: author_id, (target_type OR action)
def in_operator_array_params(scope:, array_scope_ids:, in_column: nil, in_values: nil)
if in_column
# Builds Carthesian product of the in_values and the array_scope_ids (in this case: user_ids).
# The process is described here: https://docs.gitlab.com/ee/development/database/efficient_in_operator_queries.html#multiple-in-queries
# VALUES ((array_scope_ids[0], in_values[0]), (array_scope_ids[1], in_values[0]) ...)
cartesian = array_scope_ids.product(in_values)
user_with_column_list = Arel::Nodes::ValuesList.new(cartesian)
as = "array_ids(id, #{Event.connection.quote_column_name(in_column)})"
from = Arel::Nodes::Grouping.new(user_with_column_list).as(as)
{
array_scope: User.select(:id, in_column).from(from),
array_mapping_scope: -> (author_id_expression, in_column_expression) do
Event
.merge(scope)
.where(Event.arel_table[:author_id].eq(author_id_expression))
.where(Event.arel_table[in_column].eq(in_column_expression))
end
}
else
# Builds a simple query to represent the array_scope_ids
# VALUES ((array_scope_ids[0]), (array_scope_ids[2])...)
array_ids_list = Arel::Nodes::ValuesList.new(array_scope_ids.map { |id| [id] })
from = Arel::Nodes::Grouping.new(array_ids_list).as('array_ids(id)')
{
array_scope: User.select(:id).from(from),
array_mapping_scope: -> (author_id_expression) do
Event
.merge(scope)
.where(Event.arel_table[:author_id].eq(author_id_expression))
end
}
end
end
def wiki_events(events)
events.for_wiki_page
end
......@@ -61,5 +208,6 @@ class EventFilter
[ALL, PUSH, MERGED, ISSUE, COMMENTS, TEAM, WIKI, DESIGNS]
end
end
# rubocop: enable CodeReuse/ActiveRecord
EventFilter.prepend_mod_with('EventFilter')
......@@ -120,7 +120,7 @@ module Gitlab
.from(array_cte)
.join(Arel.sql("LEFT JOIN LATERAL (#{initial_keyset_query.to_sql}) #{table_name} ON TRUE"))
order_by_columns.each { |column| q.where(column.column_expression.not_eq(nil)) }
order_by_columns.each { |c| q.where(c.column_expression.not_eq(nil)) unless c.column.nullable? }
q.as('array_scope_lateral_query')
end
......@@ -200,7 +200,7 @@ module Gitlab
.project([*order_by_columns.original_column_names_as_arel_string, Arel.sql('position')])
.from("UNNEST(#{list(order_by_columns.array_aggregated_column_names)}) WITH ORDINALITY AS u(#{list(order_by_columns.original_column_names)}, position)")
order_by_columns.each { |column| q.where(Arel.sql(column.original_column_name).not_eq(nil)) } # ignore rows where all columns are NULL
order_by_columns.each { |c| q.where(Arel.sql(c.original_column_name).not_eq(nil)) unless c.column.nullable? } # ignore rows where all columns are NULL
q.order(Arel.sql(order_by_without_table_references)).take(1)
end
......
......@@ -59,6 +59,11 @@ FactoryBot.define do
target { design }
end
factory :design_updated_event, traits: [:has_design] do
action { :updated }
target { design }
end
factory :project_created_event do
project factory: :project
action { :created }
......
......@@ -8,9 +8,9 @@ RSpec.describe UserRecentEventsFinder do
let_it_be(:private_project) { create(:project, :private, creator: project_owner) }
let_it_be(:internal_project) { create(:project, :internal, creator: project_owner) }
let_it_be(:public_project) { create(:project, :public, creator: project_owner) }
let!(:private_event) { create(:event, project: private_project, author: project_owner) }
let!(:internal_event) { create(:event, project: internal_project, author: project_owner) }
let!(:public_event) { create(:event, project: public_project, author: project_owner) }
let_it_be(:private_event) { create(:event, project: private_project, author: project_owner) }
let_it_be(:internal_event) { create(:event, project: internal_project, author: project_owner) }
let_it_be(:public_event) { create(:event, project: public_project, author: project_owner) }
let_it_be(:issue) { create(:issue, project: public_project) }
let(:limit) { nil }
......@@ -18,6 +18,7 @@ RSpec.describe UserRecentEventsFinder do
subject(:finder) { described_class.new(current_user, project_owner, nil, params) }
shared_examples 'UserRecentEventsFinder examples' do
describe '#execute' do
context 'when profile is public' do
it 'returns all the events' do
......@@ -45,11 +46,11 @@ RSpec.describe UserRecentEventsFinder do
let_it_be(:second_user, reload: true) { create(:user) }
let_it_be(:private_project_second_user) { create(:project, :private, creator: second_user) }
let(:internal_project_second_user) { create(:project, :internal, creator: second_user) }
let(:public_project_second_user) { create(:project, :public, creator: second_user) }
let!(:private_event_second_user) { create(:event, project: private_project_second_user, author: second_user) }
let!(:internal_event_second_user) { create(:event, project: internal_project_second_user, author: second_user) }
let!(:public_event_second_user) { create(:event, project: public_project_second_user, author: second_user) }
let_it_be(:internal_project_second_user) { create(:project, :internal, creator: second_user) }
let_it_be(:public_project_second_user) { create(:project, :public, creator: second_user) }
let_it_be(:private_event_second_user) { create(:event, project: private_project_second_user, author: second_user) }
let_it_be(:internal_event_second_user) { create(:event, project: internal_project_second_user, author: second_user) }
let_it_be(:public_event_second_user) { create(:event, project: public_project_second_user, author: second_user) }
it 'includes events from all users', :aggregate_failures do
events = described_class.new(current_user, [project_owner, second_user], nil, params).execute
......@@ -60,14 +61,39 @@ RSpec.describe UserRecentEventsFinder do
end
context 'selected events' do
let!(:push_event) { create(:push_event, project: public_project, author: project_owner) }
let!(:push_event_second_user) { create(:push_event, project: public_project_second_user, author: second_user) }
using RSpec::Parameterized::TableSyntax
it 'only includes selected events (PUSH) from all users', :aggregate_failures do
event_filter = EventFilter.new(EventFilter::PUSH)
let_it_be(:push_event1) { create(:push_event, project: public_project, author: project_owner) }
let_it_be(:push_event2) { create(:push_event, project: public_project_second_user, author: second_user) }
let_it_be(:merge_event1) { create(:event, :merged, target_type: MergeRequest.to_s, project: public_project, author: project_owner) }
let_it_be(:merge_event2) { create(:event, :merged, target_type: MergeRequest.to_s, project: public_project_second_user, author: second_user) }
let_it_be(:comment_event1) { create(:event, :commented, target_type: Note.to_s, project: public_project, author: project_owner) }
let_it_be(:comment_event2) { create(:event, :commented, target_type: DiffNote.to_s, project: public_project, author: project_owner) }
let_it_be(:comment_event3) { create(:event, :commented, target_type: DiscussionNote.to_s, project: public_project_second_user, author: second_user) }
let_it_be(:issue_event1) { create(:event, :created, project: public_project, target: issue, author: project_owner) }
let_it_be(:issue_event2) { create(:event, :updated, project: public_project, target: issue, author: project_owner) }
let_it_be(:issue_event3) { create(:event, :closed, project: public_project_second_user, target: issue, author: second_user) }
let_it_be(:wiki_event1) { create(:wiki_page_event, project: public_project, author: project_owner) }
let_it_be(:wiki_event2) { create(:wiki_page_event, project: public_project_second_user, author: second_user) }
let_it_be(:design_event1) { create(:design_event, project: public_project, author: project_owner) }
let_it_be(:design_event2) { create(:design_updated_event, project: public_project_second_user, author: second_user) }
where(:event_filter, :ordered_expected_events) do
EventFilter.new(EventFilter::PUSH) | lazy { [push_event1, push_event2] }
EventFilter.new(EventFilter::MERGED) | lazy { [merge_event1, merge_event2] }
EventFilter.new(EventFilter::COMMENTS) | lazy { [comment_event1, comment_event2, comment_event3] }
EventFilter.new(EventFilter::TEAM) | lazy { [private_event, internal_event, public_event, private_event_second_user, internal_event_second_user, public_event_second_user] }
EventFilter.new(EventFilter::ISSUE) | lazy { [issue_event1, issue_event2, issue_event3] }
EventFilter.new(EventFilter::WIKI) | lazy { [wiki_event1, wiki_event2] }
EventFilter.new(EventFilter::DESIGNS) | lazy { [design_event1, design_event2] }
end
with_them do
it 'only returns selected events from all users (id DESC)' do
events = described_class.new(current_user, [project_owner, second_user], event_filter, params).execute
expect(events).to contain_exactly(push_event, push_event_second_user)
expect(events).to eq(ordered_expected_events.reverse)
end
end
end
......@@ -103,13 +129,13 @@ RSpec.describe UserRecentEventsFinder do
end
context 'filter activity events' do
let!(:push_event) { create(:push_event, project: public_project, author: project_owner) }
let!(:merge_event) { create(:event, :merged, project: public_project, author: project_owner) }
let!(:issue_event) { create(:event, :closed, project: public_project, target: issue, author: project_owner) }
let!(:comment_event) { create(:event, :commented, project: public_project, author: project_owner) }
let!(:wiki_event) { create(:wiki_page_event, project: public_project, author: project_owner) }
let!(:design_event) { create(:design_event, project: public_project, author: project_owner) }
let!(:team_event) { create(:event, :joined, project: public_project, author: project_owner) }
let_it_be(:push_event) { create(:push_event, project: public_project, author: project_owner) }
let_it_be(:merge_event) { create(:event, :merged, project: public_project, author: project_owner) }
let_it_be(:issue_event) { create(:event, :closed, project: public_project, target: issue, author: project_owner) }
let_it_be(:comment_event) { create(:event, :commented, project: public_project, author: project_owner) }
let_it_be(:wiki_event) { create(:wiki_page_event, project: public_project, author: project_owner) }
let_it_be(:design_event) { create(:design_event, project: public_project, author: project_owner) }
let_it_be(:team_event) { create(:event, :joined, project: public_project, author: project_owner) }
it 'includes all events', :aggregate_failures do
event_filter = EventFilter.new(EventFilter::ALL)
......@@ -120,6 +146,19 @@ RSpec.describe UserRecentEventsFinder do
expect(events.size).to eq(10)
end
context 'when unknown filter is given' do
it 'includes returns all events', :aggregate_failures do
event_filter = EventFilter.new('unknown')
allow(event_filter).to receive(:filter).and_return('unknown')
events = described_class.new(current_user, [project_owner], event_filter, params).execute
expect(events).to include(private_event, internal_event, public_event)
expect(events).to include(push_event, merge_event, issue_event, comment_event, wiki_event, design_event, team_event)
expect(events.size).to eq(10)
end
end
it 'only includes push events', :aggregate_failures do
event_filter = EventFilter.new(EventFilter::PUSH)
events = described_class.new(current_user, project_owner, event_filter, params).execute
......@@ -224,4 +263,21 @@ RSpec.describe UserRecentEventsFinder do
end
end
end
end
context 'when the optimized_followed_users_queries FF is on' do
before do
stub_feature_flags(optimized_followed_users_queries: true)
end
it_behaves_like 'UserRecentEventsFinder examples'
end
context 'when the optimized_followed_users_queries FF is off' do
before do
stub_feature_flags(optimized_followed_users_queries: false)
end
it_behaves_like 'UserRecentEventsFinder examples'
end
end
......@@ -24,12 +24,12 @@ RSpec.describe Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder
let_it_be(:issues) do
[
create(:issue, project: project_1, created_at: three_weeks_ago, relative_position: 5),
create(:issue, project: project_1, created_at: two_weeks_ago),
create(:issue, project: project_1, created_at: two_weeks_ago, relative_position: nil),
create(:issue, project: project_2, created_at: two_weeks_ago, relative_position: 15),
create(:issue, project: project_2, created_at: two_weeks_ago),
create(:issue, project: project_3, created_at: four_weeks_ago),
create(:issue, project: project_2, created_at: two_weeks_ago, relative_position: nil),
create(:issue, project: project_3, created_at: four_weeks_ago, relative_position: nil),
create(:issue, project: project_4, created_at: five_weeks_ago, relative_position: 10),
create(:issue, project: project_5, created_at: four_weeks_ago)
create(:issue, project: project_5, created_at: four_weeks_ago, relative_position: nil)
]
end
......@@ -155,6 +155,31 @@ RSpec.describe Gitlab::Pagination::Keyset::InOperatorOptimization::QueryBuilder
it_behaves_like 'correct ordering examples'
end
context 'with condition "relative_position IS NULL"' do
let(:base_scope) { Issue.where(relative_position: nil) }
let(:scope) { base_scope.order(order) }
let(:in_operator_optimization_options) do
{
array_scope: Project.where(namespace_id: top_level_group.self_and_descendants.select(:id)).select(:id),
array_mapping_scope: -> (id_expression) { Issue.merge(base_scope.dup).where(Issue.arel_table[:project_id].eq(id_expression)) },
finder_query: -> (_relative_position_expression, id_expression) { Issue.where(Issue.arel_table[:id].eq(id_expression)) }
}
end
context 'when iterating records one by one' do
let(:batch_size) { 1 }
it_behaves_like 'correct ordering examples'
end
context 'when iterating records with LIMIT 3' do
let(:batch_size) { 3 }
it_behaves_like 'correct ordering examples'
end
end
end
context 'when ordering by issues.created_at DESC, issues.id ASC' do
......
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