WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 2 months ago

#10404 new enhancement

dbDelta creates duplicate indexes when index definition contains spaces

Reported by: Denis-de-Bernardy Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8.1
Component: Database Keywords: has-patch
Focuses: Cc:

Description

I was adding a much needed index in wp_object_term_relationships, and testing revealed it was getting added multiple times.

This works as intended:

CREATE TABLE $wpdb->term_relationships (
 object_id bigint(20) unsigned NOT NULL default 0,
 term_taxonomy_id bigint(20) unsigned NOT NULL default 0,
 term_order int(11) NOT NULL default 0,
 PRIMARY KEY  (object_id,term_taxonomy_id),
 UNIQUE KEY reverse_pkey (term_taxonomy_id,object_id),
 KEY term_taxonomy_id (term_taxonomy_id)
) $charset_collate;");

This doesn't:

CREATE TABLE $wpdb->term_relationships (
 object_id bigint(20) unsigned NOT NULL default 0,
 term_taxonomy_id bigint(20) unsigned NOT NULL default 0,
 term_order int(11) NOT NULL default 0,
 PRIMARY KEY  (object_id,term_taxonomy_id),
 UNIQUE KEY reverse_pkey (term_taxonomy_id, object_id),
 KEY term_taxonomy_id (term_taxonomy_id)
) $charset_collate;");

the only difference between the two is a space in the reverse_pkey column list. we should remove spaces in there to avoid potential bugs.

Attachments (3)

dbDelta_match_extra_spacing.diff (2.1 KB) - added by jbutkus 18 months ago.
Ignore whitespace issues within dbDelta duplicates recognition
10404.patch_case_insensitive_key_matching (2.1 KB) - added by charlestonsw 5 months ago.
Update case insensitive compare on dbDelta plus minor phpDoc updates.
10404.patch_caseinsensitive_and_noindexname_compare (5.5 KB) - added by charlestonsw 5 months ago.
Case insensitive index AND simple index with NO NAME patch (see comments for details)

Download all attachments as: .zip

Change History (17)

comment:1 nacin4 years ago

  • Keywords needs-testing added

Patch should fix it, but I can't seem to fully reproduce. Can you test and confirm?

comment:2 nacin4 years ago

  • Keywords needs-testing removed
  • Milestone changed from 2.9 to Future Release
  • Type changed from defect (bug) to task (blessed)

Patch does nothing, I jumped the gun there. The big problem here is that dbDelta wants exacting whitespace standards. The two spaces after "PRIMARY" is documented in the codex, but it also expects exactly one space elsewhere, and as you can see here, exactly no spaces separating field names in an index.

I'm setting this to Future Release and a blessed task to see if someone will pick it up, document dbDelta, and make it less fussy over whitespace.

See also #11322. At the very least, $wp_queries should lead by example.

comment:3 nacin4 years ago

  • Type changed from task (blessed) to enhancement

jbutkus18 months ago

Ignore whitespace issues within dbDelta duplicates recognition

comment:4 jbutkus18 months ago

  • Cc butkus.justas@… added
  • Keywords has-patch added

I get, that this ticket is rather old, but I had experienced the very same issues with WP 3.4.2.

My patch is created against trunk version (https://core.svn.wordpress.org/trunk).

In one place (matching columns) preg_match regexp is extended by adding '\s*' instead of plain ' ' (single space - 0x20).

For indexes - I provided a function, which attempts to normalize indexes, by unifying various edge cases, namely: remove multiple whitespaces, replace 'INDEX' for 'KEY' at the beginning of declaration, etc.

These are based on DB DDLs used by our plugin (All-in-One Event Calendar by Timely). If someone encounters another edge case, which is not covered by these modifications - I may pay time to fix for these as well.

comment:5 charlestonsw15 months ago

WP3.5.1 running WPMU is having the same issue.

Here is the error after a recent plugin activation:

WordPress database error Too many keys specified; max 64 keys allowed for query ALTER TABLE wp_2_store_locator ADD INDEX (sl_store) made by require('wp-blog-header.php'), require_once('wp-load.php'), require_once('wp-config.php'), require_once('wp-settings.php'), include_once('/plugins/store-locator-le/store-locator-le.php'), wpCSL_plugin__slplus->__construct, call_user_func_array, SLPlus_Activate::update, SLPlus_Activate->install_main_table, SLPlus_Activate->dbupdater, dbDelta

Here is the dbDelta structure from my plugin:

        	$sql = "CREATE TABLE $table_name (
                	sl_id mediumint(8) unsigned NOT NULL auto_increment,
                	sl_store varchar(255) NULL,
                	sl_address varchar(255) NULL,
                	sl_address2 varchar(255) NULL,
                	sl_city varchar(255) NULL,
                	sl_state varchar(255) NULL,
                	sl_zip varchar(255) NULL,
                	sl_country varchar(255) NULL,
                	sl_latitude varchar(255) NULL,
                	sl_longitude varchar(255) NULL,
                	sl_tags mediumtext NULL,
                	sl_description text NULL,
                	sl_email varchar(255) NULL,
                	sl_url varchar(255) NULL,
                	sl_hours varchar(255) NULL,
                	sl_phone varchar(255) NULL,
                	sl_fax varchar(255) NULL,
                	sl_image varchar(255) NULL,
                	sl_private varchar(1) NULL,
                	sl_neat_title varchar(255) NULL,
                	sl_linked_postid int NULL,
                	sl_pages_url varchar(255) NULL,
                	sl_pages_on varchar(1) NULL,
                	sl_option_value longtext NULL,
                	sl_lastupdated  timestamp NOT NULL default current_timestamp,   		 
                	PRIMARY KEY  (sl_id),
                	INDEX (sl_store),
                	INDEX (sl_longitude),
                	INDEX (sl_latitude)
                	)
                	$charset_collate
                	";
dbDelta($sql)

The table now has a number of duplicate indexes:

sl_store
sl_store_2
sl_store_3
...
sl_store_21

sl_latitude
sl_latitude_2
...
sl_latitude_21

sl_longitude
sl_longitude_2
...
sl_longitude_21

Am I using dbDelta incorrectly or is this still a known issue? Any workarounds?

Last edited 14 months ago by SergeyBiryukov (previous) (diff)

comment:6 follow-up: RonStrilaeff14 months ago

I also get that this is old but it apparently there is still a problem with dbDelta creating duplicate indexes. I am creating a table within the init action hook like this:

function sv_create_favorites_table(){
	global $wpdb;
	$table_name = $wpdb->prefix . "favorites"; 
	$sql = "CREATE TABLE IF NOT EXISTS $table_name (
		fav_id bigint(20) not null auto_increment,
		post_id bigint(20) not null,
		user_id bigint(20) not null,
		fav_date timestamp not null,
		PRIMARY KEY  (fav_id),
		KEY (post_id),
		KEY (user_id)
		);";	
		
	require_once(ABSPATH . 'wp-admin/includes/upgrade.php');
	dbDelta($sql);
};

add_action( 'init', 'sv_create_favorites_table');

... and the only way I could prevent it from creating post_id_2, 3, 4, etc was to add the "IF NOT EXISTS" clause to the CREATE TABLE statement.

Version 0, edited 14 months ago by RonStrilaeff (next)

comment:7 SergeyBiryukov14 months ago

  • Component changed from General to Database

comment:8 in reply to: ↑ 6 charlestonsw14 months ago

Replying to RonStrilaeff:

... and the only way I could prevent it from creating post_id_2, 3, 4, (and user_id_2, 3, 4), etc was to add the "IF NOT EXISTS" clause to the CREATE TABLE statement (as in my snippet above).

How does dbDelta handle the "IF NOT EXISTS" when the data structure changes, for example adding a column during a plugin update? Standard SQL would not add the column because the table exists.

That could be a bad side effect of the workaround.

comment:9 sebet13 months ago

I've ran into the duplicate key name problem when adding an index to a table. After some debugging on the dbDelta function I've found why this was happening. This criteria:

if (!(($aindex = array_search($index_string, $indices)) === false)) {

will only match an existing index if the column name includes the index length.

Example:

The comparison for this index will not match and will cause a duplicate index because the length was not specified

KEY movie_type_idx (movie_type),

The comparison for this index will match and the index will not be duplicated because the length was included

KEY movie_type_idx (movie_type(255)),

comment:10 follow-up: charlestonsw5 months ago

When using dbDelta you MUST use an uppercase KEY qualifier on indices. If you use lowercase the query string comparison will not match the database meta data which comes back with an uppercase "KEY". The loop that compares the index portion of the SQL statement is case sensitive.

The patch file above: 10404.patch_case_insensitive_key_matching has phpDoc updates, some comments, and most importantly turns the index comparison into a case insensitive operation.

The next issue I am investigating is the way the query string is built. In my specific instance I am using this to build a key:

create table x (
id mediumint(8) not null autoincrement,
KEY (id)
)

That will never match the index comparison check as the database meta data return info that dbDelta builds into the following string:

KEY id (id)

Even with the string insensitive search the resulting KEY (ID) does not match KEY ID (ID).

I can fix this by updating my create table command to match the meta data exactly by replaceing KEY (id) in my data definition with KEY id (id), but IMO dbDelta should be a little more forgiving. Otherwise there are a lot of plugins creating extra indices and thus server load on millions of WordPress installations out there. The "Too many keys specified; max 64 keys allowed" message is far too prevalent.

comment:11 in reply to: ↑ 10 SergeyBiryukov5 months ago

Replying to charlestonsw:

The patch file above: 10404.patch_case_insensitive_key_matching has phpDoc updates, some comments, and most importantly turns the index comparison into a case insensitive operation.

Looks like 10404.patch_case_insensitive_key_matching only contains four changes to the inline docs and one unrelated change in wp-admin/includes/post.php. Could you upload the correct patch?

charlestonsw5 months ago

Update case insensitive compare on dbDelta plus minor phpDoc updates.

comment:12 charlestonsw5 months ago

Revised patch added.

There are some phpDoc updates, more comments, and only a couple of lines of code changed to call strtoupper() in the appropriate places.

Still coding and testing a patch to help convert simple index creation statements like "KEY (id)" to the meta data generated "KEY id (id)" pattern to eliminate the mis-match on simple keys.

charlestonsw5 months ago

Case insensitive index AND simple index with NO NAME patch (see comments for details)

comment:13 charlestonsw5 months ago

A new patch file "10404.patch_caseinsensitive_and_noindexname_compare​" has been uploaded to help reduce the number of duplicate indices that are created by plugins that do not use the WordPress index creation methodology. Someone should probably make some notes around dbDelta in the WordPress docs as well as the syntax is rather specific. What the patch has:

  • A good bit more phpDoc info.
  • A LOT more comments around each code block.
  • Case insensitive comparison of index declaration blocks by shifting both the incoming index creation string and the metadata build strings to uppercase.
  • If the original version of the index comparison fails to find an EXACT match, it then falls back to check for "simple index keys". These are keys WITHOUT an index name defined, such as KEY (id) to index the id field.

The last part, which is well commented in the code, looks at the metadata-derived strings and converts the string that has been built as KEY <idxname> (<fldname>) and drops the optional index name portion if the index name EXACTLY MATCHES the field name portion.

In other words it converts the metadata-derived KEY <idxname> (<fldname>) to KEY (<fldname>) if both idxname and fldname are identical.

Since many plugins seem to like to use the shorthand KEY (<fldname>) syntax for building keys, this may help reduce the "too many keys" errors and improve performance during record insert/update operations on the plugin-provided tables.

This does not fix ALL possible dbDelta "too many keys" errors but it should help reduce the most common errors.

For developers the best method for using dbDelta with regard to key building is:

  • Always use an uppercase KEY directive.
  • Always include an index name in lowercase.
  • Always include your field list in lowercase.
  • Do not add field lengths to the keys.
  • When creating compound keys always eliminate whitespace. The means no spaces before or after parens or around commas.

In other words:
KEY mycompoundkey (id,slid)

Not:
key ( id , slid )

Personally I find the later (spaces around parens) hard to do after training myself to start ADDING spaces around parens to meet the WordPress coding standards which dictate logic operations to be ( a !== b ) versus (a!==b). :/

HTH some other code geeks out there.

Note: See TracTickets for help on using tickets.