maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

On Wed, Dec 26, 2018 at 8:53 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Nikita!
>
> On Dec 25, Nikita Malyavin wrote:
> > revision-id: 773d24d457a (versioning-1.0.6-100-g773d24d457a)
> > parent(s): b26736cdb11
> > author: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> > committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> > timestamp: 2018-12-21 03:13:47 +1000
> > message:
> >
> > MDEV-15951 system versioning by trx id doesn't work with partitioning
> >
> > Fix partitioning for trx_id-versioned tables.
> > `partition by hash`, `range` and others now work.
> > `partition by system_time` is forbidden.
> > Currently we cannot use row_start and row_end in `partition by`, because
> > insertion of versioned field is done by engine's handler, as well as
> > row_start/row_end's value set up, which is a transaction id -- so it's
> > also forbidden.
> >
> > The drawback is that it's now impossible to use `partition by key()`
> > without parameters for such tables, because it references row_start and
> > row_end implicitly.
> >
> > * add handler::vers_can_native()
> > * drop Table_scope_and_contents_source_st::vers_native()
> > * drop partition_element::find_engine_flag as unused
> > * forbid versioning partitioning for trx_id as not supported
> > * adopt vers tests for trx_id partitioning
> > * forbid any row_end referencing in `partition by` clauses,
> >   including implicit `by key()`
> >
> > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> > index 9e6c333d3c9..48bcb828d56 100644
> > --- a/sql/sql_partition.cc
> > +++ b/sql/sql_partition.cc
> > @@ -341,7 +341,16 @@ static bool set_up_field_array(THD *thd, TABLE
> *table,
> >    while ((field= *(ptr++)))
> >    {
> >      if (field->flags & GET_FIXED_FIELDS_FLAG)
> > +    {
> > +      if (table->versioned(VERS_TRX_ID)
> > +          && unlikely(field->flags & VERS_SYSTEM_FIELD))
> > +      {
> > +        my_error(ER_VERS_NOT_SUPPORTED, MYF(0),
> > +                 "Using ROW START/END for patitioning",
> "transactional");
>
> This is incorrect usage of error message parameters. The error message
> can be translated, parameters can be reordered - you should not use
> English as parameter values. Consider:
>
>   transactional cистемно-версионированные таблицы не поддерживают Using
> ROW START/END for patitioning
>
> But looks like ER_VERS_NOT_SUPPORTED is not used anywhere at the moment.
> So you can change it to be anything you want, even remove all
> parameters:
>
>   Transactional system versioned tables do not support partitioning by ROW
> START or ROW END
>
> Done as well.


> > +        DBUG_RETURN(TRUE);
> > +      }
> >        num_fields++;
> > +    }
> >    }
> >    if (unlikely(num_fields > MAX_REF_PARTS))
> >    {
> > diff --git a/sql/table.cc b/sql/table.cc
> > index ce7a34a8fe2..0c2ff1eea5a 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -1776,7 +1776,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD
> *thd, bool write,
> >        goto err;
> >      DBUG_PRINT("info", ("Columns with system versioning: [%d, %d]",
> row_start, row_end));
> >      versioned= VERS_TIMESTAMP;
> > -    vers_can_native= plugin_hton(se_plugin)->flags &
> HTON_NATIVE_SYS_VERSIONING;
> > +    vers_can_native= handler_file->vers_can_native(thd);
>
> Interesting. How does that work? Your ha_partition::vers_can_native() is
>
>   bool can= !thd->lex->part_info
>             || thd->lex->part_info->part_type != VERSIONING_PARTITION;
>   for (uint i= 0; i < m_tot_parts && can; i++)
>     can= can && m_file[i]->vers_can_native(thd);
>   return can;
>
> But it doesn't look like thd->lex->part_info->part_type is initialized
> at this point in time. Try to add to your test case
>
>   flush tables;
>   select * from t1;
>
> Yes, part_info is going to be filled only if creating, or altering table.
It works as follows:
If we're creating/altering, then check thd->lex->part_info;
else, check m_file array, which is initialized at that point.

Added this as the comment for clarity.

-- 
Yours truly,
Nikita Malyavin