Hi Sergei!


On Wed, Jul 1, 2020 at 12:26 AM Sergei Golubchik <serg@mariadb.org> wrote:
>
> Hi, Aleksey!
>
> On Jun 29, Aleksey Midenkov wrote:
> > revision-id: 9bf4b92cbc5 (mariadb-10.5.2-168-g9bf4b92cbc5)
> > parent(s): 478301d9b9a
> > author: Aleksey Midenkov <midenok@gmail.com>
> > committer: Aleksey Midenkov <midenok@gmail.com>
> > timestamp: 2020-04-17 17:04:24 +0300
> > message:
> >
> > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
> >
> > == Syntax change ==
> >
> > Keyword AUTO_INCREMENT (or AUTO) enables partition auto-creation.
>
> I still think that AUTO_INCREMENT here looks rather out-of-place,
> I don't know how to explain to users why AUTO_INCREMENT is allowed here.
>
> But if we'll have it, we'll have to support it here for a long time
> for compatibility reasons. "Because somebody might be already using it"
>
> I'd suggest to support only AUTO here.

Sorry, that was just the wrong message. I already removed the AUTO_INCREMENT keyword.

>
> Or, if the parser can do it easily, a much more readable syntax would be
>
>   create table t1 (x int) with system versioning
>   auto partition by system_time interval 1 hour;
>
> this would be the best, easy to read, matches the documentation (where
> it could be called "auto partitioning"). If it won't cause any difficult
> parser problems, I'd prefer us to use that.

Besides the problems of implementation this probably is not correct logically. AUTO keyword is applicable only to SYSTEM_TIME partitioning so it should go after SYSTEM_TIME keyword, but is also applicable only to rotated partitions so it should go after rotation keywords as well.


>
> > create or replace table t1 (x int) with system versioning
> > partition by system_time interval 1 hour auto_increment;
> >
> > create or replace table t1 (x int) with system versioning
> > partition by system_time limit 1000 auto;
> >
> > Or with explicit partitions:
> >
> > create or replace table t1 (x int) with system versioning
> > partition by system_time interval 1 hour auto
> > (partition p0 history, partition pn current);
> >
> > == Description ==
> >
> > Before executing history-generating DML command add N history
> > partitions, so that N would be sufficient for potentially generated
> > history. N > 1 may be required when history is rotated by INTERVAL and
> > timestamp was jumped to future further than interval value.
> >
> > It is assumed that one DML command will not generate history rows more
> > than specified LIMIT. Otherwise history would overflow generated
> > partition, a warning would be printed, and user action would be
> > required to rebuild partitions.
> >
> > Auto-creation is implemented by synchronous
> > fast_alter_partition_table() call from the thread of the executed DML
> > command before the command itself (by the fallback and retry mechanism
> > similar to Discovery feature, see Open_table_context).
> >
> > Creating history partitions was made by the principle of minimal
> > disruption of the main business process. I.e. when procedure of
> > creation fails it will not (if possible) fail the original DML
> > command. Warning info will display what happened and the last history
> > partition may overflow. In such case partition rebuild is required to
> > correctly display history info. User application may detect warning
> > code ER_VERS_HIST_PART_FAILED and stop execution if that is preferred.
> >
> > The name for newly added partitions are generated like default
> > partition names with extension of MDEV-22155 (which avoids name
> > clashes by extending assignment counter to next free-enough gap).
> >
> > These DML commands trigger auto-creation:
> >
> > * DELETE (including multi-delete, excluding DELETE HISTORY)
> > * UPDATE (including multi-update)
> > * REPLACE (including REPLACE .. SELECT)
>
> INSERT ... ON DUPLICATE KEY UPDATE ?
>

Added.

> > diff --git a/mysql-test/suite/versioning/common.inc b/mysql-test/suite/versioning/common.inc
> > index 355b571e5a0..b35a5138015 100644
> > --- a/mysql-test/suite/versioning/common.inc
> > +++ b/mysql-test/suite/versioning/common.inc
> > @@ -6,6 +6,7 @@ if (!$TEST_VERSIONING_SO)
> >  source include/have_innodb.inc;
> >
> >  set @@session.time_zone='+00:00';
> > +set @@global.time_zone='+00:00';
>
> Why is that? I'd expect your auto-adding of partitions to happen in the
> context of a session, using session time zone.

This is for the "Concurrent DML" test case. New connections do not get current session time_zone.

>
> Is it for replication?
>
> >  select ifnull(max(transaction_id), 0) into @start_trx_id from mysql.transaction_registry;
> >  set @test_start=now(6);
> >
> > diff --git a/mysql-test/suite/versioning/r/delete_history.result b/mysql-test/suite/versioning/r/delete_history.result
> > index cb865a835b3..90c9e4777bb 100644
> > --- a/mysql-test/suite/versioning/r/delete_history.result
> > +++ b/mysql-test/suite/versioning/r/delete_history.result
> > @@ -63,8 +63,6 @@ insert into t values (1);
> >  update t set a= 2;
> >  update t set a= 3;
> >  delete history from t;
> > -Warnings:
> > -Warning      4114    Versioned table `test`.`t`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions
>
> good riddance, it didn't make much sense anyway
>
> >  # The above warning is one command late (MDEV-20345) ^^^
> >  select * from t for system_time all;
> >  a
> > diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> > index 85cb736b5bd..5fb893b4f0e 100644
> > --- a/sql/ha_partition.h
> > +++ b/sql/ha_partition.h
> > @@ -1623,7 +1623,7 @@ class ha_partition :public handler
>
> could you also, please, fix (in a separate commit)
> ha_partition::part_records() to take a proper
> partition_element* argument, not void* ?

Fixed.

>
> >      for (; part_id < part_id_end; ++part_id)
> >      {
> >        handler *file= m_file[part_id];
> > -      DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions), part_id));
> > +      bitmap_set_bit(&(m_part_info->read_partitions), part_id);
> >        file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_OPEN);
> >        part_recs+= file->stats.records;
> >      }
> > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> > index e9dbf2b49c3..72312ee7ac4 100644
> > --- a/sql/partition_info.cc
> > +++ b/sql/partition_info.cc
> > @@ -814,10 +814,16 @@ bool partition_info::has_unique_name(partition_element *element)
> >      vers_info->interval   Limit by fixed time interval
> >      vers_info->hist_part  (out) Working history partition
> >  */
> > -void partition_info::vers_set_hist_part(THD *thd)
> > +uint partition_info::vers_set_hist_part(THD *thd, bool auto_inc)
>
> let's not all it auto_inc. it'll add noise to every grep for auto_inc
> issues. What about auto_add ?
>

Since auto-partitioning is already there (HA_USE_AUTO_PARTITION) auto_add or auto_hist seem to be the good choice. I prefer auto_hist for more information.

> >  {
> > +  DBUG_ASSERT(!thd->lex->last_table()->vers_conditions.delete_history);
> > +
> > +  uint create_count= 0;
> > +  auto_inc= auto_inc && vers_info->auto_inc;
> > +
> >    if (vers_info->limit)
> >    {
> > +    DBUG_ASSERT(!vers_info->interval.is_set());
> >      ha_partition *hp= (ha_partition*)(table->file);
> >      partition_element *next= NULL;
> >      List_iterator<partition_element> it(partitions);
> > @@ -836,22 +842,26 @@ void partition_info::vers_set_hist_part(THD *thd)
> >      {
> >        if (next == vers_info->now_part)
> >        {
> > -        my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG),
> > -                table->s->db.str, table->s->table_name.str,
> > -                vers_info->hist_part->partition_name, "LIMIT");
> > +        if (auto_inc)
> > +          create_count= 1;
> > +        else
> > +          my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG),
> > +                  table->s->db.str, table->s->table_name.str,
> > +                  vers_info->hist_part->partition_name, "LIMIT");
> >        }
> >        else
> >          vers_info->hist_part= next;
> >      }
> > -    return;
> > +    // reserve at least one history partition
> > +    if (auto_inc && create_count == 0 &&
> > +        vers_info->hist_part->id + 1 == vers_info->now_part->id)
> > +      create_count= 1;
>
> Questionable. What does it solve?

This is for the LOCK TABLES when the history partition is almost full:

-- source include/have_partition.inc

create or replace table t1 (x int)
with system versioning partition by system_time
limit 10 auto;

insert into t1 values (1), (2), (3), (4), (5), (6), (7), (8), (9);
update t1 set x= x + 10;

lock tables t1 write;
update t1 set x= 1 where x = 11;
update t1 set x= 2 where x = 12;
update t1 set x= 3 where x = 13;
unlock tables;

select count(x) from t1 partition (p0);
show create table t1;
drop tables t1;

Imagine 1000000 instead of 10. Locked commands operate on a small subset of records. Nevertheless they are condemned to end in the wrong partition if we didn't reserve one at the LOCK TABLES (we cannot add new partitions while the table is locked). That of course works only if LOCK TABLES is short enough to not fill the whole partition (which is expected to be a normal scenario). We will not need this when we auto-add under LOCK TABLES.

>
> >    }
> > -
> > -  if (vers_info->interval.is_set())
> > +  else if (vers_info->interval.is_set() &&
> > +           vers_info->hist_part->range_value <= thd->query_start())
> >    {
> > -    if (vers_info->hist_part->range_value > thd->query_start())
> > -      return;
> > -
> >      partition_element *next= NULL;
> > +    bool error= true;
> >      List_iterator<partition_element> it(partitions);
> >      while (next != vers_info->hist_part)
> >        next= it++;
> > @@ -860,12 +870,200 @@ void partition_info::vers_set_hist_part(THD *thd)
> >      {
> >        vers_info->hist_part= next;
> >        if (next->range_value > thd->query_start())
> > -        return;
> > +      {
> > +        error= false;
> > +        break;
>
> but here you don't "reserve at least one history partition"
> Why INTERVAL is different?

Now as you asked I understand that INTERVAL is not different. Remade this condition for both clauses and limited it to LOCK TABLES-only.

>
> > +      }
> > +    }
> > +    if (error)
> > +    {
> > +      if (auto_inc)
> > +      {
> > +        DBUG_ASSERT(thd->query_start() >= vers_info->hist_part->range_value);
> > +        my_time_t diff= thd->query_start() - vers_info->hist_part->range_value;
> > +        if (diff > 0)
> > +        {
> > +          size_t delta= vers_info->interval.seconds();
> > +          create_count= diff / delta + 1;
> > +          if (diff % delta)
> > +            create_count++;
> > +        }
> > +        else
> > +          create_count= 1;
> > +      }
> > +      else
> > +      {
> > +        my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG),
> > +                table->s->db.str, table->s->table_name.str,
> > +                vers_info->hist_part->partition_name, "INTERVAL");
> > +      }
> >      }
> > -    my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG),
> > -            table->s->db.str, table->s->table_name.str,
> > -            vers_info->hist_part->partition_name, "INTERVAL");
> >    }
> > +
> > +  return create_count;
> > +}
> > +
> > +
> > +/**
> > +  @brief Run fast_alter_partition_table() to add new history partitions
> > +         for tables requiring them.
> > +*/
> > +void vers_add_auto_parts(THD *thd, TABLE_LIST* tl, uint num_parts)
> > +{
> > +  HA_CREATE_INFO create_info;
> > +  Alter_info alter_info;
> > +  String query;
> > +  partition_info *save_part_info= thd->work_part_info;
> > +  Query_tables_list save_query_tables;
> > +  Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer;
> > +  Diagnostics_area new_stmt_da(thd->query_id, false, true);
> > +  Diagnostics_area *save_stmt_da= thd->get_stmt_da();
> > +  bool save_no_write_to_binlog= thd->lex->no_write_to_binlog;
> > +  const CSET_STRING save_query= thd->query_string;
> > +  thd->m_reprepare_observer= NULL;
> > +  thd->lex->reset_n_backup_query_tables_list(&save_query_tables);
> > +  thd->in_sub_stmt|= SUB_STMT_AUTO_HIST;
> > +  thd->lex->no_write_to_binlog= !thd->is_current_stmt_binlog_format_row();
> > +  TABLE *table= tl->table;
> > +
> > +  DBUG_ASSERT(!thd->is_error());
> > +  /* NB: we have to preserve m_affected_rows, m_row_count_func, m_last_insert_id, etc */
> > +  thd->set_stmt_da(&new_stmt_da);
> > +  new_stmt_da.set_overwrite_status(true);
>
> Is it needed?
> You've just started using a new Diagnostics_area, the status should be
> DA_EMPTY here, nothing to overwrite.

I rethought my idea of not failing the main command and removed new_stmt_da. Better and simpler is to fail the command if alter fails.

>
> > +
> > +  {
> > +    DBUG_ASSERT(table->s->get_table_ref_type() == TABLE_REF_BASE_TABLE);
> > +    DBUG_ASSERT(table->versioned());
> > +    DBUG_ASSERT(table->part_info);
> > +    DBUG_ASSERT(table->part_info->vers_info);
> > +    alter_info.reset();
> > +    alter_info.partition_flags= ALTER_PARTITION_ADD|ALTER_PARTITION_AUTO_HIST;
> > +    create_info.init();
> > +    create_info.alter_info= &alter_info;
> > +    Alter_table_ctx alter_ctx(thd, tl, 1, &table->s->db, &table->s->table_name);
> > +
> > +    MDL_REQUEST_INIT(&tl->mdl_request, MDL_key::TABLE, tl->db.str,
> > +                    tl->table_name.str, MDL_SHARED_NO_WRITE, MDL_TRANSACTION);
> > +    if (thd->mdl_context.acquire_lock(&tl->mdl_request,
> > +                                      thd->variables.lock_wait_timeout))
> > +      goto exit;
> > +    table->mdl_ticket= tl->mdl_request.ticket;
> > +
> > +    create_info.db_type= table->s->db_type();
> > +    create_info.options|= HA_VERSIONED_TABLE;
> > +    DBUG_ASSERT(create_info.db_type);
> > +
> > +    create_info.vers_info.set_start(table->s->vers_start_field()->field_name);
> > +    create_info.vers_info.set_end(table->s->vers_end_field()->field_name);
> > +
> > +    partition_info *part_info= new partition_info();
> > +    if (unlikely(!part_info))
> > +    {
> > +      my_error(ER_OUT_OF_RESOURCES, MYF(0));
> > +      goto exit;
> > +    }
> > +    part_info->use_default_num_partitions= false;
> > +    part_info->use_default_num_subpartitions= false;
> > +    part_info->num_parts= num_parts;
> > +    part_info->num_subparts= table->part_info->num_subparts;
> > +    part_info->subpart_type= table->part_info->subpart_type;
> > +    if (unlikely(part_info->vers_init_info(thd)))
> > +    {
> > +      my_error(ER_OUT_OF_RESOURCES, MYF(0));
> > +      goto exit;
> > +    }
> > +
> > +    // NB: set_ok_status() requires DA_EMPTY
> > +    thd->get_stmt_da()->reset_diagnostics_area();
>
> Is it needed?
> You've just started using a new Diagnostics_area, the status should be
> DA_EMPTY here.

Reworked according to the above.

>
> > +
> > +    thd->work_part_info= part_info;
> > +    if (part_info->set_up_defaults_for_partitioning(thd, table->file, NULL,
> > +                                    table->part_info->next_part_no(num_parts)))
> > +    {
> > +      push_warning(thd, Sql_condition::WARN_LEVEL_WARN,
> > +                   ER_VERS_HIST_PART_FAILED,
> > +                   "Auto-increment history partition: "
> > +                   "setting up defaults failed");
> > +      my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING),
> > +               tl->db.str, tl->table_name.str);
> > +      goto exit;
> > +    }
> > +    bool partition_changed= false;
> > +    bool fast_alter_partition= false;
> > +    if (prep_alter_part_table(thd, table, &alter_info, &create_info,
> > +                              &partition_changed, &fast_alter_partition))
> > +    {
> > +      push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED,
> > +                   "Auto-increment history partition: "
> > +                   "alter partitition prepare failed");
> > +      my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING),
> > +               tl->db.str, tl->table_name.str);
> > +      goto exit;
> > +    }
> > +    if (!fast_alter_partition)
> > +    {
> > +      push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED,
> > +                   "Auto-increment history partition: "
> > +                   "fast alter partitition is not possible");
> > +      my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING),
> > +               tl->db.str, tl->table_name.str);
> > +      goto exit;
> > +    }
> > +    DBUG_ASSERT(partition_changed);
> > +    if (mysql_prepare_alter_table(thd, table, &create_info, &alter_info,
> > +                                  &alter_ctx))
> > +    {
> > +      push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED,
> > +                   "Auto-increment history partition: "
> > +                   "alter prepare failed");
> > +      my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING),
> > +               tl->db.str, tl->table_name.str);
> > +      goto exit;
> > +    }
> > +
> > +    // Forge query string for rpl logging
> > +    if (!thd->lex->no_write_to_binlog)
> > +    {
> > +      query.set(STRING_WITH_LEN("ALTER TABLE `"), &my_charset_latin1);
>
> better use StringBuffer<...> query;
> because you're doing lots of reallocs now.
>
> > +
> > +      if (query.append(table->s->db) ||
> > +          query.append(STRING_WITH_LEN("`.`")) ||
> > +          query.append(table->s->table_name) ||
>
> this is wrong, the table and db names can have backticks inside.
> use append_identifier() instead.
>
> > +          query.append("` ADD PARTITION PARTITIONS ") ||
> > +          query.append_ulonglong(part_info->num_parts) ||
> > +          query.append(" AUTO"))
> > +      {
> > +        my_error(ER_OUT_OF_RESOURCES, MYF(ME_ERROR_LOG));
> > +        goto exit;
> > +      }
> > +      CSET_STRING qs(query.c_ptr(), query.length(), &my_charset_latin1);
> > +      thd->set_query(qs);
> > +    }
>
> Should it be binlogged at all? May be just leave it to the slave to
> auto-add the partition as needed?

This is for RBR where auto-add didn't work. Reworked via solution of fixing the slave side.

>
> Looks a bit suspicious now, you log an ALTER TABLE possibly in the
> middle of a transaction. It will be replayed differently, not as
> auto-adding, in particular, it will commit. So, possibly different
> results on the slave, perhaps different gtid.
>
> Do clear it out with Andrei please. Or don't binlog auto-adding at all.
>
> > +
> > +    if (fast_alter_partition_table(thd, table, &alter_info, &create_info,
> > +                                   tl, &table->s->db, &table->s->table_name))
> > +    {
> > +      push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED,
> > +                   "Auto-increment history partition: "
> > +                   "alter partition table failed");
> > +      my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING),
> > +               tl->db.str, tl->table_name.str);
> > +    }
> > +  }
> > +
> > +  if (!thd->transaction.stmt.is_empty())
> > +    trans_commit_stmt(thd);
>
> 1. why would this operation register a transaction?