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(&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
+ 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 ?
+#endif + restart: /* Close HANDLER tables which are marked for flush or against which there
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On Tue, Nov 23, 2021 at 11:29 PM Sergei Golubchik <serg@mariadb.org> 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@mariadb.org
-- @midenok
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 +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_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_t vers_skip_create; Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Mon, Dec 6, 2021 at 11:22 PM Sergei Golubchik <serg@mariadb.org> 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 +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_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_t vers_skip_create;
Done! Thanks for the tip.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik