Improve post-checkout reindexing about magento-lts HOT 16 CLOSED

Interesting.. Also worth noting is that ALTER TABLE causes in-progress transactions to be implicitly committed. I didn't look to see if these happen in the middle of a transaction or not but I'm definitely in favor of completely removing support for MyISAM wherever possible (see #105 for all reasons).

from magento-lts.

from magento-lts.

Following up on this I've been looking into the indexing that happens during checkout (specifically price & stock). I was seeing a couple of database errors when many orders were placed simultaneously:

• A deadlock on the event_* tables
• A duplicate insert error on the price indexer *_tmp tables

There are a few requirements to see this behaviour:

• The order must bring a product's inventory level below the 'Qty for Item's Status to Become Out of Stock' or 'Notify for Quantity Below'
• Ordered products must have backorders set to 'No backorders' or 'Allow Qty Below 0 and Notify Customer'
• The config option 'Display out of stock products' must be set to 'No'

While looking for the solution I found a few issues:

Some database transactions with missing/unsafe try catch blocks (though most are probably harmless)

The rest of the issues are related to Mage_CatalogInventory_Model_Observer::reindexQuoteInventory():

Stock & price indexers are called directly, bypassing the indexer locking and mode check.

• No locking can cause duplicate insert errors in the price index tmp tables
• The indexers always run, even if the indexer update mode is set to manual

If a product's stock status is likely to change (ie. it has fallen below the minimum qty) then its stock item is saved

• The stock items were loaded before the order was placed - they potentially contain stale data
• Saving the stock item triggers a full reindex (by all indexers) of the ordered products.

That last point is where things get very confusing - products are re-indexed multiple times:

1. Stock indexer is run directly in reindexQuoteInventory()
2. Stock indexer is run after the stock item is saved.
   2a. When the stock indexer runs, it also registers a mass_action catalog_product index event.
   2b. Once the stock indexer has finished, it forces the mass_action event to be processed
3. The price indexer is run directly in reindexQuoteInventory()

Step 2a. results in calls to Mage_Index_Model_Indexer::logEvent(), which causes deadlocks if it is run at the same time as step 2b. (or any other indexer)

So the stock indexer will run 3 times, the price indexer 2 times, and any other indexers set to real time mode will run once.

EDIT: The stock indexer will run 2 times, the price indexer 2 times, and if set to real-time mode these indexers will run once: catalog_product_attribute, catalogsearch_fulltext

I'm just guessing really, but I think the original intention was to make sure the price indexer runs after the stock indexer, because the price index is responsible for hiding out of stock products. But it looks like the logic has been duplicated and is being run twice, once by reindexQuoteInventory() and again by $stockItem->save(). I don't think there is any reason for the other indexers to run, its just a side effect of the mass_action event.

I've created a couple of branches with a proof of concept alternative solution:
added index locking and removed stock item save
removed mass_action index event from stock item save

from magento-lts.

Here's a version I've been using in production

I doubt it can be accepted in its current state, but I'm unable to spend any more time on it so I'm leaving it here in case anyone else is interested.

The main benefits I've seen are:

• Reduced database errors under heavy checkout load
• Increased checkout performance (orders/min), especially for backordered/low stock items

from magento-lts.

This is excellent work, @bob2021! I don't have time to play with it at the moment but I would love to see this type of improvement get merged. I'd encourage you to go ahead and submit a PR, but just don't expect it to be merged really soon since it is pretty deep.

from magento-lts.

Thanks, I've split it up into a few PRs to try and make it easier

from magento-lts.

@colinmollenhour @bob2021

Firstly, thanks for the effort here.

Wanted to share a thought/idea with you, related to this topic, and get some feedback.
If promising (and not WTF are you doing!, are you MAD? results), I will go through effort to get a PR going.

So, my thought is as thus: If a product is not setup to do stock management, then is there any need to even initiate a re-index for stock (and price) for said product? I would think no.
Thus, in a store setup, where stock management is not done at all, or is limited to less products than the total products available, eliminating the final stock/price indexing in this routine (for some/all products in order) should improve matters.

Resulting code/patch that I done to eliminate those products:

diff --git a/docroot/app/code/core/Mage/CatalogInventory/Model/Observer.php b/docroot/app/code/core/Mage/CatalogInventory/Model/Observer.php
index 1e5f1664b..ab0daa8f4 100755
--- a/docroot/app/code/core/Mage/CatalogInventory/Model/Observer.php
+++ b/docroot/app/code/core/Mage/CatalogInventory/Model/Observer.php
@@ -737,6 +737,15 @@ class Mage_CatalogInventory_Model_Observer
             }
         }

+        /**
+         * get product stock setup.
+         * We only want to re-index products that actually have stock management setup
+        **/
+        $stockCollection = Mage::getModel('cataloginventory/stock_item')->getCollection()
+            ->addFieldToFilter('product_id' , array('in' => $productIds))
+            ->addFieldToFilter('manage_stock' , array('eq' => 1));
+        $productIds = $stockCollection->getColumnValues('product_id');
+
         if (count($productIds)) {
             Mage::getResourceSingleton('cataloginventory/indexer_stock')->reindexProducts($productIds);
         }

Thoughts? See any issue I did not consider?

from magento-lts.

@ProxiBlue @bob2021

Any thoughts on this? Did you maybe find some time to test this / or even run in a production env?

from magento-lts.

@seansan

I have been using this in a production environment for about 7 months now. Not implemented as a patch, but extended the actual observer with a custom module.

We used to have a lot of checkout lock issues, which was identified as stock indexing locking the tables, and caused issues with checkout (ye old lock table timeout error) especially under big load (major promotional periods)

I had actually forgotten about this code, most likely as we have never had any lock timeouts again.

I have this in my custom module for overriding the observer

<sales_model_service_quote_submit_success>
                <observers>
                    <inventory>
                        <type>disabled</type>
                    </inventory>
                    <enjo_inventory>
                        <class>enjo_catalog/inventory_observer</class>
                        <method>reindexQuoteInventory</method>
                    </enjo_inventory>
                </observers>
            </sales_model_service_quote_submit_success>

and my custom class obviously extends the core one

class Enjo_Catalog_Model_Inventory_Observer extends Mage_CatalogInventory_Model_Observer

and my method:

public function reindexQuoteInventory($observer)
    {
        // Reindex quote ids
        $quote = $observer->getEvent()->getQuote();
        $productIds = array();
        foreach ($quote->getAllItems() as $item) {
            $productIds[$item->getProductId()] = $item->getProductId();
            $children = $item->getChildrenItems();
            if ($children) {
                foreach ($children as $childItem) {
                    $productIds[$childItem->getProductId()] = $childItem->getProductId();
                }
            }
        }

        /**
         * get product stock setup.
         * We only want to re-index products that actually have stock management setup
         **/
        $stockCollection = Mage::getModel('cataloginventory/stock_item')->getCollection()
            ->addFieldToFilter('product_id', array('in' => $productIds))
            ->addFieldToFilter('manage_stock', array('eq' => 1));
        $productIds = $stockCollection->getColumnValues('product_id');

        if (count($productIds)) {
            Mage::getResourceSingleton('cataloginventory/indexer_stock')->reindexProducts($productIds);
        }

        // Reindex previously remembered items
        $productIds = array();
        foreach ($this->_itemsForReindex as $item) {
            $item->save();
            $productIds[] = $item->getProductId();
        }
        Mage::getResourceSingleton('catalog/product_indexer_price')->reindexProductIds($productIds);

        $this->_itemsForReindex = array(); // Clear list of remembered items - we don't need it anymore

        return $this;
    }

It is from this I derived the previously noted patch
Note: unless theer is a stock-out sale on the site, none of the products ever have stock management setup. So, for us, stock re-indexing hardly ever runs, which I think is a great improvement.

from magento-lts.

@bob2021 Suffering quite badly with timeouts at checkout in our shop and we have a lot of stock that needs inventory managed. Do you know if your modifications (the one you share as having been run in production) still works OK and is believed safe to put into production?

Thanks for yours (and everyone else's) hard work on figuring this out!

from magento-lts.

Hi @doctea, I left the job where I was working on this a few years ago so I can't say for sure if the changes still work ok. They were certainly running in production at the time I originally created this issue, but unfortunately that doesn't mean they will work well in your specific Magento deployment or that problems weren't found after I left that I just don't know about. From the comments on some of the PRs it seems like these patches have issues with 3rd party indexing modules.

What tables are you seeing lock timeouts on?

from magento-lts.

Hi @doctea, I left the job where I was working on this a few years ago so I can't say for sure if the changes still work ok. They were certainly running in production at the time I originally created this issue, but unfortunately that doesn't mean they will work well in your specific Magento deployment or that problems weren't found after I left that I just don't know about. From the comments on some of the PRs it seems like these patches have issues with 3rd party indexing modules.

What tables are you seeing lock timeouts on?

Thanks for getting back to me, @bob2021 ! Seeing sporadic timeout problems in many different tables, mostly customer and sales related. (Perhaps I'm conflating multiple issues but my investigations lead me here anyway -- timeouts seemed to surround order creation/indexing mostly).

I also found some problems when using the MantiCorp_SphinxSearch module (DDL statements can't be executed in a transaction..) but I think I found a way to work around that.

In the end I found that hundreds of downloadable products had stock management enabled unnecessarily, so I've disabled all of those and hopefully that'll solve the issue. I've got a local branch based on your patch ready to try if that doesn't work though :)

Thanks again!

from magento-lts.

Did this improvement materialize in a PR or did it remain in the discussion stage?

there are many PRs linked to this issue, thanks to the original author ;-)

from magento-lts.

This seems to be merged https://github.com/OpenMage/magento-lts/blob/v20.1.0-rc2/app/code/core/Mage/CatalogInventory/Model/Observer.php#L761
from #562

from magento-lts.

Not 100% correct. This is an overall discussion on the topic. The merge you are refering to was here: #562

from magento-lts.

@ProxiBlue Okay, thanks for making it clear.

from magento-lts.