maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Tue, Nov 23, 2021 at 11:29 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> This is a review of diff b95c5105e2 2552bce607
>
> This was quite good, thanks! Just a couple of questions/comments, see
> below.
>
> On Nov 23, Aleksey Midenkov wrote:
>
> > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test
> > index 006a65e1a16..a5bb8426ae4 100644
> > --- a/mysql-test/suite/versioning/t/partition.test
> > +++ b/mysql-test/suite/versioning/t/partition.test
> > @@ -1070,15 +1080,572 @@ alter table t1 add partition partitions 2;
> ...
> > +--let $datadir= `select @@datadir`
> > +--let $dummy= $datadir/test/t1#P#p1.ibd
> > +--write_file $dummy
> > +EOF
> > +
> > +set timestamp= unix_timestamp('2000-01-01 01:00:00');
> > +--error ER_GET_ERRNO
> > +update t1 set x= x + 2;
>
> That's a good one!
>
> > +show warnings;
> > +--remove_file $dummy
> > +
> > diff --git a/sql/table.h b/sql/table.h
> > index 2e074abcea0..d861db261bf 100644
> > --- a/sql/table.h
> > +++ b/sql/table.h
> > @@ -910,6 +911,11 @@ struct TABLE_SHARE
> >    vers_kind_t versioned;
> >    period_info_t vers;
> >    period_info_t period;
> > +  /*
> > +      Protect multiple threads from repeating partition auto-create over
> > +      single share.
>
> could you please add here (to the comment)
>
>      TODO remove it when partitioning metadata will be in TABLE_SHARE

Added.

>
> > +  */
> > +  bool          vers_skip_auto_create;
> >
> >    bool init_period_from_extra2(period_info_t *period, const uchar *data,
> >                                 const uchar *end);
> > @@ -2557,6 +2567,11 @@ struct TABLE_LIST
> >    bool          merged;
> >    bool          merged_for_insert;
> >    bool          sequence;  /* Part of NEXTVAL/CURVAL/LASTVAL */
> > +  /*
> > +      Protect single thread from repeating partition auto-create over
> > +      multiple share instances (as the share is closed on backoff action).
> > +  */
> > +  bool          vers_skip_create;
>
> I don't understand it. What does it mean "multiple share instances"?
> Can there be multiple TABLE_SHARE objects for the same table? How?

Over the time. The share is released and then reacquired.

>
> >
> >    /*
> >      Items created by create_view_field and collected to change them in case
> > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> > index d7bb02bbd4e..56d5a68efd0 100644
> > --- a/sql/sql_partition.cc
> > +++ b/sql/sql_partition.cc
> > @@ -2587,11 +2587,15 @@ char *generate_partition_syntax(THD *thd, partition_info *part_info,
> >          err+= str.append(ctime, ctime_len);
> >          err+= str.append('\'');
> >        }
> > +      if (vers_info->auto_hist)
> > +        err+= str.append(STRING_WITH_LEN(" AUTO"));
> >      }
> > -    if (vers_info->limit)
> > +    else if (vers_info->limit)
> >      {
> >        err+= str.append(STRING_WITH_LEN("LIMIT "));
> >        err+= str.append_ulonglong(vers_info->limit);
> > +      if (vers_info->auto_hist)
> > +        err+= str.append(STRING_WITH_LEN(" AUTO"));
>
> Can you do this str.appent("AUTO") once, after the if()?
> Not in both branches?

Done.


>
> >      }
> >    }
> >    else if (part_info->part_expr)
> > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> > index fd92e437cac..ddcc6f413ba 100644
> > --- a/sql/partition_info.cc
> > +++ b/sql/partition_info.cc
> > @@ -865,9 +870,167 @@ 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;
> > +      }
> > +    }
> > +    if (error)
> > +    {
> > +      if (auto_hist)
> > +      {
> > +        *create_count= 0;
> > +        const my_time_t hist_end= (my_time_t) vers_info->hist_part->range_value;
> > +        DBUG_ASSERT(thd->query_start() >= hist_end);
> > +        MYSQL_TIME h0, q0;
> > +        my_tz_OFFSET0->gmt_sec_to_TIME(&h0, hist_end);
> > +        my_tz_OFFSET0->gmt_sec_to_TIME(&q0, thd->query_start());
> > +        longlong q= pack_time(&q0);
> > +        longlong h= pack_time(&h0);
> > +        while (h <= q)
> > +        {
> > +          if (date_add_interval(thd, &h0, vers_info->interval.type,
> > +                                vers_info->interval.step))
> > +            return true;
> > +          h= pack_time(&h0);
> > +          ++*create_count;
> > +          if (*create_count == MAX_PARTITIONS - 2)
> > +          {
> > +            my_error(ER_TOO_MANY_PARTITIONS_ERROR, MYF(ME_WARNING));
> > +            my_error(ER_VERS_HIST_PART_FAILED, MYF(0),
> > +                     table->s->db.str, table->s->table_name.str);
> > +            return true;
> > +          }
> > +        }
> > +      }
> > +      else
> > +      {
> > +        DBUG_ASSERT(!vers_info->auto_hist);
>
> ^^^ This assert fails for me in the update-big test

This assertion is wrong, removed that. We failed to lock the table and
now report the warning normally.

>
> > +        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 false;
> > +}
> > +
> > +
> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index ac22a63bdba..459edb614f7 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -4203,6 +4444,15 @@ bool open_tables(THD *thd, const DDL_options_st &options,
> >      }
> >
> >    thd->current_tablenr= 0;
> > +
> > +#ifdef WITH_PARTITION_STORAGE_ENGINE
> > +  if (!thd->stmt_arena->is_conventional())
> > +  {
> > +    for (tables= *start; tables; tables= tables->next_global)
> > +      tables->vers_skip_create= false;
> > +  }
>
> This looks a bit like an overkill, you do it for every prepare,
> while auto-adding a partition is a rare event, as you said.
>
> May be you can do instead in TABLE::vers_switch_partition
>
>  uint *create_count= !thd->stmt_arena->is_conventional() &&
>                      table_list->vers_skip_create ?

No, this is inside backoff action loop. That will interfere with the
normal vers_skip_create algorithm (which also applies to PS, SP). The
goal of the above code is to reset vers_skiop_create from the previous
statement execution.

>
> > +#endif
> > +
> >  restart:
> >    /*
> >      Close HANDLER tables which are marked for flush or against which there
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



--
@midenok