June 2021 - developers - lists.mariadb.org

Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
by Sergei Golubchik 24 Aug '22

24 Aug '22

Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. when system_versioning_insert_history=1 > 3. Cleaned up select_insert::send_data() from setting vers_write as > this parameter is now set on TABLE initialization. I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit? > diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ x y > 9 9001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d23b190b2c5..93f8a8434e4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #endif > } > > +bool THD::vers_modify_sys_field() const don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example). > +{ > + if (lex->sql_command != SQLCOM_INSERT && > + lex->sql_command != SQLCOM_INSERT_SELECT && > + lex->sql_command != SQLCOM_LOAD) > + return false; I don't think this is enough to reject insert ... on duplicate key update row_start=xxx > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > + return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

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] MDEV-25325 built-in documentation for performance_schema tables
by Haidong Ji 24 Jun '21

24 Jun '21

Hi, I've grabbed this JIRA story, https://jira.mariadb.org/browse/MDEV-25325 . The coding and test cases creation were all done. I've also submitted a PR here: https://github.com/MariaDB/server/pull/1830 The PR was submitted more than 1 month ago. I'm curious about next steps. Thanks, Haidong

1 0

[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

[Maria-developers] MDEV-25875: RE 02469b: MDEV-17399 JSON_TABLE Accept JSON values for the JSON fields
by Sergey Petrunia 16 Jun '21

16 Jun '21

Hi Alexey, This is review input re https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a0702…. First, our workflow dictates that this patch should have its own MDEV entry. I've filed it https://jira.mariadb.org/browse/MDEV-25875. Please use it in commit descriptions. == Updates mixed with another patch == The patch has .result changes like: > @@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT NULL ON ERROR)) jt; > ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'NULL ON ERROR)) jt' at line 2 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt; > -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON EMPTY)) jt' at line 2 > +x > +0 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt; > -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON ERROR)) jt' at line 2 > +x > +NULL > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x DATE > PATH '$.x' These are obviously from another patch (the one about "accepting non-string literals as default"). Please move them to that patch. == Comments == > @@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const json_engine_t *je) > } > > > +static int store_json_in_json(Field *f, json_engine_t *je) > +{ Please add a comment for the function. Something like "Store the current JSON element pointed by je into field f, as a separate JSON document" The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better? > + const uchar *from= je->value_begin; > + const uchar *to; > + > + if (json_value_scalar(je)) > + to= je->value_end; > + else > + { > + int error; > + if ((error= json_skip_level(je))) > + return error; > + to= je->s.c_str; > + } > + f->store((const char *) from, (uint32) (to - from), je->s.cs); > + return 0; > +} > + > + > bool Json_table_nested_path::check_error(const char *str) > { > if (m_engine.s.error) > @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd, uchar * buf, uchar *pos) > } > else > { > - if (!(error= !json_value_scalar(&je))) > + if (jc->m_format_json) > + { > + if (!(error= store_json_in_json(*f, &je))) > + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0? (My logic - er_handler.errors already had some error status, why did we call store_json_in_json? If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is this correct?) > + } > + else if (!(error= !json_value_scalar(&je))) > { > store_json_in_field(*f, &je); > error= er_handler.errors; > @@ -868,6 +893,10 @@ int Json_table_column::set(THD *thd, enum_type ctype, const LEX_CSTRING &path) > anctual content. Not sure though if we should. > */ > m_path.s.c_str= (const uchar *) path.str; > + > + if (ctype == PATH) > + m_format_json= m_field->type_handler() == &type_handler_json_longtext; This will probably cause warnings on some compiler. Please wrap into "MY_TEST(...)" or just brackets, "z= (x == y)". == A discrepancy between this and MySQL == I've found this case: select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo') ) as T; with the above patch, it produces a JSON null value: +-------+ | jscol | +-------+ | null | +-------+ while in MySQL-8 it produces an SQL NULL: +-------+ | jscol | +-------+ | NULL | +-------+ I'm not yet sure what we should do about this. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

[Maria-developers] New Member (GSoC MDEV-25015)
by Alan Cueva 15 Jun '21

15 Jun '21

Hello everyone, I am Alan Cueva, during the summer I will be working on the Custom formatting of strings in MariaDB queries (MDEV-25015 <https://jira.mariadb.org/browse/MDEV-25015>) project, but I would like to continue contributing in this community. I hope to be useful and learn from everyone. Thanks!

1 0

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] 557ab2992dd: MDEV-21117 Incremental patch for a few review notes addressed.
by Sergei Golubchik 09 Jun '21

09 Jun '21

Hi, Andrei! Again, this is a review of the git diff f09d33f521b 557ab2992dd, not only 557ab2992dd. On Jun 02, Andrei Elkin wrote: > revision-id: 557ab2992dd (mariadb-10.6.0-67-g557ab2992dd) > parent(s): 4cc5a3074da > author: Andrei Elkin <andrei.elkin(a)mariadb.com> > committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > timestamp: 2021-05-27 17:37:22 +0300 > message: > > MDEV-21117 Incremental patch for a few review notes addressed. > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > new file mode 100644 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > @@ -0,0 +1,65 @@ > +connect(master1,localhost,root,,); > +connect(master2,localhost,root,,); > +connect(master3,localhost,root,,); > + > +--connection default > + > +# First to commit few transactions > +INSERT INTO t VALUES (10); > +INSERT INTO tm VALUES (10); > + > +--connection master1 > +# Hold insert after write to binlog and before "run_commit_ordered" in engine > +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master1_ready WAIT_FOR signal_never_arrives"; > +--send_eval $query1 > + > +--connection master2 > +SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > +SET DEBUG_SYNC= "commit_before_get_LOCK_after_binlog_sync SIGNAL master2_ready"; > +if ($pre_q2) > +{ > + --send_eval $pre_q2; > +} > +# To binlog non-xid transactional group which will be truncated all right > +if (!$pre_q2) > +{ > + --send_eval $query2 > +} eh, this makes no sense. In the previous version of the patch you've run pre_q2 before sending query2. now you send either pre_q2 or query2. so you don't need pre_q2 anymore now, just set query2 to the what used to be pre_q2. > + > +--connection master3 > +SET DEBUG_SYNC= "now WAIT_FOR master2_ready"; > +SELECT @@global.gtid_binlog_pos as 'Before the crash'; > + > +--connection default > +--source include/kill_mysqld.inc > +--disconnect master1 > +--disconnect master2 > +--disconnect master3 > + > +# > +# Server restart > +# > +--let $restart_parameters= --rpl-semi-sync-slave-enabled=1 > +--source include/start_mysqld.inc > + > +# Check error log for a successful truncate message. > +--let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err > + > +--let SEARCH_FILE=$log_error_ > +--let SEARCH_PATTERN=Successfully truncated.*to remove transactions starting from GTID $truncate_gtid_pos > +--replace_regex /FOUND [0-9]+/FOUND #/ can it be found an unpredictable number of times? Why would binlog be truncated more than once? > +--source include/search_pattern_in_file.inc > + > +--echo Pre-crash binlog file content: > +--let $binlog_file= query_get_value(show binary logs, Log_name, $binlog_file_index) > +--source include/show_binlog_events.inc > + > +SELECT @@global.gtid_binlog_pos as 'After the crash'; > +--echo "One row should be present in table 't'" > +SELECT * FROM t; > + > +# prepare binlog file index for the next test > +--inc $binlog_file_index > + > +# Local cleanup > +DELETE FROM t; > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.test b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > new file mode 100644 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > @@ -0,0 +1,104 @@ > +# ==== Purpose ==== > +# > +# Test verifies the truncation of single binary log file. > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > +--source include/have_innodb.inc > +--source include/have_aria.inc > +# File: binlog_truncate_active_log.inc included in test makes use of > +# 'debug_sync' facility. > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_statement.inc > + > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > + > +# The following cases are tested: > +# A. 2pc transaction is followed by a blank "zero-engines" one > +# B. 2pc transaction follows the blank one > +# C. Similarly to A, with the XA blank transaction > + > +RESET MASTER; > +CREATE TABLE t (f INT) ENGINE=INNODB; > +CREATE TABLE t2 (f INT) ENGINE=INNODB; > +CREATE TABLE tm (f INT) ENGINE=Aria; > + > +# Old (pre-crash) binlog file index initial value. > +# It keeps incremented at the end of each case. > +--let $binlog_file_index=1 > + > +--echo # Case A. > +# Using 'debug_sync' hold 'query1' execution after 'query1' is flushed and > +# synced to binary log but not yet committed. In an another connection hold > +# 'query2' execution after 'query2' is flushed and synced to binlog. > +# Crash and restart server with --rpl-semi-sync-slave-enabled=1 > +# > +# During recovery of binary log 'query1' status is checked with InnoDB engine, > +# it will be in prepared but not yet commited. All transactions starting from > +# 'query1' onwards will be removed from the binary log. > +# Show-binlog-events is to prove that. > + > +--let $truncate_gtid_pos = 0-1-6 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $query2 = DELETE FROM t2 WHERE f = 0 /* no such record */ > +--source binlog_truncate_active_log.inc > + > +--echo # Case B. > +# The inverted sequence ends up to truncate starting from $query2 > +--let $truncate_gtid_pos = 0-1-10 > +--let $query1 = DELETE FROM t2 WHERE f = 0 > +--let $query2 = INSERT INTO t VALUES (20) > +--source binlog_truncate_active_log.inc > + > + > +--echo # Case C. > +delimiter |; > +CREATE PROCEDURE sp_blank_xa() > +BEGIN > + XA START 'blank'; > + DELETE FROM t2 WHERE f = 0 /* no such record */; > + XA END 'blank'; > + XA PREPARE 'blank'; > +END| > +delimiter ;| > + > +# The same as in A with $query2 being the zero-engine XA transaction. > +# Both $query1 and $query2 are going to be truncated. > +--let $truncate_gtid_pos = 0-1-14 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $pre_q2 = CALL sp_blank_xa > +--let $query2 = XA COMMIT 'blank' hm.. now your binlog_truncate_active_log.inc only runs query2 is pre_q2 is empty. So this XA COMMIT isn't done at all. Very confusing. > +--source binlog_truncate_active_log.inc > + > +DROP PROCEDURE sp_blank_xa; > + > + > +--echo # Case D. > +delimiter |; > +CREATE PROCEDURE sp_xa() > +BEGIN > + XA START 'xid'; > + DELETE FROM t WHERE f = 10; > + XA END 'xid'; > + XA PREPARE 'xid'; > +END| > +delimiter ;| > + > +# The same as in B with $query1 being the prepared XA transaction. > +# Truncation must occurs at $query2. > +--let $truncate_gtid_pos = 0-1-20 > +--let $query1 = CALL sp_xa > +--let $pre_q2 = > +--let $query2 = INSERT INTO t2 VALUES (20) > +--source binlog_truncate_active_log.inc > + > +DROP PROCEDURE sp_xa; > + > + > +--echo # Cleanup > +DROP TABLE t,t2,tm; > + > +--echo # End of the tests > diff --git a/sql/handler.h b/sql/handler.h > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -931,6 +932,32 @@ struct xid_t { > }; > typedef struct xid_t XID; > > +/* > + Enumerates a sequence in the order of > + their creation that is in the top-down order of the index file. > + Ranges from zero through MAX_binlog_id. > + Not confuse the value with the binlog file numerical suffix, > + neither with the binlog file line in the binlog index file. Ah! Good, thanks. I thought it was a binlog file numerical suffix, indeed. > +*/ > +typedef uint Binlog_file_id; > +const Binlog_file_id MAX_binlog_id= UINT_MAX; > +/* > + 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; > + > +/* binlog-based recovery transaction descriptor */ > +struct xid_recovery_member > +{ > + my_xid xid; > + uint in_engine_prepare; // number of engines that have xid prepared > + bool decided_to_commit; > + Binlog_offset binlog_coord; // semisync recovery binlog offset > +}; > + > /* for recover() handlerton call */ > #define MIN_XID_LIST_SIZE 128 > #define MAX_XID_LIST_SIZE (1024*128) > diff --git a/sql/log.cc b/sql/log.cc > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -1692,17 +1698,13 @@ int binlog_init(void *p) > binlog_hton->savepoint_rollback= binlog_savepoint_rollback; > binlog_hton->savepoint_rollback_can_release_mdl= > binlog_savepoint_rollback_can_release_mdl; > - binlog_hton->commit= binlog_commit; > + binlog_hton->commit= binlog_commit_dummy; you could use a lambda for that too, [](handlerton*,THD*,bool) { return 0; } > binlog_hton->rollback= binlog_rollback; > binlog_hton->drop_table= [](handlerton *, const char*) { return -1; }; Oh did you actually need it? Or it's for completeness only? I mean, was there some code that tried to drop a "binlog" table? > if (WSREP_ON || opt_bin_log) > { > binlog_hton->prepare= binlog_prepare; > binlog_hton->start_consistent_snapshot= binlog_start_consistent_snapshot; > - binlog_hton->commit_by_xid= binlog_commit_by_xid; > - binlog_hton->rollback_by_xid= binlog_rollback_by_xid; > - // recover needs to be set to make xa{commit,rollback}_handlerton effective > - binlog_hton->recover= binlog_xa_recover_dummy; > } > binlog_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN | HTON_NO_ROLLBACK; > return 0; > @@ -9975,6 +9977,154 @@ int TC_LOG::using_heuristic_recover() > /****** transaction coordinator log for 2pc - binlog() based solution ******/ > #define TC_LOG_BINLOG MYSQL_BIN_LOG > > +/** > + Truncates the current binlog to specified position. Removes the rest of binlogs > + which are present after this binlog file. > + > + @param truncate_file Holds the binlog name to be truncated > + @param truncate_pos Position within binlog from where it needs to > + truncated. > + > + @retval true ok > + @retval false error > + > +*/ > +bool MYSQL_BIN_LOG::truncate_and_remove_binlogs(const char *file_name, > + my_off_t pos, > + rpl_gtid *ptr_gtid) > +{ > + int error= 0; > +#ifdef HAVE_REPLICATION > + LOG_INFO log_info; > + THD *thd= current_thd; > + my_off_t index_file_offset= 0; > + File file= -1; > + MY_STAT s; > + my_off_t old_size; > + > + if ((error= find_log_pos(&log_info, file_name, 1))) > + { > + sql_print_error("Failed to locate binary log file:%s." > + "Error:%d", file_name, error); > + goto end; > + } > + > + while (!(error= find_next_log(&log_info, 1))) > + { > + if (!index_file_offset) > + { > + index_file_offset= log_info.index_file_start_offset; > + if ((error= open_purge_index_file(TRUE))) > + { > + sql_print_error("Failed to open purge index " > + "file:%s. Error:%d", purge_index_file_name, error); > + goto end; > + } > + } > + if ((error= register_purge_index_entry(log_info.log_file_name))) > + { > + sql_print_error("Failed to copy %s to purge index" > + " file. Error:%d", log_info.log_file_name, error); > + goto end; > + } > + } > + > + if (error != LOG_INFO_EOF) > + { > + sql_print_error("Failed to find the next binlog to " > + "add to purge index register. Error:%d", error); > + goto end; > + } > + > + if (is_inited_purge_index_file()) > + { > + if (!index_file_offset) > + index_file_offset= log_info.index_file_start_offset; > + > + if ((error= sync_purge_index_file())) > + { > + sql_print_error("Failed to flush purge index " > + "file. Error:%d", error); > + goto end; > + } > + > + // Trim index file > + if ((error= > + mysql_file_chsize(index_file.file, index_file_offset, '\n', > + MYF(MY_WME))) || > + (error= > + mysql_file_sync(index_file.file, MYF(MY_WME|MY_SYNC_FILESIZE)))) > + { > + sql_print_error("Failed to trim binlog index " > + "file:%s to offset:%llu. Error:%d", index_file_name, > + index_file_offset, error); > + goto end; > + } > + > + /* Reset data in old index cache */ > + if ((error= reinit_io_cache(&index_file, READ_CACHE, (my_off_t) 0, 0, 1))) > + { > + sql_print_error("Failed to reinit binlog index " > + "file. Error:%d", error); > + goto end; > + } > + > + /* Read each entry from purge_index_file and delete the file. */ > + if ((error= purge_index_entry(thd, NULL, TRUE))) > + { > + sql_print_error("Failed to process registered " > + "files that would be purged."); > + goto end; > + } > + } > + > + DBUG_ASSERT(pos); > + > + if ((file= mysql_file_open(key_file_binlog, file_name, > + O_RDWR | O_BINARY, MYF(MY_WME))) < 0) > + { > + error= 1; > + sql_print_error("Failed to open binlog file:%s for " > + "truncation.", file_name); > + goto end; > + } > + my_stat(file_name, &s, MYF(0)); > + old_size= s.st_size; > + clear_inuse_flag_when_closing(file); > + /* Change binlog file size to truncate_pos */ > + if ((error= > + mysql_file_chsize(file, pos, 0, MYF(MY_WME))) || > + (error= mysql_file_sync(file, MYF(MY_WME|MY_SYNC_FILESIZE)))) I'd rather not to overload if() conditions with function calls like this, they're rather annoying to single-step through and impossible to put a breakpoint on a specific function call. > + { > + sql_print_error("Failed to trim the " > + "binlog file:%s to size:%llu. Error:%d", > + file_name, pos, error); > + goto end; > + } > + else > + { > + char buf[21]; > + my_stat(file_name, &s, MYF(0)); > + my_off_t new_size= s.st_size; do you need it? new_size == pos, isn't it? You yourself use the word "size" to describe pos in sql_print_error just few lines above :) > + > + longlong10_to_str(ptr_gtid->seq_no, buf, 10); > + sql_print_information("Successfully truncated binlog file:%s " > + "to pos:%llu to remove transactions starting from " > + "GTID %u-%u-%s; " > + "former file size shrunk from %llu to %llu.", This isn't quite English. Better just "previous (or old) file size: %llu", that is not "former" and no need to specify the new size twice. Also, you say that you "truncated binlog file" here, but on failure you fail to "trim the binlog file". I suggest to use consistent wording, either always "trim" or always "truncate" (and always with or without "the") - both on success and on failure. Personally, I favor "truncate the binlog file" > + file_name, pos, > + ptr_gtid->domain_id, ptr_gtid->server_id, buf, > + old_size, new_size); > + } > + > +end: > + if (file >= 0) > + mysql_file_close(file, MYF(MY_WME)); > + > + error= error || close_purge_index_file(); > +#endif > + return error > 0; > +} > int TC_LOG_BINLOG::open(const char *opt_name) > { > int error= 1; > @@ -10449,7 +10599,528 @@ start_binlog_background_thread() ... > + > +Recovery_context::Recovery_context() : > + prev_event_pos(0), > + last_gtid_standalone(false), last_gtid_valid(false), last_gtid_no2pc(false), > + last_gtid_engines(0), > + do_truncate(rpl_semi_sync_slave_enabled), > + truncate_validated(false), truncate_reset_done(false), > + truncate_set_in_1st(false), id_binlog(MAX_binlog_id), > + checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF), gtid_maybe_to_truncate(NULL) > +{ > + last_gtid_coord= Binlog_offset(0,0); > + binlog_truncate_coord= binlog_truncate_coord_1st_round= Binlog_offset(0,0); > + binlog_unsafe_coord= Binlog_offset(0,0); Okay, I admit, just typedef'ing std::pair is a huge improvement already > + binlog_truncate_file_name[0]= 0; > + binlog_unsafe_file_name [0]= 0; > + binlog_unsafe_gtid= truncate_gtid= truncate_gtid_1st_round= rpl_gtid(); > + if (do_truncate) > + gtid_maybe_to_truncate= new Dynamic_array<rpl_gtid>(16, 16); > +} > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 1

[Maria-developers] Tips on Writing/Reading JSON
by Michael Okoko 09 Jun '21

09 Jun '21

Hi Alexey, I'm Michael Okoko, a GSoC '21 student working on Using JSON as the on-disk format for MariaDB histograms(https://jira.mariadb.org/browse/MDEV-21130). The project requires writing and reading/parsing JSON, and I was wondering if you could provide some tips and pointers regarding a proper JSON writer/JSON parser to use. Regards, Michael.

2 1