Skip to content

Commit

Permalink
rbac: introduce use_sql_view
Browse files Browse the repository at this point in the history
For queries with virtual_attributes, it can be slow to embed the
attributes in the primary query because it runs the attribute's
sub query for every row in the base table.

So even though you are using limit(20), it runs the virtual attribute
subquery for every row in the base table (think 6k times)

This is only really an issue on the services page, but other larger
tables like vms can benefit from this optimization.

This option embeds the table query in an inline view
think `select * from (select * from table) as inline_view`

This is opt-in behavior, and only the Services pages will use this
optimization for now.
  • Loading branch information
kbrock committed Mar 19, 2019
1 parent c323d0e commit 5065ebb
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 2 deletions.
1 change: 1 addition & 0 deletions app/models/miq_report/generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ def generate_basic_results(options = {})

## add in virtual attributes that can be calculated from sql
rbac_opts[:extra_cols] = va_sql_cols unless va_sql_cols.blank?
rbac_opts[:use_sql_view] = true if db_options && db_options[:use_sql_view]

results, attrs = Rbac.search(rbac_opts)
results = Metric::Helper.remove_duplicate_timestamps(results)
Expand Down
1 change: 1 addition & 0 deletions app/models/miq_report/search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def paged_view_search(options = {})
)
search_options.merge!(:limit => limit, :offset => offset, :order => order) if order
search_options[:extra_cols] = va_sql_cols if va_sql_cols.present?
search_options[:use_sql_view] = true if db_options && db_options[:use_sql_view]

if options[:parent]
targets = get_parent_targets(options[:parent], options[:association] || options[:parent_method])
Expand Down
41 changes: 40 additions & 1 deletion lib/rbac/filterer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,11 @@ def search(options = {})
ids_clause = nil
target_ids = nil

if options[:use_sql_view]
options = options.dup
extra_cols = options.delete(:extra_cols)
end

if targets.nil?
scope = apply_scope(klass, scope)
scope = apply_select(klass, scope, options[:extra_cols]) if options[:extra_cols]
Expand Down Expand Up @@ -254,10 +259,26 @@ def search(options = {})

scope = include_references(scope, klass, include_for_find, exp_includes, skip_references)
scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql]

# putting this into an inner view will allow us to apply the virtual attributes to LIMIT rows not all of them.
if options[:use_sql_view]
if attrs[:apply_limit_in_sql] &&
limit && extra_cols &&
!klass.table_name&.include?(".") && scope.respond_to?(:includes_values)
inner_scope = scope.except(:select, :includes, :references)
scope.includes_values.each { |hash| inner_scope = add_joins(klass, inner_scope, hash) }

if !options[:skip_counts] && (attrs[:apply_limit_in_sql] && limit)
auth_count = inner_scope.except(:offset, :limit, :order).count(:all)
end
scope = scope.from(Arel.sql("(#{inner_scope.to_sql})").as(scope.table_name)).except(:offset, :limit, :order, :where)
end
scope = apply_select(klass, scope, extra_cols) if extra_cols
end
targets = scope

unless options[:skip_counts]
auth_count = attrs[:apply_limit_in_sql] && limit ? targets.except(:offset, :limit, :order).count(:all) : targets.length
auth_count ||= attrs[:apply_limit_in_sql] && limit ? targets.except(:offset, :limit, :order).count(:all) : targets.length
end

if search_filter && targets && (!exp_attrs || !exp_attrs[:supported_by_sql])
Expand Down Expand Up @@ -344,6 +365,24 @@ def include_references(scope, klass, include_for_find, exp_includes, skip)
scope
end

# @param includes [Array, Hash]
def add_joins(klass, scope, includes)
return scope unless includes
includes = Array(includes) unless includes.kind_of?(Enumerable)
includes.each do |association, value|
if table_include?(klass, association)
scope = value ? scope.left_outer_joins(association => value) : scope.left_outer_joins(association)
end
end
scope
end

# this is a reference to a non polymorphic table
def table_include?(target_klass, association)
reflection = target_klass.reflect_on_association(association)
reflection && !reflection.polymorphic?
end

def polymorphic_include?(target_klass, includes)
includes.keys.any? do |incld|
reflection = target_klass.reflect_on_association(incld)
Expand Down
10 changes: 9 additions & 1 deletion spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1985,7 +1985,15 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
- IS NOT EMPTY:
field: Vm-name
")
expect { described_class.search(:class => "Vm", :filter => exp, :user => user, :order => "vms.name desc") }.not_to raise_error
expect do
described_class.search(:class => "VmOrTemplate",
:filter => exp,
:limit => 20,
:extra_cols => "host_name",
:use_sql_view => true,
:user => user,
:order => "vms.name desc")
end.not_to raise_error
end

it "works when limit, offset and user filters are passed and search expression contains columns in a sub-table" do
Expand Down

0 comments on commit 5065ebb

Please sign in to comment.