Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Add index for args column #36

Merged
merged 1 commit into from
Aug 8, 2018
Merged

Add index for args column #36

merged 1 commit into from
Aug 8, 2018

Conversation

thenbrent
Copy link
Contributor

@thenbrent thenbrent commented Aug 8, 2018

Fixes #35

While patching this, I also noticed the $max_index_length value, and found some backstory on that 191 character limit here and in the source code of wp_get_db_schema() here.

Because we are changing the length of the args column now anyway, and will be logging a notice to warn developers of that, I figured it was a good idea to constrain its length to an indexable size instead of allowing data that is knowingly too long to be indexed, and therefore, non-performant, to be used.

I also considered reducing the length doing the same for the hook, but have left that off for now even though the same argument applies.

I don't think an update process is necessary for this column given we still have only tagged 1.0.0-beta. The column can be updated manually on the few sites using this plugin already.

Copy link
Contributor

@JPry JPry left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I figured it was a good idea to constrain its length to an indexable size instead of allowing data that is knowingly too long to be indexed, and therefore, non-performant, to be used.

Do we need to make any changes to the key for the hook column, or to the hook column size?

That question doesn't really prevent this being merged, so I'm marking this as approved. Feel free to merge if you don't think any changes are warranted with the hook column.

@thenbrent
Copy link
Contributor Author

thenbrent commented Aug 8, 2018

Do we need to make any changes to the key for the hook column, or to the hook column size?

haha did you see the next sentence in my description? :)

I've opened #39 to discuss hook. I'm OK with enforcing the same limit, I left it out of this PR just to keep it super simple.

I'm marking this as approved. Feel free to merge if you don't think any changes are warranted

Perfect, thanks! Merging.

@thenbrent thenbrent merged commit 0606db1 into master Aug 8, 2018
@delete-merged-branch delete-merged-branch bot deleted the issue_35 branch August 8, 2018 23:59
@JPry
Copy link
Contributor

JPry commented Aug 9, 2018

haha did you see the next sentence in my description? :)

Man, I've apparently been skimming more than I thought I was 😳

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants