April 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] 32c73138ff4: Remove some usage of Check_level_instant_set and Sql_mode_save
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! On Apr 19, Michael Widenius wrote: > revision-id: 32c73138ff4 (mariadb-10.5.2-559-g32c73138ff4) > parent(s): 07708eb9b28 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-25 12:06:34 +0200 > message: > > Remove some usage of Check_level_instant_set and Sql_mode_save > > The reason for the removal are: > - Generates more code > - Storing and retreving THD > - Causes extra code and daata to be generated to handle possible throw > exceptions (which never happens in MariaDB code) > - Uses more stack space No, Monty. We've discussed it already, this makes code more complex, fragile and bug prone. And it does *not* generates extra code or data or stack - before writing this email I've compiled mariadbd both ways and compared the generated code for the function in question, it was *identical*. > Other things: > - Changed convert_const_to_int() to use item->save_in_field_no_warnings(), > which made the code shorter and simpler. > - Added thd as argument to store_key.copy() to make function simpler > - Added thd as argument to some subselect* constructor that inherites > from Item_subselect. these changes are good, thanks Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 6b06a99e4a4: Avoid creating the .frm file twice in some cases
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! On Apr 19, Michael Widenius wrote: > revision-id: 6b06a99e4a4 (mariadb-10.5.2-557-g6b06a99e4a4) > parent(s): 9c1e36696f3 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-25 12:06:34 +0200 > message: > > Avoid creating the .frm file twice in some cases > > diff --git a/sql/handler.cc b/sql/handler.cc > index c286aef3a9e..fe2fe6e9426 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -5536,8 +5536,9 @@ int ha_create_table(THD *thd, const char *path, > > if (frm) > { > - bool write_frm_now= !create_info->db_type->discover_table && > - !create_info->tmp_table(); > + bool write_frm_now= (!create_info->db_type->discover_table && > + !create_info->tmp_table() && > + !create_info->frm_is_created); Is this the real problem? frm created twice? Why ha_create_table() is invoked twice at all? > > share.frm_image= frm; > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index e00d3ee1ed2..73642a5bd97 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -547,8 +547,13 @@ uint build_table_filename(char *buff, size_t bufflen, const char *db, > > (void) tablename_to_filename(db, dbbuff, sizeof(dbbuff)); > > - /* Check if this is a temporary table name. Allow it if a corresponding .frm file exists */ > - if (is_prefix(table_name, tmp_file_prefix) && strlen(table_name) < NAME_CHAR_LEN && > + /* > + Check if this is a temporary table name. Allow it if a corresponding .frm > + file exists. > + */ > + if (!(flags & FN_IS_TMP) && > + is_prefix(table_name, tmp_file_prefix) && > + strlen(table_name) < NAME_CHAR_LEN && good idea! > check_if_frm_exists(tbbuff, dbbuff, table_name)) > flags|= FN_IS_TMP; > > @@ -9817,7 +9827,7 @@ do_continue:; > > if (table->s->tmp_table != NO_TMP_TABLE) > { > - /* Close lock if this is a transactional table */ > + /* Unlock lock if this is a transactional temporary table */ strictly speaking, you don't "unlock lock", you "unlock a resource" (e.g. a table) or you "release a lock" > if (thd->lock) > { > if (thd->locked_tables_mode != LTM_LOCK_TABLES && Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 9c1e36696f3: MDEV-20025: ADD_MONTHS() Oracle function
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! On Apr 16, Michael Widenius wrote: > commit 9c1e36696f3 > Author: Michael Widenius <michael.widenius(a)gmail.com> > Date: Fri Feb 5 17:10:43 2021 +0200 > > diff --git a/mysql-test/suite/compat/oracle/r/func_add_months.result b/mysql-test/suite/compat/oracle/r/func_add_months.result > new file mode 100644 > index 00000000000..0e8ca1eef94 > --- /dev/null > +++ b/mysql-test/suite/compat/oracle/r/func_add_months.result > @@ -0,0 +1,85 @@ > +Test for ADD_MONTHS > +CREATE TABLE t1(c1 int, c2 datetime, c3 date, c4 time, c5 timestamp); > +INSERT INTO t1 VALUES (1, '2011-11-12 12:10:11', '2011-11-12', '12:10:11', '2011-11-12 12:10:11'); > +INSERT INTO t1 VALUES (2, '2021-11-12 00:23:12', '2021-11-12', '00:23:12', '2021-11-12 00:23:12'); > +INSERT INTO t1 VALUES (3, '2011-01-22 16:45:45', '2011-01-22', '16:45:45', '2011-01-22 16:45:45'); > +INSERT INTO t1 VALUES (4, '2031-05-12 04:11:34', '2031-05-12', '04:11:34', '2031-05-12 04:11:34'); > +INSERT INTO t1 VALUES (5, '2031-09-02 08:15:22', '2031-09-02', '08:15:22', '2031-09-02 08:15:22'); > +INSERT INTO t1 VALUES (6, '0000-09-02 00:00:00', '0000-09-02', '00:00:00', '1980-09-02 00:00:00'); > +INSERT INTO t1 VALUES (7, '9999-09-02', '9999-09-02', '00:00:00', '1980-09-02'); there's no last-day test here. Like ADD_MONTHS('2012-01-31', 1) = '2012-02-29' should be here, as Oracle manual specifically documents this case. Regards, Sergei

2 1

Re: [Maria-developers] fbe1dad15a1: Ensure that we do not allocate strings bigger than 4G in String objects.
by Sergei Golubchik 14 May '21

14 May '21

Hi, Monty! On Apr 16, Michael Widenius wrote: > revision-id: fbe1dad15a1 (mariadb-10.5.2-555-gfbe1dad15a1) > parent(s): 57c19902326 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2021-03-25 12:06:34 +0200 > message: > > Ensure that we do not allocate strings bigger than 4G in String objects. > > This is needed as we are using uint32 for allocated and current length. > > diff --git a/sql/sql_string.cc b/sql/sql_string.cc > index 9c57bb22085..fedf5f4a48a 100644 > --- a/sql/sql_string.cc > +++ b/sql/sql_string.cc > @@ -37,6 +37,8 @@ bool Binary_string::real_alloc(size_t length) > DBUG_ASSERT(arg_length > length); > if (arg_length <= length) > return TRUE; /* Overflow */ > + if (arg_length >= UINT_MAX32) > + return FALSE; > str_length=0; > if (Alloced_length < arg_length) > { > @@ -45,7 +47,6 @@ bool Binary_string::real_alloc(size_t length) > arg_length,MYF(MY_WME | (thread_specific ? > MY_THREAD_SPECIFIC : 0))))) > return TRUE; > - DBUG_ASSERT(length < UINT_MAX32); > Alloced_length=(uint32) arg_length; > alloced=1; I liked assert more. It meant that we should never under any circumstances request more than 4G from Binary_string::real_alloc. You're saying that it's ok to do it and Binary_string::real_alloc should be ready to deal with it. I'd say the first approach was better, >4G should just never happen. Regards, Sergei

2 1