MDEV-35732 Failed ALTER TABLE causes inconsistency, changes behavior of the next statement

[LukeYL026]

Description

A failed ALTER TABLE ... RENAME INDEX ... ALGORITHM=INSTANT corrupted the
in-memory (cached) table metadata, which then changed the outcome of a
subsequent, unrelated ALTER TABLE ... ADD FOREIGN KEY. The failed statement
should have had no side effects.

Concretely:

CREATE TABLE t1 (f1 INT, f2 INT, KEY(f1), KEY(f2)) ENGINE=InnoDB;
CREATE TABLE t2 (pk INT PRIMARY KEY, a INT, b INT, KEY ind1(a),
                 FOREIGN KEY fk1 (b) REFERENCES t1 (f1)) ENGINE=InnoDB;

-- fails with ER_ALTER_OPERATION_NOT_SUPPORTED (ALGORITHM=INSTANT + ORDER BY)
ALTER TABLE t2 RENAME INDEX fk1 TO fk, ALGORITHM=INSTANT, ORDER BY a;

-- WRONGLY succeeded after the failed statement above;
-- on a fresh table it correctly fails with ER_DUP_KEYNAME ('ind1' exists)
ALTER TABLE t2 ADD FOREIGN KEY ind1 (b) REFERENCES t1 (f2);

Background — the three concepts you need

To follow the bug you need three pieces of vocabulary:

1. Index vs. column. A column (like b INT) is where data lives. An
   index is a separate lookup structure built over a column so the engine
   can find rows by that column's value quickly. Adding an index does not add a
   column.

2. Generated (auto-created) indexes. When you declare a FOREIGN KEY,
   InnoDB needs an index on the referencing column to enforce the constraint.
   If you did not also declare an explicit KEY for that column, the server
   synthesizes a supporting index for you and stamps it with the
   HA_GENERATED_KEY flag. The flag means "this index exists only to back a
   foreign key; the user did not declare it as a standalone index." It is set
   unconditionally for a FK's supporting index in the parser
   (sql/sql_yacc.yy, Key(Key::MULTIPLE, name, …, /*generated=*/true, …)),
   even when you give the FK a name — the name and the generated flag are
   independent. So in the repro, fk1 is a named but generated index.

3. The table cache. The server keeps an in-memory TABLE object (with its
   key_info array of index metadata) cached and reused across statements.
   Mutating it is mutating shared state that outlives the current statement.

Why the generated flag matters: during table preparation the server
de-duplicates indexes. If two indexes cover the same column(s), a generated
one is considered disposable ("I only made it for a constraint") and can be
silently dropped in favour of the other. That single flag is the tie-breaker
deciding which of two colliding indexes is discarded.

The example table therefore has three indexes:

index	column	generated?	why
PRIMARY	pk	no	user wrote PRIMARY KEY
ind1	a	no	user wrote KEY ind1(a)
fk1	b	yes	auto-created to back FOREIGN KEY fk1

What goes wrong, step by step

Statement 1 — the failed rename corrupts the cache

ALTER TABLE t2 RENAME INDEX fk1 TO fk, ALGORITHM=INSTANT, ORDER BY a;

1. mysql_prepare_alter_table() speculatively builds what the table's index
   list would look like if this ALTER succeeded. It loops over the cached
   TABLE's key_info.

2. It sees the RENAME INDEX fk1 TO fk request. The new name goes into a
   local variable (harmless). But it also decides "the user is renaming this
   index by hand, so stop treating it as a generated FK-support index" — a
   reasonable decision, because otherwise de-duplication might quietly drop the
   renamed index. The bug: it recorded that decision by clearing the flag
   directly on the shared cached object:

   key_info->flags &= ~HA_GENERATED_KEY;   // writes to the cached TABLE

3. mysql_prepare_alter_table() finishes and returns success — it built a
   valid list. (Note: the neighbouring long-hash edits in the same loop are
   deliberately undone via re_setup_keyinfo_hash(); this flag clear was not.)

4. Only later, back in mysql_alter_table(), does the server determine that
   ALGORITHM=INSTANT cannot perform the ORDER BY rebuild, so the statement
   fails with ER_ALTER_OPERATION_NOT_SUPPORTED.

5. The statement is rejected — no rename, no disk write, InnoDB never invoked —
   but the speculative flag clear already leaked into the cached TABLE and
   is never restored. The cached fk1 is now (wrongly) marked as a
   user-defined index. The name is still fk1 (the rename never happened);
   only the generated flag was lost.

Statement 2 — the corrupted flag flips the result

ALTER TABLE t2 ADD FOREIGN KEY ind1 (b) REFERENCES t1 (f2);

1. Parsing this creates a new FK constraint named ind1 plus its own generated
   supporting index — also named ind1, on column b.

2. The candidate index list now contains two indexes on column b: the old
   fk1(b) and the new ind1(b). De-duplication (is_foreign_key_prefix() in
   mysql_prepare_create_table()) fires for that same-column pair and must drop
   one. The tie-break looks at whether the older key (fk1) is generated:

   • Normal (fk1 generated): fk1(b) is dropped, the new ind1(b)
     survives.
   • Corrupted (fk1 not generated): the new ind1(b) is dropped
     (IGNORE_KEY), and fk1(b) survives.

3. Next, the index-naming loop checks each surviving index for a duplicate
   name. Indexes marked IGNORE_KEY are skipped before this check runs.

   • Normal: the surviving ind1(b) reaches the check; its name ind1
     collides with the user's existing ind1(a) → ER_DUP_KEYNAME. Correct.
   • Corrupted: the ind1(b) that carried the colliding name was already
     dropped in step 2, so the name check never sees it → no error, the ALTER
     silently succeeds.

So a single lost flag bit changes which same-column index survives
de-duplication, which in turn decides whether the name-collision check is even
reachable.

The end state

After the corrupted run, ADD FOREIGN KEY ind1 wrongly succeeds: the new
constraint reuses the existing fk1(b) index for support, and no new ind1
index is created. On a fresh table (or after a server restart, which rebuilds
the cache from disk) the identical statement correctly fails with
ER_DUP_KEYNAME. That is what makes this bug especially nasty:

• Silent — a wrong success, not a loud error.
• Non-reproducible — evicting/rebuilding the cached table makes it behave
  correctly, so it may not reproduce for whoever investigates.
• Order-dependent — a failed statement changed the outcome of a later
  one, when failed statements are supposed to be no-ops.

Root cause (TL;DR)

mysql_prepare_alter_table() cleared HA_GENERATED_KEY in place on the
cached TABLE's key_info while speculatively handling a RENAME INDEX, and
that mutation was never reverted when the statement later failed — leaving the
cached index metadata corrupted for subsequent statements.

Fix

Do not mutate the cached key_info->flags. Track the "renamed ⇒ no longer
generated" decision in a per-key local variable (generated_key):

• Initialize it from the current flag at the top of the loop iteration:
  bool generated_key = key_info->flags & HA_GENERATED_KEY; (a read, not a
  write).
• Flip it to false in the rename branch instead of clearing the cached
  flag.
• Consume it in the Key constructor in place of
  key_info->flags & HA_GENERATED_KEY.

Because the old code both cleared and later re-read the same
key_info->flags, routing both operations through the local reproduces the
exact in-statement result — a successful ALTER produces an identical index
list — while leaving the cached TABLE untouched. The speculative decision now
lives and dies on the stack, so a failed ALTER leaves no trace. No other code
in the loop reads the generated bit, and the local needs no cleanup (unlike the
long-hash path), so the change is limited to these three sites.

Files changed

• sql/sql_table.cc — replace the in-place HA_GENERATED_KEY flag mutation in
  mysql_prepare_alter_table() with a local generated_key variable.
• mysql-test/main/alter_table_failed_rename_index.test — new regression test.
• mysql-test/main/alter_table_failed_rename_index.result — generated with
  mysql-test-run.pl --record.

Release Notes

Resolved an issue where failed ALTER TABLE causes inconsistency, changes behavior of the next statement.

How can this PR be tested?

By running the following test case in MTR:
mysql-test/main/alter_table_failed_rename_index.test

Additionally, it is recommended to also test the following, as they are affected by this code path:

• mysql-test/main/alter_table.test
• mysql-test/suite/innodb/t/innodb-alter.test
• mysql-test/suite/innodb/t/instant_alter.test
• mysql-test/suite/innodb/t/foreign_key.test

Manually, this can also be tested by following Elena's replication instructions in MDEV-35732 Description:

--source include/have_innodb.inc
--source include/have_binlog_format_mixed.inc
--source include/master-slave.inc

CREATE TABLE t1 (f1 INT, f2 INT, KEY(f1), KEY(f2)) ENGINE=InnoDB;
CREATE TABLE t2 (pk INT PRIMARY KEY, a INT, b INT, KEY ind1(a), FOREIGN KEY fk1 (b) REFERENCES t1 (f1)) ENGINE=InnoDB;
--error ER_ALTER_OPERATION_NOT_SUPPORTED
ALTER TABLE t2 RENAME INDEX fk1 TO fk, ALGORITHM=INSTANT, ORDER BY a;
ALTER TABLE t2 ADD FOREIGN KEY ind1 (b) REFERENCES t1 (f2);

--sync_slave_with_master

--connection master
DROP TABLE t1, t2;
--source include/rpl_end.inc

Should now result in:

MariaDB [test]> ALTER TABLE t2 RENAME INDEX fk1 TO fk, ALGORITHM=INSTANT, ORDER BY a;
ERROR 1845 (0A000): ALGORITHM=INSTANT is not supported for this operation. Try ALGORITHM=COPY

MariaDB [test]> ALTER TABLE t2 ADD FOREIGN KEY ind1 (b) REFERENCES t1 (f2);
ERROR 1061 (42000): Duplicate key name 'ind1'

Basing the PR against the correct MariaDB version

This fix is based against the 11.4 branch.

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

[gkodinov]

This is a preliminary review. Thank you for your contribution!

LGTM. Please stand by for the final review.

[LukeYL026]

For sure - thanks again for taking the time to look at my submissions!