default to using inline sql views for reports #18822

kbrock · 2019-05-29T22:12:22Z

Depends upon:

quote column aliases activerecord-virtual_attributes#30

Overview

When we include virtual attributes in a query, it runs the query for every row in the result set.
For pages like the services explorer, this is very slow.

We solved this issue by introducing inline views and only running the virtual attribute for the rows in the result set. #18543 introduced it as an opt-in feature.

This has affectionately been called the "Turbo Button".

Before

Only use the improved performance in certain screens since there is a concern this may break screens. Only the Services screen has opted-in for improved performance (on hammer and master/ivanchuk)

After

Change the turbo button from op-in to an op-out. So all screens will have it.

This PR did require a change to virtual attributes and custom attribute: Fixed a bug in Virtual Attributes to properly escape sql aliases. Removed work around for said bug in Custom Attributes.

Numbers

For one example, see #18543
This will have impact on any screen that displays counts. e.g.: explorers for ems, host, and vms.

NickLaMuro

This one comment is the only thing I have to add for this. I think it is fine that are enabling this across the board and having it go through it through the QE test cycle, but I might be biased and missing something.

NickLaMuro · 2019-05-29T22:49:23Z

app/models/miq_report/generator.rb

@@ -319,7 +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]
+    rbac_opts[:use_sql_view] = true unless db_options && db_options[:use_sql_view] == false


Two alternative ideas on how to handle this:

Option 1

Maybe make this "falsey", where we still do an "existence" check, but then simply check that the value is falsey:

rbac_opts[:use_sql_view] = true unless db_options.has_key?(:use_sql_view) && !db_options[:use_sql_view]

Option 2

Alternatively, we can change this in Rbac so that this code here only sets it if it exists:

rbac_opts[:use_sql_view] = db_options[:use_sql_view] if db_options.has_key?(:use_sql_view)

And then in Rbac.search, we do a:

def search(options={}) # ... options[:use_sql_view] ||= true # ... end

Or do you think that is too heavy handed?

Or something like this:

rbac_opts[:use_sql_view] = db_options[:use_sql_view] != false

I do think there is some merit to considering option 2 though.

You can't memoize with a false value though, so perhaps something like:

def search(options={}) # ... options[:use_sql_view] = true unless options[:use_sql_view] == false # ... end

EDIT: Haha, I typod... had = false instead of == false

NickLaMuro · 2019-05-29T22:54:37Z

@kbrock You might want to look into the tests failures on this one, as they seem to be related to this change.

We might possibly want to run the test suite of the API and UI with this change in place prior to merging.

Fryguy · 2019-05-31T15:33:58Z

I also suggest running the manageiq-ui-classic specs as the UI is heavily dependent on reports

kbrock · 2019-06-03T13:25:05Z

@kbrock You might want to look into the tests failures on this one, as they seem to be related to this change.

We might possibly want to run the test suite of the API and UI with this change in place prior to merging.

@NickLaMuro

Thnx
it a hidden issue with virtual attributes for a while. I put in a PR over there for it

I've run this through all the views (which should be the only real issue) but running a full test suite is a good idea. will do today

kbrock · 2019-06-03T13:27:00Z

also, @NickLaMuro
Sorry about the &. gook, rubocop complained about the db_options && db_options.key?(:use_sql_view) -- and to be honest, this is the primary use case of that method (since && . => &.)

kbrock · 2019-06-05T16:07:26Z

WIP: waiting for new release of virtual attributes

kbrock · 2019-06-10T22:23:29Z

I also suggest running the manageiq-ui-classic specs as the UI is heavily dependent on reports

all passed.

NickLaMuro

Honestly, I don't have the capacity to put up any real stink about this PR.

I am still unsure why this is in existence to begin with, since it doesn't seem to be helping fix anything in particular for the Rails upgrade effort and there is no BZ associated, so I only can assume this is one of "Keenan's random musings" I am all to familiar with. Also, the example of "one case where this makes things better" seems very pseudo science-y and far from definitive evidence this will make the world a better place.

But I also just had something merged that broke in ten different ways and I probably am being a hypocrite, so maybe just do what you want, man. The Dude abides.

NickLaMuro · 2019-06-18T15:57:39Z

app/models/miq_report/generator.rb

@@ -319,7 +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]
+    rbac_opts[:use_sql_view] = db_options&.key?(:use_sql_view) ? db_options[:use_sql_view] : true


Nevermind, I am out... review this yourself since you used the & operator...

This is also why I hate this operator since it just adds cognitive load as a result of saving key strokes...

But anyway, is this now defaulting to true, and favoring the option value when it is present?

So the above change then translates to this:

- rbac_opts[:use_sql_view] = true if db_options && db_options[:use_sql_view] + rbac_opts[:use_sql_view] = true + rbac_opts[:use_sql_view] = db_options[:use_sql_view] if db_options && db_options.key?(:use_sql_view)

Correct?

Edit: I realized that I reviewed this PR weeks ago, and commented on this specific line, so this most likely was a "yes" to answering the last question.

But anyway, is this now defaulting to true, and favoring the option value when it is present?

Change the turbo button from op-in to an op-out. So all screens will have it.
-- the after section of the description

Yes, sorry that rubocop wanted me to change it:

- rbac_opts[:use_sql_view] = db_options && db_options.key?(:use_sql_view) ? db_options[:use_sql_view] : true + rbac_opts[:use_sql_view] = db_options&.db_options.key?(:use_sql_view) ? db_options[:use_sql_view] : true

I will change to:

- rbac_opts[:use_sql_view] = db_options && db_options.key?(:use_sql_view) ? db_options[:use_sql_view] : true + rbac_opts[:use_sql_view] = db_options == nil || db_options.fetch(:use_sql_view) { true }

NickLaMuro · 2019-06-18T15:57:54Z

app/models/miq_report/search.rb

@@ -97,7 +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]
+    search_options[:use_sql_view] = db_options&.key?(:use_sql_view) ? db_options[:use_sql_view] : true


ugh... again!!

... and to be honest, this is the primary use case of that method (since && . => &.)

@kbrock [ref]

Nope, doesn't make it better, and still looks like and overloaded version of the bitwise AND operator in my eyes.

for reports and the report data (right hand of screen) allow the use of inline sql views (aka the "turbo button") when there are virtual attributes in the primary query, they get run for every row (even those not brought back) this option allows rbac to optimize the query in these cases to run the virtual attributes in an outer query, so they only get run for the few rows brought back

virtual attributes should no contain aliases It should contain a grouping (parens on the outside) This alias was added due to a bug in virtual attributes that did not properly quote alias values. Now that that has been fixed, the alias can be removed.

miq-bot · 2019-06-18T18:37:09Z

Checked commits kbrock/manageiq@eb53085~...9dc5b0c with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

NickLaMuro

Seems fine to me. Been provided some context out of band, so I am more comfortable approving this PR now. 🐑 🇮🇹

NickLaMuro · 2019-06-18T18:59:22Z

app/models/miq_report/generator.rb

@@ -318,7 +318,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]
+    rbac_opts[:use_sql_view] = db_options.nil? || db_options.fetch(:use_sql_view) { true }


We talked about this out of band, but this could technically be:

rbac_opts[:use_sql_view] = db_options.nil? || db_options.fetch(:use_sql_view, true)

But I don't think it is worth another rebase and CI run, and rubocop doesn't seem to care. Just an FYI.

jrafanie

LGTM

jrafanie · 2019-06-18T21:05:33Z

💣 🔥 🚒

NickLaMuro · 2019-06-18T23:40:38Z

Note: We have our first victim after merging this:

https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/547432152

Quick fix done in #18892

kbrock added performance core/reporting labels May 29, 2019

kbrock requested review from jrafanie and NickLaMuro May 29, 2019 22:12

NickLaMuro reviewed May 29, 2019

View reviewed changes

kbrock force-pushed the turbo_for_all branch from e7f45e9 to 650c5e5 Compare May 30, 2019 21:02

kbrock force-pushed the turbo_for_all branch 2 times, most recently from 6a8edc8 to 0e5652b Compare June 3, 2019 13:06

kbrock changed the title ~~default to using inline sql views for reports~~ [WIP]default to using inline sql views for reports Jun 5, 2019

miq-bot added the wip label Jun 5, 2019

kbrock mentioned this pull request Jun 5, 2019

Column aliases ManageIQ/activerecord-virtual_attributes#27

Closed

miq-bot added the dependencies label Jun 6, 2019

kbrock force-pushed the turbo_for_all branch from fec5bc9 to baccbfb Compare June 6, 2019 20:14

kbrock changed the title ~~[WIP]default to using inline sql views for reports~~ default to using inline sql views for reports Jun 6, 2019

miq-bot removed the wip label Jun 6, 2019

NickLaMuro reviewed Jun 18, 2019

View reviewed changes

kbrock force-pushed the turbo_for_all branch 2 times, most recently from e7d6267 to b3babcd Compare June 18, 2019 18:32

kbrock added 2 commits June 18, 2019 14:35

kbrock force-pushed the turbo_for_all branch from b3babcd to 9dc5b0c Compare June 18, 2019 18:35

NickLaMuro approved these changes Jun 18, 2019

View reviewed changes

jrafanie approved these changes Jun 18, 2019

View reviewed changes

jrafanie added the changelog/yes label Jun 18, 2019

jrafanie self-assigned this Jun 18, 2019

jrafanie merged commit 790ee9c into ManageIQ:master Jun 18, 2019

kbrock deleted the turbo_for_all branch June 18, 2019 21:26

NickLaMuro mentioned this pull request Jun 18, 2019

Disable "turbo button" on Provisioning reports #18892

Merged

himdel mentioned this pull request Jun 19, 2019

Broken travis from other repos ManageIQ/manageiq-ui-classic#4921

Closed

kbrock mentioned this pull request Jul 2, 2019

default inline_sql_views (aka turbo button) off #18936

Merged

mfeifer added this to the Sprint 114 Ending Jun 24, 2019 milestone Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default to using inline sql views for reports #18822

default to using inline sql views for reports #18822

kbrock commented May 29, 2019 •

edited

Loading

NickLaMuro left a comment

NickLaMuro May 29, 2019

jrafanie May 30, 2019 •

edited

Loading

NickLaMuro commented May 29, 2019

Fryguy commented May 31, 2019 •

edited

Loading

kbrock commented Jun 3, 2019

kbrock commented Jun 3, 2019

kbrock commented Jun 5, 2019

kbrock commented Jun 10, 2019

NickLaMuro left a comment

NickLaMuro Jun 18, 2019

NickLaMuro Jun 18, 2019 •

edited

Loading

kbrock Jun 18, 2019

NickLaMuro Jun 18, 2019

miq-bot commented Jun 18, 2019

NickLaMuro left a comment

NickLaMuro Jun 18, 2019

jrafanie left a comment

jrafanie commented Jun 18, 2019

NickLaMuro commented Jun 18, 2019 •

edited

Loading

default to using inline sql views for reports #18822

default to using inline sql views for reports #18822

Conversation

kbrock commented May 29, 2019 • edited Loading

Overview

Before

After

Numbers

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro May 29, 2019

Choose a reason for hiding this comment

Option 1

Option 2

jrafanie May 30, 2019 • edited Loading

Choose a reason for hiding this comment

NickLaMuro commented May 29, 2019

Fryguy commented May 31, 2019 • edited Loading

kbrock commented Jun 3, 2019

kbrock commented Jun 3, 2019

kbrock commented Jun 5, 2019

kbrock commented Jun 10, 2019

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Jun 18, 2019

Choose a reason for hiding this comment

NickLaMuro Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

kbrock Jun 18, 2019

Choose a reason for hiding this comment

NickLaMuro Jun 18, 2019

Choose a reason for hiding this comment

miq-bot commented Jun 18, 2019

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Jun 18, 2019

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

jrafanie commented Jun 18, 2019

NickLaMuro commented Jun 18, 2019 • edited Loading

kbrock commented May 29, 2019 •

edited

Loading

jrafanie May 30, 2019 •

edited

Loading

Fryguy commented May 31, 2019 •

edited

Loading

NickLaMuro Jun 18, 2019 •

edited

Loading

NickLaMuro commented Jun 18, 2019 •

edited

Loading