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

Added indexes for product reference and supplier_reference #16440

Merged
merged 1 commit into from Dec 9, 2019

Conversation

@Gamesh
Copy link
Contributor

Gamesh commented Nov 15, 2019

Questions Answers
Branch? develop
Description? Added indexes for product.reference and product.supplier_reference, so lookups in imports by reference no longer causes full table scans. Should also improve search by reference in BO
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16323

How to test?

  • Don't apply create index SQL yet.
  • Turn on slow query log / queries not using indexes log on mysql
  • RUN import when product table contains at least 1000 entries before,
  • Notice there should be SELECT statement on product table where reference = "XXXX".
  • Run explain on that SQL query it should report using full table scan and scanning many rows.
  • Now add indexes and run the explain for the same query, it should now report using newly created index and scanning only 1 row if all references are unique

This change is Reviewable

@Gamesh Gamesh requested a review from PrestaShop/prestashop-core-developers as a code owner Nov 15, 2019
@Gamesh Gamesh force-pushed the Gamesh:develop branch from 3474054 to e0d48b2 Nov 18, 2019
@Gamesh

This comment has been minimized.

Copy link
Contributor Author

Gamesh commented Nov 18, 2019

build seems to be failing for random reasons like on files i didn't touch:

  ERROR  in src/PrestaShopBundle/Resources/config/routing/admin/sell/orders/orders.yml

  >> Duplicate key "admin_search_product_for_order_creation" detected whilst parsing YAML. Silent handling of duplicate mapping keys in YAML is deprecated since Symfony 3.2 and will throw \Symfony\Component\Yaml\Exception\ParseException in 4.0 on line 187 at line 187.

                                                                                

 [WARNING] 232 YAML files have valid syntax and 1 contain errors.   

or fails downloading translations 🤷‍♂

@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Nov 19, 2019

Very useful pr thanks @Gamesh

@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 19, 2019

@Gamesh I just relaunched the job in error. Could you rebase your PR for fixing conflict files, please ?

@Gamesh Gamesh force-pushed the Gamesh:develop branch from e0d48b2 to 6a0f867 Nov 19, 2019
@Gamesh

This comment has been minimized.

Copy link
Contributor Author

Gamesh commented Nov 19, 2019

@Progi1984 done. i noticed we're adding a huge amount of alter tables 😮
From practical perspective i would suggest to merge the same table's DDL into one, especially on larger databases people will feel the effect as the table will be modified only once and the tmp table will be created only once. Now if we leave as is some tables will get copied to tmp at least twice for each alter and that takes time.

@Progi1984 Progi1984 requested a review from PierreRambaud Nov 20, 2019
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 20, 2019

@Progi1984 done. i noticed we're adding a huge amount of alter tables open_mouth
From practical perspective i would suggest to merge the same table's DDL into one, especially on larger databases people will feel the effect as the table will be modified only once and the tmp table will be created only once. Now if we leave as is some tables will get copied to tmp at least twice for each alter and that takes time.

Good idea, but in this case, in an another pull request. Thanks

@Gamesh

This comment has been minimized.

Copy link
Contributor Author

Gamesh commented Nov 27, 2019

sorry @Progi1984 had to rebase again because of conflicts, can we merge this quicker somehow i suspect that SQL file is modified frequently and we will keep getting conflicts

@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 27, 2019

sorry @Progi1984 had to rebase again because of conflicts, can we merge this quicker somehow i suspect that SQL file is modified frequently and we will keep getting conflicts

@Gamesh This PR needs to pass by QA before merge.

@sarahdib sarahdib self-assigned this Dec 5, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Dec 5, 2019

hello @Gamesh
Thank you for your contribution

Can you rebase the PR in order to test it ?

Thank you

@Gamesh Gamesh force-pushed the Gamesh:develop branch from 66c7789 to 550e3dc Dec 5, 2019
@Gamesh

This comment has been minimized.

Copy link
Contributor Author

Gamesh commented Dec 5, 2019

@sarahdib done :/

@sarahdib sarahdib added this to the 1.7.7.0 milestone Dec 9, 2019
@Progi1984 Progi1984 changed the title #16323 added indexes for product reference and supplier_reference Added indexes for product reference and supplier_reference Dec 9, 2019
@Progi1984 Progi1984 merged commit 6d2f240 into PrestaShop:develop Dec 9, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sam-pires

This comment has been minimized.

Copy link

sam-pires commented Dec 9, 2019

Thanks @Gamesh 👍

@Gamesh Gamesh deleted the Gamesh:develop branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.