[PLTFRM-2722] Add index for action/instance lookups on the events table - #515
Open
WRasada wants to merge 1 commit into
Open
[PLTFRM-2722] Add index for action/instance lookups on the events table#515WRasada wants to merge 1 commit into
WRasada wants to merge 1 commit into
Conversation
Lookups by action + instance can't use ts_action_instance_status, since that key leads with timestamp and those queries don't filter on it. They fall back to scanning the whole table, which shows up on any site with a large pending queue. Adds KEY action_instance_status_ts (action(191), instance, status, timestamp), bumps DB_VERSION, and adds a version check so existing installs pick up the new index. DB_VERSION was previously written on install but never read back, so a schema change alone would only have reached new installs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
rebeccahum
reviewed
Aug 19, 2026
| PRIMARY KEY (`ID`), | ||
| UNIQUE KEY `ts_action_instance_status` (`timestamp`, `action` (191), `instance`, `status`), | ||
| KEY `status` (`status`) | ||
| KEY `status` (`status`), |
Contributor
There was a problem hiding this comment.
What about only upgrading the option version if this succeeds?
@sjinks can use your input here
Member
|
We later have: $is_installed = 1 === count( $wpdb->get_col( $wpdb->prepare( 'SELECT TABLE_NAME FROM information_schema.tables WHERE TABLE_NAME = %s', $table_name ) ) );
wp_cache_set( 'is_installed', $is_installed, 'cron-control' );
if ( $is_installed ) {
update_option( self::DB_VERSION_OPTION, self::DB_VERSION );
}But if the upgrade fails, we will still have the table but lack the index. However, |
This file contains hidden or 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
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.
Fixes #516
Lookups by
action+instancecan't usets_action_instance_status, since that key leads withtimestampand those queries don't filter on it. They fall back to scanning the whole table, which shows up on any site with a large pending queue.Both
wp_clear_scheduled_hook()(viapre_clear_scheduled_hook(), which queries withlimit => 500) andwp_next_scheduled()(limit => 1) produce that shape, and WP core calls the latter inside everywp_schedule_single_event().This adds an index that covers that access pattern, plus the version check needed for existing installs to pick it up.
What's here
KEY action_instance_status_ts (action(191), instance, status, timestamp)in the schema.DB_VERSIONbumped to2.class_init()that registers an upgrade onshutdownwhen the stored version is behind.maybe_upgrade_during_shutdown(), which mirrors the existing install method.On the upgrade path
DB_VERSIONwasn't being read anywhere - it's written on install but never compared - and_prepare_table()only runs when the table is missing. So without a version check the schema change would only reach new installs.The check sits alongside the existing install branch, so the
shutdownhook is only registered while a site is actually behind.DB_VERSION_OPTIONis autoloaded, so the check itself doesn't add a query. It runs once, the option bumps, and the hook stops registering after that.maybe_upgrade_during_shutdown()is a sibling method rather than a change to the install one, since that method returns early when the table already exists - which is exactly the case an upgrade needs to run in. It keeps the same gating, so it only fires on cron, WP-CLI, and non-ajax admin requests, and it takes its own one-minute cache lock before doing anything.If the upgrade ever failed, the option wouldn't bump and it'd retry on the next eligible request. That matches the "keep trying" behaviour the install path already has.
On running this against existing tables
Adding a secondary index in InnoDB runs with
ALGORITHM=INPLACE, LOCK=NONE, so reads and writes keep going while it builds. We ran the equivalent statement on a production table of roughly 38,000 rows and 49 MB with no measurable impact.Happy to switch the upgrade to a targeted
ALTER TABLE ... ADD INDEXinstead of reusing_prepare_table()if you'd prefer something more predictable than a fulldbDelta()pass. Smaller blast radius, just more code.Why it showed up hourly
These lookups are object cached, so the scan doesn't normally reach response times. The hourly
a8c_cron_control_purge_completed_eventsrun callsflush_event_cache()with no arguments, which bumpslast_changedfor bothcron-control-queriesandcron-control-event:Since the keys are built from
wp_cache_get_last_changed(), every cached lookup invalidates at once and they all hit the database together, each running the full scan.The index doesn't change any of that - it just makes those cold lookups cheap. On the site we were looking at, the hourly window went from 620 transactions over 15s down to 3, and the datastore max for the table went from 51.19s to 0.215s.
Worth mentioning separately: the narrower
cron-control-eventgroup exists to "avoid most bulk invalidations" per the comment at line 463, but the null-argumentflush_event_cache()call clears it anyway. That seems like its own thing rather than something to change here, so I've left it alone.Verifying
Before, on a table with about 37,000 pending rows:
After:
38,986 rows read down to 0, and 98.3 ms down to 0.027 ms.
Still to add
Tests aren't in this PR yet - happy to add them here or in a follow-up, whichever you prefer:
Related: #517 covers ready-event selection loading the full pending queue into PHP, which is a separate problem this PR doesn't touch.
Tracking: PLTFRM-2722