Mitigate performance issue in #36667 during price indexing

[ioweb-gr]

Description (*)

This is the first part of solving partially the issue #36667 tackling the performance issue of the queries coming from related / upsell and cross-sell blocks.

As described in #36667 the related / upsell and cross-sell blocks run a non optimized query breaking magento in MariaDB versions 10.4-10.6 because these long running queries prevent the indexer from renaming the tables creating a long running exclusive lock on the tables instead.

#36667 (comment)

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Randomly getting flooded with queries from related / upsell / crosssell blocks and price indexing  #36667

Manual testing scenarios (*)

1. It's not so simple to test this, you need to actually replicate the situation but you can visually check that it has an effect on the queries coming from the blocks because intially they cause a double join. With this PR the double join is removed essentially mitigating the issue.

Questions or comments

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)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @ioweb-gr. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[ioweb-gr]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[Adel-Magebinary]

Thank you for the work @ioweb-gr ! One line of code means a whole world of change for merchants using Magento 2! It took @ioweb-gr and other guys weeks of weeks to troubleshoot this issue.

[ioweb-gr]

Thanks for the kind words. I had been struggling for months with this and you were one of the few people who tried to help.
I just hope this will get merged to save a lot of headaches and the magento team will realistically think about revamping the indexers. Indexing has and will always be a pain for all shop owners with a lot of bugs, but dropping the whole website down just because a long select is running on the tables is something I would have never imagined. It's unrealistic to think that there are never going to be long running queries.

[hostep]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[Adel-Magebinary]

@ioweb-gr

To save your time on the test result

There was 1 failure:

1) Magento\CatalogInventory\Test\Unit\Model\Plugin\ProductLinksTest::testAfterGetProductCollectionShow with data set #0 (0, 1)
Expectation failed for method name is "addInStockFilterToCollection" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

FAILURES!
Tests: 23311, Assertions: 72504, Failures: 1, Warnings: 12, Skipped: 115.

[ioweb-gr]

Ah cool it just needs to be asserted that the correct method is called not the one I removed

[hostep]

@ioweb-gr: would you like to fix the failing unit test? I think you'll need to change this line.
PR's can't be merged with failing tests (except for flaky tests - if any)
Also the static tests fail with:

FILE: /var/www/html/app/code/Magento/CatalogInventory/Model/Plugin/ProductLinks.php
-----------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------
 38 | WARNING | Missing short description
 39 | WARNING | There must be exactly one blank line before tags
-----------------------------------------------------------------------------------

I know this isn't due to your change, but because this class hasn't been touched in 6 years and Magento coding standards did change over the years. They now require a short description in the phpdocs section to explain what this afterGetProductCollection method is good for. If you don't know a good explanation, then we'll have to ask somebody from the internal Adobe team to try to figure this out.

It would also be appreciated if somebody from the internal team with a lot of experience in this area could review this change and confirm this is a good fix and won't cause regressions, @kandy maybe?

[ioweb-gr]

Yeah I'll submit a change for the failing test as soon as possible. Unfortunately I don't know as much about the code usage to describe it. Circumstances brought me to it unfortunately.

As for the change itself, I still don't understand why we need two functions which do the exact same thing regarding filtering salable products based on the configuration.

I think if someone from the internal team can review it, one of the functions, the least significant one should be refactored out of the system entirely to avoid confusion on which to use or mark the one that's worse as deprecated

[ioweb-gr]

@hostep I'm not sure I understand this test warning

There must be exactly one blank line before tags

Any ideas? I think it's due to the earlier missing description

[ioweb-gr]

Also the very first commit on the file mentions it fixes

simple products which are not shown as associated in grouped when set out of stock

Perhaps we can just follow up with this as a short description

[hostep]

Sure, so to fix the static tests, the docblock should look a bit like so:

    /**
     * short description goes here  <==== adjust this
     *                              <==== here's that empty line it wants
     * @param Link $subject
     * @param Collection $collection
     * @return Collection
     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
     */

Also, you can run the static tests locally before commiting, by executing:

vendor/bin/phpcs -s --standard=Magento2 app/code/Magento/CatalogInventory/Model/Plugin/ProductLinks.php

[ioweb-gr]

OK great, so only the description needed if someone knows exactly why this was added :)

[Nuranto]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[Nuranto]

@ioweb-gr there's still :

FILE: /var/www/html/app/code/Magento/CatalogInventory/Model/Plugin/ProductLinks.php
-----------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------
 38 | WARNING | Missing short description
-----------------------------------------------------------------------------------

[ioweb-gr]

Hi @Nuranto unfortunately I'm not sure why the original function was written here as it's old code. Someone who knows should let us know what description to put here this was discussed earlier

[Nuranto]

Hi !

I see, the initial commit was : 1eaca43

So I guess you can use the commit message as method description : Simple product don't shown as associated in grouped when set out of stock, or if I understand correctly, something like this Filter on stock status based on configuration would be better

[ioweb-gr]

OK @Nuranto I've added the original commit message as description. Hopefully it will work out like this

[ioweb-gr]

Correct me if I'm wrong as I can see there is only one warning

I'm assuming it's targetting this

    /**
     * ProductLinks constructor.
     *
     * @param Configuration $configuration
     * @param Stock $stockHelper
     */
    public function __construct(
        private readonly Configuration $configuration,
        private readonly Stock $stockHelper
    ) {}

And wants it like this?

    /**
     * ProductLinks constructor.
     *
     * @param Configuration $configuration
     * @param Stock $stockHelper
     */
    public function __construct(
        private readonly Configuration $configuration,
        private readonly Stock $stockHelper
    ) {
    }

However this syntax for the constructor in php 8.2 looks very weird. Normally constructors with no body would have the curly braces in one line after the closing parenthesis.

Is it really OK to modify like this? @engcom-Hotel

[hostep]

@ioweb-gr: If Magento's coding standard would follow the latest PER-CS 2.0 coding style then you would be correct, as it says:

If a function or method contains no statements or comments (such as an empty no-op implementation or when using constructor property promotion), then the body SHOULD be abbreviated as {} and placed on the same line as the previous symbol, separated by a space. For example:

class Point
{
   public function __construct(
     public readonly int $x,
     public readonly int $y,
   ) {}
}

But at the moment Magento's coding standard is a bit outdated and still follows PSR-2 or PSR-12 (not sure which one), which both were created before PHP 8 existed and property promotion was a feature.

So for now, unfortunately yes, please change it to put the brace on the next line. One day (hopefully) the Magento Coding standard will switch to PER-CS 2 (or newer) 🤞

[ioweb-gr]

OK done

[hostep]

@magento run Static Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

Failed tests seem flaky to me, hence approving the PR.

[engcom-Hotel]

@magento run Functional Tests B2B, Functional Tests EE

[engcom-Hotel]

@magento run Functional Tests B2B, Functional Tests EE

[engcom-Hotel]

✔️ QA Passed

** Manual testing scenarios (*) **
It's not so simple to generate the scenario and test this PR, but as per internal discussion, we can visually check that it affects the queries coming from the blocks because initially, they cause a double join.

With this PR the double join is removed essentially mitigating the issue. Moving this PR further by looking into code changes and comparing both the methods (Previous and Current).

Actual Result: ✔️
Mitigate performance issue in #36667 during price indexing due to double join in the query.

Tested all the manual scenarios, no impact on regression testing.*

Failed tests seem flaky to me, they are different in 2 consecutive runs. Please refer to the below screenshot:

Function Test EE:

Run 1

Run 2

Function Test B2B:

Run 1

Run 2

Hence moving the PR in Merge in progress

Thanks