May 2021 - developers - lists.mariadb.org

Re: [Maria-developers] 2f9bbf392072: MDEV-18734 optimization by taking ownership via String::swap()
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Aleksey! Don't be confused, the commit header and the comment come from your last commit, but the diff below includes all three commits that mention MDEV-18734. On Nov 02, Aleksey Midenkov wrote: > revision-id: 2f9bbf392072 (mariadb-10.2.31-543-g2f9bbf392072) > parent(s): 1f4960e3f2ac > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-11-02 14:26:04 +0300 > message: > > MDEV-18734 optimization by taking ownership via String::swap() > > ha_partition stores records in array of m_ordered_rec_buffer and uses > it for prio queue in ordered index scan. When the records are restored > from the array the blob buffers may be already freed or > rewritten. This can happen when blob buffers are cached in > Field_blob::value or Field_blob::read_value. > > Previous solution worked via copying blob buffer into > mem_root-allocated buffer. That was not optimal as it requires more > memory and processing time. > > This patch avoids memory copy by taking temporary ownership of cached > blob buffers via String::swap(). It also fixes problem when > value.str_length is wrong after Field_blob::store(). > diff --git a/mysql-test/suite/federated/federated_partition.test b/mysql-test/suite/federated/federated_partition.test > index ef1e27ec5054..33ee025442f4 100644 > --- a/mysql-test/suite/federated/federated_partition.test > +++ b/mysql-test/suite/federated/federated_partition.test > @@ -50,4 +50,30 @@ drop table federated.t1_2; > > --echo End of 5.1 tests > > +--echo # > +--echo # MDEV-18734 ASAN heap-use-after-free upon sorting by blob column from partitioned table > +--echo # > +connection slave; > +use federated; > +create table t1_1 (x int, b text, key(x)); > +create table t1_2 (x int, b text, key(x)); > +connection master; > +--replace_result $SLAVE_MYPORT SLAVE_PORT > +eval create table t1 (x int, b text, key(x)) engine=federated > + partition by range columns (x) ( > + partition p1 values less than (40) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1', > + partition pn values less than (maxvalue) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2' > +); > +insert t1 values (1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6), (7, 7), (8, 8); > +insert t1 select x + 8, x + 8 from t1; > +insert t1 select x + 16, x + 16 from t1; > +insert t1 select x + 49, repeat(x + 49, 100) from t1; > +--echo # This produces wrong result before MDEV-17573 > +select * from t1; What do you mean, simple `select * from t1` produces incorrect result? > +flush tables; > +select x, b from t1 where x > 30 and x < 60 order by b; Is this ASAN-only test? > +drop table t1; > +connection slave; > +drop table t1_1, t1_2; > + > source include/federated_cleanup.inc; > diff --git a/mysql-test/suite/vcol/t/partition.test b/mysql-test/suite/vcol/t/partition.test > index 889724fb1c5c..f10ee0491ccc 100644 > --- a/mysql-test/suite/vcol/t/partition.test > +++ b/mysql-test/suite/vcol/t/partition.test > @@ -30,3 +30,28 @@ subpartition by hash(v) subpartitions 3 ( > insert t1 set i= 0; > set statement sql_mode= '' for update t1 set i= 1, v= 2; > drop table t1; > + > +--echo # > +--echo # MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > +--echo # > +--echo # Cover queue_fix() in ha_partition::handle_ordered_index_scan() > +create table t1 ( > + x int auto_increment primary key, > + b text, v mediumtext as (b) virtual, > + index (v(10)) > +) partition by range columns (x) ( > + partition p1 values less than (3), > + partition p2 values less than (6), > + partition pn values less than (maxvalue)); > +insert into t1 (b) values ('q'), ('a'), ('b'); > +update t1 set b= 'bar' where v > 'a'; > +drop table t1; This test didn't fail for me in an ASAN build on the vanilla 10.5 without your fix. That is, I'm not sure it actually tests the fix. > + > +--echo # Cover return_top_record() in ha_partition::handle_ordered_index_scan() > +create table t1 (x int primary key, b tinytext, v text as (b) virtual) > +partition by range columns (x) ( > + partition p1 values less than (4), > + partition pn values less than (maxvalue)); > +insert into t1 (x, b) values (1, ''), (2, ''), (3, 'a'), (4, 'b'); > +update t1 set b= 'bar' where x > 0 order by v limit 2; This fails without a fix all right. Still, I wonder whether it's possible to create a test that'll fail in a normal optimized build w/o ASAN > +drop table t1; > diff --git a/sql/field.cc b/sql/field.cc > index bdaaecc20269..0fd40c979d2c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -8310,6 +8310,7 @@ int Field_blob::store(const char *from,uint length,CHARSET_INFO *cs) > copy_length= copier.well_formed_copy(field_charset, > (char*) value.ptr(), new_length, > cs, from, length); > + value.length(copy_length); good! Could this be a distinct bug with its own test case? > Field_blob::store_length(copy_length); > bmove(ptr+packlength,(uchar*) &tmp,sizeof(char*)); > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 9d82df0235c7..7c658082d397 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -5125,14 +5128,14 @@ bool ha_partition::init_record_priority_queue() > uint alloc_len; > uint used_parts= bitmap_bits_set(&m_part_info->read_partitions); > /* Allocate record buffer for each used partition. */ > - m_priority_queue_rec_len= m_rec_length + PARTITION_BYTES_IN_POS; > + m_priority_queue_rec_len= m_rec_length + ORDERED_REC_OFFSET; > if (!m_using_extended_keys) > m_priority_queue_rec_len += m_file[0]->ref_length; > alloc_len= used_parts * m_priority_queue_rec_len; > /* Allocate a key for temporary use when setting up the scan. */ > alloc_len+= table_share->max_key_length; > > - if (!(m_ordered_rec_buffer= (uchar*)my_malloc(alloc_len, MYF(MY_WME)))) > + if (!(m_ordered_rec_buffer= (uchar*) alloc_root(&m_ordered_root, alloc_len))) I don't see why you need a memroot here. memroot is needed when you allocate later at difeerent points in time some initially unpredictable amount of memory that has the same lifetime and needs to be freed all at once. But here it seems that you only allocate once. You could just make m_priority_queue_rec_len = m_rec_length + PARTITION_BYTES_IN_POS + table->s->blob_fields * sizeof(Ordered_blob_storage) (plus necessary alignment). > DBUG_RETURN(true); > > /* > @@ -6178,7 +6203,11 @@ int ha_partition::handle_ordered_index_scan(uchar *buf, bool reverse_order) > */ > error= file->read_range_first(m_start_key.key? &m_start_key: NULL, > end_range, eq_range, TRUE); > - memcpy(rec_buf_ptr, table->record[0], m_rec_length); > + if (!error) > + { > + memcpy(rec_buf_ptr, table->record[0], m_rec_length); > + } did you have a bug because of that? something didn't work? > + > reverse_order= FALSE; > break; > } > @@ -6310,6 +6349,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() > } > > > +void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) > +{ > + uint *ptr, *end; > + uint blob_n= 0; > + table->move_fields(table->field, rec_buf, table->record[0]); > + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; > + ptr != end; ++ptr, ++blob_n) > + { > + DBUG_ASSERT(*ptr < table->s->fields); > + Field_blob *blob= (Field_blob*) table->field[*ptr]; > + DBUG_ASSERT(blob->flags & BLOB_FLAG); > + DBUG_ASSERT(blob->field_index == *ptr); > + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) > + continue; > + > + Ordered_blob_storage &s= *storage[blob_n]; > + > + if (restore) > + { > + if (!s.blob.is_empty()) > + blob->swap(s.blob, s.set_read_value); looks somewhat risky, why blob.length == 0 means that you don't need to restore the value? > + } > + else > + { > + bool set_read_value; > + String *cached= blob->cached(set_read_value); > + if (cached) > + { > + cached->swap(s.blob); > + s.set_read_value= set_read_value; is it indeed possible here for a value to be either in `value` or in `read_value` ? When happens what? > + } > + } > + } > + table->move_fields(table->field, table->record[0], rec_buf); > +} > + > + > /* > Common routine to handle index_next with ordered results > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 11

Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 20 Jul '21

20 Jul '21

Hi, Aleksey! Like last time, it's a review of the diff bc04ded2353 22158b3c05a, so not only commit 38a888da0f1. The main thing below - I didn't understand a couple of changes and asked for clarifications. On May 30, Aleksey Midenkov wrote: > revision-id: 38a888da0f1 (mariadb-10.5.2-653-g38a888da0f1) > parent(s): bc04ded2353 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-04-22 23:35:06 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > diff --git a/mysql-test/suite/versioning/r/delete_history.result b/mysql-test/suite/versioning/r/delete_history.result > --- a/mysql-test/suite/versioning/r/delete_history.result > +++ b/mysql-test/suite/versioning/r/delete_history.result > @@ -154,3 +152,21 @@ 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: > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t (a int) with system versioning > +partition by system_time interval 1 hour auto; > +set timestamp= unix_timestamp('2000-01-01 10:00:00'); perhaps, make it only 02:00:00 to avoid any chance that a partition won't be created because of max-auto-create limit. it's possible we put the limit at 10, but 2 is very unlikely. or does max-auto-create limit always result in a warning? > +delete history from t; > +set timestamp= default; > +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 INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO > +PARTITIONS 2 > +drop table t; > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` ( > PARTITION `p5` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `pn` CURRENT ENGINE = DEFAULT_ENGINE) > alter table t1 add partition partitions 8; > +drop table t1; > +# > +# End of 10.5 tests > +# > +# > +# MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > +# > +create or replace table t1 (x int) with system versioning > +partition by system_time limit 1 auto; > 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 LIMIT 1 > -(PARTITION `p2` HISTORY ENGINE = DEFAULT_ENGINE, > - PARTITION `p8` HISTORY ENGINE = DEFAULT_ENGINE, > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 2 > +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; > +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 LIMIT 1 AUTO > +PARTITIONS 2 > +# 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'); > +update t1 set x= x + 1; > +ERROR HY000: Too many partitions (including subpartitions) were defined > +# Partition overflow error and manual fix seems to be a wrong comment? no manual fix here. > +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; > +insert into t1 values (333); > +set timestamp= unix_timestamp('2000-01-01 01:00:00'); > +update t1 set x= x + 1; > +Warnings: > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions > +drop table t1; > +# Partition overflow error and manual fix > +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; > +insert into t1 values (440); > +set timestamp= unix_timestamp('2000-01-01 00:10:00'); > +update t1 set x= x + 1; > +# Check how pruning boundaries work > +explain partitions select * from t1 for system_time as of '2000-01-01 00:59:58'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 p0,pn # NULL NULL NULL NULL # # > +explain partitions select * from t1 for system_time as of '2000-01-01 00:59:59'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 pn # NULL NULL NULL NULL # # Why the first select looks in both partitions and the second one - only in pn? > +explain partitions select * from t1 for system_time as of '2000-01-01 01:00:00'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 pn # NULL NULL NULL NULL # # > +select * from t1 for system_time as of '2000-01-01 00:09:59'; > +x > +440 > +set timestamp= unix_timestamp('2000-01-01 02:00:00'); > +update t1 set x= x + 1; > +Warnings: > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions > +select * from t1 for system_time as of '2000-01-01 01:00:00'; > +x > +select * from t1 partition (p0) order by x; > +x > +440 > +441 > +alter table t1 add partition partitions 3; > +select * from t1 for system_time as of '2000-01-01 01:00:00'; > +x > +441 > +select * from t1 partition (p0) order by x; > +x > +440 would be good to have EXPLAINs after the overflow and after the manual fix. to show how ALTER changes what partition will be pruned > +drop table t1; > +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; > +affected rows: 0 > +insert into t1 values (1); > +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 3600 SECOND STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO > +PARTITIONS 3 > ... > diff --git a/mysql-test/suite/versioning/r/rpl_mix.result b/mysql-test/suite/versioning/r/rpl_mix.result > --- a/mysql-test/suite/versioning/r/rpl_mix.result > +++ b/mysql-test/suite/versioning/r/rpl_mix.result > @@ -8,4 +8,59 @@ DELETE HISTORY FROM t1; > connection slave; > connection master; > drop table t1; > +# > +# MDEV-25347 DML events for auto-partitioned tables are written into binary log twice > +# > +flush binary logs; > +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; better do a+2 the second time. otherwise it's a bit confusing, the bug says "written twice" and you indeed have update a=a+1 written twice :) with clearly different statements the test will prove that every event is written once, and not one is skipped and the other is duplicated > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000002 # Binlog_checkpoint # # master-bin.000002 > +master-bin.000002 # Gtid # # GTID #-#-# > +master-bin.000002 # Query # # use `test`; create table t1 (a int) with system versioning > +partition by system_time limit 1 auto > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # insert into t1 values (1) > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +connection slave; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +connection master; > +drop table t1; > include/rpl_end.inc > diff --git a/mysql-test/suite/versioning/r/rpl_row.result b/mysql-test/suite/versioning/r/rpl_row.result > --- a/mysql-test/suite/versioning/r/rpl_row.result > +++ b/mysql-test/suite/versioning/r/rpl_row.result > @@ -11,4 +11,59 @@ connection slave; > connection master; > drop table t1; > set binlog_row_image= @old_row_image; > +# > +# MDEV-25347 DML events for auto-partitioned tables are written into binary log twice > +# > +flush binary logs; > +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; same. as a general rule it's helpful to use slightly different statement, to be able to map them to binlog events uniquely. a+2, or 1+a, or whatever. > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000002 # Binlog_checkpoint # # master-bin.000002 > +master-bin.000002 # Gtid # # GTID #-#-# > +master-bin.000002 # Query # # use `test`; create table t1 (a int) with system versioning > +partition by system_time limit 1 auto > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # insert into t1 values (1) > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +connection slave; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +connection master; > +drop table t1; > include/rpl_end.inc > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -1068,13 +1078,456 @@ 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 # End of 10.5 tests > +--echo # > + > +--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; > + in the previous version of this patch you had clarifying comments here, like --echo # INSERT, INSERT .. SELECT don't auto-create partitions or --echo # Number of partitions goes from 3 to 5 I kind of miss them now, they were helpful > +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; > +--replace_result $default_engine DEFAULT_ENGINE > +show create table t1; > + > ... > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1607,7 +1607,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); would be good to have a comment before the method that it's only used in vers_set_hist_part(), if (vers_info->limit). because if it's a generic method to return the number of rows in a partition (incl. subpartitions) then it's totally not clear why it modifies read_partitions bitmap. or may be you'd want to rename the method to make it look more specific for LIMIT partitions in vers_set_hist_part() ? > 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 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1574,6 +1574,8 @@ int ha_commit_trans(THD *thd, bool all) > DBUG_ASSERT(thd->transaction->stmt.ha_list == NULL || > trans == &thd->transaction->stmt); > > + DBUG_ASSERT(!thd->in_sub_stmt); > if (thd->in_sub_stmt) > { > DBUG_ASSERT(0); This is a bit redundant, don't you think? One DBUG_ASSERT would be enough. (personally, I'd prefer the one you added, not DBUG_ASSERT(0)) > diff --git a/sql/lock.cc b/sql/lock.cc > --- a/sql/lock.cc > +++ b/sql/lock.cc > @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b) > DBUG_PRINT("enter", ("a->lock_count: %u b->lock_count: %u", > a->lock_count, b->lock_count)); > > - if (!(sql_lock= (MYSQL_LOCK*) > - my_malloc(key_memory_MYSQL_LOCK, sizeof(*sql_lock) + > - sizeof(THR_LOCK_DATA*)*((a->lock_count+b->lock_count)*2) + > - sizeof(TABLE*)*(a->table_count+b->table_count),MYF(MY_WME)))) > - DBUG_RETURN(0); // Fatal error > + const size_t lock_size= sizeof(*sql_lock) + > + sizeof(THR_LOCK_DATA *) * ((a->lock_count + b->lock_count) * 2) + > + sizeof(TABLE *) * (a->table_count + b->table_count); > + if (thd) > + { > + sql_lock= (MYSQL_LOCK *) thd->alloc(lock_size); > + if (!sql_lock) > + DBUG_RETURN(0); > + sql_lock->flags= GET_LOCK_ON_THD; why? the commit comment for MDEV-23639 doesn't mention it. generally, I think it's a good idea and, likely, should be used more than just in recover_from_failed_open(). But why right now and in this commit? Does this commit (22158b3c) rely on locks being on THD memroot? How so? > + } > + else > + { > + sql_lock= (MYSQL_LOCK *) > + my_malloc(key_memory_MYSQL_LOCK, lock_size, MYF(MY_WME)); > + if (!sql_lock) > + DBUG_RETURN(0); > + sql_lock->flags= 0; > + } > sql_lock->lock_count=a->lock_count+b->lock_count; > sql_lock->table_count=a->table_count+b->table_count; > sql_lock->locks=(THR_LOCK_DATA**) (sql_lock+1); > sql_lock->table=(TABLE**) (sql_lock->locks+sql_lock->lock_count*2); > - sql_lock->flags= 0; > memcpy(sql_lock->locks,a->locks,a->lock_count*sizeof(*a->locks)); > memcpy(sql_lock->locks+a->lock_count,b->locks, > b->lock_count*sizeof(*b->locks)); > @@ -705,8 +717,10 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b) > a->lock_count, b->lock_count); > > /* Delete old, not needed locks */ > - my_free(a); > - my_free(b); > + if (!(a->flags & GET_LOCK_ON_THD)) > + my_free(a); > + if (!(b->flags & GET_LOCK_ON_THD)) > + my_free(b); this even looks like a bug, no check that old locks weren't on THD memroot. (probably impossible in this particular case, but still rather fragile) thanks! > DBUG_RETURN(sql_lock); > } > > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7985,3 +7985,5 @@ ER_JSON_TABLE_MULTIPLE_MATCHES > eng "Can't store multiple matches of the path in the column '%s' of JSON_TABLE '%s'." > ER_WITH_TIES_NEEDS_ORDER > eng "FETCH ... WITH TIES requires ORDER BY clause to be present" > +ER_VERS_HIST_PART_FAILED > + eng "Versioned table %`s.%`s: adding HISTORY partition failed" Can you come up with a test for this error? > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -931,7 +934,7 @@ void close_thread_table(THD *thd, TABLE **table_ptr) > DBUG_PRINT("tcache", ("table: '%s'.'%s' %p", table->s->db.str, > table->s->table_name.str, table)); > DBUG_ASSERT(!table->file->keyread_enabled()); > - DBUG_ASSERT(!table->file || table->file->inited == handler::NONE); > + DBUG_ASSERT(table->file->inited == handler::NONE); indeed > > /* > The metadata lock must be released after giving back > @@ -1625,6 +1625,125 @@ bool is_locked_view(THD *thd, TABLE_LIST *t) > } > > > +#ifdef WITH_PARTITION_STORAGE_ENGINE > +/** > + Switch part_info->hist_part and request partition creation if needed. > + > + @retval true Error or partition creation was requested. > + @retval false No error > +*/ > +bool TABLE::vers_switch_partition(THD *thd, TABLE_LIST *table_list, > + Open_table_context *ot_ctx) > +{ > + 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) > + { > + return false; > + } > + > + switch (thd->lex->sql_command) > + { > + case SQLCOM_INSERT: > + if (thd->lex->duplicates != DUP_UPDATE) > + return false; > + break; > + case SQLCOM_LOAD: > + if (thd->lex->duplicates != DUP_REPLACE) > + return false; > + break; > + 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: > + break; > + default: > + /* > + TODO: make row events set thd->lex->sql_command appropriately. > + > + Sergei Golubchik: f.ex. 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. > + */ > + if (thd->rgi_slave && thd->rgi_slave->current_event && > + thd->lex->sql_command == SQLCOM_END) > + { > + 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: > + break; > + default:; > + return false; > + } > + } > + break; > + } > + > + TABLE *table= this; > + > + /* > + NOTE: The semantics of vers_set_hist_part() is double: even when we twofold > + don't need auto-create, we need to update part_info->hist_part. > + */ > + uint *create_count= table_list->vers_skip_create ? > + NULL : &ot_ctx->vers_create_count; > + table_list->vers_skip_create= true; > + if (table->part_info->vers_set_hist_part(thd, create_count)) > + { > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); > + return true; > + } > + if (ot_ctx->vers_create_count) > + { > + Open_table_context::enum_open_table_action action; > + TABLE_LIST *table_arg; > + mysql_mutex_lock(&table->s->LOCK_share); > + if (!table->s->vers_skip_auto_create) > + { > + table->s->vers_skip_auto_create= true; > + action= Open_table_context::OT_ADD_HISTORY_PARTITION; > + table_arg= table_list; > + } > + else > + { > + /* > + NOTE: this may repeat multiple times until creating thread acquires > + MDL_EXCLUSIVE. Since auto-creation is rare operation this is acceptable. > + We could suspend this thread on cond-var but we must first exit > + MDL_SHARED_WRITE first and we cannot store cond-var into TABLE_SHARE > + because it is already released and there is no guarantee that it will > + be same instance if we acquire it again. > + */ > + table_list->vers_skip_create= false; > + ot_ctx->vers_create_count= 0; > + action= Open_table_context::OT_REOPEN_TABLES; > + table_arg= NULL; > + } I'm afraid I don't understand. All this business with vers_skip_create and vers_skip_auto_create, it wasn't in the previous version of the patch. So, I believe it was a fix for one of the MDEV bugs reported and fixed meanwhile. What MDEV was it? Could you explain what was the issue and what was the fix, please? > + mysql_mutex_unlock(&table->s->LOCK_share); > + if (!thd->locked_tables_mode) > + { > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); > + } > + ot_ctx->request_backoff_action(action, table_arg); > + return true; > + } > + > + return false; > +} > +#endif /* WITH_PARTITION_STORAGE_ENGINE */ > + > + > /** > Open a base table. > > @@ -2572,11 +2699,13 @@ void Locked_tables_list::mark_table_for_reopen(THD *thd, TABLE *table) > table_list; table_list= table_list->next_global) > { > if (table_list->table->s == share) > + { > table_list->table->internal_set_needs_reopen(true); > + some_table_marked_for_reopen= 1; > + } > } > /* This is needed in the case where lock tables where not used */ > table->internal_set_needs_reopen(true); why ^^^ this doesn't count as "some_table_marked_for_reopen" ? > - some_table_marked_for_reopen= 1; > } > > > @@ -3172,13 +3304,49 @@ 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: > + if (!m_thd->locked_tables_mode) > + result= lock_table_names(m_thd, m_thd->lex->create_info, m_failed_table, > + NULL, get_timeout(), 0); > + else > + { > + DBUG_ASSERT(!result); > + DBUG_ASSERT(m_action == OT_ADD_HISTORY_PARTITION); It seems you've solved recover_from_failed_open under LOCK TABLES, so it could be possible to make OT_DISCOVER and OT_REPAIR to work there too. Is that right? It's just a question, I don't imply it should be part of this MDEV. > + } > + /* > + We are now under MDL_EXCLUSIVE mode. Other threads have no table share > + acquired: they are blocked either at open_table_get_mdl_lock() in > + open_table() or at lock_table_names() here. > + */ > + if (result) > + { > + if (m_action == OT_ADD_HISTORY_PARTITION) > + { > + TABLE_SHARE *share= tdc_acquire_share(m_thd, m_failed_table, > + GTS_TABLE, NULL); > + if (share) > + { > + share->vers_skip_auto_create= false; > + tdc_release_share(share); > + } > + if (m_thd->get_stmt_da()->sql_errno() == ER_LOCK_WAIT_TIMEOUT) > + { > + // MDEV-23642 Locking timeout caused by auto-creation affects original DML > + m_thd->clear_error(); > + vers_create_count= 0; > + result= false; > + } > + } > break; > + } > > - tdc_remove_table(m_thd, m_failed_table->db.str, > - m_failed_table->table_name.str); > + /* > + We don't need to remove share under OT_ADD_HISTORY_PARTITION. > + Moreover fast_alter_partition_table() works with TABLE instance. > + */ > + if (m_action != OT_ADD_HISTORY_PARTITION) > + tdc_remove_table(m_thd, m_failed_table->db.str, > + m_failed_table->table_name.str); > > switch (m_action) > { > @@ -3206,6 +3374,70 @@ Open_table_context::recover_from_failed_open() > case OT_REPAIR: > result= auto_repair_table(m_thd, m_failed_table); > break; > + case OT_ADD_HISTORY_PARTITION: > + { > + result= false; > + TABLE *table= open_ltable(m_thd, m_failed_table, TL_WRITE, > + MYSQL_OPEN_HAS_MDL_LOCK | MYSQL_OPEN_IGNORE_LOGGING_FORMAT); > + if (table == NULL) > + { > + m_thd->clear_error(); > + break; > + } > + > + DBUG_ASSERT(vers_create_count); > + result= vers_create_partitions(m_thd, m_failed_table, vers_create_count); > + vers_create_count= 0; > + if (!m_thd->transaction->stmt.is_empty()) > + trans_commit_stmt(m_thd); > + DBUG_ASSERT(!result || > + !m_thd->locked_tables_mode || > + m_thd->lock->lock_count); > + if (result) > + break; > + if (!m_thd->locked_tables_mode) > + { > + /* > + alter_partition_lock_handling() does mysql_lock_remove() but > + does not clear thd->lock completely. > + */ > + DBUG_ASSERT(m_thd->lock->lock_count == 0); > + if (!(m_thd->lock->flags & GET_LOCK_ON_THD)) > + my_free(m_thd->lock); > + m_thd->lock= NULL; > + } > + else if (m_thd->locked_tables_mode == LTM_PRELOCKED) > + { > + MYSQL_LOCK *lock; > + MYSQL_LOCK *merged_lock; > + > + /* > + In LTM_LOCK_TABLES table was reopened via locked_tables_list, > + but not in prelocked environment where we have to reopen > + the table manually. > + */ > + Open_table_context ot_ctx(m_thd, MYSQL_OPEN_REOPEN); > + if (open_table(m_thd, m_failed_table, &ot_ctx)) > + { > + result= true; > + break; > + } > + TABLE *table= m_failed_table->table; > + table->reginfo.lock_type= m_thd->update_lock_default; > + m_thd->in_lock_tables= 1; > + lock= mysql_lock_tables(m_thd, &table, 1, > + MYSQL_OPEN_REOPEN | MYSQL_LOCK_USE_MALLOC); > + m_thd->in_lock_tables= 0; > + if (lock == NULL || > + !(merged_lock= mysql_lock_merge(m_thd->lock, lock, m_thd))) > + { > + result= true; > + break; > + } > + m_thd->lock= merged_lock; What MDEV does it fix? I wonder why other cases in recover_from_failed_open doesn't do that > + } > + break; > + } > case OT_BACKOFF_AND_RETRY: > case OT_REOPEN_TABLES: > case OT_NO_ACTION: > @@ -4370,6 +4611,8 @@ bool open_tables(THD *thd, const DDL_options_st &options, > { > if (ot_ctx.can_recover_from_failed_open()) > { > + // FIXME: is this really used? > + DBUG_ASSERT(0); I take it, you weren't able to get this assert to fire? > close_tables_for_reopen(thd, start, > ot_ctx.start_of_statement_svp()); > if (ot_ctx.recover_from_failed_open()) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 13

[Maria-developers] JSON_TABLE: on default values
by Sergey Petrunia 16 Jun '21

16 Jun '21

Hi Alexey, At the moment MariaDB requires that the values in DEFAULT clauses are quoted. Example: select * from json_table( '{"intval": 1000}', '$' columns( col1 int path '$.intval_' default '100' on empty ) ) as T; here, "100" must be quoted, otherwise one gets a parse error. However, the quoted value is interpreted as an SQL literal. This looks puzzling. MySQL-8 also requires that the default value is quoted, but they have a (very odd) reason for it: they interpret the default value as JSON: https://docs.oracle.com/cd/E17952_01/mysql-8.0-en/json-table-functions.html says: DEFAULT json_string ON EMPTY: the provided json_string is parsed as JSON, as long as it is valid, and stored instead of the missing value. Column type rules also apply to the default value. I am not sure why MySQL chose to do this. Looking into the SQL Standard, one can see: <JSON table regular column definition> ::= <column name> <data type> [ PATH <JSON table column path specification> ] [ <JSON table column empty behavior> ON EMPTY ] [ <JSON table column error behavior> ON ERROR ] <JSON table column empty behavior> ::= ERROR | NULL | DEFAULT <value expression> ... This doesn't say whether the <value expression> should be interepreted as JSON or just as a value. But one can find this passage: <quote> Without Feature T826, “General value expression in ON ERROR or ON EMPTY clauses”, the <value expression> contained in <JSON table column empty behavior> or <JSON table column error behavior> contained in a <JSON table regular column definition> JTRCD shall be a <literal> that can be cast to the data type specified by the <data type> contained in JTRCD without raising an exception condition according to the General Rules of Subclause 6.13, “<cast specification>”. </quote> The important part is: ... shall be a <literal> that can be cast to the data type specified ... which means it is not JSON. It is just a literal, and literal can be a string literal (in quotes, 'string') or an integer literal (without quotes) or other kind of literal. Btw, Oracle Database allows non-string literals in the default clause: https://dbfiddle.uk/?rdbms=oracle_18&fiddle=9af7e43ede77ee285e1a65f1f419d3bd What are your thoughts on this? Is MariaDB's behavior intentional? Should we follow the standard and allow all kinds of literals? What was the reason for the limitation that default values are quoted? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 4

Re: [Maria-developers] 27060eb6ba5: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 10 Jun '21

10 Jun '21

Hi, Oleksandr! On Oct 12, Oleksandr Byelkin wrote: > revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5) > parent(s): 861cd4ce286 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-10-07 15:22:41 +0200 > message: > > MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values > > To allocate new net buffer to avoid changing bufer we are reading. You still didn't clarify the commit comment > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index 7280236e43f..5aaff3cf623 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -685,8 +685,14 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > !table->prepare_triggers_for_delete_stmt_or_event()) > will_batch= !table->file->start_bulk_delete(); > > - if (returning) > + /* > + thd->get_stmt_da()->is_set() means first iteration of prepared statement > + with array binding operation execution (non optimized so it is not > + INSERT) > + */ > + if (returning && !thd->get_stmt_da()->is_set()) > { > + DBUG_ASSERT(thd->lex->sql_command != SQLCOM_INSERT); a strange assert to see in sql_delete.cc :) can one even reach mysql_delete() with SQLCOM_INSERT? not just with returning and !thd->get_stmt_da()->is_set(), anywhere? > if (result->send_result_set_metadata(returning->item_list, > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > goto cleanup; > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > index b3ef0d89a98..a753af2b34d 100644 > --- a/sql/sql_error.cc > +++ b/sql/sql_error.cc > @@ -380,16 +380,33 @@ Diagnostics_area::set_eof_status(THD *thd) > if (unlikely(is_error() || is_disabled())) > return; > > + if (m_status == DA_EOF_BULK) > + { > /* > If inside a stored procedure, do not return the total > number of warnings, since they are not available to the client > anyway. > */ > + if (!thd->spcont) > + m_statement_warn_count+= current_statement_warn_count(); > + } > + else > + { > + /* > + If inside a stored procedure, do not return the total > + number of warnings, since they are not available to the client > + anyway. > + */ I don't think it helps to duplicate the comment. You could just put it once before the if. > if (thd->spcont) > { > m_statement_warn_count= 0; > + m_affected_rows= 0; why do you reset m_affected_rows too? > } > else > m_statement_warn_count= current_statement_warn_count(); > + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); > + } do we have tests for bulk operations and CALL? > > - m_status= DA_EOF; > DBUG_VOID_RETURN; > } > > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index ecb56e70f88..c144d3a8d7e 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4357,24 +4361,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, > > + /* > + Here second buffer for not optimized commands, > + optimized commands do it inside thier internal loop. > + */ > + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && why "SP" and not "PS" ? > + this->lex->has_returning()) what about CALL? It won't have lex->has_returning(). What about SELECT? Can you bind an array to a parameter in SELECT or CALL? > + { > + // Above check can be true for SELECT in future > + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); how can SQLCOM_SELECT have lex->has_returning() ? > + readbuff= thd->net.buff; // old buffer > + if (net_allocate_new_packet(&thd->net, thd, MYF(MY_THREAD_SPECIFIC))) > + { > + readbuff= NULL; // failure, net_allocate_new_packet keeps old buffer > + goto err; > + } > } > > #ifndef EMBEDDED_LIBRARY Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] ef2519fee4e: MDEV-16546 System versioning setting to allow history modification
by Aleksey Midenkov 08 Jun '21

08 Jun '21

Hello, Sergei! On Fri, May 3, 2019 at 8:43 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On May 03, Aleksey Midenkov wrote: > > revision-id: ef2519fee4e (versioning-1.0.5-17-gef2519fee4e) > > parent(s): 56145be2951 > > author: Aleksey Midenkov <midenok(a)gmail.com> > > committer: Aleksey Midenkov <midenok(a)gmail.com> > > timestamp: 2018-06-28 13:42:09 +0300 > > message: > > > > MDEV-16546 System versioning setting to allow history modification > > > > 1. Add server variable system_versioning_modify_history which will > > allow to set values for row_start, row_end in DML operations. > > > > 2. If secure_timestamp is YES or REPLICATION, > > system_versioning_modify_history does not have effect. If > > secure_timestamp is SUPER, system_versioning_modify_history requires > > special privilege (same as for setting current timestamp). > > I thought more about this idea. We don't really want to have the history > editable, do we? Well, I'm thinking about rollback table data to specific point in time. That could be a useful feature. > But it's needed for replication, to keep the master and > slave identical. That's what secure_timestamp is for. > > The idea was that this new variable, system_versioning_modify_history, > will be just a convenience feature, it will not allow history editing > any more than one can do without it. > > But now I suspect that even with secure_timestamp=NO one cannot truly > edit history. One can only insert new rows with arbitrary timestamps. > For example, to insert a row with row_start=1000 and row_end=2000, one > needs to do (if secure_timestamp=NO): > > set timestamp=1000; > insert row; > set timestamp=2000; > delete row; > > But I don't see how one can update or delete a history row with > secure_timestamp=NO. > > Now, with a SUPER privilege and secure_timestamp=NO or SUPER, one can > use the BINLOG command and truly edit the history arbitrarily, by faking > row events. I don't really get it why this is so important: since there is some limitation by configuration and privilege, we are just fine. Everything can be changed at filesystem level after all. > > The conclusion, I believe, is that system_versioning_modify_history > should allow INSERTs when secure_timestamp=NO, and it should allow > UPDATE/DELETE only for a SUPER user when secure_timestamp=NO or SUPER. I don't see a reason to argue on that. The only thing that is not clear, why we don't allow INSERTs when secure_timestamp=SUPER? > > The second thing I don't like at all, is when a table is created like > > CREATE TABLE t1 (a int) WITH SYSTEM VERSIONING > > with row_start/row_end implicit. You don't have it in the test, but > anyway one should be able to load history into such a table, while the > table does not have row_start and row_end columns. From the user point > of view these columns don't exist, they're pseudo-columns, like ROWID. > They just cannot be insertable-into, conceptually. But a user will want > to restore the history, right? I don't have a solution for this yet :( > Any ideas? We don't have to follow the conception if it doesn't help us. Since we have physical row_start/row_end, we don't have to pretend they don't exist. Who will win from that? > > See below a couple of minor comments about the patch itself. > > ... These are going to be fixed. > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 15

Re: [Maria-developers] e92037989f7: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 31 May '21

31 May '21

Hi, Andrei! On May 27, Andrei Elkin wrote: >>> >>> I had a rather sobering followup, slipped communicating to you >>> though, sorry. >>> >>> It's actually turns out not to be easy. A sequence of execution events >>> >>> E1.prepare(trx), E2.prepare(trx), E1.commit(trx), *crash* >>> >>> does not make E1 such as Innodb committed persistently in file. At >>> recovery in E1 trx may be found in prepared state, unless before the >>> crash I'd do something like Binlog CheckPoint (BCP) notification request >>> and wait of it. >> >> I don't understand, what a binlog checkpoint has to do with it? > > So E1=Innodb. > In the above case in order to create partially committed trx > Innodb.commit(trx) > must be followed with flushing trx to disk, which is typically > asynchronous. > > I mentioned BCP 'cos it ensures a trx (actually the last in a binlog > file) is flushed. With --innodb-flush-log-at-trx-commit=1 it should be perfectly synnchronous, should it not? > >> In this just a selected engine would be requested, unlike in BCP > >> event case that is generated when both (in this case) have > >> persistently committed. (That is we even can't involve BCP event to > >> wait for, and the involvement would require some simulation as > >> well). > >> > >> So I propose to keep the current method leaving the "honest" crash > >> to RQG that Alice has been training the patch with. > > > > Alice was loading the server with transactions and doing `kill -9` > > at some point. Many interesting cases that we discuss (for example, > > the one above) happen in a very small time window, so I don't know > > whether random killing could provide an adequate test coverage. > > No doubt it's technically possible to wait for the flushing and crash, > but still does not look easy. > Consider if we defer this sort of mtr test to MDEV-18959? But we've spent numerous hours and emails discussing and you've spend countless days and weeks implementing various tricky corner cases are are and won't be ever tested. Here's an idea of how to test it. We're only interested in crashes during commit. So * when starting a commit - atomically increment some in_commit counter * on leaving the commit - decrement it * on signal (some unused one) - crash the server if in_commit > 0 * after the crash - get the stack traces from the core to see where in the commit did it crash. I've attached a small patch to illustrate this idea. Hopefully, it'll cover everything, but if not, then after analyzing stack traces we'll see where it did not crash, and move atomic increment/decrements to be around this area only. > >> >> diff --git a/sql/handler.h b/sql/handler.h > >> >> --- a/sql/handler.h > >> >> +++ b/sql/handler.h > >> >> @@ -873,6 +874,15 @@ typedef struct xid_t XID; > >> >> /* The 'buf' has to have space for at least SQL_XIDSIZE bytes. */ > >> >> uint get_sql_xid(XID *xid, char *buf); > >> >> > >> >> +/* struct for semisync slave binlog truncate recovery */ > >> >> +struct xid_recovery_member > >> >> +{ > >> >> + my_xid xid; > >> >> + uint in_engine_prepare; // number of engines that have xid prepared > >> >> + bool decided_to_commit; > >> >> + std::pair<uint, my_off_t> binlog_coord; // semisync recovery binlog offset > >> > > >> > wouldn't it be clearer to have a struct with named members? > >> > >> I am not sure if there's a need for naming. But we do need > >> comparison that std::pair provides for granted. > >> > >> I could consider (a) new typedef(s) to represent the binlog id and > >> offset in the file to make it more readable. > >> > >> You say? > >> > >> > in fact, I'm somewhat surprised there's no such struct for binlog coords > >> > already. > >> > >> I had started that back at HB implementation actually, but that struct > >> did not attact much of attention. > > > > > I'd still suggest a structure, like > > > > struct binlog_coords_t { > > uint binlog_num; > > my_off_t offset; > > }; > > Well, with this, like I said about std:pair, we'd also have to have > bool operator < (const binlog_coords_t) like bool operator <(const binlog_coords_t c) { return binlog_num < c.binlog_num || binlog_num == c.binlog_num && offset < c.offset; } > I seriously rate refuse to reuse as a deadly sin of programming. > (Unlike normal sins it may be soften by realization though :-)). > > Let's waive it away, right :-)? > > Hoping on that `72a59e4b068` already defined the following: ... > +/* > + Compound binlog-id and byte offset of transaction's first event > + in a sequence (e.g the recovery sequence) of binlog files. > + Binlog_offset(0,0) is the minimum value to mean > + the first byte of the first binlog file. > +*/ > +typedef std::pair<Binlog_file_id, my_off_t> Binlog_offset; This is better than writing std::pair<uint, my_off_t> everywhere. Not as clear as a binlog_coords_t from above, but _perhaps_ it'll do, let's see how it'll be used. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Using JSON_TABLE to create JSON columns?
by Sergey Petrunia 31 May '21

31 May '21

Hi Alexey and everyone, I was looking at whether it's possible use JSON_TABLE to extract portions of JSON document. Apparently it is possible in MySQL with JSON datatype: Q1: select * from json_table('[{"color": "blue", "price": { "high": 10, "low": 5}}, {"color": "red", "price": { "high": 20, "low": 8}}]', '$[*]' columns(color varchar(100) path '$.color', price json path '$.price' ) ) as T; +-------+------------------------+ | color | price | +-------+------------------------+ | blue | {"low": 5, "high": 10} | | red | {"low": 8, "high": 20} | +-------+------------------------+ Note that if one uses any datatype other than JSON, they get NULLs: Q2: select * from json_table('[{"color": "blue", "price": { "high": 10, "low": 5}}, {"color": "red", "price": { "high": 20, "low": 8}}]', '$[*]' columns(color varchar(100) path '$.color', price text path '$.price' ) ) as T; +-------+-------+ | color | price | +-------+-------+ | blue | NULL | | red | NULL | +-------+-------+ Oracle-the-database doesn't yet(*) have a JSON datatype. So I can only run Q2 and then I get NULLs in the price column. MariaDB accepts JSON as datatype so query Q1 is accepted. However the logic in MDEV-17399 code doesn't have support for dumping a portion of JSON document, so one gets empty strings in the price column. Should we support Q1 with JSON output in the price column? If yes, should we do it within the scope of MDEV-17399 or create another task for this? (*) I see this: https://docs.oracle.com/en/database/oracle/oracle-database/20/newft/new-jso… BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

3 2

[Maria-developers] GSoC Student Introduction for MDEV-24676
by Владислав Какурин 31 May '21

31 May '21

Hello, my name is Vladislav Kakurin, and this year I'm a GSOC 2021 participant for MariaDB. I will work on concurrent ring-buffer for improvement of IO_CACHE (https://jira.mariadb.org/browse/MDEV-24676) I hope I will succeed in this matter, as well as become a part of the MariaDB community

1 0

[Maria-developers] GSoC Student Introduction for MDEV-21130
by Michael Okoko 27 May '21

27 May '21

Hi, I'm Michael Okoko, a GSOC 2021 mentee for MariaDB and I'd be working on switching the on-disk storage format for MariaDB histograms to JSON ( https://jira.mariadb.org/browse/MDEV-21130). I'm looking forward to working with everyone in the community and contributing more to the MariaDB during and after GSOC.

1 0

Re: [Maria-developers] e92037989f7: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 26 May '21

26 May '21

Hi, Andrei! On Apr 27, Andrei Elkin wrote: > >> +--let $case = B: one engine has committed its transaction branch > >> +# Hold off after one engine has committed. > >> +--let $shutdown_timeout=0 > >> +--let $debug_sync_action = "commit_after_run_commit_ordered SIGNAL con1_ready WAIT_FOR signal_no_signal" > >> +# Both debug_sync and debug-dbug are required to make sure Engines remember the commit state > >> +# debug_sync alone will not help. > >> +--let $restart_parameters = --rpl-semi-sync-slave-enabled=1 --debug-dbug=d,binlog_truncate_partial_commit > > > > in the first review I wrote > > > > this seems to be a rather crude way of faking a partially committed > > transaction. better to crash after the first engine has committed, > > that'd be much more natural. > > > > and you replied > > > > This simulation aimed at (allows for) more complicated recovery time > > event sequences. > > In this case, indeed, crashing by demand is about of the same efforts. > > I can convert to that. > > > > [x] > > I had a rather sobering followup, slipped communicating to you though, sorry. > > It's actually turns out not to be easy. A sequence of execution events > > E1.prepare(trx), E2.prepare(trx), E1.commit(trx), *crash* > > does not make E1 such as Innodb committed persistently in file. At > recovery in E1 trx may be found in prepared state, unless before the > crash I'd do something like Binlog CheckPoint (BCP) notification request > and wait of it. I don't understand, what a binlog checkpoint has to do with it? > In this just a selected engine would be requested, unlike in BCP event > case that is generated when both (in this case) have persistently committed. > (That is we even can't involve BCP event to wait for, and the involvement > would require some simulation as well). > > So I propose to keep the current method leaving the "honest" crash to > RQG that Alice has been training the patch with. Alice was loading the server with transactions and doing `kill -9` at some point. Many interesting cases that we discuss (for example, the one above) happen in a very small time window, so I don't know whether random killing could provide an adequate test coverage. > >> diff --git a/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test > >> --- /dev/null > >> +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_fail_over.test > >> @@ -0,0 +1,143 @@ > >> + > >> +--source include/have_innodb.inc > >> +--source include/have_debug_sync.inc > >> +--source include/have_binlog_format_row.inc > >> +--let $rpl_topology=1->2 > >> +--source include/rpl_init.inc > > > > why not to source master-slave.inc if you're using a standard master-slave > > topology anyway? > > Using numbers it's easier to manipulate with failover operatations. > Besides it's truly uncomfortable both read and write > when `master` would be serving the slave role as it happens in the test :-). I believe I already replied to that earlier. master-slave.inc creates server_1 and server_2 connections all right. > >> diff --git a/sql/handler.h b/sql/handler.h > >> --- a/sql/handler.h > >> +++ b/sql/handler.h > >> @@ -873,6 +874,15 @@ typedef struct xid_t XID; > >> /* The 'buf' has to have space for at least SQL_XIDSIZE bytes. */ > >> uint get_sql_xid(XID *xid, char *buf); > >> > >> +/* struct for semisync slave binlog truncate recovery */ > >> +struct xid_recovery_member > >> +{ > >> + my_xid xid; > >> + uint in_engine_prepare; // number of engines that have xid prepared > >> + bool decided_to_commit; > >> + std::pair<uint, my_off_t> binlog_coord; // semisync recovery binlog offset > > > > wouldn't it be clearer to have a struct with named members? > > I am not sure if there's a need for naming. But we do need > comparison that std::pair provides for granted. > > I could consider (a) new typedef(s) to represent the binlog id and > offset in the file to make it more readable. > > You say? > > > in fact, I'm somewhat surprised there's no such struct for binlog coords > > already. > > I had started that back at HB implementation actually, but that struct > did not attact much of attention. I'd still suggest a structure, like struct binlog_coords_t { uint binlog_num; my_off_t offset; }; > > > >> +}; > >> + > >> /* for recover() handlerton call */ > >> #define MIN_XID_LIST_SIZE 128 > >> #define MAX_XID_LIST_SIZE (1024*128) > >> @@ -4820,7 +4830,8 @@ int ha_commit_one_phase(THD *thd, bool all); > >> int ha_commit_trans(THD *thd, bool all); > >> int ha_rollback_trans(THD *thd, bool all); > >> int ha_prepare(THD *thd); > >> -int ha_recover(HASH *commit_list); > >> +int ha_recover(HASH *commit_list, MEM_ROOT *mem_root= NULL); > >> +uint ha_recover_complete(HASH *commit_list, std::pair<uint, my_off_t> *coord= NULL); > > > > is coord a truncation position? > > So NULL is for the normal recovery. > Non-NULL is an estimate of the 1st to rollback. > Trx with lesser offsets are to commit, greater of equal - to rollback. > > Why it's only an estimate is 'cos the function also handles > no-truncation cases, it then passes as `coord` the last committed > trx's coordinate. > The last committed can't be decided to rollback because of higher precedence of > > xid_recovery_member::decided_to_commit > > which is `true` under current circumstance, in > > +static bool xarecover_decide(xid_recovery_member* member, > + xid_t x, std::pair<uint, my_off_t> *coord_ptr) > +{ > + return > + member->decided_to_commit ? true : > + !coord_ptr ? false : > + (member->binlog_coord < *coord_ptr > > I did not reflect the subtlety of the above in function header comments. > To be done, if you're sastified here. yes, please, reflect it. thanks :) > >> diff --git a/sql/log_event.h b/sql/log_event.h > >> --- a/sql/log_event.h > >> +++ b/sql/log_event.h > >> @@ -482,6 +482,16 @@ class String; > >> */ > >> #define LOG_EVENT_IGNORABLE_F 0x80 > >> > >> +/** > >> + @def LOG_EVENT_ACCEPT_OWN_F > >> + > >> + Flag sets by the semisync slave for accepting > >> + the same server_id ("own") events which the slave must not have > >> + in its state. Typically such events were never committed by > >> + their originator (this server) and discared at its semisync-slave recovery. > >> +*/ > >> +#define LOG_EVENT_ACCEPT_OWN_F 0x4000 > > > > may be, add an assert on all received events that such a flag is not set? > > it can only be set on events in relay log. > > There's no place the flag gets logged. It'd be way too fool-proof in my > opinion :-)! > Or if you have a case I'll do. too fool-proof is when you do a=1; assert(a == 1); if you write a binlog event to disk, another thread reads it and sends over the network to another server, which also writes it to disk first, then another thread reads it again and executes - I don't think it's "too" to check that the event doesn't have this flag. Also, I personally value a lot a documentation role of asserts. Assert, like DBUG_ASSERT(ev->flags & LOG_EVENT_ACCEPT_OWN_F == 0); is basically saying /* LOG_EVENT_ACCEPT_OWN_F must be not set here */ but it's a self-fulfilling comment, it documents the code and at the same time guarantees that the documentation is up-to-date. > >> @@ -3357,6 +3367,12 @@ class Gtid_log_event: public Log_event > >> uint64 commit_id; > >> uint32 domain_id; > >> uchar flags2; > >> + uint flags_extra; // more flags area placed after the regular flags2's one > >> + /* > >> + Extra to a "base" engine recoverable engines participating > >> + in the transaction. Zero, when the base engine only is present. > > > > what's a "base engine"? > > The 1st ha_info one that is not binlog. why not to say "number of participating engines minus one" ? this won't puzzle anyone with a new concept of a "base engine" > > > >> + */ > >> + uint8 extra_engines; > >> > >> /* Flags2. */ > >> > >> diff --git a/sql/handler.cc b/sql/handler.cc > >> --- a/sql/handler.cc > >> +++ b/sql/handler.cc > >> @@ -1637,9 +1672,17 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans) > >> DEBUG_SYNC(thd, "commit_one_phase_2"); > >> if (ha_info) > >> { > >> + int err; > >> + > > >> + if (has_binlog_hton(ha_info) && > > > > can you replace has_binlog_hton() with, like, if trx cache is not empty or > > binlog enabled or something like that? > > I see you're driving at removing binlog ha_info from the list, are not > you? I'm not sure :) Yes, I'd like to get rid of binlog_hton, as we discussed. But here, perhaps, I just didn't like that you scan the list for something that most likely could be checked much easier. > I already made some experiments to have observed various complications > in the way of implementing an equivalent of > has_binlog_hton(). > The closest would be > non-empty cache. > > But `binlog_commit` may need invoking even in this case, as its branch > shows > > if (cache_mngr->trx_cache.empty()) > { > /* > we're here because cache_log was flushed in MYSQL_BIN_LOG::log_xid() > */ > cache_mngr->reset(false, true); > > > Overall, I'd stay with the current approach to defer its refinement to > time binlog_hton stops registering for transaction. > > Your comment? Okay. I just don't like scanning the list, aesthetically, when one can check a value in some fixed location or a variable. But it's not a performance concern, the list has usually only two elements and hardly ever can have more than three. So okay, let's stay with the current approach. > > @@ -1962,8 +2008,177 @@ struct xarecover_st > > >> sql_print_warning("%s transaction with xid %llu", > > > > may be not a sql_print_warning, but a sql_print_information? > > it's just an informational message > > [->] I presume, it means [x] > >> + member->decided_to_commit ? > >> + "Committed" : "Rolled back", (ulonglong) member->xid); > >> + } > >> + > >> + return false; > >> +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0