Marko Mäkelä added a comment - 2021-07-23 10:48 Before MDEV-15563 in 10.4.3, an AUTO_INCREMENT attribute could only be removed by ALGORITHM=COPY : --source include/have_innodb.inc CREATE TABLE t (c INT AUTO_INCREMENT NULL UNIQUE ) ENGINE=InnoDB; ALTER TABLE t MODIFY c INT NOT NULL , ALGORITHM=INPLACE; DROP TABLE t; 10.2 742b3a0d39875584f994f4bd01d57cc31d1dddca mysqltest: At line 3: query 'ALTER TABLE t MODIFY c INT NOT NULL, ALGORITHM=INPLACE' failed: 1846: ALGORITHM=INPLACE is not supported. Reason: Cannot change column type INPLACE. Try ALGORITHM=COPY

Marko Mäkelä added a comment - 2021-07-23 11:29 The following test shows a problem with FORCE (table rebuild) as well. We would expect the fields to contain 0, but the Perl code is reading the value 1 from both files. --source include/have_innodb.inc CREATE TABLE t1 (c INT AUTO_INCREMENT NULL UNIQUE ) ENGINE=InnoDB; ALTER TABLE t1 MODIFY c INT NULL , ALGORITHM=INSTANT; INSERT INTO t1 SET c=1; CREATE TABLE t2 (c INT AUTO_INCREMENT NULL UNIQUE ) ENGINE=InnoDB; ALTER TABLE t2 MODIFY c INT NULL , FORCE , ALGORITHM=INPLACE; INSERT INTO t2 SET c=1; FLUSH TABLES t1,t2 FOR EXPORT; --let INNODB_PAGE_SIZE=`select @@innodb_page_size` --let MYSQLD_DATADIR = `SELECT @@datadir` --perl my $ps= $ENV{INNODB_PAGE_SIZE}; my $file= "$ENV{MYSQLD_DATADIR}/test/t1.ibd" ;   open (FILE, "<" , $file) || die "Unable to open $file\n" ; sysseek(FILE, 3*$ps+38+18+4, 0) || die "Unable to seek $file\n" ; die "Unable to read $file\n" unless sysread(FILE, $_, 4) == 4; my $t1=unpack( "N" ,$_); close (FILE);   $file= "$ENV{MYSQLD_DATADIR}/test/t2.ibd" ; open (FILE, "<" , $file) || die "Unable to open $file\n" ; sysseek(FILE, 3*$ps+38+18+4, 0) || die "Unable to seek $file\n" ; die "Unable to read $file\n" unless sysread(FILE, $t_, 4) == 4; my $t2=unpack( "N" ,$_); close (FILE); print "AUTO_INCREMENT: $t1,$t2\n" ; EOF UNLOCK TABLES;   DROP TABLE t1,t2;

Marko Mäkelä added a comment - 2021-07-23 12:02 - edited There exist assertions in dict_table_t::instant_column() and elsewhere that assume that the AUTO_INCREMENT attribute will not change, it is not trivial to fix this, and the fix will have to be tested thoroughly with RQG against assertion failures. To fix my test case, we should change commit_cache_norebuild() and commit_try_rebuild() and probably introduce a flag for changing (removing) an AUTO_INCREMENT attribute.

Marko Mäkelä added a comment - 2021-07-23 12:33 It turns out that this bug is actually caused by MDEV-12836 , which specifically allows the removal of an AUTO_INCREMENT attribute.

Vladislav Lesin added a comment - 2022-04-11 17:00 The above test contains error in perl code. PAGE_ROOT_AUTO_INC fields is read int $t_ instead of $_ for t2 table, but $_ is unpacked into $t2, so $t2 shows t1's PAGE_ROOT_AUTO_INC. If we fix it, we will see that INPLACE algorithm is not affected. For INPLACE algorithm new index metadata is created in prepare_inplace_alter_table_dict(), so there is no need to reset table->persistent_autoinc: prepare_inplace_alter_table_dict(...) { ... for (ulint a = 0 ; a < ctx->num_to_add_index; a++) { dict_index_t* index = ctx->add_index[a]; ... index = create_index_dict(ctx->trx, index, add_v); ... } ... } For INSTANT algorithm ha_innobase::prepare_inplace_alter_table() even don't call prepare_inplace_alter_table_dict(), because !(ha_alter_info->handler_flags & ~INNOBASE_INPLACE_IGNORE) condition is true. And this is also the reason why we can't call commit_cache_norebuild() in ha_innobase::commit_inplace_alter_table(). To call commit_cache_norebuild() we need ha_alter_info->handler_ctx, which, in turns, is created in ha_innobase::prepare_inplace_alter_table if ha_alter_info->handler_flags & ~INNOBASE_INPLACE_IGNORE: ha_innobase::prepare_inplace_alter_table(...) { ... if (!(ha_alter_info->handler_flags & ~INNOBASE_INPLACE_IGNORE)) { /* Nothing to do */ ... DBUG_RETURN( false ); } ... if (!(ha_alter_info->handler_flags & INNOBASE_ALTER_DATA) || ((ha_alter_info->handler_flags & ~(INNOBASE_INPLACE_IGNORE | INNOBASE_ALTER_NOCREATE | INNOBASE_ALTER_INSTANT)) == ALTER_OPTIONS && !alter_options_need_rebuild(ha_alter_info, table))) { ha_innobase_inplace_ctx *ctx = NULL; if (heap) { ctx = new ha_innobase_inplace_ctx(...); ha_alter_info->handler_ctx = ctx; } ... DBUG_RETURN( false ); } ... ha_alter_info->handler_ctx = new ha_innobase_inplace_ctx(...); DBUG_RETURN(prepare_inplace_alter_table_dict(...)); } INNOBASE_INPLACE_IGNORE also contains ALTER_COLUMN_DEFAULT, which is set for "ALTER TABLE t1 MODIFY c INT NULL" statement: static const alter_table_operations INNOBASE_INPLACE_IGNORE = ALTER_COLUMN_DEFAULT | ALTER_PARTITIONED | ALTER_COLUMN_COLUMN_FORMAT | ALTER_COLUMN_STORAGE_TYPE | ALTER_CONVERT_TO | ALTER_VIRTUAL_GCOL_EXPR | ALTER_DROP_CHECK_CONSTRAINT | ALTER_RENAME | ALTER_COLUMN_INDEX_LENGTH | ALTER_CHANGE_INDEX_COMMENT; Marko explained why ALTER_COLUMN_DEFAULT flag is ignored: "Normally, changing the default values is not something that InnoDB cares about. Default values are handled by the SQL layer, not InnoDB. The only exception is instant ADD COLUMN where we must know the default value of the instantly added column, and that value must be constant for all rows during the ALTER TABLE execution." So to call commit_cache_norebuild() in ha_innobase::commit_inplace_alter_table() we need to change the logic ha_innobase::prepare_inplace_alter_table(). At the other hand, why do we need to call ommit_cache_norebuild()? Why just don't reset table->persistent_autoinc in ha_innobase::commit_inplace_alter_table() directly: --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ - 11036 , 6 + 11036 , 10 @@ ha_innobase::commit_inplace_alter_table( DBUG_ASSERT(!ctx0); MONITOR_ATOMIC_DEC(MONITOR_PENDING_ALTER_TABLE); ha_alter_info->group_commit_ctx = NULL; + if (table->found_next_number_field + && !altered_table->found_next_number_field) { + m_prebuilt->table->persistent_autoinc = 0 ; + } DBUG_RETURN( false ); } persistent_autoinc is set if TABLE::found_next_number_field is set (during table open) in SQL layer. So persistent_autoinc reset should be crash-safe. And it's not supposed to be rolled back if ha_innobase::commit_inplace_alter_table() is finished successfully.

Marko Mäkelä added a comment - 2022-04-20 07:19 I think that it could be cleaner if the assignment to reset ha_alter_info->group_commit_ctx=NULL was not skipped in ha_innobase::commit_inplace_alter_table() , but the metadata for all partitions were updated. The proposed solution looks technically correct, but then the comment before the relaxed assertion in ha_partition::commit_inplace_alter_table() will have to be adjusted.

Vladislav Lesin added a comment - 2022-04-20 08:21 - edited marko , below is edited version of my message about partitions iteration for INNOBASE_INPLACE_IGNORE flags in ha_innobase::commit_inplace_alter_table() from our slack discussion. Why we can't do the same thing for INNOBASE_INPLACE_IGNORE alter table flags in ha_innobase::commit_inplace_alter_table() as for commit_cache_norebuild(), i.e. iterate partitions with ha_alter_info->group_commit_ctx and don't change the code in ha_partition? The question is the same as for none-partitioned tables. The answer is the same as here . ha_alter_info->group_commit_ctx is analogue of ha_alter_info->handler_ctx for partitioned tables. ha_alter_info->handler_ctx is filled in ha_innobase::prepare_inplace_alter_table(), and partition engine invokes ha_innobase::prepare_inplace_alter_table() for each partition and pushes created ha_alter_info->handler_ctx into ha_alter_info->group_commit_ctx array. ha_innobase::prepare_inplace_alter_table() don't create ha_alter_info->handler_ctx for INNOBASE_INPLACE_IGNORE flags(the details are here ), so ha_alter_info->group_commit_ctx array is filled with NULL's for INNOBASE_INPLACE_IGNORE. To change metadata for all partitions for INNOBASE_INPLACE_IGNORE flags in ha_innobase::commit_inplace_alter_table() we need to change the logic in ha_innobase::prepare_inplace_alter_table() for those flags. I would not do this for such small change as resetting persistent_autoinc.

Marko Mäkelä added a comment - 2022-04-20 09:22 vlad.lesin , I agree that your proposed solution is technically correct, and possibly also the simplest solution. The reason why changes to partitioned tables are committed in a single call is durability. This change does not affect durability, because it is solely about updating the InnoDB data dictionary caches (one InnoDB dict_table_t per partition or sub-partition). I would only request you to revise the comment in ha_partition::commit_inplace_alter_table() to match the revised design.