Prestashop: Low performance on great catalogs

Created on 13 Mar 2019  路  17Comments  路  Source: PrestaShop/PrestaShop

Describe the bug
On great catalogs, it's a big kick in the eggs of the database.
In each delete action I had, for example, 4900 products to process just to update de date_upd

To Reproduce
Steps to reproduce the behavior:
Delete a product on a great shop

Additionnal information
PrestaShop version: 1.7.x

1.7.5.1 BO Improvement Performance Products TBS

Most helpful comment

What ???
@khouloudbelguith The queries from your browser console have nothing to do with SQL queries !!!

If you want to know how many and which queries have been executed you must cipher the SQL debug

@kpodemski Prestashop has always updated positions after deletion

All 17 comments

what? you're deleting one product, why would you want to update the rest?

I'm not sure if the line has been saved... I mind the bucle on Product::cleanPositions:

for ($i = 0; $i < $total; $i++) {
    $return &= Db::getInstance()->execute(
        'UPDATE `'._DB_PREFIX_.'product` p'.Shop::addSqlAssociation('product', 'p').'
        SET p.`date_upd` = "'.date('Y-m-d H:i:s').'", product_shop.`date_upd` = "'.date('Y-m-d H:i:s').'"
         WHERE p.`id_product` = '.(int)$result[$i]['id_product']
    );
}

Into our shop, total takes the value 4000 approx, so, the update query will be executed 4000 times

CleanPositions is called from deleteCategories who is called from delete()

I fixed this replacing it with this code:

public static function cleanPositions($id_category, $position = 0)
    {
        $return = true;

        if (!(int)$position) {
            $result = Db::getInstance()->executeS('
                SELECT `id_product`
                FROM `'._DB_PREFIX_.'category_product`
                WHERE `id_category` = '.(int)$id_category.'
                ORDER BY `position`
            ');
            $total = count($result);

            for ($i = 0; $i < $total; $i++) {
                $return &= Db::getInstance()->update(
                    'category_product',
                    array('position' => $i),
                    '`id_category` = '.(int)$id_category.' AND `id_product` = '.(int)$result[$i]['id_product']
                );
                $return &= Db::getInstance()->execute(
                    'UPDATE `'._DB_PREFIX_.'product` p'.Shop::addSqlAssociation('product', 'p').'
                    SET p.`date_upd` = "'.date('Y-m-d H:i:s').'", product_shop.`date_upd` = "'.date('Y-m-d H:i:s').'"
                    WHERE p.`id_product` = '.(int)$result[$i]['id_product']
                );
            }
        } else {
            Db::getInstance()->executeS('
                Update '._DB_PREFIX_.'product` p'.Shop::addSqlAssociation('product', 'p').'
                INNER JOIN `'._DB_PREFIX_.'category_product` cp ON cp.id_product = p.id_product
                SET p.`date_upd` = "'.date('Y-m-d H:i:s').'", product_shop.`date_upd` = "'.date('Y-m-d H:i:s').'"
                WHERE cp.`id_category` = '.(int)$id_category.' AND cp.`position` > '.(int)$position);
            $return &= Db::getInstance()->update(
                'category_product',
                array('position' => array('type' => 'sql', 'value' => '`position`-1')),
                '`id_category` = '.(int)$id_category.' AND `position` > '.(int)$position
            );
        }
        return $return;
    }

Hi @prestaquality,

I did not manage to reproduce the issue with PS1.7.5.1.
I have more than 4000 products.
I attached a video record.
https://drive.google.com/file/d/1cD-hsJ7V6QDl4JBZ6xWAyaw4pwN_2a0n/view
When I delete a product, only 51 requests created.
image
Thanks to check & feedback.

What ???
@khouloudbelguith The queries from your browser console have nothing to do with SQL queries !!!

If you want to know how many and which queries have been executed you must cipher the SQL debug

@kpodemski Prestashop has always updated positions after deletion

The solution is there, you do what you want ^^
https://twitter.com/ttoine/status/1110203427706060801

Hi @Eolia, thank you for the suggestion. So this is an improvement of https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L781 and https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L800, right ?

If we look closer at this public static function cleanPositions 1st it updates the category positions then it updates the products linked to these categories in order to update timestamp date_upd. I'm thinking maybe we can go even further and improve this 2nd processing with something like

Db::getInstance()->execute(
                    'UPDATE `' . _DB_PREFIX_ . 'product` p' . Shop::addSqlAssociation('product', 'p') . '
                    SET p.`date_upd` = "' . date('Y-m-d H:i:s') . '", product_shop.`date_upd` = "' . date('Y-m-d H:i:s') . '"
                    WHERE p.`id_product` IN (' . implode(',', $productIds) .' )'
                );

That would avoid a foreach loop 馃 but I'm worried for big catalogs as this single query could run for a very long time and trigger deadlocks. Do you have an opinion on that ?

Hi @matks
The first updates the position of the products IN the category passed in parameter. (The positions of the categories themselves are not in this table but in ps_category_shop)

The second is useless. What important element of these products has been updated? No.
The position is the same, it's just the index number that is changed.

Prestashop wrote this required function because the numbers must follow, without holes, for the table drag'n drop positions in BO

The second is useless. What important element of these products has been updated? No.
The position is the same, it's just the index number that is changed.

I understand your point and it seems relevant but I've been able to trace this feature to http://forge.prestashop.com/browse/PSCSX-6664 so it's hard to remove it as it was a feature request validated and implemented way before me ^^ . If they did it, it probably is useful at least to some extent. The JIRA ticket mentioned hook not being executed, maybe the date_upd timestamp is updated on purpose in order to trigger this hook (which enables modules to react on this change) ?

You are serious ?
The product is updated in the previous function updatePosition ($ way, $ position) => https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L741

Why modify ALL products?

Moreover, a hook never triggers on a direct update in database, only if you use a Model object or launch a Hook :: exec (...) as on the line https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L756

And in the case described above (product deletion) it is useless to update it because it no longer exists^^

The product is updated in the previous function updatePosition ($ way, $ position) => https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L741

Why modify ALL products?

As you said for deletion usecase, cleanPositions can be called outside of updatePosition, sounfortunately we cannot rely on updatePosition for the upd update.

Why modify ALL products ? That's a good question indeed. I agree with you that most merchants probably do not need the timestamps to be updated following a product removal or position update. But this JIRA ticket was created by someone who needed it, so there's a need, expressed by a PS user.

Moreover it would be hard to remove this behavior in a minor version of PS 1.7 as it used to work in first 1.7 versions. We could remove it in next major, though.

An idea would be to control this behavior with a Configuration boolean. Something like _PS_PRODUCT_TIMESTAMP_GREEDY_UPDATE_. Set to true, it would perform the timestamp update which is resource-costing, set to false it would skip it. That would allows merchants to fine-tune this processing following their needs.

But that means another configuration setting in a software that is already quite complex 馃 .

That would avoid a foreach loop 馃 but I'm worried for big catalogs as this single query could run for a very long time and trigger deadlocks. Do you have an opinion on that ?

Answer to myself: can be done in batches (like 100 by 100) to avoid gigantic queries

Definitely, no.

I re-read the initial problem:
"Both hooks will not be executet when change the position via drag and drop."
and
"I think Prestashop should update the date_upd event when a user change the position."

The Hook :: exec() did not exist before in the upadePosition() function, it was added at this time.

When is called the functions cleanPosition ()? Only by
Product :: deleteCategory() (The name is not explicit, it should be deleteProductFromCategory() or deleteCategoryAssociation(), but ...)
and
Product :: deleteCategories() (Idem, it should be deleteProductFromAllCategories() or deleteAllCategoriesAssociations() )
If there is a hook to add it is in these 2 functions and nowhere else.

If you decide to attack this cleanPosition model, it is used in many classes:
Attribute.php code ok (@doekia to @gRoussac by irc in 2014 )
AttributeGroup.php code not ok
Carrier.php code not ok
Category.php code not ok
CMS.php code not ok
CMSCategory code not ok
Feature.php code ok (@doekia to @gRoussac by irc in 2014 )
Module.php code not ok
Product.php code not ok
Tab.php code not ok

Hi, same thing for me, always present in 1.7.6.1
If you have 1000 products on a category, there will be 1000 requests, just to change date_upd product to 2 tables.

For me, I have each product in 4 categories.
So, this function just change in each category, for each product, the date_upd field.
Unusefull and a lot of time !

I test today on 1.7.6.1 :

  • it takes more than 1mn per product to be deleted !!!
  • with modification, only 1 sec per product.

@khouloudbelguith : just put a lot of products on the same category (with not in several categories), then delete one of them, and check your request in database....

The product is updated in the previous function updatePosition ($ way, $ position) => https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Product.php#L741
Why modify ALL products?

As you said for deletion usecase, cleanPositions can be called outside of updatePosition, sounfortunately we cannot rely on updatePosition for the upd update.

Why modify ALL products ? That's a good question indeed. I agree with you that most merchants probably do not need the timestamps to be updated following a product removal or position update. But this JIRA ticket was created by someone who needed it, so there's a need, expressed by a PS user.

Moreover it would be hard to remove this behavior in a minor version of PS 1.7 as it used to work in first 1.7 versions. We could remove it in next major, though.

An idea would be to control this behavior with a Configuration boolean. Something like _PS_PRODUCT_TIMESTAMP_GREEDY_UPDATE_. Set to true, it would perform the timestamp update which is resource-costing, set to false it would skip it. That would allows merchants to fine-tune this processing following their needs.

But that means another configuration setting in a software that is already quite complex 馃 .

That would avoid a foreach loop 馃 but I'm worried for big catalogs as this single query could run for a very long time and trigger deadlocks. Do you have an opinion on that ?

Answer to myself: can be done in batches (like 100 by 100) to avoid gigantic queries

This is an old issue but if this feature needed for one PS user why 99% store owners have to have performance decrease by this feature? :)

I think this date_upd thing should be removed or fixed by adding prestashop config (only in configuration table without ability to change in BO) which is set by default to false.

  1. Maybe add a config switch "Do not update 'date_upd' on change position." default true.
  2. On bulk delete, only opdate at end. (Related https://github.com/PrestaShop/PrestaShop/issues/19605)
  3. As matks says, I think it is best to do it in batches of 100, 250, 500 or 1000. It would be necessary to measure what would be a good value.

rename issue to "Low performance on big catalogs"

Was this page helpful?
0 / 5 - 0 ratings