March 2021 - developers - lists.mariadb.org

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

21 Jul '21

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

2 11

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

10 Jun '21

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

2 2

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

08 Jun '21

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

2 15

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

31 May '21

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

3 2

Re: [Maria-developers] 308c28e8a60: MDEV-15888 Implement FLUSH TABLES tbl_name [, tbl_name] ... WITH READ LOCK for views.
by Sergei Golubchik 23 May '21

23 May '21

Hi, Alexey! On Mar 21, Alexey Botchkov wrote: > revision-id: 308c28e8a60 (mariadb-10.5.2-393-g308c28e8a60) > parent(s): 786bc312b85 > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2021-02-09 00:59:55 +0400 > message: > > MDEV-15888 Implement FLUSH TABLES tbl_name [, tbl_name] ... WITH READ LOCK for views. > > Kind of 'naive' solution that surprisingly worked so trying to > understand why. > > diff --git a/mysql-test/main/flush-innodb.result b/mysql-test/main/flush-innodb.result > index 21e5bda7785..2c886e4f9fc 100644 > --- a/mysql-test/main/flush-innodb.result > +++ b/mysql-test/main/flush-innodb.result > @@ -60,7 +60,7 @@ DROP TABLE export; > CREATE VIEW v1 AS SELECT 1; > CREATE TEMPORARY TABLE t1 (a INT); > FLUSH TABLES v1 FOR EXPORT; > -ERROR HY000: 'test.v1' is not of type 'BASE TABLE' > +UNLOCK TABLES; good, but it only tests that there's no error. may be you need to add tests to verify that tables are actually locked? > FLUSH TABLES t1 FOR EXPORT; > ERROR 42S02: Table 'test.t1' doesn't exist > FLUSH TABLES non_existent FOR EXPORT; > diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc > index 8f87d633d19..c146b7693c1 100644 > --- a/sql/sql_reload.cc > +++ b/sql/sql_reload.cc > @@ -543,6 +543,7 @@ bool flush_tables_with_read_lock(THD *thd, TABLE_LIST *all_tables) > > if (thd->lex->type & REFRESH_READ_LOCK) > { > +#ifdef MDEV15887 > /* > Acquire SNW locks on tables to be flushed. Don't acquire global > IX and database-scope IX locks on the tables as this will make > @@ -559,6 +560,7 @@ bool flush_tables_with_read_lock(THD *thd, TABLE_LIST *all_tables) > for (auto table_list= all_tables; table_list; > table_list= table_list->next_global) > table_list->mdl_request.ticket= NULL; > +#endif /*MDEV15887*/ what's that? btw, the number is MDEV15888 > } > > thd->variables.option_bits|= OPTION_TABLE_LOCK; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 2be9b69f4ff: MDEV-23001 Precreate static Item_bool() to simplify code
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! On Mar 25, Michael Widenius wrote: > revision-id: 2be9b69f4ff (mariadb-10.5.2-510-g2be9b69f4ff) > parent(s): cb545f11169 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:05:15 +0200 > message: > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's agree > - Less and faster code I don't quite agree with that, it's not less code: > 7 files changed, 65 insertions(+), 43 deletions(-) and I doubt that speedup can be measured in any benchmarks whatsoever. > diff --git a/sql/item.cc b/sql/item.cc > index 1a86b8b3114..0f160fa257b 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -55,7 +55,8 @@ const char *item_empty_name=""; > const char *item_used_name= "\0"; > > static int save_field_in_field(Field *, bool *, Field *, bool); > - > +const Item_bool_static Item_false("FALSE", 0); > +const Item_bool_static Item_true("TRUE", 1); The main question - we have tons of modifiable Item members. How do you know none of them will modified in your Item_bool_static objects? > @@ -421,6 +421,9 @@ Item::Item(THD *thd): > /* Initially this item is not attached to any JOIN_TAB. */ > join_tab_idx= MAX_TABLES; > > + /* thd is NULL in case of static items like Item_true */ > + if (!thd) > + return; No, please, don't. thd can be NULL only twice, for statically initialized Item_true and Item_false. For that you removed a useful assert and added an if() that is completely unnecessary for billions of items created runtime. Better to have special contructor just for these two very special items. > /* Put item in free list so that we can free all items at end */ > next= thd->free_list; > thd->free_list= this; > @@ -3637,6 +3641,19 @@ void Item_int::print(String *str, enum_query_type query_type) > } > > > +/* > + This function is needed to ensure that Item_bool_static doesn't change > + the value of the member str_value. > +*/ > + > +void Item_bool::print(String *str, enum_query_type query_type) > +{ > + // my_charset_bin is good enough for numbers > + String tmp(value ? (char*) "1" : (char*) "0" , 1, &my_charset_bin); > + str->append(tmp); > +} This is not needed anymore. Item_bool inherits from Item_int, and Item_int::print is void Item_int::print(String *str, enum_query_type query_type) { StringBuffer<LONGLONG_BUFFER_SIZE> buf; // my_charset_bin is good enough for numbers buf.set_int(value, unsigned_flag, &my_charset_bin); str->append(buf); } You removed str_value from Item_int::print in July. > Item *Item_bool::neg_transformer(THD *thd) > { > value= !value; > diff --git a/sql/item.h b/sql/item.h > index a1c288ab1f0..1087c08869e 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1976,7 +1976,7 @@ class Item: public Value_source, > virtual bool limit_index_condition_pushdown_processor(void *arg) { return 0; } > virtual bool exists2in_processor(void *arg) { return 0; } > virtual bool find_selective_predicates_list_processor(void *arg) { return 0; } > - bool cleanup_is_expensive_cache_processor(void *arg) > + virtual bool cleanup_is_expensive_cache_processor(void *arg) You've added a new virtual method? First you try so hard to remove virtual methods, then you add a new one specifically for these two static items. Why does it matter what cleanup_is_expensive_cache_processor will do with them? You overwrite is_expensive() anyway. > { > is_expensive_cache= (int8)(-1); > return 0; > @@ -3220,9 +3220,11 @@ class Item_literal: public Item_basic_constant > Item_literal(THD *thd): Item_basic_constant(thd) > { } > Type type() const override { return CONST_ITEM; } > - bool check_partition_func_processor(void *) override { return false;} > + bool check_partition_func_processor(void *int_arg) override { return false;} > bool const_item() const override { return true; } > bool basic_const_item() const override { return true; } > + bool is_expensive() override { return false; } > + bool cleanup_is_expensive_cache_processor(void *arg) override { return 0; } > }; > > > diff --git a/sql/opt_index_cond_pushdown.cc b/sql/opt_index_cond_pushdown.cc > index 15bc2074e1f..e950ad1b7ca 100644 > --- a/sql/opt_index_cond_pushdown.cc > +++ b/sql/opt_index_cond_pushdown.cc > @@ -185,8 +185,8 @@ bool uses_index_fields_only(Item *item, TABLE *tbl, uint keyno, > static Item *make_cond_for_index(THD *thd, Item *cond, TABLE *table, uint keyno, > bool other_tbls_ok) > { > - if (!cond) > - return NULL; > + if (!cond || cond->basic_const_item()) > + return cond; The effect of this change is that you don't set cond->marker= ICP_COND_USES_INDEX_ONLY for basic_const_item's anymore. This looked suspicious at first. But then I noticed (and Sergey Petrunia confirmed) that ICP_COND_USES_INDEX_ONLY is not used at all. Since 5.3. So here I'd suggest to revert this your change and completely remove any ICP_COND_USES_INDEX_ONLY manipulations together with the ICP_COND_USES_INDEX_ONLY definition. > if (cond->type() == Item::COND_ITEM) > { > uint n_marked= 0; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 119c7360f07..e63e120a02b 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2395,6 +2395,7 @@ dispatch_command_return dispatch_command(enum enum_server_command command, THD * > thd->update_server_status(); > thd->protocol->end_statement(); > query_cache_end_of_result(thd); > + /* Check that the query didn't change const items */ what does that mean? Is it a TODO comment? Or it documents what happens below? > } > if (drop_more_results) > thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 93f5d3591ed..894d511adf4 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -2364,7 +2364,7 @@ int JOIN::optimize_stage2() > if (!conds && outer_join) > { > /* Handle the case where we have an OUTER JOIN without a WHERE */ > - conds= new (thd->mem_root) Item_bool(thd, true); // Always true > + conds= (Item*) &Item_true; Hmm, does casting const away even work in the Intel compiler? > } > > if (impossible_where) > @@ -22743,6 +22743,8 @@ make_cond_for_table_from_pred(THD *thd, Item *root_cond, Item *cond, > return new_cond; > } > } > + else if (cond->basic_const_item()) > + return cond; > > if (is_top_and_level && used_table == rand_table_bit && > (cond->used_tables() & ~OUTER_REF_TABLE_BIT) != rand_table_bit) this is rather fragile, isn't it? You skip item->set_join_tab_idx(join_tab_idx_arg), quite understandably. And currently, as far as I can see, item->get_join_tab_idx() is always used under if (!item->const_item()), so what you did is safe. But can it change in the future? Or is join_tab_idx something that conceptually doesn't make sense for const items? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] c157c285db9: Optimize Sql_alloc
by Sergei Golubchik 14 May '21

14 May '21

Hi, Michael! On Mar 27, Michael Widenius wrote: > revision-id: c157c285db9 (mariadb-10.5.2-514-gc157c285db9) > parent(s): 16e38888c06 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:31:53 +0200 > message: > > Optimize Sql_alloc > > - Remove 'dummy_for_valgrind' overrun marker as this doesn't help much. > The element also distorts the sizes of objects a bit, which makes it > harder to calculate gain in object sizes when doing size optimizations. > - Avoid one extra call indirection when using thd_get_current_thd(), which > is used by Sql_alloc. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 4f522d96a9f..1ab52401705 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -662,7 +662,15 @@ static std::atomic<char*> shutdown_user; > > static thread_local THD *THR_THD; > > +/** > + Get current THD object from thread local data > + > + @retval The THD object for the thread, NULL if not connection thread > +*/ > + > MYSQL_THD _current_thd() { return THR_THD; } > +THD *thd_get_current_thd() { return THR_THD; } I'd rather remove thd_get_current_thd() completely, why do we need two identical functions? Sql_alloc can use _current_thd() just fine, I've tried (also rocksdb uses thd_get_current_thd in one place, but it can use current_thd like the rest of the server code). Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 21eb8969ce9: Improved storage size for Item, Field and some other classes
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! Looks ok, but again it doesn't seem you've squashed intermediate commits as you said you did. Bit fields and non-existent commits in columnstore - it's is clearly an intermediate work-in-progress state, all fixed in your later commits. On Sep 08, Michael Widenius wrote: > revision-id: 21eb8969ce9 (mariadb-10.5.2-270-g21eb8969ce9) > parent(s): c3ecf0d6243 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:34 +0300 > message: > > Improved storage size for Item, Field and some other classes > > - Changed order of class fields to remove dead alignment space. > - Changed bool fields in Item to bit fields. > - Used packed enum's for some fields in common classes > - Removed not used Item::rsize. > - Changed some class variables from uint/int to smaller type int's. > - Ensured that field_index is uint16 in all classes and functions. Fixed > also that we proparly compare with NO_CACHED_FIELD_INDEX when checking > if variable is not set. > - Removed checking of highest bit of unireg_check (has not been used in > a long time) > - Fixed wrong arguments to make_cond_for_table() for join_tab_idx_arg > from false to 0. > > One of the result was reducing the size if class Item with ~24 bytes ... > @@ -929,18 +904,48 @@ class Item: public Value_source, > + > + /* > + str_values's main purpose is to be used to cache the value in > + save_in_field > + */ > + String str_value; > + > + LEX_CSTRING name; /* Name of item */ > + /* Original item name (if it was renamed)*/ > + const char *orig_name; > + > + uint32 /* All common bool variables for Item stored here */ > + maybe_null:1, /* If item may be null */ > + in_rollup:1, /* If used in GROUP BY list of a query with ROLLUP */ > + with_param:1, /* True if Item contains an SP parameter */ > + with_window_func:1, /* True if item contains a window func */ > + with_field:1, /* True if any item except Item_sum contains a field. > + Set during parsing. */ > + fixed:1; /* If item was fixed with fix_fields */ > + > + int16 marker; > + > diff --git a/storage/columnstore/columnstore b/storage/columnstore/columnstore > index b6b02ed516f..f606e76fb77 160000 > --- a/storage/columnstore/columnstore > +++ b/storage/columnstore/columnstore > @@ -1 +1 @@ > -Subproject commit b6b02ed516f92055127d416370799d91a82754ea > +Subproject commit f606e76fb779e40f3376693fff9969e4f2b7669a Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 4088269c578d: Improved storage size for Item, Field and some other classes
by Sergei Golubchik 14 May '21

14 May '21

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 4088269c578d (mariadb-10.5.2-270-g4088269c578d) > parent(s): 994ea2af3973 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-17 12:25:44 +0300 > message: > > Improved storage size for Item, Field and some other classes > > diff --git a/storage/columnstore/columnstore b/storage/columnstore/columnstore > index b6b02ed516f9..f606e76fb779 160000 > --- a/storage/columnstore/columnstore > +++ b/storage/columnstore/columnstore > @@ -1 +1 @@ > -Subproject commit b6b02ed516f92055127d416370799d91a82754ea > +Subproject commit f606e76fb779e40f3376693fff9969e4f2b7669a Again. You should not push invalid commit hashes into the main branch: storage/columnstore/columnstore $ git fetch origin storage/columnstore/columnstore $ git show f606e76fb779e40f3376693fff9969e4f2b7669a fatal: bad object f606e76fb779e40f3376693fff9969e4f2b7669a Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 16e38888c06: Improved storage size for Item, Field and some other classes
by Sergei Golubchik 14 May '21

14 May '21

Hi, Michael! Did you consider trying -Wpadded? '-Wpadded' Warn if padding is included in a structure, either to align an element of the structure or to align the whole structure. Sometimes when this happens it is possible to rearrange the fields of the structure to reduce the padding and so make the structure smaller. Might reveal more useful cases where fields could be rearranged. On Mar 27, Michael Widenius wrote: > revision-id: 16e38888c06 (mariadb-10.5.2-513-g16e38888c06) > parent(s): 4cfb680b685 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-24 14:31:53 +0200 > message: > > Improved storage size for Item, Field and some other classes > > - Changed order of class fields to remove dead alignment space. > - Changed bool fields in Item to bit fields. > - Used packed enum's for some fields in common classes > - Removed not used Item::rsize. > - Changed some class variables from uint/int to smaller type int's. > - Ensured that field_index is uint16 in all classes and functions. Fixed > also that we proparly compare with NO_CACHED_FIELD_INDEX when checking > if variable is not set. > - Removed checking of highest bit of unireg_check (has not been used in > a long time) > - Fixed wrong arguments to make_cond_for_table() for join_tab_idx_arg > from false to 0. > > One of the result was reducing the size if class Item with ~24 bytes Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1