Fixes #39336 - Batch taxonomy subtree lookups to avoid N+1 queries

[adamruzicka]

Heavily inspired by https://community.theforeman.org/t/bug-orgs-and-locs-job-templates-are-broken-in-3-18-probably-3-17-as-well/46388/15

When a user belongs to many organizations, authorization performed 1+N SQL queries per taxonomy type to resolve subtree IDs (one subtree_ids pluck per organization). This patch replaces the per-taxonomy loop with a single batched query using OR conditions on the ancestry column.

Alternatives would be using recursive CTEs to let postgres walk the subtrees with a single query, but CTEs are still somewhat uncommon in rails lands.

The problem grows purely with the number of taxonomies assigned to the user. Here I have a user with 337 organizations and 1801 locations.

Before

# time curl --silent -q -u $username:$password -k https://$(hostname -f)/api/v2/job_templates | jq '.total'
79

real	0m12.970s
user	0m0.016s
sys	0m0.012s

After

# time curl --silent -q -u $username:$password -k https://$(hostname -f)/api/v2/job_templates | jq '.total'
79

real	0m4.527s
user	0m0.015s
sys	0m0.014s

[adamruzicka]

With the ignored taxonomies commit:

# time curl --silent -q -u $username:$password -k https://$(hostname -f)/api/v2/job_templates | jq '.total'
79

real    0m1.884s
user    0m0.013s
sys     0m0.012s

[adamruzicka]

# time curl --silent -q -u $username:$password -k https://$(hostname -f)/api/v2/hosts | jq '.total'
1

real    0m25.790s
user    0m0.015s
sys     0m0.014s

# time curl --silent -q -u $username:$password -k https://$(hostname -f)/api/v2/hosts | jq '.total'
1

real    0m4.581s
user    0m0.016s
sys     0m0.011s

[sbernhard]

will you continue on this (and theforeman/foreman_remote_execution#1035) @adamruzicka ?

[adamruzicka]

As far as I'm concerned this one should be a nice improvement on its own and I'm not really planning on adding anything else here. The only failing check is redmine and that would get resolved by a squash just before merging.

About the ones in rex:

• API performance fixes foreman_remote_execution#1034 should be good to go
• Stop serializing all hosts and template invocations in job invocation foreman_remote_execution#1036 then builds on top of 1034
• Once 1034 and 1036 get in, Performance  foreman_remote_execution#1035 would be only about frontend fixes . It might need some finishing touches.

The tl;dr is all those things are in a stage where I can't really move those forward by myself.

[adamruzicka]

I polished it a bit more, just in case

[ofedoren]

PR #10990 Review — Batch taxonomy subtree lookups to avoid N+1 queries

The batch_subtree_ids optimization is solid — replacing N+1 subtree_ids queries with a single batched OR query is a clean win and the performance numbers speak for themselves.

Two concerns below.

---

1. Missing integration test for taxonomy_and_child_ids

All tests cover batch_subtree_ids and potentially_ignoring in isolation, but there is no integration test that calls taxonomy_and_child_ids on a User with assigned orgs + orgs that ignore 'user'. This is the actual hot path from the bug report — a regression there would go undetected.

There are also no pre-existing tests for taxonomy_and_child_ids anywhere in the test suite, so this PR is a good opportunity to add one.

---

2. potentially_ignoring scope builds on a fragile foundation

The ignore_types column is a YAML-serialized text column. The new potentially_ignoring scope queries it with LIKE '%User%', which:

• Forces a sequential scan (leading-wildcard LIKE cannot use indexes)
• Over-matches (e.g. User matches UserGroup), requiring a Ruby .ignore? post-filter
• Couples to the YAML serialization format remaining stable

This is strictly better than the old code (which loaded every taxonomy into Ruby), but it's a new optimization built on top of a known-fragile data model. Since this PR is specifically about performance, it's worth considering whether to address the root cause rather than work around it.

Suggested path forward: migrate ignore_types from serialized text to a jsonb column. This would:

• Enable exact containment queries (WHERE ignore_types @> '["User"]') — no over-matching, no Ruby post-filter
• Support GIN indexing for O(1) lookup instead of a sequential scan
• Decouple from the serialization format entirely

This could land as a follow-up PR to keep the batch_subtree_ids win merging quickly. But in that case, the potentially_ignoring LIKE scope should be clearly marked as temporary.

[ofedoren]

The review above was assisted by Claude Code (claude-opus-4-6).

[ofedoren]

Thanks, @adamruzicka ! All in all, this is great ❤️ It's just my personal issue that I don't quite like potentially_ignoring scope, it just feels quite weird to me, thus why the comment above has 2. point, I'd really like to hear your thoughts on that. I understand that this seems as micro-optimization and a nit instead of the main optimization you've done, but still... Not necessary for this PR, but at least a ticket or a follow up.

Regarding the 1. point: another test never hurts, but it's up to you, otherwise I'd approve and get this in, will wait for your feedback first though.

P.S. Needs squashing as well.

[adamruzicka]

I'll take a stab at both. I never really used jsonb columns anywhere, so this will be a nice opportunity to learn something new for a change.

[adamruzicka]

please bear with me, there's more to be done about ignore_types

[adamruzicka]

Here we have to use the string representation as the original Environment class was extracted into foreman-puppet as ForemanPuppet::Environment

[adamruzicka]

note for myself:
similar change will need to be done in f-puppet in https://github.com/theforeman/foreman_puppet/blob/master/db/migrate/20220208135305_migrate_environment_ignore_type.foreman_puppet.rb#L7

[adamruzicka]

This would do it. Leaving as individual commits for review, will squash once approved, that will also take care of the single commit and redmine checks.

[adamruzicka]

Assert Single Commit (non-blocking) / build (pull_request)Failing after 6s

Go home single commit assertion, you're drunk

[ofedoren]

Thanks, @adamruzicka !

Just a note that the jsonb part was extracted and be dealt with in the #11046. As for this improvement, there is nothing to add, let's get this in. UPD: The tests were green, thus not waiting after the squash.