Transactions, caching, and version transient cleanup issues #20537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
claudiulodro
added
needs: author feedback
mikejolley
added
status: needs review
and removed
needs: author feedback
includes/class-wc-cache-helper.php
Outdated
| @@ -136,7 +136,7 @@ public static function get_transient_version( $group, $refresh = false ) { | |||
| $transient_value = get_transient( $transient_name ); | |||
|
|
|||
| if ( false === $transient_value || true === $refresh ) { | |||
| self::delete_version_transients( $transient_value ); | |||
| self::queue_delete_version_transients( $transient_value ); | |||
There was a problem hiding this comment.
There is still one failing unit test. It seems to be caused by this change, as reverting this line to instantly delete makes it pass again.
If there is potential for this to cause issues, maybe have the deferred transaction delete only happen if a defined constant is true (e.g. WC_DEFER_TRANSACTION_DELETE)?
There was a problem hiding this comment.
Tests run so quick I assume it's because time() is returning the same value. Thats a bug.
Codecov Report
@@ Coverage Diff @@
## master #20537 +/- ##
=========================================
Coverage ? 28.21%
Complexity ? 13227
=========================================
Files ? 356
Lines ? 49707
Branches ? 0
=========================================
Hits ? 14021
Misses ? 35686
Partials ? 0
Continue to review full report at Codecov.
|
claudiulodro
approved these changes
Jun 15, 2018
7 tasks
3 tasks
rodrigoprimo
added a commit
that referenced
this pull request
Sep 5, 2018
This commit reverts commits 2f8a3ea and 17e97c2 that were created to defer transient cleanup (see #20537) and avoid deadlocks on the wp_options table (see #20528 and #17632). The problem is that deferring transient cleanup to a cron job created an issue when creating or importing multiple products at once (see #21100 and woocommerce/wc-smooth-generator#14 (comment)) and has the potential to impact the checkout as well if we start using more versioned transients for orders. This problem is happening because when importing or creating multiple products at once, for each product that is created or imported, WooCommerce core enqueues a few 'delete_version_transients' cron events. Events are enqueued faster than they are executed and after a few hundred products are generated, the size of the cron queue, which is stored in a single wp_options entry, starts to impact WordPress performance in general. To reduce the chance of deadlocks happening again after this change, I already created another PR to optimize the query used to delete transients (#21274) by avoiding an unnecessary filesort, and I'm planning, on a subsequent commit, to improve it further by prefixing the transient name with its version instead of suffixing it as it is currently done. But the ultimate solution for high traffic stores is to use a persistent cache plugin.
mikejolley
pushed a commit
that referenced
this pull request
Jan 21, 2019
This commit reverts commits 2f8a3ea and 17e97c2 that were created to defer transient cleanup (see #20537) and avoid deadlocks on the wp_options table (see #20528 and #17632). The problem is that deferring transient cleanup to a cron job created an issue when creating or importing multiple products at once (see #21100 and woocommerce/wc-smooth-generator#14 (comment)) and has the potential to impact the checkout as well if we start using more versioned transients for orders. This problem is happening because when importing or creating multiple products at once, for each product that is created or imported, WooCommerce core enqueues a few 'delete_version_transients' cron events. Events are enqueued faster than they are executed and after a few hundred products are generated, the size of the cron queue, which is stored in a single wp_options entry, starts to impact WordPress performance in general. To reduce the chance of deadlocks happening again after this change, I already created another PR to optimize the query used to delete transients (#21274) by avoiding an unnecessary filesort, and I'm planning, on a subsequent commit, to improve it further by prefixing the transient name with its version instead of suffixing it as it is currently done. But the ultimate solution for high traffic stores is to use a persistent cache plugin.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
3 changes here, all semi-related, which should be rolled out to prevent current conflicts.
This should also help with #17660, however we're not 100% sure as we're unable to replicate the cases in that thread.
This PR reverts some changes made in 1918e2e which has been causing odd issues on stores due to deadlocks, and is potentially guilty of causing double charges on woo.com (cc @kloon).
To Test:
Transactions
Transactions were used so changes could be rolled back, however, it seems this is causing issue if large amounts of data are written due to all the hooks that fire. e.g. in #20528 Not using transactions could lead to partial updates, however, these are not expected and only occur if something (plugin?) causes an error. As such, I'm confident logging will be enough for an admin to resolve.
Disabling transactions seemed to have a positive impact on woo.com, so rolling this out as a fix.
Transient cleanup
Related to the above and raised in #20528. I noticed cleanup runs immediately. We don't need to clean up right away and slow the current process. Deferring the event via cron will work.
Cache ID 0
Not confirmed, but we theorised caching for order ID 0 could be a factor in #17660. This PR simply checks to ensure there is an ID before using caching.
Closes #17660
Closes #20528