Skip to content

fix(MySQL): combine the drop and the creation of an index over the same columns - #7475

Merged
morozov merged 4 commits into
doctrine:4.4.xfrom
fballiano:fix/mysql-index-fold-quoted-columns
Aug 10, 2026
Merged

fix(MySQL): combine the drop and the creation of an index over the same columns#7475
morozov merged 4 commits into
doctrine:4.4.xfrom
fballiano:fix/mysql-index-fold-quoted-columns

Conversation

@fballiano

@fballiano fballiano commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
Q A
Type bug
Fixed issues n/a

Summary

AbstractMySQLPlatform combines the drop of an index and the creation of another one over the same columns into a single ALTER TABLE ... DROP INDEX a, ADD INDEX b (...) statement, so that the columns are never left unindexed in between. The two column lists are compared through the legacy Index::getColumns() accessor, which returns the raw map keys: the introspection API marks them as quoted ('"parent_id"'), while an index built in memory keeps them bare ('parent_id'). They therefore never match when the dropped index comes from an introspected table, the statements are not combined, and the drop is emitted on its own.

That makes alterTable() fail whenever the dropped index is the one backing a foreign key. InnoDB rejects the drop even though the added index would provide the same cover, and disabling foreign_key_checks does not lift the restriction:

$conn->executeStatement('CREATE TABLE parent (id INT NOT NULL, PRIMARY KEY (id)) ENGINE = InnoDB');
$conn->executeStatement(<<<'SQL'
    CREATE TABLE child (
        id INT NOT NULL,
        parent_id INT NOT NULL,
        val INT NOT NULL,
        PRIMARY KEY (id),
        UNIQUE KEY uniq_parent_val (parent_id, val),
        CONSTRAINT fk_child_parent FOREIGN KEY (parent_id) REFERENCES parent (id)
    ) ENGINE = InnoDB
SQL);

$sm  = $conn->createSchemaManager();
$old = $sm->introspectTableByUnquotedName('child');

// Redeclare the unique key as a plain index over the same columns.
$new = $old->edit()
    ->setIndexes(
        Index::editor()
            ->setUnquotedName('idx_parent_val')
            ->setUnquotedColumnNames('parent_id', 'val')
            ->create(),
    )
    ->create();

$sm->alterTable($sm->createComparator()->compareTables($old, $new));
// SQLSTATE[HY000]: General error: 1553 Cannot drop index 'uniq_parent_val':
// needed in a foreign key constraint

Emitted before the fix, where the drop runs on its own and never reaches the creation:

DROP INDEX `uniq_parent_val` ON child;
CREATE INDEX idx_parent_val ON child (parent_id, val);

and after:

ALTER TABLE child DROP INDEX uniq_parent_val, ADD INDEX idx_parent_val (parent_id, val);

Fix

Read the column names through the non-deprecated Index::getIndexedColumns() accessor and compare their unquoted identifier values, so the comparison is agnostic of whether the index was introspected or built in memory. This is the same treatment #7392 applied to the same accessor in the SQLite table-recreation path.

The regression test goes to MySQLSchemaManagerTest: the combining is MySQL-specific, and so is the foreign-key index requirement that makes the uncombined form fail. It errors with 1553 on 4.4.x and passes with the fix.

…me columns

When an index is dropped and another one over the same columns is added,
AbstractMySQLPlatform combines both into a single ALTER TABLE statement,
so that the columns are never left unindexed. It compares the two column
lists through the legacy Index::getColumns() accessor, which returns the
raw map keys: the introspection API marks them as quoted ('"parent_id"'),
while an index built in memory keeps them bare ('parent_id'). The two
lists therefore never match when the dropped index comes from an
introspected table, the statements are not combined, and the drop is
emitted on its own.

That makes the alteration fail whenever the dropped index is the one
backing a foreign key: InnoDB rejects the drop with "Cannot drop index
'X': needed in a foreign key constraint" (which disabling
foreign_key_checks does not lift), even though the added index would
provide the same cover.

Read the column names through the non-deprecated
Index::getIndexedColumns() accessor and compare their unquoted identifier
values, so the comparison is agnostic of whether the index was
introspected or built in memory, as done in doctrine#7392 for the same accessor
in the SQLite table-recreation path.

Signed-off-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>

@morozov morozov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@fballiano thank you for the patch. Please see inline comments.

Comment thread src/Platforms/AbstractMySQLPlatform.php Outdated
Comment thread src/Platforms/AbstractMySQLPlatform.php Outdated
Comment thread tests/Functional/Schema/MySQLSchemaManagerTest.php Outdated
…make the test platform-agnostic

The dropped and added index columns are now compared pairwise through
UnqualifiedName::equals() with the platform's unquoted-identifier
folding, instead of comparing extracted values.

The test moves to SchemaManagerTest and builds the tables through the
editor API, with no platform-specific SQL. The unique index spans
exactly the referencing column of the foreign key, so that no implicit
index is created and the unique index remains the only cover of the
constraint: with an extra covering index, InnoDB would allow the
uncombined drop and the test would pass even without the fix.
Verified failing with error 1553 on MySQL without the fix.

Signed-off-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano

Copy link
Copy Markdown
Contributor Author

Hi @morozov, thank you for the review and sorry for the mistakes, I've rewritten the whole thing with your suggestions, let's see if CI agrees.

🙏

Comment thread src/Platforms/AbstractMySQLPlatform.php Outdated
Index::getIndexedColumns() throws InvalidState on an index without
valid columns, which would be a behavior break in 4.x. Compare the
lists returned by Index::getUnquotedColumns() instead, as done in
the SQLite table-recreation path.

Signed-off-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Signed-off-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano

Copy link
Copy Markdown
Contributor Author

it was so easy 🤦🏻 sorry 😅

@morozov
morozov merged commit 0115030 into doctrine:4.4.x Aug 10, 2026
130 checks passed
@morozov

morozov commented Aug 10, 2026

Copy link
Copy Markdown
Member

Merged. Thanks, @fballiano!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants