[MDEV-6362] The partition engine does not call check_if_supported_inplace_alter. Created: 2014-06-18  Updated: 2014-07-08  Resolved: 2014-07-08

Status: Closed
Project: MariaDB Server
Component/s: None
Affects Version/s: 10.0.10
Fix Version/s: 10.0.12

Type: Bug Priority: Major
Reporter: Olivier Bertrand Assignee: Sergei Golubchik
Resolution: Fixed Votes: 0
Labels: None


 Description   

This probably was because all engines currently supporting partitioning do not support in place alter. However, this is wrong for engines that do support in place alter.

For instance when doing:

alter table partinp add index XID(id);
 
alter table partinp drop index XID;

the data of CONNECT outward tables is modified (all rows are doubled)

Although it was possible to avoid this, calling check_if_supported_inplace_alter should be done to avoid unnecessary and time consuming operations.



 Comments   
Comment by Sergei Golubchik [ 2014-07-03 ]

bertrandop, as far as I can see, partition engine calls check_if_supported_inplace_alter(), it's right there in ha_partition::check_if_supported_inplace_alter() method.

Could you provide a complete test case for the buggy behavior that you observed? Starting from create table and everything. The connect test suite (in storage/connect/mysql-test/connect) contains no examples of partitioned connect tables.

Comment by Olivier Bertrand [ 2014-07-05 ]

Sure enough the function exists in ha_partition. However, it does not call the engine used by the partition engine and, as it is compiled by default, returns unconditionally HA_ALTER_INPLACE_NOT_SUPPORTED while for CONNECT outward tables it should return HA_ALTER_INPLACE_NO_LOCK.
I am not sure this would be enough but it shows once again that the partition engine was written when only a few storage engines were supporting partitioning and is not really adapted for any new storage engines willing to support partitioning.
By the way, there are no example of partition tables in the current test suite because supporting partitioning by CONNECT is a work in progress and the current version of CONNECT does not support partitioning yet.

Comment by Sergei Golubchik [ 2014-07-07 ]

Okay, any way I can repeat this behavior? Because as far as I can see ha_partition::check_if_supported_inplace_alter() does call engine's check_if_supported_inplace_alter(), see:

enum_alter_inplace_result
 
ha_partition::check_if_supported_inplace_alter(TABLE *altered_table,
 
                                               Alter_inplace_info *ha_alter_info)
 
{
 
  uint index= 0;
 
  enum_alter_inplace_result result= HA_ALTER_INPLACE_NO_LOCK;
 
  ha_partition_inplace_ctx *part_inplace_ctx;
 
  bool first_is_set= false;
 
  THD *thd= ha_thd();
 
 
 
  DBUG_ENTER("ha_partition::check_if_supported_inplace_alter");
 
  if (ha_alter_info->alter_info->flags == Alter_info::ALTER_PARTITION)
 
    DBUG_RETURN(HA_ALTER_INPLACE_NO_LOCK);
 
 
 
  part_inplace_ctx= new (thd->mem_root) ha_partition_inplace_ctx(thd, m_tot_parts);
 
  if (!part_inplace_ctx)
 
    DBUG_RETURN(HA_ALTER_ERROR);
 
 
 
  part_inplace_ctx->handler_ctx_array= (inplace_alter_handler_ctx **)
 
    thd->alloc(sizeof(inplace_alter_handler_ctx *) * (m_tot_parts + 1));
 
  if (!part_inplace_ctx->handler_ctx_array)
 
    DBUG_RETURN(HA_ALTER_ERROR);
 
 
 
  ha_alter_info->handler_flags |= Alter_inplace_info::ALTER_PARTITIONED;
 
  for (index= 0; index < m_tot_parts; index++)
 
  {
 
    enum_alter_inplace_result p_result=
 
      m_file[index]->check_if_supported_inplace_alter(altered_table, ha_alter_info);
 
}

As you can see, it returns HA_ALTER_INPLACE_NO_LOCK for ALTER PARTITION, returns an error if memory allocation fails, and then calls check_if_supported_inplace_alter for every partition.

Comment by Olivier Bertrand [ 2014-07-07 ]

Sergei: On my machine, the complete code of ha_partition::check_if_supported_inplace_alter is:

enum_alter_inplace_result
 
ha_partition::check_if_supported_inplace_alter(TABLE *altered_table,
 
                                               Alter_inplace_info *ha_alter_info)
 
{
 
#ifdef PARTITION_SUPPORTS_INPLACE_ALTER
 
  uint index= 0;
 
  enum_alter_inplace_result result= HA_ALTER_INPLACE_NO_LOCK;
 
  ha_partition_inplace_ctx *part_inplace_ctx;
 
  THD *thd= ha_thd();
 
#else
 
  enum_alter_inplace_result result= HA_ALTER_INPLACE_NOT_SUPPORTED;
 
#endif
 
 
 
  DBUG_ENTER("ha_partition::check_if_supported_inplace_alter");
 
  /*
 
    Support inplace change of KEY () -> KEY ALGORITHM = N ().
 
    Any other change would set partition_changed in
 
    prep_alter_part_table() in mysql_alter_table().
 
  */
 
  if (ha_alter_info->alter_info->flags == Alter_info::ALTER_PARTITION)
 
    DBUG_RETURN(HA_ALTER_INPLACE_NO_LOCK);
 
 
 
#ifndef PARTITION_SUPPORTS_INPLACE_ALTER
 
  /*
 
    Due to bug#14760210 partitions can be out-of-sync in case
 
    commit_inplace_alter_table fails after the first partition.
 
 
 
    Until we can either commit all partitions at the same time or
 
    have an atomic recover on failure/crash we don't support any
 
    inplace alter.
 
 
 
    TODO: investigate what happens when indexes are out-of-sync
 
    between partitions. If safe and possible to recover from,
 
    then we could allow ADD/DROP INDEX.
 
  */
 
  DBUG_RETURN(result);
 
#else
 
  part_inplace_ctx=
 
    new (thd->mem_root) ha_partition_inplace_ctx(thd, m_tot_parts);
 
  if (!part_inplace_ctx)
 
    DBUG_RETURN(HA_ALTER_ERROR);
 
 
 
  part_inplace_ctx->handler_ctx_array= (inplace_alter_handler_ctx **)
 
    thd->alloc(sizeof(inplace_alter_handler_ctx *) * m_tot_parts);
 
  if (!part_inplace_ctx->handler_ctx_array)
 
    DBUG_RETURN(HA_ALTER_ERROR);
 
 
 
  for (index= 0; index < m_tot_parts; index++)
 
    part_inplace_ctx->handler_ctx_array[index]= NULL;
 
 
 
  for (index= 0; index < m_tot_parts; index++)
 
  {
 
    enum_alter_inplace_result p_result=
 
      m_file[index]->check_if_supported_inplace_alter(altered_table,
 
                                                      ha_alter_info);
 
    part_inplace_ctx->handler_ctx_array[index]= ha_alter_info->handler_ctx;
 
 
 
    if (p_result < result)
 
      result= p_result;
 
    if (result == HA_ALTER_ERROR)
 
      break;
 
  }
 
  ha_alter_info->handler_ctx= part_inplace_ctx;
 
 
 
  DBUG_RETURN(result);
 
#endif
 
}

It was compiled by default without PARTITION_SUPPORTS_INPLACE_ALTER defined. This is why the check_if_supported_inplace_alter function was not called for each partition. I shall recompile it after defining PARTITION_SUPPORTS_INPLACE_ALTER and let you know of what happens.

Comment by Olivier Bertrand [ 2014-07-08 ]

Apparently, recompiling the partition engine with PARTITION_SUPPORTS_INPLACE_ALTER defined solves the problems I add. In particular, DROP INDEX and CREATE INDEX that used to fail now works.

Note however the issue of this pre-compiler variable. Is it still used? If yes, it should be defined by default.

Comment by Sergei Golubchik [ 2014-07-08 ]

Please pull the latest 10.0 source tree. This PARTITION_SUPPORTS_INPLACE_ALTER was removed in 10.0.12.

Comment by Olivier Bertrand [ 2014-07-08 ]

I did try on
bzr+ssh://bertrandop@bazaar.launchpad.net/~maria-captains/maria/10.0-connect/
but it says:

No revisions or tags to pull.

Generated at Thu Feb 08 07:11:19 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.