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

Fixes #19462 - changed includes to preload #254

Merged
merged 1 commit into from May 12, 2017
Merged

Conversation

ShimShtein
Copy link
Member

No description provided.

@shlomizadok
Copy link
Member

LGTM, pending Jenkins

@ares
Copy link
Member

ares commented May 5, 2017

Thanks @ShimShtein, do you have some testing data or something where I could reproduce the issue? My understanding is that preload enforces always uses 2 SQL queries, while includes can fallback to LEFT OUTER JOIN in case there's some condition on the joined table. This change would make the base_scope limited in term of adding where with conditions on policies table. Also I was under impression, that include does the same things as preload (2 queries) as long as there's no such condition , so I wonder how this helps with memory consumption.

@iNecas
Copy link
Member

iNecas commented May 5, 2017

FYI https://bugzilla.redhat.com/show_bug.cgi?id=1447958#c13 confirms this change really has impact on the memory. The issue seems to be that the include generates too big queries to the rdbms to handle

@ShimShtein
Copy link
Member Author

@iNecas already stated the reason. If you want a bit more info, I have explained this issue in Katello's part of this chage here.

Basically, I didn't see a problem with policies, but I am not sure it couldn't create problems in some esoteric use case somewhere down the road. IMHO using preload (two queries) is justified, if there is a big degree of connectivity (many hosts are connected to the same small set of policies).

@ares
Copy link
Member

ares commented May 12, 2017

Thanks @ShimShtein, works fine with hosts that have policies. I'm merging since it's unlikely that this would break testing. I'll fix them if that is the case :-)

@ares ares merged commit d63925e into theforeman:master May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants