Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Sergei!
On Mon, Dec 6, 2021 at 11:22 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Dec 05, Aleksey Midenkov wrote:
> > > > 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 +,15 @@ bool open_tables(THD *thd, const DDL_options_st
> > > > ,
> > > > }
> > > >
> > > >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_skip_create from the previous
> > statement execution.
>
> In that case you can use an auto-resetting value, like,
> if tables->vers_skip_create == thd->query_id it means, "skip".
> That is vers_skip_create must be of query_id_t. You set it with
> tables->vers_skip_create= thd->query_id;
> And on the next statement it automatically expires.
>
> This semantics is a bit more complex than boolean, so it'd need a
> comment, like
>
> /*
> Protect single thread from repeating partition auto-create over
> multiple share instances (as the share is closed on backoff action).
> Skips auto-create only for one given query id.
> */
> query_id_tvers_skip_create;
Done! Thanks for the tip.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
@midenok
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Dec 05, Aleksey Midenkov wrote:
> > > 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 +,15 @@ bool open_tables(THD *thd, const DDL_options_st
> > > ,
> > > }
> > >
> > >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_skip_create from the previous
> statement execution.
In that case you can use an auto-resetting value, like,
if tables->vers_skip_create == thd->query_id it means, "skip".
That is vers_skip_create must be of query_id_t. You set it with
tables->vers_skip_create= thd->query_id;
And on the next statement it automatically expires.
This semantics is a bit more complex than boolean, so it'd need a
comment, like
/*
Protect single thread from repeating partition auto-create over
multiple share instances (as the share is closed on backoff action).
Skips auto-create only for one given query id.
*/
query_id_tvers_skip_create;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi Sergei,
On Tue, Nov 23, 2021 at 11:29 PM Sergei Golubchik 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(, hist_end);
> > +my_tz_OFFSET0->gmt_sec_to_TIME(, thd->query_start());
> > +longlong q= pack_time();
> > +longlong h= pack_time();
> > +while (h <= q)
> > +{
> > + if (date_add_interval(thd, , vers_info->interval.type,
> > +vers_info->interval.step))
> > +return true;
> > + h= pack_time();
> > + ++*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
Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
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
> + */
> + 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?
>
>/*
> 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?
> }
>}
>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(, hist_end);
> +my_tz_OFFSET0->gmt_sec_to_TIME(, thd->query_start());
> +longlong q= pack_time();
> +longlong h= pack_time();
> +while (h <= q)
> +{
> + if (date_add_interval(thd, , vers_info->interval.type,
> +vers_info->interval.step))
> +return true;
> + h= pack_time();
> + ++*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
> +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 +,15 @@ bool open_tables(THD *thd, const DDL_options_st
> ,
> }
>
