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

Remove array indexing by entityId #11086

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Remove array indexing by entityId #11086

merged 1 commit into from
Oct 11, 2017

Conversation

thisisandrew
Copy link
Contributor

@thisisandrew thisisandrew commented Sep 27, 2017

Description

Remove the indexing of the $values array. It is unnecessary and introduces and assumption that the entity_id and row_id are the same in the catalog_category_entity table. When the entity_id and row_id differ then it is possible that the $values array can contain indexes for an entity_id which will be inserted into the indexer tmp table and when a row with the same row_id as this entity_id is attempted to insert it will cause a SQL constraint violation on the tmp table.

On line 384 the index of the $values array is overwritten by the row_id of the row. If the entity_id and row_id are not the same then some array indexes will not be "filled" by a row. These unfilled indexes create row in the tmp table at insert time which will later be filled by actual row_id s from real rows.

e.g. Category Flat Data indexer process unknown error:
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '501' for key 'PRIMARY', query was: INSERT INTO catalog_category_flat_store_2_tmp (row_id,entity_id,created_in,updated_in,attribute_set_id,parent_id,created_at,updated_at,path,position,level,children_count,store_id,all_children,automatic_sorting,available_sort_by,children,custom_apply_to_products,custom_design,custom_design_from,custom_design_to,custom_layout_update,custom_use_parent_settings,default_sort_by,description,display_mode,filter_price_range,fredhopper_category_id,image,include_in_menu,include_vat,is_active,is_anchor,landing_page,meta_description,meta_keywords,meta_title,name,page_layout,path_in_store,url_key,url_path) VALUES ...

This seems to be possible when the chunk size is less than the total categories in the tree for a store (we have ~1300) because the same row_id gets used twice in 2 different inserts.

Fixed Issues (if relevant)

Could not find any open issues and bug exists in

Manual testing scenarios

  1. Create a catalog where the category entities have different row_id and entity_id (recreated where these value differed by 1) and where there are more than 500 categories in the tree for any 1 store. (500 is the chunk size for the $entityIds array)
  2. Run the catalog_category_flat indexer and get a SQL constraint error - see above
  3. Remove this indexing of the $values array and re-run the indexer
  4. No errors in indexer and category flat table is accurate

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@vkublytskyi vkublytskyi self-assigned this Sep 28, 2017
@vkublytskyi vkublytskyi added this to the September 2017 milestone Sep 28, 2017
@magento-team magento-team merged commit 505fd9f into magento:develop Oct 11, 2017
magento-team pushed a commit that referenced this pull request Oct 11, 2017
 - fixed failed FAT
 - improved flat category indexer test (removed implicit coupling between test methods)
 - refactored abstract flat category indexer to reduce methods complexity
denisristic pushed a commit to denisristic/magento2 that referenced this pull request Oct 12, 2017
* 'develop' of github.com:magento/magento2: (4059 commits)
  MAGETWO-81324: Fix functional test fails randomly
  MAGETWO-80916: Save region correctly to save sales address from admin magento#11234
  Improve constructor arguments, arguments by reference is not valid and change SuppressWarnings
  MQE-428: Update Magento2 Acceptance Readme File
  MQE-352: Review and Update SampleCest.xml for added functionality
  MQE-415: Change required-entity's persistedKey in test schema to createDataKey
  MQE-335: Headless Browser Spike - Updating all @env tags for tests. Only listing the ones that the test works in currently. - Updating README. Replacing "Acceptance" with "Functional". - Adding Headless command to robo.
  MAGETWO-80319: Remove array indexing by entityId magento#11086
  Error CBO coupling between object.
  Error CBO coupling between object.
  Error CBO coupling between object.
  Error CBO coupling between object.
  Fix Code Style and Codacy Variable too long
  Error CBO coupling between object.
  Add Test to cover cancel Invoice
  MQE-424: Fix static code issues in MFTF tests and MFTF
  Solve error Field declared dynamically
  Solve error PHP Code Sniffer
  FR#StateNotDisplayedSalesAdminAddress Save region correctly to save sales address from admin
  MQE-419: README.MD should not reference the magento-pangolin org
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants