Skip to content
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

Allow MiqReport.paged_view_search to take advantage of Rbac :extra_cols #17474

Conversation

NickLaMuro
Copy link
Member

Allows reports, specifically the ones in product/views, to take advantage of the :extra_cols functionality. This allows virtual_attributes to be gathered via SQL in the main query, instead of via includes/references.

Links

Used to determine which `MiqReport#cols` are virtual_attributes, and
when passed through Rbac, can be added to the `:extra_cols` hash.
Allows this code to be re-used elsewhere.
Allows virtual_attributes to be respected in Rbac queries for reports
run through `MiqReport#paged_view_search`.
@miq-bot
Copy link
Member

miq-bot commented May 23, 2018

Checked commits NickLaMuro/manageiq@f1603f0~...1351db7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@kbrock kbrock self-assigned this May 29, 2018
@kbrock kbrock added this to the Sprint 87 Ending Jun 4, 2018 milestone May 30, 2018
@kbrock kbrock merged commit 6f9b9c8 into ManageIQ:master May 30, 2018
NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Feb 26, 2019
Over time, the `compliances` table will get quiet large, and when paired
with the rest of the tables in this report, you end up getting a query
executed that looks like this:

    SELECT "vms"."id" AS t0_r0, ..., "vms"."memory_hot_add_increment" AS t0_r75,
           "hosts"."id" AS t1_r0, ..., "hosts"."physical_server_id" AS t1_r36,
           "storages"."id" AS t2_r0, ..., "storages"."storage_domain_type" AS t2_r18,
           "ext_management_systems"."id" AS t3_r0, ..., "ext_management_systems"."tenant_mapping_enabled" AS t3_r23,
           "snapshots"."id" AS t4_r0, ..., "snapshots"."ems_ref" AS t4_r16,
           "compliances"."id" AS t5_r0, ..., "compliances"."event_type" AS t5_r6,
           "operating_systems"."id" AS t6_r0, ..., "operating_systems"."kernel_version" AS t6_r25,
           "hardwares"."id" AS t7_r0, ..., "hardwares"."provision_state" AS t7_r34,
           "tags"."id" AS t8_r0, "tags"."name" AS t8_r1
    FROM "vms"
    LEFT OUTER JOIN "hosts" ON "hosts"."id" = "vms"."host_id"
    LEFT OUTER JOIN "storages" ON "storages"."id" = "vms"."storage_id"
    LEFT OUTER JOIN "ext_management_systems" ON "ext_management_systems"."id" = "vms"."ems_id"
    LEFT OUTER JOIN "snapshots" ON "snapshots"."vm_or_template_id" = "vms"."id"
    LEFT OUTER JOIN "compliances" ON "compliances"."resource_id" = "vms"."id" AND "compliances"."resource_type" = $1
    LEFT OUTER JOIN "operating_systems" ON "operating_systems"."vm_or_template_id" = "vms"."id"
    LEFT OUTER JOIN "hardwares" ON "hardwares"."vm_or_template_id" = "vms"."id"
    LEFT OUTER JOIN "taggings" ON "taggings"."taggable_id" = "vms"."id" AND "taggings"."taggable_type" = $2
    LEFT OUTER JOIN "tags" ON "tags"."id" = "taggings"."tag_id"
    WHERE "vms"."type" IN (...)
      AND "vms"."template" = $3
      AND (lower(vms.name) like '%win2k%' escape '`')
      AND "vms"."id" IN (...)
    ORDER BY LOWER("vms"."name") ASC

This ends up creating 243 columns per record, and the number of rows
returned has been observed to be over 80k with just 20 VMs being
targeted in the `"vms"."id" IN (...)` portion of the query.

Since some fixes have been made to avoid the N+1 that results from doing
this:

- ManageIQ/manageiq#17473
- ManageIQ/manageiq#17474
- ManageIQ/manageiq#17475

It is much better to do use those facilities.  Even with the N+1, it is
much better making extra round trips than the gigs of data that would be
returned as a result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants