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

[FIX] res_currency: add order by to _select_companies_rates query #98508

Closed
wants to merge 1 commit into from

Conversation

Aurelienvd
Copy link
Contributor

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 performance
bottleneck. 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 Merge 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, introduced
in 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. Because Node 7 is a merge join, ordering
the CTE by the Join Filter condition changes the query plan and removes this double subquery evaluation, leading
to a faster query overall. 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

Number of POs Before PR After PR
500 2s 115ms
2000 7s 345ms
4000 13.5s 640ms
7000 23s 1.1s

Changing the number of res_currency_rate

Number of res_currency_rate Before PR After PR
200 1.7s 150ms
500 4s 270ms
1000 6.8s 488ms
2666 23s 1.1s

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Aug 19, 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 materialized, meaning
that PG created a sort of temp table on the fly to store the result
of the CTE's evaluation.

While this leads to performance improvement 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 Merge 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 clause in the CTE fixes this issue.

opw-2930578
@C3POdoo C3POdoo requested review from a team August 19, 2022 15:49
@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Aug 19, 2022
Copy link
Collaborator

@xmo-odoo xmo-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this was added way back before 9.0, and 13.0 is still supported for now, maybe this improvement / fixed should be merged in 13.0 rather than just 14.0? The linked purchase_report query seems to have been added in 13.0 as well so...

@Aurelienvd
Copy link
Contributor Author

13.0 PR: #98844

@Aurelienvd Aurelienvd closed this Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants