WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 4 weeks 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 10 months ago.
Updated patch
13310-420.patch (1.7 KB) - added by khromov 7 months ago.
13310-430.diff (999 bytes) - added by MikeHansenMe 3 months ago.
Updated for 4.3
13310-430.patch (916 bytes) - added by khromov 3 months ago.
13310-430.2.diff (1.0 KB) - added by MikeHansenMe 3 months 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 2 years ago by nacin (previous) (next) (diff)

comment:14 @ocean902 years ago

#25035 was marked as a duplicate.

comment:15 @betzster2 years ago

  • Cc j@… added

comment:16 @sc0ttkclark21 months ago

  • Cc lol@… added

comment:17 @tivnet21 months ago

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

comment:18 follow-up: @sc0ttkclark20 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 @DrewAPicture14 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 @DrewAPicture14 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 @khromov14 months ago

Really hoping for a schema change!

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

comment:22 @ircbot14 months ago

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

comment:23 @archon81014 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 @ryan13 months ago

  • Owner ryan deleted
  • Status changed from new to assigned

comment:25 @ircbot12 months ago

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

comment:26 follow-up: @DrewAPicture12 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 @tivnet12 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 @OriginalEXE10 months ago

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

comment:29 @Hube210 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 10 months ago by Hube2 (previous) (diff)

comment:30 @sc0ttkclark10 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 @OriginalEXE10 months ago

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

comment:32 @archon81010 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 @Hube210 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.

@OriginalEXE10 months ago

Updated patch

comment:34 @OriginalEXE10 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: @jdgrimes10 months ago

#30048 was marked as a duplicate.

comment:36 in reply to: ↑ 35 @akshay_raje10 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 @khromov10 months ago

@akshay_raje

Check: #15058

comment:38 @Hube210 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 @SergeyBiryukov10 months ago

#29879 was marked as a duplicate.

comment:40 @nerrad8 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 @Hube28 months ago

Any word on this being in 4.1?

comment:42 follow-up: @nerrad8 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 @tivnet8 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: @Hube28 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 @nerrad8 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 @Hube28 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 @khromov7 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.

@khromov7 months ago

comment:48 @khromov6 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 @Hube26 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 @wonderboymusic6 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 @slackbot6 months ago

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

comment:52 @pento6 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 @khromov5 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 @khromov4 months ago

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

comment:55 @pento4 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 @obenland3 months ago

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

@MikeHansenMe3 months ago

Updated for 4.3

comment:57 @MikeHansenMe3 months ago

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

comment:58 @johnbillion3 months 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 @khromov3 months ago

Hey @johnbillion

Attaching new patch for upgrading to WP 4.3.

@khromov3 months ago

@MikeHansenMe3 months ago

inside the conditional

comment:60 @MikeHansenMe2 months ago

  • Keywords has-patch added; needs-patch removed

comment:61 follow-up: @dd325 weeks 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 @obenland4 weeks 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.