MDEV-19114 gcol.innodb_virtual_debug: Assertion n_fields>0 failed

This is a regression due to MDEV-16376
commit 8dc70c862b8ec115fd9a3c2b37c746ffc4f0d3cc.
To make dict_index_t::detach_columns() idempotent,
we cleared dict_index_t::n_fields. But, this could
cause trouble with purge after a secondary index
creation failed (not even involving virtual columns).

A better way is to clear the dict_field_t::col pointers
that point to virtual columns that are being freed
due to aborting index creation on an index that depends
on a virtual column.

Note: the v_cols[] of an existing dict_table_t object will
never be modified. If any virtual columns are added or removed,
ha_innobase::commit_inplace_alter_table() would invoke
dict_table_remove_from_cache() and reload the table to dict_sys.
Index creation is a special case where the dict_index_t points
to virtual columns that do not yet exist in dict_table_t.

3 files changed, 45 insertions, 42 deletions

diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc
index cff213dea3d..3dfbe92e34c 100644
--- a/storage/innobase/dict/dict0dict.cc
+++ b/storage/innobase/dict/dict0dict.cc
@@ -2200,7 +2200,7 @@ dict_index_remove_from_v_col_list(dict_index_t* index) {
 
                 for (ulint i = 0; i < dict_index_get_n_fields(index); i++) {
                         col =  dict_index_get_nth_col(index, i);
-                        if (dict_col_is_virtual(col)) {
+                        if (col && col->is_virtual()) {
                                 vcol = reinterpret_cast<const dict_v_col_t*>(
                                         col);
 				/* This could be NULL, when we do add
diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
index 9b1d99d3730..9afbcf4d1f4 100644
--- a/storage/innobase/handler/handler0alter.cc
+++ b/storage/innobase/handler/handler0alter.cc
@@ -248,16 +248,12 @@ struct ha_innobase_inplace_ctx : public inplace_alter_handler_ctx
 	@return whether the table will be rebuilt */
 	bool need_rebuild () const { return(old_table != new_table); }
 
-	/** Clear uncommmitted added indexes after a failed operation. */
-	void clear_added_indexes()
-	{
-		for (ulint i = 0; i < num_to_add_index; i++) {
-			if (!add_index[i]->is_committed()) {
-				add_index[i]->detach_columns();
-				add_index[i]->n_fields = 0;
-			}
-		}
-	}
+  /** Clear uncommmitted added indexes after a failed operation. */
+  void clear_added_indexes()
+  {
+    for (ulint i= 0; i < num_to_add_index; i++)
+      add_index[i]->detach_columns(true);
+  }
 
 	/** Share context between partitions.
 	@param[in] ctx	context from another partition of the table */
diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h
index 80d18dcbb2a..0d4ee9d23ec 100644
--- a/storage/innobase/include/dict0mem.h
+++ b/storage/innobase/include/dict0mem.h
@@ -634,12 +634,12 @@ struct dict_col_t{
 					this column. Our current max limit is
 					3072 for Barracuda table */
 
-	/** @return whether this is a virtual column */
-	bool is_virtual() const { return prtype & DATA_VIRTUAL; }
+  /** @return whether this is a virtual column */
+  bool is_virtual() const { return prtype & DATA_VIRTUAL; }
 
-	/** Detach the column from an index.
-	@param[in]	index	index to be detached from */
-	inline void detach(const dict_index_t& index);
+  /** Detach a virtual column from an index.
+  @param index  being-freed index */
+  inline void detach(const dict_index_t &index);
 };
 
 /** Index information put in a list of virtual column structure. Index
@@ -1017,15 +1017,22 @@ struct dict_index_t{
 	/** @return whether the index is corrupted */
 	inline bool is_corrupted() const;
 
-	/** Detach the columns from the index that is to be freed. */
-	void detach_columns()
-	{
-		if (has_virtual()) {
-			for (unsigned i = 0; i < n_fields; i++) {
-				fields[i].col->detach(*this);
-			}
-		}
-	}
+  /** Detach the virtual columns from the index that is to be removed.
+  @param   whether to reset fields[].col */
+  void detach_columns(bool clear= false)
+  {
+    if (!has_virtual())
+      return;
+    for (unsigned i= 0; i < n_fields; i++)
+    {
+      dict_col_t* col= fields[i].col;
+      if (!col || !col->is_virtual())
+        continue;
+      col->detach(*this);
+      if (clear)
+        fields[i].col= NULL;
+    }
+  }
 
 #ifdef BTR_CUR_HASH_ADAPT
   /** @return a clone of this */
@@ -1102,24 +1109,24 @@ struct dict_index_t{
 	inline record_size_info_t record_size_info() const;
 };
 
-/** Detach a column from an index.
-@param[in]	index	index to be detached from */
-inline void dict_col_t::detach(const dict_index_t& index)
+/** Detach a virtual column from an index.
+@param index  being-freed index */
+inline void dict_col_t::detach(const dict_index_t &index)
 {
-	if (!is_virtual()) {
-		return;
-	}
-
-	if (dict_v_idx_list* v_indexes = reinterpret_cast<const dict_v_col_t*>
-	    (this)->v_indexes) {
-		for (dict_v_idx_list::iterator i = v_indexes->begin();
-		     i != v_indexes->end(); i++) {
-			if (i->index == &index) {
-				v_indexes->erase(i);
-				return;
-			}
-		}
-	}
+  ut_ad(is_virtual());
+
+  if (dict_v_idx_list *v_indexes= reinterpret_cast<const dict_v_col_t*>(this)
+      ->v_indexes)
+  {
+    for (dict_v_idx_list::iterator i= v_indexes->begin();
+         i != v_indexes->end(); i++)
+    {
+      if (i->index == &index) {
+        v_indexes->erase(i);
+        return;
+      }
+    }
+  }
 }
 
 /** The status of online index creation */