Closed adrianduffell closed 3 years ago
becdetat
commented
3 years ago That 10up article only talks about large numbers of rows being used in the wp_options table, not large amounts of data stored in a single row. Is there the same performance issue present in this scenario?
adrianduffell
commented
3 years ago @becdetat Thanks for taking a look. I agree the article is primarily concerned about rows, but I was thinking this part of it applies:
And yet, this table can become bloated quite quickly and keeping its size under control is not a small effort. 10up Systems Engineers often see wp_options ballooning due to:
Plugins installed on large WordPress sites that were designed for smaller sites. These plugins often store data in wp_options rather than tables designed for larger data sets (such as the postmeta table for post-specific data) or external platforms better suited to “big data” like analytics
...
In many cases, issues like these slowly bloat the table and degrade performance over time, offering no clear warning signs.
I think there is still a performance concern here. For example, as an option this data is retrieved on every page view and needs to processed by PHP. This includes front-end views where the data is not used.
I don't think we are placing any upper limit to the size of this value so I think the impact will be greater as more notes are added to the remote inbox.
Note: I didn't realize the value was being stored without whitespace, which is good. I've updated the description to remove the whitespace I had introduced.
becdetat
commented
3 years ago as an option this data is retrieved on every page view and needs to processed by PHP
That option isn't autoloaded though IIRC, so it shouldn't be loaded on every request.
I don't know if mysql/mariadb is different but from experience with SQL Server a TEXT column is just a reference to an out of table store (like a C pointer). The option_value column in wp_options is a longtext which I would expect is a similar kind of thing. I can't find any references to large amounts of text being stored in a longtext column however generally as long as you aren't querying the longtext column it shouldn't have any effect on performance.
The fact that option_value was implemented using longtext rather than something smaller like text or varchar is further indicating to me that large values are ok.
adrianduffell
commented
3 years ago That option isn't autoloaded though IIRC, so it shouldn't be loaded on every request.
In my environment the autoload flag is enabled.
Disabling autoload would help but then fetching the option value requires an additional SQL query to be executed. My understanding is it is more performant to limit the amount of queries made to the database overall. A transient has the benefit of being stored in a cache where it can be retrieved faster. From the docs:
Also of note is that Transients are inherently sped up by caching plugins, where normal Options are not. A memcached plugin, for example, would make WordPress store transient values in fast memory instead of in the database. For this reason, transients should be used to store any data that is expected to expire, or which can expire at any time. Transients should also never be assumed to be in the database, since they may not be stored there at all.
moon0326
commented
3 years ago This is related to https://github.com/woocommerce/woocommerce-admin/issues/6781
I also see that the option has autoload set to yes. wc_remote_inbox_notifications_specs is only used once daily when wc_admin_daily runs in the background as far as I understand. I think using transient to speed up is a nice option, but I'm not sure if it's worth updating the code when the option is used in a specific use case only. Changing autoload to no is sufficient in my opinion.
adrianduffell
commented
3 years ago Thanks @moon0326 , I didn't realize it was used only once daily.
I guess it raises the question why even store this data at all when there is such small usage. Wouldn't the data just be stale the next day when wc_admin_daily runs? I'm happy to investigate this later in a separate issue if we just want to change the autoload flag here.
moon0326
commented
3 years ago Thanks @moon0326 , I didn't realize it was used only once daily.
I guess it raises the question why even store this data at all when there is such small usage. Wouldn't the data just be stale the next day when
wc_admin_dailyruns? I'm happy to investigate this later in a separate issue if we just want to change theautoloadflag here.
@adrianduffell I think you're correct. This line calls DataSourcePoller::read_specs_from_data_sources(), which overwrites the existing data.
It would be nice if you can take a look at the autoload update 🙇♂️ . I can also work on it if you're currently occupied. I think we should change autoload to no at least.
Update: We should change autoload to no for the following options.
wc_remote_inbox_notifications_specs wc_remote_inbox_notifications_stored_state wc_remote_inbox_notifications_wca_updated
adrianduffell
commented
3 years ago @moon0326 NP I could work on the autoload=no update later this week so it makes our next release. Are those other remote inbox options also accessed just once per day?
moon0326
commented
3 years ago @moon0326 NP I could work on the
autoload=noupdate later this week so it makes our next release. Are those other remote inbox options also accessed just once per day?
Thank you! Yup, I think those are also accessed once per day.
adrianduffell
commented
3 years ago @moon0326 I started working on changing these options to autoload=no but discovered the remote inbox is triggered whenever a product is added/updated as well. This is discussed more in issue https://github.com/woocommerce/woocommerce-admin/issues/6189.
I think then that autoload=no would introduce a performance issue due to the extra database queries it creates. For example I think this would effect bulk product imports where the database queries would multiply in a short period of time.
Because of this I'm back to thinking transients are the best path forward.
becdetat
commented
3 years ago @adrianduffell I've got a pr (#6995) which offloads the engine run from the product add/update. I'll do up a PR now changing autoload for these options.
Describe the bug
A large amount of data is present in the
wc_remote_inbox_notifications_specsoption. This type of data seems better suited as a transient, since overall it is a good practice to keep the options table light for performance. More info.To Reproduce
On a site with WooCommerce onboarding completed,
wc_remote_inbox_notifications_specsoption. For example:Expected behavior
Expecting to see this data stored in a transient to avoid performance issues with options.
Additional context
My
wc_remote_inbox_notifications_specsoption value: