MDEV-22867 Assertion instant.n_core_fields == n_core_fields failed

This is a race condition where a table on which a 10.3-style
instant ADD COLUMN is emptied during the execution of
ALTER TABLE ... DROP COLUMN ..., DROP INDEX ..., ALGORITHM=NOCOPY.

In commit 2c4844c9e76427525e8c39a2d72686085efe89c3 the
function instant_metadata_lock() would prevent this race condition.
But, it would also hold a page latch on the leftmost leaf page of
clustered index for the duration of a possible DROP INDEX operation.

The race could be fixed by restoring the function
instant_metadata_lock() that was removed in
commit ea37b144094a0c2ebfc6774047fd473c1b2a8658
but it would be more future-proof to prevent the
dict_index_t::clear_instant_add() call from being issued at all.

We at some point support DROP COLUMN ..., ADD INDEX ..., ALGORITHM=NOCOPY
and that would spend a non-trivial amount of
execution time in ha_innobase::inplace_alter(),
making a server hang possible. Currently this is not supported
and our added test case will notice when the support is introduced.

dict_index_t::must_avoid_clear_instant_add(): Determine if
a call to clear_instant_add() must be avoided.

btr_discard_only_page_on_level(): Preserve the metadata record
if must_avoid_clear_instant_add() holds.

btr_cur_optimistic_delete_func(), btr_cur_pessimistic_delete():
Do not remove the metadata record even if the table becomes empty
but must_avoid_clear_instant_add() holds.

btr_pcur_store_position(): Relax a debug assertion.

This is joint work with Thirunarayanan Balathandayuthapani.

6 files changed, 101 insertions, 4 deletions

diff --git a/mysql-test/suite/innodb/r/instant_alter_debug.result b/mysql-test/suite/innodb/r/instant_alter_debug.result
index 9fcb8b05a34..50b69801043 100644
--- a/mysql-test/suite/innodb/r/instant_alter_debug.result
+++ b/mysql-test/suite/innodb/r/instant_alter_debug.result
@@ -370,3 +370,36 @@ b
 SET DEBUG_SYNC='RESET';
 disconnect con2;
 DROP TABLE t1;
+#
+# MDEV-22867 Assertion instant.n_core_fields == n_core_fields
+# in dict_index_t::instant_add_field
+#
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT UNIQUE) ENGINE=InnoDB;
+INSERT INTO t1 SET a=1;
+connect  prevent_purge,localhost,root,,;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+connection default;
+ALTER TABLE t1 ADD COLUMN c INT;
+DELETE FROM t1;
+connect  con2,localhost,root,,test;
+ALTER TABLE t1 DROP b, ADD INDEX(c), ALGORITHM=NOCOPY;
+ERROR 0A000: ALGORITHM=NOCOPY is not supported for this operation. Try ALGORITHM=INPLACE
+SET DEBUG_SYNC='innodb_inplace_alter_table_enter SIGNAL ddl WAIT_FOR emptied';
+ALTER TABLE t1 DROP b;
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR ddl';
+BEGIN;
+INSERT INTO t1 SET a=1;
+connection prevent_purge;
+COMMIT;
+disconnect prevent_purge;
+connection default;
+ROLLBACK;
+SELECT * FROM t1;
+a	b	c
+SET DEBUG_SYNC='now SIGNAL emptied';
+connection con2;
+disconnect con2;
+connection default;
+DROP TABLE t1;
+SET DEBUG_SYNC=RESET;
diff --git a/mysql-test/suite/innodb/t/instant_alter_debug.test b/mysql-test/suite/innodb/t/instant_alter_debug.test
index fe80de2ca51..b71e52a5f80 100644
--- a/mysql-test/suite/innodb/t/instant_alter_debug.test
+++ b/mysql-test/suite/innodb/t/instant_alter_debug.test
@@ -414,3 +414,46 @@ SELECT * FROM t1;
 SET DEBUG_SYNC='RESET';
 --disconnect con2
 DROP TABLE t1;
+
+--echo #
+--echo # MDEV-22867 Assertion instant.n_core_fields == n_core_fields
+--echo # in dict_index_t::instant_add_field
+--echo #
+
+CREATE TABLE t1 (a INT PRIMARY KEY, b INT UNIQUE) ENGINE=InnoDB;
+INSERT INTO t1 SET a=1;
+
+connect (prevent_purge,localhost,root,,);
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+connection default;
+ALTER TABLE t1 ADD COLUMN c INT;
+DELETE FROM t1;
+
+connect (con2,localhost,root,,test);
+# FIXME: If this is implemented then also i->online_log must be checked in
+# dict_index_t::must_avoid_clear_instant_add(). See the comment there.
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+ALTER TABLE t1 DROP b, ADD INDEX(c), ALGORITHM=NOCOPY;
+SET DEBUG_SYNC='innodb_inplace_alter_table_enter SIGNAL ddl WAIT_FOR emptied';
+send ALTER TABLE t1 DROP b;
+
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR ddl';
+BEGIN;
+INSERT INTO t1 SET a=1;
+
+connection prevent_purge;
+COMMIT;
+disconnect prevent_purge;
+
+connection default;
+ROLLBACK;
+SELECT * FROM t1;
+SET DEBUG_SYNC='now SIGNAL emptied';
+
+connection con2;
+reap;
+disconnect con2;
+connection default;
+DROP TABLE t1;
+SET DEBUG_SYNC=RESET;
diff --git a/storage/innobase/btr/btr0btr.cc b/storage/innobase/btr/btr0btr.cc
index 03eb1e076e4..d9c99f59132 100644
--- a/storage/innobase/btr/btr0btr.cc
+++ b/storage/innobase/btr/btr0btr.cc
@@ -3944,11 +3944,14 @@ btr_discard_only_page_on_level(
 	mem_heap_t* heap = NULL;
 	const rec_t* rec = NULL;
 	rec_offs* offsets = NULL;
-	if (index->table->instant) {
+
+	if (index->table->instant || index->must_avoid_clear_instant_add()) {
 		const rec_t* r = page_rec_get_next(page_get_infimum_rec(
 							   block->frame));
 		ut_ad(rec_is_metadata(r, *index) == index->is_instant());
-		if (rec_is_alter_metadata(r, *index)) {
+		if (!rec_is_metadata(r, *index)) {
+		} else if (!index->table->instant
+			   || rec_is_alter_metadata(r, *index)) {
 			heap = mem_heap_create(srv_page_size);
 			offsets = rec_get_offsets(r, index, NULL, true,
 						  ULINT_UNDEFINED, &heap);
diff --git a/storage/innobase/btr/btr0cur.cc b/storage/innobase/btr/btr0cur.cc
index f036f677712..4fbd1d653ac 100644
--- a/storage/innobase/btr/btr0cur.cc
+++ b/storage/innobase/btr/btr0cur.cc
@@ -5450,7 +5450,8 @@ btr_cur_optimistic_delete_func(
 	if (UNIV_UNLIKELY(block->page.id().page_no() == cursor->index->page
 			  && page_get_n_recs(block->frame) == 1
 			  + (cursor->index->is_instant()
-			     && !rec_is_metadata(rec, *cursor->index)))) {
+			     && !rec_is_metadata(rec, *cursor->index))
+			  && !cursor->index->must_avoid_clear_instant_add())) {
 		/* The whole index (and table) becomes logically empty.
 		Empty the whole page. That is, if we are deleting the
 		only user record, also delete the metadata record
@@ -5687,7 +5688,8 @@ btr_cur_pessimistic_delete(
 				goto discard_page;
 			}
 		} else if (page_get_n_recs(page) == 1
-			   + (index->is_instant() && !is_metadata)) {
+			   + (index->is_instant() && !is_metadata)
+			   && !index->must_avoid_clear_instant_add()) {
 			/* The whole index (and table) becomes logically empty.
 			Empty the whole page. That is, if we are deleting the
 			only user record, also delete the metadata record
diff --git a/storage/innobase/btr/btr0pcur.cc b/storage/innobase/btr/btr0pcur.cc
index 9ecddf4904c..aac080d1a6e 100644
--- a/storage/innobase/btr/btr0pcur.cc
+++ b/storage/innobase/btr/btr0pcur.cc
@@ -149,7 +149,13 @@ before_first:
 
 		ut_ad(!page_rec_is_infimum(rec));
 		if (UNIV_UNLIKELY(rec_is_metadata(rec, *index))) {
+#if 0 /* MDEV-22867 had to relax this */
+			/* If the table is emptied during an ALGORITHM=NOCOPY
+			DROP COLUMN ... that is not ALGORITHM=INSTANT,
+			then we must preserve any instant ADD metadata. */
 			ut_ad(index->table->instant);
+#endif
+			ut_ad(index->is_instant());
 			ut_ad(page_get_n_recs(block->frame) == 1);
 			ut_ad(page_is_leaf(block->frame));
 			ut_ad(!page_has_siblings(block->frame));
diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h
index 731ff545685..694c7081574 100644
--- a/storage/innobase/include/dict0mem.h
+++ b/storage/innobase/include/dict0mem.h
@@ -1306,6 +1306,16 @@ public:
   void set_freed() { ut_ad(!freed()); page= 1; }
 #endif /* BTR_CUR_HASH_ADAPT */
 
+  /** @return whether it is forbidden to invoke clear_instant_add() */
+  bool must_avoid_clear_instant_add() const
+  {
+    if (is_instant())
+      for (auto i= this; (i= UT_LIST_GET_NEXT(indexes, i)) != nullptr; )
+        if (i->to_be_dropped /* || i->online_log*/)
+          return true;
+    return false;
+  }
+
 	/** This ad-hoc class is used by record_size_info only.	*/
 	class record_size_info_t {
 	public: