Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#40357 new enhancement

dbDelta can't change primary keys, create AUTO_INCREMENT columns and doesn't make significant index changes

Reported by: stuporglue's profile stuporglue Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Database Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

dbDelta has three inter-related issues which center around changing indexes.

1) It isn't possible to change which column is the primary key
2) It isn't possible to add a new AUTO_INCREMENT column
2b) It isn't possible to modify an existing AUTO_INCREMENT to no longer be AUTO_INCREMENT
3) Indices with the same name are not dropped/re-created, even when the index definition is changed significantly.

Use case

A client had been tracking inventory in a custom table where the product ID was the primary key. When he opened a new location, we added a location column, and wanted to be able to track how many of each product was in each location.

  1. A table's purpose is being expanded, or otherwise doesn't meet the needs of the data.

Since the primary key is unique, we needed to add a new key column and change which column was designated as the primary key.

  1. A table was originally defined without an AUTO_INCREMENT column and the need for such a column arises.

The new column we wanted to add and use for the key was simply an AUTO_INCREMENT integer column. In testing we defined the new column and also defined a new UNIQUE index (so, not changing the primary key yet).

Since dbDelta adds new columns before adding the new indices, and in separate ALTER TABLE statements, MySQL refuses to add a new AUTO_INCREMENT column without an index.

The solution is to add the new column without the AUTO_INCREMENT designation, then add the UNIQUE index, then alter the table to use AUTO_INCREMENT.

  1. A primary (or other key) could be significantly and intentionally altered, significantly changing how queries are run.

I understand that WP doesn't want to drop and recreate indices when changing the sub-part of an index (see: https://core.trac.wordpress.org/ticket/34870#comment:21)

However I think that it should change the index, if the definition is significantly altered.

In the use case above, we could've changed the primary key to be PRIMARY KEY(productId,location) instead of adding a new column and switching the index to that column.

In other use cases, changing from a BTREE to FULLTEXT index would change which types of queries need to a full table scan.

This Patch

This patch does the following:

  1. You can now add a new AUTO_INCREMENT column to an existing table

When a new AUTO_INCREMENT column is added to an existing table, the column creation is done in two parts. First the column is created as a non-AUTO_INCREMENT column, and a separate ALTER TABLE statement is set to run after index creation to enable AUTO_INCREMENT.

Note: The CREATE TABLE statement given to dbDelta must provide the required indexes that MySQL expects.

  1. You can now modify a column with AUTO_INCREMENT to no longer use AUTO_INCREMENT
  1. You can change which column is the primary key
  1. Significant index definitions cause an index to be dropped and re-created

The cases that cause an index to be dropped and re-created are:

  • An index which wasn't UNIQUE, but now is or vice-versa
  • An index which changes index type (eg. FULLTEXT => BTREE)
  • An index which contains a different number of columns
  • An index which contains a different column order
  • An index which contains different columns

Note: Changing the index sub-part or no longer defining the index in the table does not cause it to be dropped.

Other notes

  1. I've tried to use WP coding standards and comment my code well. I'd love feedback if there are things I can do better.
  1. I've included a file, test.php which takes 13 table definitions, takes them two at a time, and converts between each possible combination.

To run it, put it in the root WordPress directory and run php ./test.php.

  1. Also, the dbDelta phpunit tests still pass.

Attachments (2)

upgrade.php.diff (9.2 KB) - added by stuporglue 7 years ago.
The patch that implements the changes outlined in this ticket.
test.php (4.7 KB) - added by stuporglue 7 years ago.
A file that tests the changes in this ticket.

Download all attachments as: .zip

Change History (3)

@stuporglue
7 years ago

The patch that implements the changes outlined in this ticket.

@stuporglue
7 years ago

A file that tests the changes in this ticket.

#1 @johnjamesjacoby
7 years ago

  • Keywords has-patch needs-testing needs-unit-tests added

Hey @stuporglue, great second ticket here!

cc @pento, who really loves working with dbDelta() ❤️

I've noticed similar issues with many different types of ALTERs, where dbDelta() has internal conditions to support but seem to get skipped over in the majority of cases, so I'll try this patch out also and report back my results.

If we get to a point where patches need massaging to make them ready for core (think of turning your tests.php to unit tests) I'll try to be a resource if you need one.

Note: See TracTickets for help on using tickets.