feat(catalog): faster category product count #25

damienwebdev · 2023-04-08T18:25:59Z

Previously, in the admin panel, computing the CategoryTree UI (the left-hand
element of the page that loads when you try to view the categories
of your store) was wildly slow for the root scope when there were
many is_anchor categories. This was due to several bugs:

Use of a missing index: catalog_category_product_index that is now defunct
and always empty. It was replaced by catalog_category_product_index_store*.
However, this table did not exist for the store0. This diff both adds
that missing table and changes the collection to use the new tables.
A bad computation of the rootCategoryIds for the root store, causing
a bad join condition against the temporary category recursion table
resulting in the resulting index table (catalog_category_product_index_store0)
always being empty.
An exceedingly slow private method (getProductsCountFromCategoryTable)
which should not have been merged in the first place given how slow it is.
I have removed this method. Removing this does cause a small UX bug
when the index is empty: the category page will show 0 products before
an index has run. It's either this or perform the computation on
every page refresh. I see this an acceptable break, given how slow
it is to attempt to compute these values manually.

@Nuranto I have extracted your commit from magento/magento2#34111 and I am personally sorry for the way that PR was handled.

Fixes: magento/magento2#34109
Fixes: magento/magento2#35216

Here is a patch for

v2.4.4-p3

Previously, in the admin panel, computing the CategoryTree UI (the left-hand element of the page that loads when you try to view the categories of your store) was wildly slow for the root scope when there were many `is_anchor` categories. This was due to several bugs: 1: Use of a missing index: `catalog_category_product_index` that is now defunct and always empty. It was replaced by `catalog_category_product_index_store*`. However, this table did not exist for the `store0`. This diff both adds that missing table and changes the collection to use the new tables. 2. A bad computation of the `rootCategoryIds` for the root store, causing a bad join condition against the temporary category recursion table resulting in the resulting index table (catalog_category_product_index_store0) always being empty. 3. An exceedingly slow private method (getProductsCountFromCategoryTable) which should not have been merged in the first place given how slow it is. I have removed this method. Removing this does cause a small UX bug when the index is empty: the category page will show 0 products before an index has run. It's either this or perform the computation on every page refresh. I see this an acceptable break, given how slow it is to attempt to compute these values manually. Co-authored-by: Vincent ENJALBERT <chef@web-cooking.net>

damienwebdev · 2023-04-08T18:47:01Z

@Vinai I think the semver check is broken. Additionally, I have no idea how to write tests for this, I have no idea wtf to do with the temp table/indexer changes I made. Debugging this was a nightmare. @vitaliy-golomoziy

Vinai · 2023-10-05T20:27:46Z

Thanks @damienwebdev - I agree the semver check seems to be broken. Probably we should disable it until we have the resources.

It would be great if you could push an empty commit to this PR so it triggers the current set of checks. I can't force a run manually myself.

Vinai · 2023-10-07T18:11:16Z

app/code/Magento/Catalog/Model/ResourceModel/Category/Collection.php

@@ -125,6 +131,7 @@ public function __construct(
            \Magento\Framework\App\ObjectManager::getInstance()->get(ScopeConfigInterface::class);
        $this->catalogProductVisibility = $catalogProductVisibility ?:
            \Magento\Framework\App\ObjectManager::getInstance()->get(Visibility::class);
+        $this->tableMaintainer = \Magento\Framework\App\ObjectManager::getInstance()->get(TableMaintainer::class);


Out of curiosity, why is the TableMaintainer dependency not added as a constructor argument in the same way as the Visibility instance?

Vinai · 2023-10-07T18:12:27Z

app/code/Magento/Catalog/Model/ResourceModel/Category/Collection.php

@@ -329,7 +336,7 @@ public function loadProductCount($items, $countRegular = true, $countAnchor = tr
            foreach ($anchor as $item) {
                $productsCount = isset($categoryProductsCount[$item->getId()])
                    ? (int)$categoryProductsCount[$item->getId()]
-                    : $this->getProductsCountFromCategoryTable($item, $websiteId);
+                    : 0;


I would like to have a comment here noting the 0 will be used when the index hasn't been built yet.
The PR notes explained it nicely, but I can imagine someone (me?) stumbling over this in future and wondering about the reasoning behind it.

Vinai · 2023-10-07T18:20:22Z

I'll approve the changes and merge them despite my two small comments, as I've gone over the patch and understand what it does and tried it out locally.

However, if one of you could get back to me with an answer to the question when you have time I would appreciate it.

This fixes a regression introduced by pull request mage-os#25 Under certain conditions saving or loading a product caused a table-not-exists exception. This happened for example for the integration test \Magento\AdvancedPricingImportExport\Model\Import\AdvancedPricingTest::testImportDelete With this change, the index table is created by the schema patch, just like all other store index tables, and the error no longer occurs. Closes mage-os#44

This fixes a regression introduced by pull request #25 Under certain conditions saving or loading a product caused a table-not-exists exception for the store 0 index table. This happened for example for the integration test \Magento\AdvancedPricingImportExport\Model\Import\AdvancedPricingTest::testImportDelete With this change, the index table is created by the schema patch, just like all other store index tables, and the error no longer occurs. Closes #44

* Fix \Magento\Paypal\Model\Express\CheckoutTest - dbIsolation missing * Fix tests Magento\CatalogImportExport\Model\Import\ProductTest\ProductStockTest::testProductStockStatusShouldBeUpdatedOnSchedule Zend_Db_Exception: Table "catalog_category_product_index_store0_replica" does not exist Partially reverting fix from Vinai: `Create category-product index table for store 0 (#47) This fixes a regression introduced by pull request #25 * Fix failing tests with error `undefined key 'product_id`' from app/code/Magento/CatalogInventory/Model/StockManagement.php:110 Test: dev/tests/integration/testsuite/Magento/Downloadable/Block/Sales/Order/Email/Items/Order/DownloadableTest.php * Fix failed tests: \Magento\Wishlist\Controller\Index\AddTest Failed due to visibility filter in whishlist collection. Products were not added to root category and not reindexed. So visibility filter ued to exclude them from wishlist items collection: app/code/Magento/Wishlist/Model/ResourceModel/Item/Collection.php:390 * Decrease test-cases - use php 8.2 only for testing purposes Include and use changed workflow to have more unitary tests executions * Mark tests as incomplete as actual fix is in: mage-os/mageos-magento-zend-db#1 Error that was fixed: Unable to revert fixture: Magento/Framework/Backup/_files/trigger.php #0 /var/www/html/dev/tests/integration/testsuite/Magento/Framework/Backup/DbTest.php(42): Magento\Framework\Backup\DbTest->testBackupAndRollbackIncludesCustomTriggers() ... * Fix integration tests error: Magento\Framework\Image\Adapter\InterfaceTest::testRotate with data set #4 ('/var/www/html/dev/tests/integ...st.png', 45, array(157, 35), 'IMAGEMAGICK') ImagickException: unrecognized color `srgb255,255,255' @ warning/color.c/GetColorCompliance/1064 * Fix integration tests error: Magento\Framework\Image\Adapter\InterfaceTest::testCreatePngFromString with data set #1 (array(5, 12), array(0, 0, 0), array(0, 20), array(255, 255, 255), 'IMAGEMAGICK') ImagickException: unable to read font `' @ error/annotate.c/RenderFreetype/1636 * Fix integration tests error: Magento\Dhl\Model\CarrierTest::testRequestToShip with data set #0 ('GB', 'EU', 'US') Failed asserting that two DOM documents are equal. - <SoftwareName>Magento</SoftwareName> + <SoftwareName>Mage-OS</SoftwareName> * Fix integration tests error: Magento\Framework\Image\Adapter\InterfaceTest::testCreatePngFromString with data set #1 (array(5, 12), array(0, 0, 0), array(0, 20), array(255, 255, 255), 'IMAGEMAGICK') PHPUnit\Framework\Exception: Deprecated: Implicit conversion from float 11.34375 to int loses precision in /var/www/html/lib/internal/Magento/Framework/Image/Adapter/ImageMagick.php:505. * Fix integration tests error: Magento\Framework\Image\Adapter\InterfaceTest::testCreatePngFromString with data set #1 (array(5, 12), array(0, 0, 0), array(0, 20), array(255, 255, 255), 'IMAGEMAGICK') PHPUnit\Framework\Exception: Deprecated: Implicit conversion from float 11.34375 to int loses precision in /var/www/html/lib/internal/Magento/Framework/Image/Adapter/ImageMagick.php:505. * Fix integration tests error: Magento\Framework\Image\Adapter\InterfaceTest::testRotate with data set #4 ('/var/www/html/dev/tests/integ...st.png', 45, array(157, 35), 'IMAGEMAGICK') ImagickException: unrecognized color `srgb255,255,255' @ warning/color.c/GetColorCompliance/1064 * Revert "feat(catalog): faster category product count (#25)" This reverts commit f01535b. * Revert "Create category-product index table for store 0 (#47)" This reverts commit 7be2613. * Add optional param to run tests over specific directory only. * Fix failed tests: \Magento\Version\Controller\Index\IndexTest Update package name to mage-os * Fix integration tests (most likely caused by incorrect merge conflict resolving): error msg: 1) Magento\Catalog\Block\Adminhtml\Category\Checkboxes\TreeTest::testGetTreeJson Error: Call to a member function getConnectionName() on null /var/www/html/lib/internal/Magento/Framework/App/ResourceConnection.php:110 /var/www/html/app/code/Magento/Catalog/Model/ResourceModel/Category/Collection.php:570 ... ref: https://github.com/vpodorozh/mageos-magento2/actions/runs/7472531139/job/20334972566#step:6:39 * Fix failed integration tests in \Magento\Customer * Revert "Fix failed integration tests in \Magento\Customer" * Update full-integration-tests.yaml --------- Co-authored-by: Maksym Novik <m.novik@vconnect.dk> Co-authored-by: Maksym Novik <novik.kor@gmail.com> Co-authored-by: Ihor Sviziev <ihor-sviziev@users.noreply.github.com>

damienwebdev requested a review from a team as a code owner April 8, 2023 18:26

damienwebdev force-pushed the faster-product-count branch from e6732b8 to 823b58e Compare April 8, 2023 18:35

damienwebdev changed the title ~~feat(catalog): faster root category product count~~ feat(catalog): faster category product count Apr 8, 2023

damienwebdev force-pushed the faster-product-count branch from 823b58e to f857bc6 Compare April 8, 2023 18:40

Vinai requested changes Oct 7, 2023

View reviewed changes

Vinai approved these changes Oct 7, 2023

View reviewed changes

Vinai merged commit f01535b into mage-os:2.4-develop Oct 7, 2023
0 of 2 checks passed

Vinai mentioned this pull request Oct 7, 2023

Faster category tree PR integration test regression #44

Closed

Vinai mentioned this pull request Oct 8, 2023

Create category-product index table for store 0 #47

Merged

5 tasks

ihor-sviziev mentioned this pull request Dec 18, 2023

Bug on category count (performance issue) magento/magento2#34109

Open

damienwebdev deleted the faster-product-count branch December 21, 2023 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(catalog): faster category product count #25

feat(catalog): faster category product count #25

damienwebdev commented Apr 8, 2023 •

edited

damienwebdev commented Apr 8, 2023

Vinai commented Oct 5, 2023

Vinai Oct 7, 2023

Vinai Oct 7, 2023

Vinai commented Oct 7, 2023 •

edited

feat(catalog): faster category product count #25

feat(catalog): faster category product count #25

Conversation

damienwebdev commented Apr 8, 2023 • edited

damienwebdev commented Apr 8, 2023

Vinai commented Oct 5, 2023

Vinai Oct 7, 2023

Choose a reason for hiding this comment

Vinai Oct 7, 2023

Choose a reason for hiding this comment

Vinai commented Oct 7, 2023 • edited

damienwebdev commented Apr 8, 2023 •

edited

Vinai commented Oct 7, 2023 •

edited