Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extended indexes for hook_status_scheduled_date_gmt and status_sheduled_date_gmt #992

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

prettyboymp
Copy link
Contributor

@prettyboymp prettyboymp commented Sep 20, 2023

Fixes #978

The current hook and status indexes can be pretty useless as there isn't enough uniqueness in the generated index to add much value to query performance. Extending these keys to include the status and scheduled_date_gmt make them much more useful for functions like as_next_scheduled_action() and ActionScheduler_Store::has_pending_actions_due().

Changelog

Fix - Improve indicies within the actions table.

…heduled_date_gmt

Fixes #978

The current `hook` and `status` indexes can be pretty useless as there isn't enough uniqueness in the generated index to add much value to query performance.  Extending these keys to include the `status` and `scheduled_date_gmt` make them much more useful for functions like `as_next_scheduled_action()` and
`ActionScheduler_Store::has_pending_actions_due()`.
@prettyboymp prettyboymp marked this pull request as ready for review September 20, 2023 19:28
@lsinger
Copy link
Contributor

lsinger commented Sep 29, 2023

Hello @prettyboymp! You assigned @peterfabian to this PR -- did you mean to request a review from him?

In any case he has switched teams -- if this PR is ready for review then please request a review from @woocommerce/proton. ☺️

@prettyboymp prettyboymp requested review from a team and barryhughes and removed request for a team September 29, 2023 11:33
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't take effect for existing installations without some migration code: we don't necessarily need that to merge (there are valid reasons not to), but I wanted to check and make sure that's also fine from your perspective, @prettyboymp?

@prettyboymp
Copy link
Contributor Author

This won't take effect for existing installations without some migration code: we don't necessarily need that to merge (there are valid reasons not to), but I wanted to check and make sure that's also fine from your perspective, @prettyboymp?

🤔 This is what I wasn't sure about. I haven't looked into the update process for Action Scheduler yet. I wanted to get some initial feedback before diving in too deep with it. So Action Scheduler tables are excluded from the dbDelta queries during a Woo update?

Either way, it may be a good idea to add a schema update step for this so the previous indexes can be dropped to save disk space. I can work on that.

@barryhughes
Copy link
Member

The table definitions still pass through dbDelta, but my memory is that key changes will not be applied if the tables already exist.

...That said, we've had some discussion in the past as to whether it is even a good idea to modify keys for existing installs (since, especially for sites of a certain size, they may anyway have been tweaked and fine-tuned in a project-specific way).

@prettyboymp
Copy link
Contributor Author

Looking through the dbDelta code, I believe that new indexes will be automatically created, but existing indexes won't be modified, i.e. if the name stays the same, but the parts change, dbDelta won't modify the index.

In this case, both indexes had the names changed as well, so it would add them on the next dbDelta run but won't remove the previous indexes. That should be fine for most use cases but allow those that specifically need to drop the old indexes for space have that separate control.

@barryhughes barryhughes added this to the 3.6.5 milestone Oct 23, 2023
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks (and sorry for the delay in updating this)!

@barryhughes barryhughes merged commit 664feba into trunk Oct 23, 2023
36 of 38 checks passed
@barryhughes barryhughes deleted the fix/add-expanded-indexes branch October 23, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor query performance in as_has_scheduled_action() and as_next_scheduled_action()
4 participants