Chris, I'm not sure the rollback is needed. Comment below.
Christopher Powers wrote:
> #At file:///home/cpowers/work/dev/dev-04/mysql/
>
> 2756 Christopher Powers 2009-07-16
> Bug#45885 "Primary key added to Falcon table allosw non-unique values"
>
> Temporarily disabled online alter add primary key/unique index until
> Bug#46118 "Server raises fatal exception if engine returns error during ONLINE
> ALTER"
>
> Added unique value check and exception handling to online add index.
> modified:
> mysql-test/suite/falcon/r/falcon_bug_45775.result
> mysql-test/suite/falcon/r/falcon_online_index.result
> mysql-test/suite/falcon/t/disabled.def
> mysql-test/suite/falcon/t/falcon_online_index.test
> storage/falcon/Statement.cpp
> storage/falcon/Table.cpp
> storage/falcon/Table.h
> storage/falcon/ha_falcon.cpp
>
> per-file messages:
> mysql-test/suite/falcon/r/falcon_bug_45775.result
> Updated test results to conform with online alter changes
> mysql-test/suite/falcon/r/falcon_online_index.result
> Updated test result to conform with online alter changes
> mysql-test/suite/falcon/t/disabled.def
> Disabled testcase falcon_bug_32082:
> 1) Associated bug is a duplicate of Bug#34479
> 2) Test incorrectly passes due to bug in online add unique index
> mysql-test/suite/falcon/t/falcon_online_index.test
> Temporarily disabled online alter add unique/primary index tests
> storage/falcon/Statement.cpp
> Statement::createIndex()
> - Add exception handling for Table::populateIndex()
> storage/falcon/Table.cpp
> Table::populateIndex()
> - Added checkUnique option to force duplicate check
> storage/falcon/Table.h
> Added checkUnique boolean to Table::populateIndex()
> storage/falcon/ha_falcon.cpp
> StorageInterface::check_if_supported_alter()
> - Temporarily disabled online alter add primary/unique index
> === modified file 'mysql-test/suite/falcon/r/falcon_bug_45775.result'
> --- a/mysql-test/suite/falcon/r/falcon_bug_45775.result 2009-06-28 18:58:38 +0000
> +++ b/mysql-test/suite/falcon/r/falcon_bug_45775.result 2009-07-17 00:26:36 +0000
> @@ -22,7 +22,7 @@ t1 CREATE TABLE `t1` (
> ) ENGINE=Falcon DEFAULT CHARSET=latin1
> INSERT INTO t1 VALUES (1,1,1);
> INSERT INTO t1 VALUES (1,1,2);
> -ERROR 23000: Duplicate entry '1' for key 'a'
> +ERROR 23000: Duplicate entry '1' for key 'PRIMARY'
> INSERT INTO t1 VALUES (1,2,2);
> ERROR 23000: Duplicate entry '1' for key 'a'
> INSERT INTO t1 VALUES (2,1,2);
>
> === modified file 'mysql-test/suite/falcon/r/falcon_online_index.result'
> --- a/mysql-test/suite/falcon/r/falcon_online_index.result 2009-01-21 00:28:07 +0000
> +++ b/mysql-test/suite/falcon/r/falcon_online_index.result 2009-07-17 00:26:36 +0000
> @@ -133,12 +133,12 @@ t1 0 PRIMARY 1 a NULL 10 NULL NULL BTRE
> ALTER ONLINE TABLE t1 DROP PRIMARY KEY;
> SHOW INDEXES FROM t1;
>
> Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_Comment
> -ALTER ONLINE TABLE t1 ADD PRIMARY KEY (a);
> +ALTER TABLE t1 ADD PRIMARY KEY (a);
> SHOW INDEXES FROM t1;
>
> Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_Comment
> t1 0 PRIMARY 1 a NULL 10 NULL NULL BTREE
> #-------- Test: UNIQUE --------#
> -ALTER ONLINE TABLE t2 ADD UNIQUE INDEX ix_unique_c (c);
> +ALTER TABLE t2 ADD UNIQUE INDEX ix_unique_c (c);
> EXPLAIN SELECT * FROM t2 WHERE c < 25 AND c > 20 ORDER BY c;
> id select_type table type possible_keys key key_len ref rows Extra
> 1 SIMPLE t2 range ix_unique_c ix_unique_c 5 NULL 1 Using where; Using MRR; Using
> filesort
>
> === modified file 'mysql-test/suite/falcon/t/disabled.def'
> --- a/mysql-test/suite/falcon/t/disabled.def 2009-04-03 15:29:58 +0000
> +++ b/mysql-test/suite/falcon/t/disabled.def 2009-07-17 00:26:36 +0000
> @@ -10,3 +10,4 @@
> #
> ##############################################################################
>
> +falcon_bug_30282 : BUG#32082 2009-07-15 cpowers Duplicate of BUG#34479. Also, test
> fails due to fix for BUG#45885.
>
> === modified file 'mysql-test/suite/falcon/t/falcon_online_index.test'
> --- a/mysql-test/suite/falcon/t/falcon_online_index.test 2009-02-02 11:17:53 +0000
> +++ b/mysql-test/suite/falcon/t/falcon_online_index.test 2009-07-17 00:26:36 +0000
> @@ -197,7 +197,9 @@ SHOW INDEXES FROM t1;
> ALTER ONLINE TABLE t1 DROP PRIMARY KEY;
> SHOW INDEXES FROM t1;
> # Re-set primary key on 'a'
> -ALTER ONLINE TABLE t1 ADD PRIMARY KEY (a);
> +# Disabled: Bug#45885
> +# ALTER ONLINE TABLE t1 ADD PRIMARY KEY (a);
> +ALTER TABLE t1 ADD PRIMARY KEY (a);
> SHOW INDEXES FROM t1;
>
> ##
> @@ -207,7 +209,9 @@ SHOW INDEXES FROM t1;
> --echo #-------- Test: UNIQUE --------#
>
> ## Test adding UNIQUE index
> -ALTER ONLINE TABLE t2 ADD UNIQUE INDEX ix_unique_c (c);
> +# Disabled: Bug#45885
> +#ALTER ONLINE TABLE t2 ADD UNIQUE INDEX ix_unique_c (c);
> +ALTER TABLE t2 ADD UNIQUE INDEX ix_unique_c (c);
> EXPLAIN SELECT * FROM t2 WHERE c < 25 AND c > 20 ORDER BY c;
> SELECT * FROM t2 WHERE c < 25 AND c > 20 ORDER BY c;
> SHOW INDEXES FROM t2;
>
> === modified file 'storage/falcon/Statement.cpp'
> --- a/storage/falcon/Statement.cpp 2009-04-16 11:25:48 +0000
> +++ b/storage/falcon/Statement.cpp 2009-07-17 00:26:36 +0000
> @@ -334,23 +334,38 @@ void Statement::createIndex(Syntax * syn
>
> // If not adding system tables, create and populate the index
>
> - if (!database->formatting)
> - {
> - Transaction *sysTransaction = database->getSystemTransaction();
> - index->create(sysTransaction);
> - index->save();
> - database->commitSystemTransaction();
> + if (database->formatting)
> + return;
>
> + Transaction *sysTransaction = database->getSystemTransaction();
> + index->create(sysTransaction);
> + index->save();
> + database->commitSystemTransaction();
> +
> + try
> + {
> sysTransaction = database->getSystemTransaction();
> - table->populateIndex (index, sysTransaction);
> + table->populateIndex (index, sysTransaction, true);
> + database->commitSystemTransaction();
> + database->invalidateCompiledStatements (table);
> + }
> + catch (...)
> + {
> +
> + // Populate index failed, so rollback the transaction and delete the new index
> +
> + database->rollbackSystemTransaction();
Are you sure this is needed? There are no records attached to this transaction so there
is nothing to rollback. But since there might be other changes by other threads added to
this system trans, "in theory", maybe you should not do the rollback. The
table->deleteIndex() should get rid of all the DeferredIndexes involved.
Is this rollback needed for the sake of the serial log? Will the following committed
deleteIndex do the same thing to assure that the serial log will end up without any
orphans?
> +
> + sysTransaction = database->getSystemTransaction();
> + table->deleteIndex(index, sysTransaction);
> + Index::deleteIndex(database, table->schemaName, name);
> database->commitSystemTransaction();
>
> database->invalidateCompiledStatements (table);
> - //database->flush();
> + throw;
> }
> }
>
> -
> void Statement::allocParameters(int count)
> {
> parameters.alloc (count);
>
> === modified file 'storage/falcon/Table.cpp'
> --- a/storage/falcon/Table.cpp 2009-07-14 14:09:58 +0000
> +++ b/storage/falcon/Table.cpp 2009-07-17 00:26:36 +0000
> @@ -1888,7 +1888,7 @@ const char* Table::getSchema()
> return schemaName;
> }
>
> -void Table::populateIndex(Index * index, Transaction *transaction)
> +void Table::populateIndex(Index * index, Transaction *transaction, bool
> checkUnique)
> {
> Record *record;
> CycleLock cycleLock(database);
> @@ -1899,13 +1899,16 @@ void Table::populateIndex(Index * index,
>
> for (Record *version = record; version; version = version->getPriorVersion())
> if (version->hasRecord())
> - index->insert(version, transaction);
> + if (checkUnique)
> + insertIndexes(transaction, (RecordVersion*)version);
> + else
> + index->insert(version, transaction);
>
> record->release(REC_HISTORY);
>
> #ifdef _DEBUG
> - if (count && count % 100000 == 0)
> - Log::debug("populateIndex: %d records indexed\n", count);
> + if (count && count % 100000 == 0)
> + Log::debug("populateIndex: %d records indexed\n", count);
> #endif
> }
>
>
> === modified file 'storage/falcon/Table.h'
> --- a/storage/falcon/Table.h 2009-06-25 22:48:40 +0000
> +++ b/storage/falcon/Table.h 2009-07-17 00:26:36 +0000
> @@ -150,7 +150,7 @@ public:
> void setView (View *view);
> Index* findIndex (const char *indexName);
> virtual PrivObject getPrivilegeType();
> - void populateIndex (Index *index, Transaction *transaction);
> + void populateIndex (Index *index, Transaction *transaction, bool checkUnique =
> false);
> const char* getSchema();
> ForeignKey* dropForeignKey (ForeignKey *key);
> void dropField (Field *field);
>
> === modified file 'storage/falcon/ha_falcon.cpp'
> --- a/storage/falcon/ha_falcon.cpp 2009-06-28 18:58:38 +0000
> +++ b/storage/falcon/ha_falcon.cpp 2009-07-17 00:26:36 +0000
> @@ -2494,10 +2494,14 @@ int StorageInterface::check_if_supported
> DBUG_ENTER("StorageInterface::check_if_supported_alter");
> tempTable = (create_info->options & HA_LEX_CREATE_TMP_TABLE) ? true :
> false;
> HA_ALTER_FLAGS supported;
> - supported = supported | HA_ADD_INDEX | HA_DROP_INDEX | HA_ADD_UNIQUE_INDEX |
> HA_DROP_UNIQUE_INDEX
> - | HA_ADD_PK_INDEX | HA_DROP_PK_INDEX | HA_ADD_COLUMN;
> - HA_ALTER_FLAGS notSupported = ~(supported);
> +
> + // Add unique index is disabled until a duplicate key error can be returned to the
> server
> + // without triggering a fatal exception
>
> + supported = supported | HA_ADD_INDEX | HA_DROP_INDEX | /*HA_ADD_UNIQUE_INDEX |*/
> HA_DROP_UNIQUE_INDEX
> + | /*HA_ADD_PK_INDEX |*/ HA_DROP_PK_INDEX | HA_ADD_COLUMN;
> + HA_ALTER_FLAGS notSupported = ~(supported);
> +
> #ifndef ONLINE_ALTER
> DBUG_RETURN(HA_ALTER_NOT_SUPPORTED);
> #endif
> @@ -2535,7 +2539,7 @@ int StorageInterface::check_if_supported
> int StorageInterface::alter_table_phase1(THD* thd, TABLE* altered_table,
> HA_CREATE_INFO* create_info, HA_ALTER_INFO* alter_info, HA_ALTER_FLAGS* alter_flags)
> {
> DBUG_ENTER("StorageInterface::alter_table_phase1");
> -
> +
> DBUG_RETURN(0);
> }
>
>
>
