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

Improve performance of subselect query when fetching attributes of current group #8264

Merged
merged 2 commits into from Oct 18, 2017

Conversation

Projects
None yet
6 participants
@NachE
Contributor

NachE commented Aug 22, 2017

Questions Answers
Branch? develop
Description? Improve performance of subselect query when fetching attributes of current group
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
How to test? With a high number of products and lots of combinations in every product

This change is Reviewable

@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Aug 22, 2017

Collaborator

Hello NachE!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

Collaborator

prestonBot commented Aug 22, 2017

Hello NachE!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@NachE NachE changed the title from Improve performance of subselect query when fetching attributes of cu… to FO: Improve performance of subselect query when fetching attributes of cu… Aug 22, 2017

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Aug 22, 2017

I'm also hitting this issue, please merge asap if everything is ok.

alexandernst commented Aug 22, 2017

I'm also hitting this issue, please merge asap if everything is ok.

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 22, 2017

Contributor

@NachE Could you post the EXPLAIN result of the query before and after the fix ?

Contributor

jocel1 commented Aug 22, 2017

@NachE Could you post the EXPLAIN result of the query before and after the fix ?

@NachE

This comment has been minimized.

Show comment
Hide comment
@NachE

NachE Aug 22, 2017

Contributor

@jocel1

Before:

id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY pac2 range PRIMARY PRIMARY 4 NULL 257750 100.00 "Using where;Using index"
2 "DEPENDENT SUBQUERY" pa ref PRIMARY, product_default, product_attribute_product, id_product_id_product_attribute product_default 4 const 512 100.00 "Using index; Using temporary; Using filesort"
2 "DEPENDENT SUBQUERY" pac eq_ref PRIMARY, id_product_attribute PRIMARY 8 const, c28ps.pa.id_product_attribute 1 100.00 "Using index"

After:

id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY pac2 range PRIMARY PRIMARY 4 NULL 252018 100.00 "Using where; Using index"
2 "DEPENDENT SUBQUERY" ALL NULL NULL NULL NULL 27 100.00 "Using where"
3 DERIVED pa ref PRIMARY, product_default, product_attribute_product, id_product_id_product_attribute product_default 4 512 100.00 "Using index; Using temporary; Using filesort"
3 DERIVED pac eq_ref PRIMARY, id_product_attribute PRIMARY 8 c28ps.pa.id_product_attribute 1 100.00 "Using index"

In a real query, before:
Error Code: 2013. Lost connection to MySQL server during query, Duration / Fetch Time: 30.001 sec

In a real query, after:
54 row(s) returned, Duration / Fetch Time: 0.574 sec / 0.000011 sec

Contributor

NachE commented Aug 22, 2017

@jocel1

Before:

id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY pac2 range PRIMARY PRIMARY 4 NULL 257750 100.00 "Using where;Using index"
2 "DEPENDENT SUBQUERY" pa ref PRIMARY, product_default, product_attribute_product, id_product_id_product_attribute product_default 4 const 512 100.00 "Using index; Using temporary; Using filesort"
2 "DEPENDENT SUBQUERY" pac eq_ref PRIMARY, id_product_attribute PRIMARY 8 const, c28ps.pa.id_product_attribute 1 100.00 "Using index"

After:

id select_type table type possible_keys key key_len ref rows filtered Extra
1 PRIMARY pac2 range PRIMARY PRIMARY 4 NULL 252018 100.00 "Using where; Using index"
2 "DEPENDENT SUBQUERY" ALL NULL NULL NULL NULL 27 100.00 "Using where"
3 DERIVED pa ref PRIMARY, product_default, product_attribute_product, id_product_id_product_attribute product_default 4 512 100.00 "Using index; Using temporary; Using filesort"
3 DERIVED pac eq_ref PRIMARY, id_product_attribute PRIMARY 8 c28ps.pa.id_product_attribute 1 100.00 "Using index"

In a real query, before:
Error Code: 2013. Lost connection to MySQL server during query, Duration / Fetch Time: 30.001 sec

In a real query, after:
54 row(s) returned, Duration / Fetch Time: 0.574 sec / 0.000011 sec

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 22, 2017

Contributor

@NachE I see, by adding the extra SELECT * FROM you are forcing the materialization of the result, which makes sense since the SELECT inside the IN (SELECT ) doesn't depend on the pac2 table.
Could you try the speed of fetching the current SUBQUERY results (which doesn't return a lot of rows) in a php variable, and injecting those values in the IN ( ) ?

Contributor

jocel1 commented Aug 22, 2017

@NachE I see, by adding the extra SELECT * FROM you are forcing the materialization of the result, which makes sense since the SELECT inside the IN (SELECT ) doesn't depend on the pac2 table.
Could you try the speed of fetching the current SUBQUERY results (which doesn't return a lot of rows) in a php variable, and injecting those values in the IN ( ) ?

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Aug 22, 2017

@jocel1 What you're asking doesn't make any sense in terms of performance. Why would you fetch data through PHP's link with the database, just to send that same data back thought the same link, feeding a second query?

Keeping everything on the database's engine is much cheaper in terms of performance.

The SELECT * wrapping the SUBSELECT already acts like a fixed-value which the IN can use without any further modifications nor performance penalties.

alexandernst commented Aug 22, 2017

@jocel1 What you're asking doesn't make any sense in terms of performance. Why would you fetch data through PHP's link with the database, just to send that same data back thought the same link, feeding a second query?

Keeping everything on the database's engine is much cheaper in terms of performance.

The SELECT * wrapping the SUBSELECT already acts like a fixed-value which the IN can use without any further modifications nor performance penalties.

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 22, 2017

Contributor

@alexandernst because depending on the version of MySQL you're using, it could be really inefficient at doing SUBQUERY. Basically it could creates internally a temp table, which takes some time as well.

If you execute the original query in MariaDB for example, it will choose automatically to do the materialization, and you'll not need the "ugly" wrapping SELECT * FROM (SELECT id).
But I agree generally we should better avoid any extra queries to save network roundtrip.

For this case I just want to compare the performance of two queries vs one query with a SUBSELECT

Contributor

jocel1 commented Aug 22, 2017

@alexandernst because depending on the version of MySQL you're using, it could be really inefficient at doing SUBQUERY. Basically it could creates internally a temp table, which takes some time as well.

If you execute the original query in MariaDB for example, it will choose automatically to do the materialization, and you'll not need the "ugly" wrapping SELECT * FROM (SELECT id).
But I agree generally we should better avoid any extra queries to save network roundtrip.

For this case I just want to compare the performance of two queries vs one query with a SUBSELECT

@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Aug 22, 2017

Oh, so your point is that in order to prevent this bug from happening on older *SQL engines, check what are the costs of 2 queries compared to 1 and see if the tradeoff pays? Fair enough.

alexandernst commented Aug 22, 2017

Oh, so your point is that in order to prevent this bug from happening on older *SQL engines, check what are the costs of 2 queries compared to 1 and see if the tradeoff pays? Fair enough.

@jocel1

This comment has been minimized.

Show comment
Hide comment
@jocel1

jocel1 Aug 22, 2017

Contributor

yup that's it. Old version of MySQL are crap with subqueries, it can even execute the same subquery for each lines, with the awful performances you can imagine!

Contributor

jocel1 commented Aug 22, 2017

yup that's it. Old version of MySQL are crap with subqueries, it can even execute the same subquery for each lines, with the awful performances you can imagine!

@NachE

This comment has been minimized.

Show comment
Hide comment
@NachE

NachE Aug 23, 2017

Contributor

@jocel1 Patch done. The speed of the two queries is still good. I hope this one is okay.

Contributor

NachE commented Aug 23, 2017

@jocel1 Patch done. The speed of the two queries is still good. I hope this one is okay.

@jocel1

jocel1 approved these changes Aug 23, 2017

@eternoendless eternoendless added this to the 1.7.3.0 milestone Aug 23, 2017

@eternoendless eternoendless changed the title from FO: Improve performance of subselect query when fetching attributes of cu… to Improve performance of subselect query when fetching attributes of current group Oct 9, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit 5898f0f into PrestaShop:develop Oct 18, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 18, 2017

Member

Thank you @NachE

Member

Quetzacoalt91 commented Oct 18, 2017

Thank you @NachE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment