WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 34 hours ago

#13310 assigned enhancement

Extend option_name to varchar(255)

Reported by: scribu Owned by: pento
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Database Keywords: needs-patch
Focuses: Cc:

Description

option_name is currently set to varchar(64). This raises problems when one tries to use transients with slightly longer names and a timeout:

_transient_timeout_feed_mod_23a137101df6920fbf6047... has 60 chars already.

Attachments (6)

schema.diff (561 bytes) - added by scribu 5 years ago.
schema.2.diff (541 bytes) - added by OriginalEXE 9 months ago.
Updated patch
13310-420.patch (1.7 KB) - added by khromov 6 months ago.
13310-430.diff (999 bytes) - added by MikeHansenMe 7 weeks ago.
Updated for 4.3
13310-430.patch (916 bytes) - added by khromov 7 weeks ago.
13310-430.2.diff (1.0 KB) - added by MikeHansenMe 7 weeks ago.
inside the conditional

Download all attachments as: .zip

Change History (68)

@scribu5 years ago

comment:1 @wpmuguru5 years ago

  • Keywords commit removed
  • Milestone changed from 3.0 to Unassigned

This schema change would have a significant impact on large WP networks.

IMO, it should be punted to 3.1 to give admins of large installs an opportunity to implement the schema change prior to running the upgrade.

comment:2 @nacin5 years ago

  • Keywords 2nd-opinion added

Yeah, no way we're rushing in a schema change. I'm not sure it's necessary, frankly.

Going to leave the unassigned milestone and tag for 2nd-opinion, but expecting wontfix.

comment:3 @scribu5 years ago

Related: #15058

comment:4 @chrisbliss185 years ago

This isn't a bad idea. Especially the specific length.

The get_site_option function uses the options table when the site isn't multisite and uses the sitemeta table when it is multisite. This translates into a maximum name size of 64 characters (length of option_name) for site option storage for non-multisite and 255 characters (length of meta_key) for multisite.

comment:5 @nacin5 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:6 @fabio843 years ago

  • Cc fabio84 added
  • Version set to 3.4.2

Wow, in 2 years you were not able to make option_name a varchar(255)... I tried to use get_transient and set_transient, but I got stuck by this "feature".

As reported also in #15058 with transient values the limit is 45 characters, and if you use hashes and long identifiers to avoid collisions it's quite easy to reach the limit.

Please excuse me for this rant, but developing WordPress plugins using its APIs sometimes is frustrating.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

comment:7 follow-up: @tivnet2 years ago

  • Type changed from enhancement to defect (bug)

Still not fixed in 2013!
Version: WP 3.6-beta3-24265

Example of the error:

[22-May-2013 22:29:06 UTC] WordPress database error
Data too long for column 'option_name' at row 1 for query
INSERT INTO `wp_options` (`option_name`, `option_value`, `autoload`)
VALUES ('_transient_timeout_woocommerce-gateway-paypal-express/woocommerce-gateway-paypal-express.php_version_data'

@SergeyBiryukov - varchar(255) - is that really hard to make? While 3.6 is still in beta...

comment:8 @tivnet2 years ago

  • Cc gregory@… added

comment:9 follow-up: @markoheijnen2 years ago

I do agree that we should fix it but the error you have is also stupid from WooCommerce or the PayPal plugin.

comment:10 in reply to: ↑ 9 ; follow-up: @tivnet2 years ago

Replying to markoheijnen:

I do agree that we should fix it but the error you have is also stupid from WooCommerce or the PayPal plugin.

Well, why this error is more stupid than that one from @Scribu, the original reporter? Anyway, I believe, there is no reason to keep varchar(64) these days, whether the transients are stupid or smart :-)

comment:11 in reply to: ↑ 7 ; follow-up: @SergeyBiryukov2 years ago

Replying to tivnet:

@SergeyBiryukov - varchar(255) - is that really hard to make? While 3.6 is still in beta...

It's simple on single site installs, but can be an issue on large network installs, as noted in comment:1.

We're in late beta, when lots of things are being punted to a future release. Anything that requires a schema change should be brought up early in the cycle.

comment:12 in reply to: ↑ 10 @markoheijnen2 years ago

Replying to tivnet:

Replying to markoheijnen:

I do agree that we should fix it but the error you have is also stupid from WooCommerce or the PayPal plugin.

Well, why this error is more stupid than that one from @Scribu, the original reporter? Anyway, I believe, there is no reason to keep varchar(64) these days, whether the transients are stupid or smart :-)

The name of the transient is way too long. It's includes the plugin location of the plugin with the name. So for a fast fix you should contact WooCommerce team and see what they can do.

comment:13 in reply to: ↑ 11 @tivnet2 years ago

We're in late beta, when lots of things are being punted to a future release. Anything that requires a schema change should be brought up early in the cycle.

Well, I guess this is a dead horse then. 3 years ago was not early enough :-)

Started with that, but... it will probably take 5 years to Woo, if it takes 3 years to WP :-)

Thank you!

Version 1, edited 23 months ago by nacin (previous) (next) (diff)

comment:14 @ocean9023 months ago

#25035 was marked as a duplicate.

comment:15 @betzster22 months ago

  • Cc j@… added

comment:16 @sc0ttkclark20 months ago

  • Cc lol@… added

comment:17 @tivnet20 months ago

Celebrating 4 years of this soon... MAZAL TOV! :-)))

comment:18 follow-up: @sc0ttkclark19 months ago

I'd really like to see this move forward, what's exactly the holdup? Is it the lack of use-cases for using longer option names? If that's the case, I'd think that most use-cases currently use bad hack arounds and just deal with it, and the lack of characters in option_name begets use-cases for longer character use.

comment:19 @DrewAPicture13 months ago

In 28735:

Improve inline documention for set_transient() and set_site_transient() to specify the 45- and 40-character limits for their respective transient name values.

Props edwin-at-studiojoyo.com for the original patch.
See #15058, #13310. Fixes #28467.

comment:20 in reply to: ↑ 18 @DrewAPicture13 months ago

  • Milestone changed from Future Release to 4.0

Replying to sc0ttkclark:

I'd really like to see this move forward, what's exactly the holdup?

I'm assuming the primary holdup is the schema change. Moving to 4.0 for consideration.

comment:21 @khromov13 months ago

Really hoping for a schema change!

The Network Update feature should make updating the schema a breeze, no?

comment:22 @ircbot13 months ago

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.

comment:23 @archon81013 months ago

Unbelievable that this ticket has now spent 4 years in discussions that went nowhere. I just ran into a bug that kept failing transients, and the culprit was the key name.

At the very least, the set_transient function should error out altogether, with a message of some sort when the key is known to get truncated.

I'm going to work on getting the keys smaller and I'll probably manually up the key length to 255.

Btw, it was quite easy to run into this issue by adding a domain to the transient key name, along with a few more things. I only caught this problem after looking at the database data.

comment:24 @ryan12 months ago

  • Owner ryan deleted
  • Status changed from new to assigned

comment:25 @ircbot11 months ago

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.

comment:26 follow-up: @DrewAPicture11 months ago

  • Keywords 4.1-early added
  • Milestone changed from 4.0 to Future Release

To late for a schema change. Maybe 4.1-early will be the ticket.

comment:27 in reply to: ↑ 26 @tivnet11 months ago

Replying to DrewAPicture:

To late for a schema change. Maybe 4.1-early will be the ticket.

This ticket is cursed...

How about moving transients to a separate table instead?

comment:28 @OriginalEXE9 months ago

I believe now would be the time to bump this :)

comment:29 @Hube29 months ago

Yes please, since updating to 4.0 reset this field to a length of 64 on a site where I needed to increase it to 255 to make the site work. Which means that I need to either find a way to disable this reset or not upgrade the site. Why did 4.0 update mess with my tables?

Last edited 9 months ago by Hube2 (previous) (diff)

comment:30 @sc0ttkclark9 months ago

@Hube2 Upgrades run a DB delta check, so any inconsistencies will get fixed, or in our case, any customizations to the field lengths.

comment:31 @OriginalEXE9 months ago

I'll see if we can maybe tackle this on contributors day #wceu

comment:32 @archon8109 months ago

Odd. I changed mine to 255 4 months ago, and it's still 255, so it didn't get reset during the 4.0 update.

comment:33 @Hube29 months ago

I believe this will prevent it from happening http://codex.wordpress.org/Editing_wp-config.php#Do_not_upgrade_global_tables. Not sure it's the best temporary solution though.

@OriginalEXE, I'm sure not only I would be extremely grateful. There is a very popular plugin that saves values to the options table for fields added to terms as well as other admin pages. Due to the way that plugin generates field names "option_name" the length of them can grow quite long at times.

@OriginalEXE9 months ago

Updated patch

comment:34 @OriginalEXE9 months ago

I have uploaded the updated patch. Now I just need to get this going with someone with commit access.

comment:35 follow-up: @jdgrimes9 months ago

#30048 was marked as a duplicate.

comment:36 in reply to: ↑ 35 @akshay_raje9 months ago

Replying to jdgrimes:

#30048 was marked as a duplicate.

Didn't know this is 4 year old issue! Having a 64 character key with 11-19 characters reserved (for _transient_ or _transient_timeout_) really leaves little scope to use any hashes as keys.

Please target to fix this in next release.

comment:37 @khromov9 months ago

@akshay_raje

Check: #15058

comment:38 @Hube29 months ago

For anyone in the same situation as myself that has altered options_name in the DB to allow 255 characters, I have also created a plugin that can be used to prevent WP from changing it back again. https://github.com/Hube2/wp-update-prevent-db-changes

Validation is not sufficient because as was just mentioned, 64 characters is simply not a sufficient size under all conditions.

comment:39 @SergeyBiryukov9 months ago

#29879 was marked as a duplicate.

comment:40 @nerrad7 months ago

If this isn't going to get fixed, then transients should get their own bonafide table. Which imo is a better option(!) anyways.

comment:41 @Hube27 months ago

Any word on this being in 4.1?

comment:42 follow-up: @nerrad7 months ago

If this isn't going to get fixed, then transients should get their own bonafide table. Which imo is a better option(!) anyways

Either that or add a timestamp column to the options table so that transients don't require two rows for each transient, which might be a good interim measure.

comment:43 in reply to: ↑ 42 @tivnet7 months ago

Replying to nerrad:

If this isn't going to get fixed, then transients should get their own bonafide table. Which imo is a better option(!) anyways

Separate table, of course. Just TRUNCATE it to clear all transients!

comment:44 follow-up: @Hube27 months ago

I really think that transients and putting in a new table is a completely separate discussion.

Changing option_name from varchar(64) to varchar(255) is really a rather simple schema change that should have no other effects on WP other then stopping the silent failure to get values from the database because the option_name value was too long and truncated when the value was saved. I just don't understand why this is 5 years old.

comment:45 in reply to: ↑ 44 @nerrad7 months ago

Replying to Hube2:

I really think that transients and putting in a new table is a completely separate discussion.

Yeah true, however with the lack of traction that has gone on for this "simple" schema change maybe there's other things that should be done that will resolve the existing issue.

Either way, there really should be some feedback as to:

  1. Why this has been lingering for 5 years (yeah network install WP sites is the given reason... but why?)
  2. It's looking like this will NOT go in 4.1 because of where we are in the 4.1 release cycle... if not WHY? (which will likely lead to the answer for number 1.

So really, if a simple schema change is really not being considered.

  1. What needs to change to make it considered?
  2. Do we need to evaluate whether using the options table for transients is what exposed this problem to begin with. And if so, maybe that reveals the need for something different to be done for transients (which would yes warrant another ticket likely and then CLOSING this ticket that is getting no action).
  3. OR as I also suggested, would adding a timestamp column to the options table and introducing a change for transients to use one row instead of two be a good interim measure both to incrementally fix the issue raised by this ticket and satisfy the holdup for making a "simple" schema change on an existing column?

comment:46 @Hube27 months ago

@nerrad

I completely agree.

What exposed this problem for me?

I use the plugin Advanced Custom Fields (ACF). For those not familiar with it, you can add custom fields to options pages. The values for options pages are stored in the options table. This is not a problem most of the time. The problem manifests itself when using nested repeater fields or flexible content fields because of the way that ACF generates the option_name values of these types of fields.

I didn't get into using transients until after I discovered the issue with ACF, and being already aware of the that issue I already knew I needed to be careful when generating the option_name for any transients I was creating.

But as you say, I would really like to understand why, what I think is an extremely simple fix, would cause any issues for any site, network or not. I have gone into many sites, both network sites and not, small sites and huge sites, and manually changed option_name from 64 to 255 and never had a problem. I've even created my own MU plugin that I install on these sites to prevent WP from changing it back again. So I'm really confused about what problems this could create and why it has been waiting 5 years with no action taken.

comment:47 @khromov6 months ago

In hopes of getting this resolved for 4.2, I am attaching a full patch (against trunk) which covers both schema change for new installations as well as schema update for existing installations. I have tested this on multiple sites without any issues. Feedback welcome.

@khromov6 months ago

comment:48 @khromov5 months ago

@scribu @ryan

Any chance to lift this discussion for 4.3? It's been punted a couple of times already so starting the discussion early could maybe help this get resolved (one way or another).

comment:49 @Hube25 months ago

they are asking for requests, already posted this one but people could 2nd, 3rd, 4th, whatever it. https://make.wordpress.org/core/2015/01/28/trac-tickets-the-bands-taking-requests/

comment:50 @wonderboymusic5 months ago

  • Owner set to pento

@pento can speak to this one, and give his expert advice in some direction or the other

comment:51 @slackbot5 months ago

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.

comment:52 @pento5 months ago

  • Keywords 4.3-early added; 2nd-opinion 4.1-early removed

Huh, I could've sworn I'd posted an opinion on this previously.

Anyway, I have no problem doing this in 4.3. We already have several time-consuming schema changes scheduled for 4.2, I'd like to avoid adding any more.

comment:53 @khromov4 months ago

@pento,

I think a lot of people both here and in #15058 would love to see an increase to 255 chars. The patch I wrote still works, just needs to update the function name to upgrade_430().

https://core.trac.wordpress.org/ticket/13310#comment:47

Seeing as there was a huge schema change in 4.2 (switching to utf8mb4), this is an incredibly minor tweak in comparison.

comment:54 @khromov3 months ago

@pento @scribu - Any way to get this into 4.3? If you need anything, let us know!

comment:55 @pento3 months ago

I think this is a good candidate for 4.3. If I haven't done anything about by early May (I usually look at my x-early tickets in the week or two following the previous release), please ping me again, either here or in Slack. :-)

comment:56 @obenland2 months ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

@MikeHansenMe7 weeks ago

Updated for 4.3

comment:57 @MikeHansenMe7 weeks ago

When this goes in we need to make sure #15058 get closed since they are 2 approaches to the same problem.

comment:58 @johnbillion7 weeks ago

  • Keywords needs-patch added; has-patch removed
  • Version 3.4.2 deleted

This needs to be inside a database version conditional, probably the one right above it.

comment:59 @khromov7 weeks ago

Hey @johnbillion

Attaching new patch for upgrading to WP 4.3.

@khromov7 weeks ago

@MikeHansenMe7 weeks ago

inside the conditional

comment:60 @MikeHansenMe6 weeks ago

  • Keywords has-patch added; needs-patch removed

comment:61 follow-up: @dd327 days ago

  • Keywords needs-patch added; has-patch removed
  • Type changed from defect (bug) to enhancement

Shifting to an enhancement - Although some might find the field too short, it's not a bug that it's that length IMHO.

The patch will also need to alter the index length from unbounded to 191 characters to abide by the utf8mb4 767 byte InnoDB index limits.
I don't expect that this will make it into 4.3, but I would like to see it done at some point.

comment:62 in reply to: ↑ 61 @obenland34 hours ago

  • Milestone changed from 4.3 to Future Release

Replying to dd32:

I don't expect that this will make it into 4.3, but I would like to see it done at some point.

Let's pick it up in 4.4 again then.

Note: See TracTickets for help on using tickets.