- developers - lists.mariadb.org

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Rucha! On Jul 23, Rucha Deodhar wrote: > On Fri, Jul 23, 2021 at 1:55 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > > Consider this - thd->killed can be changed literally any time. We cannot > > test for it after every second line, can we? Clearly some places are > > better for this test than others. > > > There was no specific reason except it seemed a better choice for this > particular test case. > But yes, it can also arrive before warning or later. Now I think it would > be better check it before exiting handle_if_exists_option() > and return appropriate value. Yes, you're right, it would be better. but KILL can come after handle_if_exists_options() too :) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MariaDB Connector/J 3.x Status and Plans
by Ondrej Dubaj 23 Jul '21

23 Jul '21

Hello, I would like to ask about the current status of MariaDB Connector/J 3.x. I maintain the package in Fedora and RHEL. Currently, we are facing a decision about which major version to pack to RHEL 9, which is currently being developed. We would like to ship RHEL 9 with the latest stable version of the Java connector, and avoid rebasing to any next major release during the RHEL 9 life, if possible. However the 3.x version is only available in ALPHA quality and I can't see any plan for it to reach further stages in the Jira roadmap for the next half a year. https://jira.mariadb.org/secure/Dashboard.jspa I believe the best solution for us would be you releasing the GA, or at least BETA version ASAP, or at least in the near future. We still have time to update the package from the ALPHA version to GA version before the RHEL 9 development freeze, but the decision whether to take this road has to be made ASAP. After RHEL 9 GA, it will be very complicated for us to deal with rebasing to a new, incompatible version of mariadb-connector-j, where this rebase can break existing user applications and brings a lot of problems with it. Currently mariadb-java-client-2.7.2 is present in the repositories, but as already said, we want to aim for the 3.x version. You, of course, might have very different plans for the connector and different idea about how ready it is to become BETA or GA. We would like to hear your plans with the Java connector 3.x version and to discuss our possible paths to take. Thank you for your time in advance. Regards, Ondrej Dubaj

2 2

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Rucha! On Jul 23, Rucha Deodhar wrote: > > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > > > index f58ce8f997d..0b722984fca 100644 > > > --- a/sql/sql_table.cc > > > +++ b/sql/sql_table.cc > > > @@ -6489,6 +6489,12 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info, > > > ER_BAD_FIELD_ERROR, > > > ER_THD(thd, ER_BAD_FIELD_ERROR), > > > sql_field->change.str, table->s->table_name.str); > > > + /* > > > + if thread is killed (for example because local memory used execeeds > > > + maximum session memory used), return TRUE and do rollback (done later). > > > + */ > > > + if (thd->killed) > > > + DBUG_RETURN(true); > > > it.remove(); > > > > Looks like a completely arbitrary place to put a check in. > > What if the kill signal arrives later? > > We want to stop execution of the query as soon as we get kill. In this case > it happens when we are giving out a warning. > So I put the check here to return error right after giving warning. You can get KILL anytime. Was there any reason to put this check here and not few lines earlier or later? What if KILL would arrive before the warning? or after it.remove() ? Consider this - thd->killed can be changed literally any time. We cannot test for it after every second line, can we? Clearly some places are better for this test than others. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! On Jul 22, Rucha Deodhar wrote: > revision-id: ae1f4e4d727 (mariadb-10.5.11-1-gae1f4e4d727) > parent(s): dc82effa5df > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-06-21 20:24:43 +0530 > message: > > MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in > Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK) > > Analysis: KILL_QUERY is not ignored when local memory used exceeds maximum > session memory. Hence the query proceeds, OK is sent and we end up > reopening tables that are marked for reopen. During this, kill status is > eventually checked and assertion failure happens during trying to send error > message because OK has already been sent. > Fix: If local memory used exceeds maximum session memory and query is killed > before it has completed execution, return TRUE. > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index f58ce8f997d..0b722984fca 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6489,6 +6489,12 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info, > ER_BAD_FIELD_ERROR, > ER_THD(thd, ER_BAD_FIELD_ERROR), > sql_field->change.str, table->s->table_name.str); > + /* > + if thread is killed (for example because local memory used execeeds > + maximum session memory used), return TRUE and do rollback (done later). > + */ > + if (thd->killed) > + DBUG_RETURN(true); > it.remove(); Looks like a completely arbitrary place to put a check in. What if the kill signal arrives later? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f5196787294: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! ok to push! thanks On Jul 22, Rucha Deodhar wrote: > revision-id: f5196787294 (mariadb-10.5.11-28-gf5196787294) > parent(s): b1e57179242 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-22 18:54:36 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the > RETURNING clause, '*' is not resolved properly because of wrong context. > context->table_list is NULL or has incorrect table because context->table_list > has tables from the FROM clause. For INSERT/REPLACE...SELECT...RETURNING, > context->table_list has table we are inserting from. While in other > INSERT/REPLACE syntax, context->table_list is NULL because there is no FROM > clause. > Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, > use first_name_resolution_table for correct resolution of item. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d91c4eea668: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! On Jul 22, Rucha Deodhar wrote: > revision-id: d91c4eea668 (mariadb-10.5.11-28-gd91c4eea668) > parent(s): b1e57179242 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-17 18:47:15 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the > RETURNING clause, '*' is not resolved properly because of wrong context. > context->table_list is NULL or has incorrect table because context->table_list > has tables from the FROM clause. For INSERT/REPLACE...SELECT...RETURNING, > context->table_list has table we are inserting from. While in other > INSERT/REPLACE syntax, context->table_list is NULL because there is no FROM > clause. > Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, > use first_name_resolution_table for correct resolution of item. > @@ -271,6 +289,8 @@ INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING(SELECT > --error ER_SUBQUERY_NO_1_ROW > INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1=2 RETURNING (SELECT > id2 FROM t2); > +#--error ER_BAD_TABLE_ERROR > +#INSERT INTO t2(id2,val2) SELECT t1.* FROM t1 WHERE id1=2 RETURNING t1.*; Why is this test commented out? (same for REPLACE) > @@ -8031,10 +8034,20 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > If table names are qualified, then loop over all tables used in the query, > else treat natural joins as leaves and do not iterate over their underlying > tables. > + If filling fields instead of '*' for qualified asterisk in RETURNING clause, > + use first_name_resolution_table for correct resolution of item because > + context->table_list is either NULL or has incorrect table because > + context->table_list has tables from the FROM clause. > + In case of INSERT/REPLACE...SELECT..RETURNING context->table_list will have > + table we are inserting and NULL for other cases because there is no > + FROM clause. > */ > - TABLE_LIST *first= context->first_name_resolution_table; > - TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; > - if (table_name) > + if (returning_field || !table_name) > + { > + first= context->first_name_resolution_table; > + next= &TABLE_LIST::next_name_resolution_table; These are the values already set before, so assignments are redundant. Either remove initializers, like TABLE_LIST *first; TABLE_LIST *TABLE_LIST::* next; if (returning_field || !table_name) or reverse your condition: TABLE_LIST *first= context->first_name_resolution_table; TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; if (table_name && !returning_field) > + } > + else > { > first= context->table_list; > next= &TABLE_LIST::next_local; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5e16a49f5d9: MDEV-26013 distinct not work properly in some cases for spider tables
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Nayuta! On Jul 21, Nayuta Yanagisawa wrote: > revision-id: 5e16a49f5d9 (mariadb-10.4.20-32-g5e16a49f5d9) > parent(s): 78735dcaf75 > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2021-07-21 09:58:34 +0000 > message: > > MDEV-26013 distinct not work properly in some cases for spider tables > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d8116b20cbe..cfd04d93662 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3274,9 +3274,17 @@ bool JOIN::make_aggr_tables_info() > > if (ht && ht->create_group_by) > { > - /* Check if the storage engine can intercept the query */ > - Query query= {&all_fields, select_distinct, tables_list, conds, > - group_list, order ? order : group_list, having}; > + /* > + Check if the storage engine can intercept the query. > + > + JOIN::optimize_stage2() might convert DISTINCT into GROUP BY and then > + optimize away GROUP BY. In such a case, we need to notify a storage engine > + supporting a group by handler of the existence of the original DISTINCT. > + Thus, we set select_distinct || (no_order && group_optimized_away) to > + Query::distinct. > + */ > + Query query= {&all_fields, select_distinct || (no_order && group_optimized_away), > + tables_list, conds, group_list, order ? order : group_list, having}; If DISTINCT was coverted to a GROUP BY, why would the engine need to know whether there was DISTINCT or not originally? There is no DISTINCT on the execution plan now, that should be sufficient, shouldn't it? Why does the query fail with GROUP BY? > group_by_handler *gbh= ht->create_group_by(thd, &query); > > if (gbh) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

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] 5d19829df5f: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Rucha! Here the test is ok, buildbot is green. But you've only removed -e. What about -c, -n, -g ? On Jul 21, Rucha Deodhar wrote: > revision-id: 5d19829df5f (mariadb-10.6.1-39-g5d19829df5f) > parent(s): 2c4d1fb5440 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-15 13:29:25 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: -e is very inconsistent with how --default* options work in any other > executable. > Fix: Removed the support for -e starting from 10.7 to avoid inconsistency in the > long term. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 61dcf8d940c: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Rucha! On Jul 21, Rucha Deodhar wrote: > revision-id: 61dcf8d940c (mariadb-10.5.11-35-g61dcf8d940c) > parent(s): f88d130e710 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-15 16:07:18 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). The code change looks ok. But I thought we've seen from your previous push that this test doesn't work. Haven't we? See how it failed all over in buildbot: https://buildbot.askmonty.org/buildbot/grid?category=main&branch=bb-10.5-MD… Please, generally, first check that your commit passes buildbot, then ask for a review. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0