-
Notifications
You must be signed in to change notification settings - Fork 23.1k
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
[FW][FIX] res_currency: add order by to _select_companies_rates query #106078
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PG12 introduced an optimization for CTEs that automatically inlines CTEs if they are only refered once in the parent query. Prior to that CTEs were always materialzed, meaning that PG created a sort of temp table on the fly to store the result of the CTE's evaluation. Whereas this leads to performance improvements in general, in the particular case of _select_companies_rates this inlining becomes a performance bottleneck. This is because while the currency_rate CTE is only refered once in both purchase_report and product_margin, the join condition (cr.date_end is null or cr.date_end > ...) requires evaluating the CTE's date_end subquery twice. This, combined with the fact that in PG12 the planner goes for a Nested Loop JOIN instead of a HASH Join in PG10 makes the performances of the whole query much worse in PG12 than in PG10. Adding an ORDER BY (or an OFFSET 0, the resulting plan is the same) creates a kind of optimization fence that forces PG to evaluate the subquery first using its own plan. This removes the need to rescan the subquery each time the Merge JOIN filter has to be applied, which is a good strategy in this specific situation. The same result could be achieved by adding the keyword "MATERIALIZED" in the CTE definition. The issue is that this keyword did not exist in PG 10 so using it would require to check the PG version at runtime from python. Examples of query timings change before and after PR: Number of POs | Before PR | After PR 2000 | 7s | 345ms 7000 | 23s | 1.1s opw-2930578 X-original-commit: 8b7a394
This PR targets 14.0 and is part of the forward-port chain. Further PRs will be created up to master. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port |
This was referenced Nov 18, 2022
robodoo
pushed a commit
that referenced
this pull request
Nov 25, 2022
PG12 introduced an optimization for CTEs that automatically inlines CTEs if they are only refered once in the parent query. Prior to that CTEs were always materialzed, meaning that PG created a sort of temp table on the fly to store the result of the CTE's evaluation. Whereas this leads to performance improvements in general, in the particular case of _select_companies_rates this inlining becomes a performance bottleneck. This is because while the currency_rate CTE is only refered once in both purchase_report and product_margin, the join condition (cr.date_end is null or cr.date_end > ...) requires evaluating the CTE's date_end subquery twice. This, combined with the fact that in PG12 the planner goes for a Nested Loop JOIN instead of a HASH Join in PG10 makes the performances of the whole query much worse in PG12 than in PG10. Adding an ORDER BY (or an OFFSET 0, the resulting plan is the same) creates a kind of optimization fence that forces PG to evaluate the subquery first using its own plan. This removes the need to rescan the subquery each time the Merge JOIN filter has to be applied, which is a good strategy in this specific situation. The same result could be achieved by adding the keyword "MATERIALIZED" in the CTE definition. The issue is that this keyword did not exist in PG 10 so using it would require to check the PG version at runtime from python. Examples of query timings change before and after PR: Number of POs | Before PR | After PR 2000 | 7s | 345ms 7000 | 23s | 1.1s opw-2930578 closes #106078 X-original-commit: 8b7a394 Signed-off-by: Raphael Collet <rco@odoo.com>
jdoutreloux
pushed a commit
to acsone/odoo
that referenced
this pull request
Jul 5, 2023
PG12 introduced an optimization for CTEs that automatically inlines CTEs if they are only refered once in the parent query. Prior to that CTEs were always materialzed, meaning that PG created a sort of temp table on the fly to store the result of the CTE's evaluation. Whereas this leads to performance improvements in general, in the particular case of _select_companies_rates this inlining becomes a performance bottleneck. This is because while the currency_rate CTE is only refered once in both purchase_report and product_margin, the join condition (cr.date_end is null or cr.date_end > ...) requires evaluating the CTE's date_end subquery twice. This, combined with the fact that in PG12 the planner goes for a Nested Loop JOIN instead of a HASH Join in PG10 makes the performances of the whole query much worse in PG12 than in PG10. Adding an ORDER BY (or an OFFSET 0, the resulting plan is the same) creates a kind of optimization fence that forces PG to evaluate the subquery first using its own plan. This removes the need to rescan the subquery each time the Merge JOIN filter has to be applied, which is a good strategy in this specific situation. The same result could be achieved by adding the keyword "MATERIALIZED" in the CTE definition. The issue is that this keyword did not exist in PG 10 so using it would require to check the PG version at runtime from python. Examples of query timings change before and after PR: Number of POs | Before PR | After PR 2000 | 7s | 345ms 7000 | 23s | 1.1s opw-2930578 closes odoo#106078 X-original-commit: 8b7a394 Signed-off-by: Raphael Collet <rco@odoo.com>
jdoutreloux
pushed a commit
to acsone/odoo
that referenced
this pull request
Aug 10, 2023
PG12 introduced an optimization for CTEs that automatically inlines CTEs if they are only refered once in the parent query. Prior to that CTEs were always materialzed, meaning that PG created a sort of temp table on the fly to store the result of the CTE's evaluation. Whereas this leads to performance improvements in general, in the particular case of _select_companies_rates this inlining becomes a performance bottleneck. This is because while the currency_rate CTE is only refered once in both purchase_report and product_margin, the join condition (cr.date_end is null or cr.date_end > ...) requires evaluating the CTE's date_end subquery twice. This, combined with the fact that in PG12 the planner goes for a Nested Loop JOIN instead of a HASH Join in PG10 makes the performances of the whole query much worse in PG12 than in PG10. Adding an ORDER BY (or an OFFSET 0, the resulting plan is the same) creates a kind of optimization fence that forces PG to evaluate the subquery first using its own plan. This removes the need to rescan the subquery each time the Merge JOIN filter has to be applied, which is a good strategy in this specific situation. The same result could be achieved by adding the keyword "MATERIALIZED" in the CTE definition. The issue is that this keyword did not exist in PG 10 so using it would require to check the PG version at runtime from python. Examples of query timings change before and after PR: Number of POs | Before PR | After PR 2000 | 7s | 345ms 7000 | 23s | 1.1s opw-2930578 closes odoo#106078 X-original-commit: 8b7a394 Signed-off-by: Raphael Collet <rco@odoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PG12 introduced an optimization for CTEs that automatically inlines
CTEs if they are only refered once in the parent query. Prior
to that CTEs were always materialized, meaning
that PG created a sort of temp table on the fly to store the result
of the CTE's evaluation.
Whereas this leads to performance improvements in general, in the particular
case of
_select_companies_rates
this inlining becomes a performancebottleneck. This is because while the currency_rate CTE is
only refered once in both purchase_report and product_margin,
the Merge Join Filter
cr.date_end is null or cr.date_end > ...
requires evaluating the CTE's date_end subquery twice. This, combined
with the fact that in PG12 the planner goes for a Nested Loop Join instead
of a Hash Join in PG10 makes the performances of the whole query
much worse in PG12 than in PG10. PG10 query plan vs PG12 query plan.
We can see on the PG12 Query Plan that the Merge Join Node 7 scans the CTE twice (SubPlan 1 and SubPlan 2).
An eazy solution would be to add the keyword
Materialized
before the CTE definition. This keyword, introducedin PG12, forces the materialization of the CTE, avoiding its inlining and making the PG12 plan the same as the PG10 one. Unfortunately,
Materialized
wasn't there in PG10 so we cannot use this solution here since some Odoo instances still run on PG10.Instead, this PR adds an
ORDER BY date_end
clause at the end of the CTE. This works like an OFFSET 0as it is kind of an optimizer fence, forcing PG to evaluate the subquery first Ordered PG12 query plan
Speedup
Test DB with 7000 purchase_order, 2666 res_currency_rate and 6 active currencies.
Purchase_report query timings in PG12, changing the number of purchase_orders
Changing the number of res_currency_rate
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
Forward-Port-Of: #98844