Hi Sergei, On Sun, Apr 4, 2021 at 2:48 PM Sergei Golubchik wrote:

Hi, Aleksey!

Note, it's a review of the `git diff 82e44d60d1e 8175ce8a5f1` so it's not only commit c4de76aeff8

On Apr 04, Aleksey Midenkov wrote:

...

revision-id: c4de76aeff8 (mariadb-10.5.2-540-gc4de76aeff8) parent(s): 82e44d60d1e author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2021-03-31 21:17:55 +0300 message:

MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT

Overall it's very good! I have few minor questions/comments, see below. But it's almost done, I think.

...

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 @@ -154,3 +152,18 @@ select * from t1; a 1 drop table t1; +# +# MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT +# +# Don't auto-create new partition on DELETE HISTORY: +create or replace table t (a int) with system versioning +partition by system_time limit 1000 auto; +delete history from t; +show create table t; +Table Create Table +t CREATE TABLE `t` ( + `a` int(11) DEFAULT NULL +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME LIMIT 1000 AUTO +PARTITIONS 2

Hmm, and if DELETE HISTORY would auto-create new partitions, what output would one expect here?

I mean, how one can see whether the test result is correct or wrong?

By PARTITIONS value. Stale test case, fixed.

...

+drop table t; diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test index 445f5844630..57b80ca0b47 100644 --- a/mysql-test/suite/versioning/t/partition.test +++ b/mysql-test/suite/versioning/t/partition.test @@ -1068,11 +1078,412 @@ alter table t1 add partition partitions 2; --replace_result $default_engine DEFAULT_ENGINE show create table t1; alter table t1 add partition partitions 8; +drop table t1; + +--echo # +--echo # MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT +--echo # +create or replace table t1 (x int) with system versioning +partition by system_time limit 1 auto; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +--echo # INSERT, INSERT .. SELECT don't increment partitions

it's not really "increment", better say "don't auto-create"

Actually I like "increment" more. "Auto-create" overcomplicates phrases: --echo # Increment from 3 to 5 --echo # Increment from 3 to 6, manual names, LOCK TABLES --echo # Multiple increments in single command Besides "increment" is correct because PARTITIONS number is incremented.

...

+insert into t1 values (1); + +create or replace table t2 (y int); +insert into t2 values (2); + +insert into t1 select * from t2; +insert into t2 select * from t1;

why do you need a t2 table in this test?

t1 is not incremented in case of insert into t2 select * from t1;

...

+--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +--echo # Too many partitions error +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto; +set timestamp= unix_timestamp('2001-01-01 00:01:00'); +--error ER_TOO_MANY_PARTITIONS_ERROR +update t1 set x= x + 1; + +--enable_info +--echo # Increment from 3 to 5 +create or replace table t1 (x int) with system versioning +partition by system_time interval 3600 second +starts '2000-01-01 00:00:00' auto partitions 3; + +insert into t1 values (1); +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 02:00:00'); +update t1 set x= x + 1; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +update t1 set x= x + 2; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +--echo # Increment from 3 to 6, manual names, LOCK TABLES +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto ( + partition p1 history, + partition p3 history, + partition pn current); + +insert into t1 values (1); --replace_result $default_engine DEFAULT_ENGINE show create table t1;

+set timestamp= unix_timestamp('2000-01-01 02:00:00'); +update t1 set x= x + 3; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +update t1 set x= x + 4; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 04:00:00'); +lock tables t1 write; +update t1 set x= x + 5; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +unlock tables; +set timestamp= default; + +--echo # Couple of more LOCK TABLES cases +create or replace table t1 (x int) with system versioning +partition by system_time limit 1000 auto; +lock tables t1 write; +insert into t1 values (1); +update t1 set x= x + 1; +unlock tables; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1;

what does this test case demonstrate?

Fixed stale test case.

...

+ +--echo # Overflow prevention under LOCK TABLES +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); +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +drop tables t1; + +--echo # Test VIEW, LOCK TABLES +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto partitions 3; +create or replace view v1 as select * from t1; + +insert into v1 values (1);

why would a view matter in this test case?

Made it matter.

...

+--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 01:00:00'); +lock tables t1 write; +update t1 set x= x + 3; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +unlock tables; + +drop view v1; drop tables t1;

+--echo # Multiple increments in single command +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto partitions 3; + +create or replace table t2 (y int) with system versioning +partition by system_time interval 1 hour auto; + +insert into t1 values (1); +insert into t2 values (2); + +set timestamp= unix_timestamp('2000-01-01 01:00:00'); +update t1, t2 set x= x + 1, y= y + 1; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +--replace_result $default_engine DEFAULT_ENGINE +show create table t2; + +set timestamp= unix_timestamp('2000-01-01 02:00:00'); +update t1, t2 set x= x + 1, y= y + 1; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +--replace_result $default_engine DEFAULT_ENGINE +show create table t2; + +--echo # Here t2 is incremented too, but not updated +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +update t1, t2 set t1.x= 0 where t1.x< t2.y; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +# Multiupdate_prelocking_strategy::handle_end() is processed after table open. +# For PS it is possible to skip unneeded auto-creation because the above happens at +# prepare stage and auto-creation is done at execute stage. +--replace_result $default_engine DEFAULT_ENGINE 'PARTITIONS 4' 'PARTITIONS ok' 'PARTITIONS 5' 'PARTITIONS ok'

eh... I don't think this is really "ok". As far as I remember, Multiupdate_prelocking_strategy knows what tables should be opened for writing and what for reading. Why would a new partition be created for t2?

It knows this after tables are opened. Look at handle_end(), specifically mysql_handle_derived(), handle_derived(), setup_fields_with_no_wrap() and check_fields(). I believe all these calls are required to get proper get_table_map(). To get this working properly there must be 2-staged open tables, something like PS does.

...

+show create table t2; + +drop tables t1, t2; + +--echo # PS, SP, LOCK TABLES +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto; + +insert into t1 values (1); + +set timestamp= unix_timestamp('2000-01-01 01:00:00'); +execute immediate 'update t1 set x= x + 5'; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +prepare s from 'update t1 set x= x + 6'; +set timestamp= unix_timestamp('2000-01-01 02:00:00'); +execute s; execute s; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +lock tables t1 write; +execute s; execute s; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +unlock tables; +drop prepare s; + +create procedure sp() update t1 set x= x + 7; +set timestamp= unix_timestamp('2000-01-01 04:00:00'); +call sp; call sp; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +set timestamp= unix_timestamp('2000-01-01 05:00:00'); +lock tables t1 write; +call sp; call sp; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +unlock tables; +drop procedure sp; + +set timestamp= unix_timestamp('2001-01-01 00:00:00'); +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '2000-01-01 00:00:00'; +insert into t1 values (0); +set timestamp= unix_timestamp('2001-01-01 00:00:01'); +prepare s from 'update t1 set i= i + 1'; +execute s; +set timestamp= unix_timestamp('2001-01-02 00:00:01'); +execute s; +drop prepare s; + +if (!$MTR_COMBINATION_HEAP)

because of blobs?

Because of blobs.

...

+{ +--echo # Complex table +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 ( + x int primary key auto_increment, ... +--echo # Transaction +create or replace table t1 (x int) with system versioning engine innodb +partition by system_time interval 1 hour auto; +start transaction; +insert into t1 values (1); +select * from t1; + +--connect con1, localhost, root +set lock_wait_timeout= 1; +set innodb_lock_wait_timeout= 1; +--error ER_LOCK_WAIT_TIMEOUT +update t1 set x= x + 111; +select * from t1;

what do you test here? (there is no show create table in the test)

This was MENT-675 Assertion `thd->transaction.stmt.is_empty() || thd->in_sub_stmt' failed I moved it to MDEV-25339 and added a comment.

...

+ +# cleanup +--disconnect con1 +--connection default +drop table t1; + +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning engine innodb +partition by system_time interval 1 hour auto; + +insert into t1 values (1); +set timestamp= unix_timestamp('2000-01-01 01:00:00'); +start transaction; +update t1 set x= 0; +--connect con1, localhost, root +select * from t1;

if you add a show create table here, what would it show?

Added. +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `x` int(11) DEFAULT NULL +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO +PARTITIONS 3

...

+--connection default +commit; +show create table t1; + +set timestamp= unix_timestamp('2000-01-01 02:00:00'); +start transaction; +update t1 set x= 1; +--connection con1 +select * from t1; +--connection default +rollback; +show create table t1; +--disconnect con1 +--connection default +drop table t1; + +--echo # +--echo # MENT-724 Locking timeout caused by auto-creation affects original DML

I'd better avoid MENT references here. Let's only mention Jira issues that users can actually see

Fixed.

...

+--echo # +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int primary key) with system versioning engine innodb +partition by system_time interval 1 hour auto; + +insert into t1 values (1); +start transaction; +insert into t1 values (2); + +--connect con1, localhost, root +set lock_wait_timeout= 1; +set innodb_lock_wait_timeout= 1; +set timestamp= unix_timestamp('2000-01-01 01:00:01'); +update t1 set x= x + 122 where x = 1;

isn't it a test that you already have above? with x = x + 111

Not fully. That was the original bug posted by Elena (MDEV-25339). And this modification is posted by me (MDEV-23642).

...

+--disconnect con1 +--connection default +select * from t1; + +# cleanup +drop table t1; +set timestamp= default; + --echo # --echo # End of 10.5 tests --echo # diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result index 2a277b1c2ea..8369b40d98c 100644 --- a/mysql-test/suite/versioning/r/partition.result +++ b/mysql-test/suite/versioning/r/partition.result @@ -1236,27 +1270,752 @@ t1 CREATE TABLE `t1` ( ... +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +affected rows: 0 +lock tables t1 write; +affected rows: 0 +execute s; +affected rows: 1 +info: Rows matched: 1 Changed: 1 Inserted: 1 Warnings: 0 +execute s; +affected rows: 1 +info: Rows matched: 1 Changed: 1 Inserted: 1 Warnings: 0 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `x` int(11) DEFAULT NULL +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO +PARTITIONS 6

why did it add two partitions here?

Because of this condition /* When hist_part is almost full LOCK TABLES my overflow the partition as we can't add new partitions under LOCK TABLES. Reserve one more for this case. */ if (auto_hist && create_count < 2 && thd->lex->sql_command == SQLCOM_LOCK_TABLES && vers_info->hist_part->id + 1 == vers_info->now_part->id) create_count++;

...

+affected rows: 1 +unlock tables; +affected rows: 0 +drop prepare s; +affected rows: 0 +create procedure sp() update t1 set x= x + 7; +affected rows: 0 +set timestamp= unix_timestamp('2000-01-01 04:00:00'); +affected rows: 0 +call sp; +affected rows: 1 +call sp; +affected rows: 1 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `x` int(11) DEFAULT NULL +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO +PARTITIONS 6 +affected rows: 1 +set timestamp= unix_timestamp('2000-01-01 05:00:00'); +affected rows: 0 +lock tables t1 write; +affected rows: 0 +call sp; +affected rows: 1 +call sp; +affected rows: 1 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `x` int(11) DEFAULT NULL +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO +PARTITIONS 8

and two here

Same here.

...

+affected rows: 1 +unlock tables; +affected rows: 0 +drop procedure sp; +affected rows: 0 +set timestamp= unix_timestamp('2001-01-01 00:00:00'); +affected rows: 0 +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '2000-01-01 00:00:00'; +affected rows: 0 +insert into t1 values (0); +affected rows: 1 +set timestamp= unix_timestamp('2001-01-01 00:00:01'); +affected rows: 0 +prepare s from 'update t1 set i= i + 1'; +affected rows: 0 +info: Statement prepared +execute s; +affected rows: 1 +info: Rows matched: 1 Changed: 1 Inserted: 1 Warnings: 1 +Warnings: +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions

why?

Actually this is a typo in the test case: year 2000 vs 2001. Fixed.

...

+set timestamp= unix_timestamp('2001-01-02 00:00:01'); +affected rows: 0 +execute s; +affected rows: 1 +info: Rows matched: 1 Changed: 1 Inserted: 1 Warnings: 1 +Warnings: +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions +drop prepare s; +affected rows: 0 +# Complex table ... diff --git a/mysql-test/suite/versioning/t/rpl.test b/mysql-test/suite/versioning/t/rpl.test index b5be68feece..54258a8bdf1 100644 --- a/mysql-test/suite/versioning/t/rpl.test +++ b/mysql-test/suite/versioning/t/rpl.test @@ -133,4 +133,44 @@ sync_slave_with_master; connection master; drop table t1;

+--echo # +--echo # MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT +--echo # +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (x int) with system versioning +partition by system_time interval 1 hour auto; +insert t1 values (); +set timestamp= unix_timestamp('2000-01-01 01:00:00'); +delete from t1; +--replace_result InnoDB ENGINE MyISAM ENGINE MEMORY ENGINE +show create table t1; +--sync_slave_with_master +--replace_result InnoDB ENGINE MyISAM ENGINE MEMORY ENGINE +show create table t1; +--connection master +drop table t1; +set timestamp= default; + +--echo # +--echo # MENT-685 DML events for auto-partitioned tables are written into binary log twice

same comment about MENT

Fixed.

...

+--echo # +create table t1 (a int) with system versioning +partition by system_time limit 1 auto; + +insert into t1 values (1); +update t1 set a= a + 1; +update t1 set a= a + 1; +--replace_result InnoDB ENGINE MyISAM ENGINE MEMORY ENGINE +show create table t1; +select * from t1; + +--sync_slave_with_master +--replace_result InnoDB ENGINE MyISAM ENGINE MEMORY ENGINE +show create table t1; + +select * from t1; +--connection master +# cleanup +drop table t1;

may be show binlog events? you know, to verify that DML events aren't written into binary log twice

Added.

...

+ --source include/rpl_end.inc diff --git a/sql/ha_partition.h b/sql/ha_partition.h index 60a2d7f6762..7ade4419c22 100644 --- a/sql/ha_partition.h +++ b/sql/ha_partition.h @@ -1610,7 +1610,7 @@ class ha_partition :public handler 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);

Where was it set before your patch?

#1 0x0000000000a99d04 in partition_info::set_partition_bitmaps (this=0x7fc214b7af80, partition_names=0x0 ) at ../src/sql/partition_info.cc:274 #2 0x0000000001087da4 in ha_partition::change_partitions_to_open (this=0x7fc214b787a0, partition_names=0 x0) at ../src/sql/ha_partition.cc:8644 #3 0x0000000000822c01 in set_partitions_as_used (tl=0x7fc214015988, t=0x7fc214a61fb8) at ../src/sql/sql_ base.cc:1562 #4 0x00000000008222a7 in open_table (thd=0x7fc214000d48, table_list=0x7fc214015988, ot_ctx=0x7fc23c0a19c 8) at ../src/sql/sql_base.cc:1990 #5 0x0000000000827497 in open_and_process_table (thd=0x7fc214000d48, tables=0x7fc214015988, counter=0x7f c23c0a1acc, flags=0, prelocking_strategy=0x7fc23c0a1b40, has_prelocking_list=false, ot_ctx=0x7fc23c0a19c8 ) at ../src/sql/sql_base.cc:3801 #6 0x0000000000825de0 in open_tables (thd=0x7fc214000d48, options=..., start=0x7fc23c0a1ae0, counter=0x7 fc23c0a1acc, flags=0, prelocking_strategy=0x7fc23c0a1b40) at ../src/sql/sql_base.cc:4275

...

file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_OPEN); part_recs+= file->stats.records; } diff --git a/sql/handler.cc b/sql/handler.cc index b312635c8ee..6bb6c279193 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1572,7 +1572,7 @@ int ha_commit_trans(THD *thd, bool all) DBUG_ASSERT(thd->transaction->stmt.ha_list == NULL || trans == &thd->transaction->stmt);

- if (thd->in_sub_stmt) + if (thd->in_sub_stmt & ~SUB_STMT_AUTO_HIST) {

where is this ha_commit_trans(thd, false) called from? mysql_alter_table that adds a new partition?

Looks like garbage. Removed SUB_STMT_AUTO_HIST.

...

DBUG_ASSERT(0); /* diff --git a/sql/partition_info.cc b/sql/partition_info.cc index 871411cf6c4..cf8dd140553 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -816,10 +816,17 @@ 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_hist) { + DBUG_ASSERT(!thd->lex->last_table() || + !thd->lex->last_table()->vers_conditions.delete_history);

is that right? Conceptually you need to test vers_conditions.delete_history for the table that owns this partition_info. Is it always last_table() ?

It is always last_table() because DELETE HISTORY works with one table.

I'd say it'd be more logical to do, like

part_field_array[0]->table->pos_in_table_list

Fixed.

...

+ + uint create_count= 0; + auto_hist= auto_hist && vers_info->auto_hist; + if (vers_info->limit) { + DBUG_ASSERT(!vers_info->interval.is_set()); ha_partition *hp= (ha_partition*)(table->file); partition_element *next= NULL; List_iterator it(partitions); @@ -862,9 +869,183 @@ 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) + { + DBUG_ASSERT(thd->query_start() >= vers_info->hist_part->range_value); + my_time_t diff= thd->query_start() - (my_time_t) vers_info->hist_part->range_value; + if (diff > 0) + { + size_t delta= vers_info->interval.seconds(); + create_count= (uint) (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),

after such an overflow a table becomes essentially corrupted, rows are in a wrong partition. So new history partitions cannot be added anymore without reorganizing the last partition.

How is it handled now? (it's just a question, not part of the review, as it's not something you were changing in your patch)

...

+ table->s->db.str, table->s->table_name.str, + vers_info->hist_part->partition_name, "INTERVAL"); + } } } + + /* + When hist_part is almost full LOCK TABLES my overflow the partition as we

s/my/may/

Fixed.

...

+ can't add new partitions under LOCK TABLES. Reserve one for this case. + */ + if (auto_hist && create_count == 0 && + thd->lex->sql_command == SQLCOM_LOCK_TABLES && + vers_info->hist_part->id + 1 == vers_info->now_part->id) + create_count= 1; + + return create_count; +} + + +/** + @brief Run fast_alter_partition_table() to add new history partitions + for tables requiring them. +*/ +bool vers_add_auto_hist_parts(THD *thd, TABLE_LIST* tl, uint num_parts) +{ + bool result= true; + HA_CREATE_INFO create_info; + Alter_info alter_info; + partition_info *save_part_info= thd->work_part_info; + Query_tables_list save_query_tables; + Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer; + bool save_no_write_to_binlog= thd->lex->no_write_to_binlog; + 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= true; + TABLE *table= tl->table; + + DBUG_ASSERT(!thd->is_error()); + + { + 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();

would it not be DA_EMPTY without a reset? this is at the beginning of a statement, I'd expect it normally be DA_EMPTY here. What other DA states can you get here?

You are right. Removed.

...

+ + 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; + } + + 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); + goto exit; + } + } + + result= false; + // NB: we have to return DA_EMPTY for new command

may be DBUG_ASSERT(thd->get_stmt_da()->is_ok());

Added.

...

+ thd->get_stmt_da()->reset_diagnostics_area(); + +exit: + thd->work_part_info= save_part_info; + thd->m_reprepare_observer= save_reprepare_observer; + thd->lex->restore_backup_query_tables_list(&save_query_tables); + thd->in_sub_stmt&= ~SUB_STMT_AUTO_HIST; + thd->lex->no_write_to_binlog= save_no_write_to_binlog; + return result; }

diff --git a/sql/partition_info.h b/sql/partition_info.h index 0656238ec07..94093353d60 100644 --- a/sql/partition_info.h +++ b/sql/partition_info.h @@ -72,9 +73,26 @@ struct Vers_part_info : public Sql_alloc my_time_t start; INTERVAL step; enum interval_type type; - bool is_set() { return type < INTERVAL_LAST; } + bool is_set() const { return type < INTERVAL_LAST; } + size_t seconds() const + { + if (step.second) + return step.second; + if (step.minute) + return step.minute * 60; + if (step.hour) + return step.hour * 3600; + if (step.day) + return step.day * 3600 * 24; + // comparison is used in rough estimates, it doesn't need to be calendar-correct + if (step.month) + return step.month * 3600 * 24 * 30;

shouldn't it be `* 28` ? you need a most pessimistic estimate to make sure you never miss to create a partition when one is needed. You can sometimes create one when it's not needed yet, but it's less of a problem.

Good catch. Made it calendar-correct.

...

+ DBUG_ASSERT(step.year); + return step.year * 86400 * 30 * 365;

that's one `* 30` too many :)

...

+ } } interval; ulonglong limit; + bool auto_hist; partition_element *now_part; partition_element *hist_part; }; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1a1186aca73..d0e255186da 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1625,6 +1625,52 @@ bool is_locked_view(THD *thd, TABLE_LIST *t) }

+bool TABLE::vers_need_hist_part(const THD *thd, const TABLE_LIST *table_list) const +{ +#ifdef WITH_PARTITION_STORAGE_ENGINE + if (part_info && part_info->part_type == VERSIONING_PARTITION && + !table_list->vers_conditions.delete_history && + !thd->stmt_arena->is_stmt_prepare() && + table_list->lock_type >= TL_WRITE_ALLOW_WRITE && + table_list->mdl_request.type >= MDL_SHARED_WRITE && + table_list->mdl_request.type < MDL_EXCLUSIVE) + { + switch (thd->lex->sql_command) + {

SQLCOM_LOAD with DUP_REPLACE?

Added.

...

+ case SQLCOM_INSERT: + if (thd->lex->duplicates != DUP_UPDATE) + break; + /* fall-through: */ + case SQLCOM_LOCK_TABLES: + case SQLCOM_DELETE: + case SQLCOM_UPDATE: + case SQLCOM_REPLACE: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_DELETE_MULTI: + case SQLCOM_UPDATE_MULTI: + return true; + default:; + break; + } + if (thd->rgi_slave && thd->rgi_slave->current_event && thd->lex->sql_command == SQLCOM_END)

I wonder, would it be possible, instead of introducing rgi_slave->current_event, just make row events set thd->lex->sql_command appropriately? For example, currently row events increment thd->status_var.com_stat[] each event for its own SQLCOM_xxx, it won't be needed if they'll just set thd->lex->sql_command.

Do you think this is a quick refactoring? I had the same idea when I did this. And probably I tried to do that quickly already. Added TODO comment.

...

+ { + switch (thd->rgi_slave->current_event->get_type_code()) + { + case UPDATE_ROWS_EVENT: + case UPDATE_ROWS_EVENT_V1: + case DELETE_ROWS_EVENT: + case DELETE_ROWS_EVENT_V1: + return true; + default:; + break; + } + } + } +#endif + return false; +} + + /** Open a base table.

@@ -1777,6 +1823,11 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) DBUG_PRINT("info",("Using locked table")); #ifdef WITH_PARTITION_STORAGE_ENGINE part_names_error= set_partitions_as_used(table_list, table); + if (table->vers_need_hist_part(thd, table_list)) + { + /* Rotation is still needed under LOCK TABLES */

a bit confusing comment, you left out stuff that are obvious to you but not to others. I'd suggest something like

/* New partitions are not auto-created under LOCK TABLES (TODO: fix it) but rotation can still happen. */

Fixed.

...

+ table->part_info->vers_set_hist_part(thd, false); + } #endif goto reset; } @@ -2032,6 +2083,20 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) tc_add_table(thd, table); }

+ if (!ot_ctx->vers_create_count &&

what does this condition mean? When is ot_ctx->vers_create_count > 0 ?

When we already requested backoff action.

...

+ table->vers_need_hist_part(thd, table_list)) + { + ot_ctx->vers_create_count= table->part_info->vers_set_hist_part(thd, true); + if (ot_ctx->vers_create_count) + { + MYSQL_UNBIND_TABLE(table->file); + tc_release_table(table); + ot_ctx->request_backoff_action(Open_table_context::OT_ADD_HISTORY_PARTITION, + table_list); + DBUG_RETURN(TRUE); + } + } + if (!(flags & MYSQL_OPEN_HAS_MDL_LOCK) && table->s->table_category < TABLE_CATEGORY_INFORMATION) { @@ -3172,10 +3239,18 @@ Open_table_context::recover_from_failed_open() break; case OT_DISCOVER: case OT_REPAIR: - if ((result= lock_table_names(m_thd, m_thd->lex->create_info, - m_failed_table, NULL, - get_timeout(), 0))) + case OT_ADD_HISTORY_PARTITION: + result= lock_table_names(m_thd, m_thd->lex->create_info, m_failed_table, + NULL, get_timeout(), 0); + if (result) + { + if (m_action == OT_ADD_HISTORY_PARTITION) + { + m_thd->clear_error();

why?

Because of MDEV-23642.

...

+ result= false; + } break; + }

tdc_remove_table(m_thd, m_failed_table->db.str, m_failed_table->table_name.str); diff --git a/sql/sql_error.h b/sql/sql_error.h index 318d5076534..92076616adb 100644 --- a/sql/sql_error.h +++ b/sql/sql_error.h @@ -1195,7 +1195,6 @@ class Diagnostics_area: public Sql_state_errno,

void copy_non_errors_from_wi(THD *thd, const Warning_info *src_wi);

-private:

It doesn't seem you're using get_warning_info() anywhere

Reverted.

...

Warning_info *get_warning_info() { return m_wi_stack.front(); }

const Warning_info *get_warning_info() const { return m_wi_stack.front(); } diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc index 86ba393fb7b..f4a960afe47 100644 --- a/sql/sql_partition.cc +++ b/sql/sql_partition.cc @@ -7237,7 +7246,8 @@ uint fast_alter_partition_table(THD *thd, TABLE *table, } else if ((alter_info->partition_flags & ALTER_PARTITION_ADD) && (part_info->part_type == RANGE_PARTITION || - part_info->part_type == LIST_PARTITION)) + part_info->part_type == LIST_PARTITION || + alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST))

it'd be good it adding new empty VERSIONING partitions would always go this way, auto or not. but it's a separate task.

Added comment.

...

{ /* ADD RANGE/LIST PARTITIONS @@ -7281,7 +7291,7 @@ uint fast_alter_partition_table(THD *thd, TABLE *table, write_log_add_change_partition(lpt) || ERROR_INJECT_CRASH("crash_add_partition_4") || ERROR_INJECT_ERROR("fail_add_partition_4") || - mysql_change_partitions(lpt) || + mysql_change_partitions(lpt, false) ||

this way you skip trans_commit_stmt/trans_commit_implicit for ALTER TABLE ... ADD RANGE/LIST partition. is it always ok?

A safer alternative would've been

mysql_change_partitions(lpt, !(alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST))

but it's more complex, so I'd prefer your variant, if we can be sure that it doesn't break anything.

The condition is if (copy_data && mysql_trans_commit_alter_copy_data(thd)) Data is not needed to be copied in ADD RANGE/LIST partition, is it? We can be sure only from testing period until GA.

...

ERROR_INJECT_CRASH("crash_add_partition_5") || ERROR_INJECT_ERROR("fail_add_partition_5") || alter_close_table(lpt) ||

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org

-- All the best, Aleksey Midenkov @midenok