MDEV-14632 Assertion `!((new_col->prtype ^ col->prtype) & ~256U)' fai…

…led in row_log_table_apply_convert_mrec

SQL, IB: proper fix is to disable unimplemented Online DDL for system-versioned tables inside InnoDB

mysql-test/suite/versioning/r/alter.result

@@ -305,10 +305,6 @@ a
 1
 call verify_vtq;
 No	A	B	C	D
-alter table t drop system versioning, algorithm=inplace;
-call verify_vtq;
-No	A	B	C	D
-alter table t add system versioning;
 alter table t drop system versioning, algorithm=copy;
 show create table t;
 Table	Create Table

mysql-test/suite/versioning/r/online.result

@@ -0,0 +1,32 @@
+set system_versioning_alter_history=keep;
+create or replace table t (a int) engine=innodb;
+alter table t add system versioning, lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+alter table t add system versioning;
+alter table t add index idx(a), lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+alter table t drop system versioning, lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+set global system_versioning_transaction_registry=on;
+Warnings:
+Warning	4144	Transaction-based system versioning is EXPERIMENTAL and is subject to change in future.
+create or replace table t (a int) engine=innodb;
+alter table t
+add s bigint unsigned as row start,
+add e bigint unsigned as row end,
+add period for system_time(s, e),
+add system versioning,
+lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+alter table t
+add s bigint unsigned as row start,
+add e bigint unsigned as row end,
+add period for system_time(s, e),
+add system versioning;
+alter table t add index idx(a), lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+alter table t drop column s, drop column e;
+alter table t drop system versioning, lock=none;
+ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
+set global system_versioning_transaction_registry=off;
+drop table t;

mysql-test/suite/versioning/t/alter.test

@@ -198,11 +198,12 @@ show create table t;
 select * from t for system_time all;
 call verify_vtq;
 
-alter table t drop system versioning, algorithm=inplace;
-call verify_vtq;
 ## FIXME: #414 IB: inplace for VERS_TIMESTAMP versioning
 if (0)
 {
+alter table t drop system versioning, algorithm=inplace;
+call verify_vtq;
+
 alter table t add system versioning, algorithm=inplace;
 call verify_vtq;
 show create table t;
@@ -220,7 +221,6 @@ alter table t drop column b, algorithm=inplace;
 show create table t;
 select * from t for system_time all;
 }
-alter table t add system versioning;
 ## FIXME END
 
 alter table t drop system versioning, algorithm=copy;

mysql-test/suite/versioning/t/online.test

@@ -0,0 +1,39 @@
+--source include/have_innodb.inc
+
+set system_versioning_alter_history=keep;
+
+create or replace table t (a int) engine=innodb;
+
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t add system versioning, lock=none;
+alter table t add system versioning;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t add index idx(a), lock=none;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t drop system versioning, lock=none;
+
+
+set global system_versioning_transaction_registry=on;
+create or replace table t (a int) engine=innodb;
+
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t
+  add s bigint unsigned as row start,
+  add e bigint unsigned as row end,
+  add period for system_time(s, e),
+  add system versioning,
+  lock=none;
+alter table t
+  add s bigint unsigned as row start,
+  add e bigint unsigned as row end,
+  add period for system_time(s, e),
+  add system versioning;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t add index idx(a), lock=none;
+alter table t drop column s, drop column e;
+--error ER_ALTER_OPERATION_NOT_SUPPORTED
+alter table t drop system versioning, lock=none;
+
+set global system_versioning_transaction_registry=off;
+
+drop table t;

sql/handler.cc

@@ -6885,7 +6885,7 @@ bool Table_scope_and_contents_source_st::vers_fix_system_fields(
   List<Item> *items,
   bool *versioned_write)
 {
-  DBUG_ASSERT(!vers_info.without_system_versioning);
+  DBUG_ASSERT(!(alter_info->flags & Alter_info::ALTER_DROP_SYSTEM_VERSIONING));
   int vers_tables= 0;
 
   if (select_tables)
@@ -6901,13 +6901,13 @@ bool Table_scope_and_contents_source_st::vers_fix_system_fields(
   // then created table will be versioned.
   if (thd->variables.vers_force)
   {
-    vers_info.with_system_versioning= true;
+    alter_info->flags|= Alter_info::ALTER_ADD_SYSTEM_VERSIONING;
     options|= HA_VERSIONED_TABLE;
   }
 
   // Possibly override default storage engine to match one used in source table.
-  if (vers_tables && vers_info.with_system_versioning &&
-    !(used_fields & HA_CREATE_USED_ENGINE))
+  if (vers_tables && alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING &&
+      !(used_fields & HA_CREATE_USED_ENGINE))
   {
     List_iterator_fast<Create_field> it(alter_info->create_list);
     while (Create_field *f= it++)
@@ -6923,19 +6923,18 @@ bool Table_scope_and_contents_source_st::vers_fix_system_fields(
     }
   }
 
-  if (!vers_info.need_check())
+  if (!vers_info.need_check(alter_info))
     return false;
 
-  if (!vers_info.versioned_fields &&
-    vers_info.unversioned_fields &&
-    !vers_info.with_system_versioning)
+  if (!vers_info.versioned_fields && vers_info.unversioned_fields &&
+      !(alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING))
   {
     // All is correct but this table is not versioned.
     options&= ~HA_VERSIONED_TABLE;
     return false;
   }
 
-  if (!vers_info.with_system_versioning && vers_info)
+  if (!(alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING) && vers_info)
   {
     my_error(ER_MISSING, MYF(0), create_table.table_name, "WITH SYSTEM VERSIONING");
     return true;
@@ -6953,7 +6952,7 @@ bool Table_scope_and_contents_source_st::vers_fix_system_fields(
   while (Create_field *f= it++)
   {
     if ((f->versioning == Column_definition::VERSIONING_NOT_SET &&
-         !vers_info.with_system_versioning) ||
+         !(alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING)) ||
         f->versioning == Column_definition::WITHOUT_VERSIONING)
     {
       f->flags|= VERS_UPDATE_UNVERSIONED_FLAG;
@@ -7110,28 +7109,16 @@ bool Vers_parse_info::fix_alter_info(THD *thd, Alter_info *alter_info,
   TABLE_SHARE *share= table->s;
   const char *table_name= share->table_name.str;
 
-  if (!need_check() && !share->versioned)
+  if (!need_check(alter_info) && !share->versioned)
     return false;
 
-  if (with_system_versioning || without_system_versioning)
-  {
-    // Disable Online DDL which is not implemented yet for ADD/DROP SYSTEM VERSIONING.
-    if (thd->mdl_context.upgrade_shared_lock(table->mdl_ticket, MDL_EXCLUSIVE,
-                                             thd->variables.lock_wait_timeout))
-    {
-      my_error(ER_LOCK_WAIT_TIMEOUT, MYF(0));
-      return true;
-    }
-    alter_info->requested_lock= Alter_info::ALTER_TABLE_LOCK_EXCLUSIVE;
-  }
-
-  if (with_system_versioning && table->versioned())
+  if (alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING && table->versioned())
   {
     my_error(ER_VERS_ALREADY_VERSIONED, MYF(0), table_name);
     return true;
   }
 
-  if (without_system_versioning)
+  if (alter_info->flags & Alter_info::ALTER_DROP_SYSTEM_VERSIONING)
   {
     if (!share->versioned)
     {
@@ -7300,7 +7287,7 @@ bool Vers_parse_info::fix_alter_info(THD *thd, Alter_info *alter_info,
   if (fix_implicit(thd, alter_info))
     return true;
 
-  if (with_system_versioning)
+  if (alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING)
   {
     if (check_with_conditions(table_name))
       return true;
@@ -7370,6 +7357,12 @@ Vers_parse_info::fix_create_like(Alter_info &alter_info, HA_CREATE_INFO &create_
   return false;
 }
 
+bool Vers_parse_info::need_check(const Alter_info *alter_info) const
+{
+  return versioned_fields || unversioned_fields ||
+         alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING ||
+         alter_info->flags & Alter_info::ALTER_DROP_SYSTEM_VERSIONING || *this;
+}
 
 bool Vers_parse_info::check_with_conditions(const char *table_name) const
 {

sql/handler.h

@@ -1715,8 +1715,6 @@ class Create_field;
 struct Vers_parse_info
 {
   Vers_parse_info() :
-    with_system_versioning(false),
-    without_system_versioning(false),
     versioned_fields(false),
     unversioned_fields(false)
   {}
@@ -1762,15 +1760,7 @@ struct Vers_parse_info
   {
     return as_row.start || as_row.end || system_time.start || system_time.end;
   }
-  bool need_check() const
-  {
-    return
-      versioned_fields ||
-      unversioned_fields ||
-      with_system_versioning ||
-      without_system_versioning ||
-      *this;
-  }
+  bool need_check(const Alter_info *alter_info) const;
   bool check_with_conditions(const char *table_name) const;
   bool check_sys_fields(const char *table_name, Alter_info *alter_info,
                         bool native) const;
@@ -1784,10 +1774,6 @@ struct Vers_parse_info
   bool fix_create_like(Alter_info &alter_info, HA_CREATE_INFO &create_info,
                        TABLE_LIST &src_table, TABLE_LIST &table);
 
-  /** Table definition has 'WITH/WITHOUT SYSTEM VERSIONING' */
-  bool with_system_versioning : 1;
-  bool without_system_versioning : 1;
-
   /**
      At least one field was specified 'WITH/WITHOUT SYSTEM VERSIONING'.
      Useful for error handling.
@@ -2170,6 +2156,10 @@ class Alter_inplace_info
 
   static const HA_ALTER_FLAGS ALTER_COLUMN_UNVERSIONED   = 1ULL << 42;
 
+  static const HA_ALTER_FLAGS ALTER_ADD_SYSTEM_VERSIONING= 1ULL << 43;
+
+  static const HA_ALTER_FLAGS ALTER_DROP_SYSTEM_VERSIONING= 1ULL << 44;
+
   /**
     Create options (like MAX_ROWS) for the new version of table.
 

sql/sql_alter.h

@@ -97,6 +97,8 @@ class Alter_info
     ALTER_ADD_CHECK_CONSTRAINT  = 1L << 27,
     ALTER_DROP_CHECK_CONSTRAINT = 1L << 28,
     ALTER_COLUMN_UNVERSIONED    = 1L << 29,
+    ALTER_ADD_SYSTEM_VERSIONING = 1L << 30,
+    ALTER_DROP_SYSTEM_VERSIONING= 1L << 31,
   };
 
   enum enum_enable_or_disable { LEAVE_AS_IS, ENABLE, DISABLE };

sql/sql_table.cc

@@ -6564,6 +6564,10 @@ static bool fill_alter_inplace_info(THD *thd,
     ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_DROP_HISTORICAL;
   if (alter_info->flags & Alter_info::ALTER_COLUMN_UNVERSIONED)
     ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_UNVERSIONED;
+  if (alter_info->flags & Alter_info::ALTER_ADD_SYSTEM_VERSIONING)
+    ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_ADD_SYSTEM_VERSIONING;
+  if (alter_info->flags & Alter_info::ALTER_DROP_SYSTEM_VERSIONING)
+    ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_DROP_SYSTEM_VERSIONING;
 
   /*
     If we altering table with old VARCHAR fields we will be automatically

sql/sql_yacc.yy

@@ -6242,7 +6242,7 @@ versioning_option:
             }
             else
             {
-              Lex->vers_get_info().with_system_versioning= true;
+              Lex->alter_info.flags|= Alter_info::ALTER_ADD_SYSTEM_VERSIONING;
               Lex->create_info.options|= HA_VERSIONED_TABLE;
             }
           }
@@ -8250,12 +8250,12 @@ alter_list_item:
         | alter_lock_option
         | ADD SYSTEM VERSIONING_SYM
           {
-            Lex->vers_get_info().with_system_versioning= true;
+            Lex->alter_info.flags|= Alter_info::ALTER_ADD_SYSTEM_VERSIONING;
             Lex->create_info.options|= HA_VERSIONED_TABLE;
           }
         | DROP SYSTEM VERSIONING_SYM
           {
-            Lex->vers_get_info().without_system_versioning= true;
+            Lex->alter_info.flags|= Alter_info::ALTER_DROP_SYSTEM_VERSIONING;
           }
         ;
 

storage/innobase/handler/handler0alter.cc

@@ -633,7 +633,7 @@ instant_alter_column_possible(
 	const TABLE*			table)
 {
 	// Making table system-versioned instantly is not implemented yet.
-	if (ha_alter_info->create_info->vers_info.with_system_versioning) {
+	if (ha_alter_info->handler_flags & Alter_inplace_info::ALTER_ADD_SYSTEM_VERSIONING) {
 		return false;
 	}
 
@@ -694,9 +694,10 @@ ha_innobase::check_if_supported_inplace_alter(
 {
 	DBUG_ENTER("check_if_supported_inplace_alter");
 
-	  if (altered_table->versioned(VERS_TIMESTAMP)) {
+	if (altered_table->versioned(VERS_TIMESTAMP)
+	    || ha_alter_info->handler_flags & Alter_inplace_info::ALTER_DROP_SYSTEM_VERSIONING) {
 		DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
-	  }
+	}
 
 	/* Before 10.2.2 information about virtual columns was not stored in
 	system tables. We need to do a full alter to rebuild proper 10.2.2+
@@ -1226,6 +1227,13 @@ ha_innobase::check_if_supported_inplace_alter(
 		}
 	}
 
+	// FIXME: implement Online DDL for system-versioned tables
+	DBUG_ASSERT(!altered_table->versioned(VERS_TIMESTAMP));
+	if (altered_table->versioned(VERS_TRX_ID)
+	    || ha_alter_info->handler_flags & Alter_inplace_info::ALTER_DROP_SYSTEM_VERSIONING) {
+		online = false;
+	}
+
 	DBUG_RETURN(online
 		    ? HA_ALTER_INPLACE_NO_LOCK_AFTER_PREPARE
 		    : HA_ALTER_INPLACE_SHARED_LOCK_AFTER_PREPARE);