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
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.
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 :
@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 ofupdatePosition
, sounfortunately we cannot rely onupdatePosition
for theupd
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 totrue
, it would perform the timestamp update which is resource-costing, set tofalse
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.
rename issue to "Low performance on big catalogs"
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