-
Notifications
You must be signed in to change notification settings - Fork 991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #30555 - Authorizer uses subselect for joined_on #7877
Fixes #30555 - Authorizer uses subselect for joined_on #7877
Conversation
Issues: #30555 |
c99b452
to
8e144f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's better than failing, but can you share an explain plan for such query? I think it can get quite resource - intensive on the DB side. Especially when the sub-select is quite big.
# Get a subselect based on the scope search criteria | ||
subselect = resource_class.joins(scope_components[:includes]) | ||
subselect = scope_components[:where].inject(subselect) { |scope_build, where| scope_build.where(where) } | ||
scope = scope.where(assoc.foreign_key => subselect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we modify the SELECT
part of the subquery to return only the assoc.primary_key
column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails does that automatically if you pass scope to the id field.
The test builds following query SELECT "fact_values".* FROM "fact_values" INNER JOIN "hosts" ON "hosts"."id" = "fact_values"."host_id" WHERE "fact_values"."host_id" IN (SELECT "hosts"."id" FROM "hosts" INNER JOIN "operatingsystems" ON "operatingsystems"."id" = "hosts"."operatingsystem_id" WHERE "hosts"."type" = 'Host::Managed' AND (("operatingsystems"."name" = 'Debian'))) Which have the following plan
I'm not sure if the subquery is efficient or not, although the data throughput should be higher as we don't pull the additional joined data. The rest should be more or less the same, but depends on the optimizer 🤷 |
81b7cee
to
9eed565
Compare
@ShimShtein any other issues? :) |
if we use joined_on class, we are using the where clause on that class through the association. This is very volatile and it doesnt play well with the Host STI. See the test for example failure
9eed565
to
e4420e2
Compare
scope = options[:joined_on].joins(assoc.name) | ||
if scope_components[:where].present? | ||
# Get a subselect based on the scope search criteria | ||
subselect = resource_class.left_outer_joins(scope_components[:includes]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
includes
here doesn't work as if used in subselect, it does select only id and Rails then considers the join useless, as includes
primary intention was to preload. Thus I had to go with left_outer_joins
here.
@ShimShtein #7913 should fix the issue you've hitted in your setup, could you try these together to see if it really helps? |
ACK, works as expected! And #7913 indeed helped with my setup. Thanks @ezr-ondrej! |
if we use joined_on class, we are using the where clause on that class through the association.
This is very volatile and it doesnt play well with the Host STI.
See the test file for example failure
The original idea for the fix came from @sufanek1, kudos! 👍