Bug #18842
open
Avoid N+1 query in foreman hosts index by declaring proper scope
Description
The query following query gets executed N time during host index API call:
SELECT "foreman_tasks_tasks".* FROM "foreman_tasks_tasks" INNER JOIN "foreman_tasks_locks" ON "foreman_tasks_locks"."task_id" = "foreman_tasks_tasks"."id" WHERE "foreman_tasks_tasks"."type" IN ('ForemanTasks::Task::DynflowTask') AND "foreman_tasks_tasks"."label" = 'Actions::RemoteExecution::RunHostJob' AND "foreman_tasks_locks"."resource_id" = 51101 AND "foreman_tasks_locks"."resource_type" = 'Host::Managed'
From the following callstack:
/opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/postgresql_adapter.rb:822:in `block in exec_no_cache' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract_adapter.rb:373:in `block in log' /opt/rh/rh-ror41/root/usr/share/gems/gems/activesupport-4.1.5/lib/active_support/notifications/instrumenter.rb:20:in `instrument' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract_adapter.rb:367:in `log' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/postgresql_adapter.rb:822:in `exec_no_cache' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/postgresql/database_statements.rb:137:in `exec_query' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/postgresql_adapter.rb:954:in `select' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract/database_statements.rb:24:in `select_all' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `block in select_all' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract/query_cache.rb:83:in `cache_sql' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `select_all' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/querying.rb:39:in `find_by_sql' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/relation.rb:604:in `exec_queries' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/relation.rb:486:in `load' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/relation.rb:231:in `to_a' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/relation.rb:587:in `blank?' /opt/rh/rh-ror41/root/usr/share/gems/gems/activesupport-4.1.5/lib/active_support/core_ext/object/blank.rb:24:in `present?' /opt/theforeman/tfm/root/usr/share/gems/gems/foreman_remote_execution-0.3.0.13/app/models/host_status/execution_status.rb:14:in `relevant?' /usr/share/foreman/app/models/host_status/global.rb:13:in `block in build' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/associations/collection_association.rb:71:in `block in select' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/associations/collection_association.rb:71:in `select' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/associations/collection_association.rb:71:in `each' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/associations/collection_association.rb:71:in `select' /opt/rh/rh-ror41/root/usr/share/gems/gems/activerecord-4.1.5/lib/active_record/associations/collection_proxy.rb:110:in `select' /usr/share/foreman/app/models/host_status/global.rb:10:in `build' /usr/share/foreman/app/models/host/managed.rb:883:in `global_status_label' /usr/share/foreman/app/views/api/v2/hosts/main.json.rabl:6:in `eval_source' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:417:in `instance_eval' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:417:in `eval_source' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:35:in `apply' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/helpers.rb:136:in `object_to_engine' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/partials.rb:15:in `partial_as_engine' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/builder.rb:196:in `extends' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/builder.rb:114:in `block in compile_settings' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/builder.rb:113:in `each' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/builder.rb:113:in `compile_settings' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/builder.rb:33:in `engines' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/multi_builder.rb:45:in `block in map_engines_to_builders' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/multi_builder.rb:44:in `each' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/multi_builder.rb:44:in `map_engines_to_builders' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/multi_builder.rb:22:in `to_a' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:88:in `to_hash' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:101:in `to_dumpable' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:110:in `to_json' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:48:in `block in render' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:382:in `cache_results' /opt/theforeman/tfm/root/usr/share/gems/gems/rabl-0.11.6/lib/rabl/engine.rb:47:in `render' /usr/share/foreman/app/views/api/v2/hosts/index.json.rabl:3:in `_54bcd28489908e6a80cc10544a40c553' /opt/rh/rh-ror41/root/usr/share/gems/gems/actionview-4.1.5/lib/action_view/template.rb:145:in `block in render' /opt/rh/rh-ror41/root/usr/share/gems/gems/activesupport-4.1.5/lib/active_support/notifications.rb:161:in `instrument' /opt/rh/rh-ror41/root/usr/share/gems/gems/actionview-4.1.5/lib/action_view/template.rb:339:in `instrument' /opt/rh/rh-ror41/root/usr/share/gems/gems/actionview-4.1.5/lib/action_view/template.rb:143:in `render' /opt/theforeman/tfm/root/usr/share/gems/gems/deface-1.0.2/lib/deface/action_view_extensions.rb:41:in `render' /opt/rh/rh-ror41/root/usr/share/gems/gems/actionview-4.1.5/lib/action_view/renderer/template_renderer.rb:55:in `block (2 levels) in render_template'
Updated by Ivan Necas over 7 years ago
I wonder if the host index page should call the `relevent` method at all. Why don't we consider the sub-state to be relevant just by the fact that this kind of status is present for the host? Maybe question for @marek?
Updated by Marek Hulán over 7 years ago
The only reason I see is that relevant result might change based on current settings, e.g. build status checks unattended setting and is never displayed even when the setting is disabled, even if in history the host was provisioned. This might be edge case, but if you look at config status there it's more complicated. Some users wanted to see the status even if there was no report, since that's also kind of status. So we'd need to make sure somehow that the config status would be displayed in this case. But since in such case the configuration status is always relevant, it should be always created while global status is being refreshed. Compliance status is not displayed even if some ARF report exist but the policy for the host was already removed. I think this would be the only issue with the suggested change. The problem is there would be no way to hide compliance status even if you stop using openscap completely.
Other statuses I know about seems ok:
Updated by Ivan Necas over 7 years ago
Calling the `relevant?` method on the host index page looks to me as the real issue there and I would like to avoid fixing the issue in remote execution just by adding the additional includes to the host index scope. For remote execution, I would rather solve it by returning `true` if the remote execution status is saved (which means it will be relevant, if there was a job executed against the host.
The real solution IMO would be to recalculate the relevancy of the statuses at the events that influence it (the job is run, the openscap is enabled/disabled), first report rather than calling the relevant method on every host row. Otherwise, we will just keep adding includes, that will eventually lead also to more complicated sql, increasing the complexity, as well as additional demands on the the status developer to think about this.
Updated by Shimon Shtein over 7 years ago
In general, I think that the most effective performance optimization in not calling a method.
If we can cache the status information in a more convenient way - I am all for it. In addition to what @Ivan Necas said, we should place the status field (or at least relevancy information) in a place that will be accessed by the query anyway - like a facet table. This way status relevancy could be calculated without loading extra objects.
Updated by Ivan Necas over 7 years ago
- Related to Refactor #18975: Store info about relevancy of a host status in database added
Updated by Ivan Necas over 7 years ago
I've opened an issue about this in Foreman (http://projects.theforeman.org/issues/18975), as it also influences other things, such as status charts, that also include irrelevant data.