MySQL Bugs: #25250: alter table allows simultaneous drop/add foreign keys

Bug #25250	alter table allows simultaneous drop/add foreign keys
Submitted:	22 Dec 2006 15:47	Modified:	9 Jun 2009 12:25
Reporter:	Scott Noyes (Basic Quality Contributor)	Email Updates:
Status:	Won't fix	Impact on me:	None
Category:	MySQL Server: Parser	Severity:	S3 (Non-critical)
Version:	5.0.27/4.1/5.1	OS:	Any
Assigned to:	Konstantin Osipov	CPU Architecture:	Any
Tags:	qc

Description:
http://dev.mysql.com/doc/refman/5.0/en/innodb-foreign-key-constraints.html says, "You cannot add a foreign key and drop a foreign key in separate clauses of a single ALTER TABLE statement. Separate statements are required."

In practice, ALTER TABLE allows add and drop foreign key (in either order) in one statement.

How to repeat:
CREATE TABLE t1 (t1Id int, index(t1Id)) ENGINE=InnoDB;
CREATE TABLE t2 (t2Id int, index(t2Id), FOREIGN KEY (t2Id) REFERENCES t1 (t1Id)) ENGINE=InnoDB;

SHOW CREATE TABLE t2;
/* Result:
CREATE TABLE `t2` (
  `t2Id` int(11) default NULL,
  KEY `t2Id` (`t2Id`),
  CONSTRAINT `t2_ibfk_1` FOREIGN KEY (`t2Id`) REFERENCES `t1` (`t1Id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
*/

ALTER TABLE t2 DROP FOREIGN KEY t2_ibfk_1, ADD FOREIGN KEY (t2Id) REFERENCES t1 (t1Id);
SHOW CREATE TABLE t2;
/* Result:
CREATE TABLE `t2` (
  `t2Id` int(11) default NULL,
  KEY `t2Id` (`t2Id`),
  CONSTRAINT `t2_ibfk_2` FOREIGN KEY (`t2Id`) REFERENCES `t1` (`t1Id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
*/

ALTER TABLE t2 ADD FOREIGN KEY (t2Id) REFERENCES t1 (t1Id), DROP FOREIGN KEY t2_ibfk_2;
SHOW CREATE TABLE t2;
/* Result:
CREATE TABLE `t2` (
  `t2Id` int(11) default NULL,
  KEY `t2Id` (`t2Id`),
  CONSTRAINT `t2_ibfk_3` FOREIGN KEY (`t2Id`) REFERENCES `t1` (`t1Id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1
*/

SELECT VERSION();
/* Result:
+---------------------+
| VERSION()           |
+---------------------+
| 5.0.27-community-nt |
+---------------------+
*/

Suggested fix:
Adjust the documentation to remove the erroneous line, or explain under what circumstances such ALTER TABLE statements fail.

Thank you for the bug report.

Per Heikki:

It is still an 'undocumented feature'. It might work in some cases, but it has not been tested.

Therefore, the manual is correct. Reliance on undocumented features is done at your own
risk.

Server team: please disable the ability to use ALTER TABLE in this undocumented manner.

Server team:  here is additional background:

[Paul]
The bug report notes that you can add and drop a foreign index with
a single ALTER TABLE statement, contrary to what the manual says.

Per Heikki's comment, I *could* revise the manual to say merely that
using them both in a single statement is unsupported (rather than that
you *must* use separate statements).

But I am wondering if instead perhaps it should be considered a bug
that it _might_ work.  If so, this turns into something other than
a docs bug.

[Trudy]
My preference is that we consider it a bug
that ALTER TABLE does not stop one from
adding and dropping a foreign key with the
same statement. We should never allow
undocumented features that may or may not
work; that's just asking for trouble (and
the type of bug reported as #25250).

My suggestion: Add a comment to the bug report
that states Heikki's opinion/response and say
"the Reference Manual is correct; undocumented
features are used at your own risk." Then say
"Server team: please disable the ability to use
ALTER TABLE in this undocumented manner" and
set the bug back to "verified" in the "Server"
category (at least I assume this isn't really
an InnoDB bug, but our own bug in ALTER TABLE).

I consider this a low priority item, so the
bug priority can remain P3, IMO.

A patch from a prospect
--- a/sql_yacc.yy       2008-02-12 13:26:10.000000000 +0300
+++ b/sql_yacc.yy       2008-02-22 16:14:51.000000000 +0300
@@ -5869,6 +5869,13 @@
           add_column column_def opt_place { }
         | ADD key_def
           {
+            /* #25250 */
+            if ( (Lex->alter_info.flags & ALTER_DROP_INDEX)
+               &&(Lex->alter_info.flags & ALTER_FOREIGN_KEY))
+            {
+              my_parse_error(ER(ER_SYNTAX_ERROR));
+              MYSQL_YYABORT;
+            }
             Lex->alter_info.flags|= ALTER_ADD_INDEX;
           }
         | add_column '(' field_list ')'
@@ -5916,6 +5923,14 @@
           }
         | DROP FOREIGN KEY_SYM opt_ident
           {
+            /* #25250 */
+            if ( (Lex->alter_info.flags & ALTER_ADD_INDEX) 
+              && (Lex->alter_info.flags & ALTER_FOREIGN_KEY)) 
+            {
+              my_parse_error(ER(ER_SYNTAX_ERROR));
+              MYSQL_YYABORT;
+            }
+            
             Lex->alter_info.flags|= ALTER_DROP_INDEX | ALTER_FOREIGN_KEY;
           }
         | DROP PRIMARY_SYM KEY_SYM

No CLA is signed, so please do not use the patch.

In 6.1 foreign keys we plan to allow it.
It is the only way to modify the column definition for a column that participates in a foreign key constraint - you can't drop a referenced or referencing column via an ALTER, neither you can modify it.

So the only way to modify a foreign key in a single ALTER is to allow
ALTER TABLE <table> DROP FOREIGN KEY, CHANGE COLUMN, ADD FOREIGN KEY

This syntax is a good candidate for ONLINE ALTER as well. 
Provided it "worked", even though it wasn't documented, all along, and that we plan to make it fully supported in 6.1, I don't see a point in fixing it only to allow again a few releases down the road. 

I don't encourage documenting it either, since Heikki indicated that this is an "unsupported", "use at your own risk" feature.