October 2020 - developers - lists.mariadb.org

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] 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] 9bf4b92cbc5: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 31 Mar '21

31 Mar '21

Hi, Aleksey! On Jun 29, Aleksey Midenkov wrote: > revision-id: 9bf4b92cbc5 (mariadb-10.5.2-168-g9bf4b92cbc5) > parent(s): 478301d9b9a > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-04-17 17:04:24 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > > == Syntax change == > > Keyword AUTO_INCREMENT (or AUTO) enables partition auto-creation. I still think that AUTO_INCREMENT here looks rather out-of-place, I don't know how to explain to users why AUTO_INCREMENT is allowed here. But if we'll have it, we'll have to support it here for a long time for compatibility reasons. "Because somebody might be already using it" I'd suggest to support only AUTO here. Or, if the parser can do it easily, a much more readable syntax would be create table t1 (x int) with system versioning auto partition by system_time interval 1 hour; this would be the best, easy to read, matches the documentation (where it could be called "auto partitioning"). If it won't cause any difficult parser problems, I'd prefer us to use that. > create or replace table t1 (x int) with system versioning > partition by system_time interval 1 hour auto_increment; > > create or replace table t1 (x int) with system versioning > partition by system_time limit 1000 auto; > > Or with explicit partitions: > > create or replace table t1 (x int) with system versioning > partition by system_time interval 1 hour auto > (partition p0 history, partition pn current); > > == Description == > > Before executing history-generating DML command add N history > partitions, so that N would be sufficient for potentially generated > history. N > 1 may be required when history is rotated by INTERVAL and > timestamp was jumped to future further than interval value. > > It is assumed that one DML command will not generate history rows more > than specified LIMIT. Otherwise history would overflow generated > partition, a warning would be printed, and user action would be > required to rebuild partitions. > > Auto-creation is implemented by synchronous > fast_alter_partition_table() call from the thread of the executed DML > command before the command itself (by the fallback and retry mechanism > similar to Discovery feature, see Open_table_context). > > Creating history partitions was made by the principle of minimal > disruption of the main business process. I.e. when procedure of > creation fails it will not (if possible) fail the original DML > command. Warning info will display what happened and the last history > partition may overflow. In such case partition rebuild is required to > correctly display history info. User application may detect warning > code ER_VERS_HIST_PART_FAILED and stop execution if that is preferred. > > The name for newly added partitions are generated like default > partition names with extension of MDEV-22155 (which avoids name > clashes by extending assignment counter to next free-enough gap). > > These DML commands trigger auto-creation: > > * DELETE (including multi-delete, excluding DELETE HISTORY) > * UPDATE (including multi-update) > * REPLACE (including REPLACE .. SELECT) INSERT ... ON DUPLICATE KEY UPDATE ? > diff --git a/mysql-test/suite/versioning/common.inc b/mysql-test/suite/versioning/common.inc > index 355b571e5a0..b35a5138015 100644 > --- a/mysql-test/suite/versioning/common.inc > +++ b/mysql-test/suite/versioning/common.inc > @@ -6,6 +6,7 @@ if (!$TEST_VERSIONING_SO) > source include/have_innodb.inc; > > set @@session.time_zone='+00:00'; > +set @@global.time_zone='+00:00'; Why is that? I'd expect your auto-adding of partitions to happen in the context of a session, using session time zone. Is it for replication? > select ifnull(max(transaction_id), 0) into @start_trx_id from mysql.transaction_registry; > set @test_start=now(6); > > diff --git a/mysql-test/suite/versioning/r/delete_history.result b/mysql-test/suite/versioning/r/delete_history.result > index cb865a835b3..90c9e4777bb 100644 > --- a/mysql-test/suite/versioning/r/delete_history.result > +++ b/mysql-test/suite/versioning/r/delete_history.result > @@ -63,8 +63,6 @@ insert into t values (1); > update t set a= 2; > update t set a= 3; > delete history from t; > -Warnings: > -Warning 4114 Versioned table `test`.`t`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions good riddance, it didn't make much sense anyway > # The above warning is one command late (MDEV-20345) ^^^ > select * from t for system_time all; > a > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index 85cb736b5bd..5fb893b4f0e 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1623,7 +1623,7 @@ class ha_partition :public handler could you also, please, fix (in a separate commit) ha_partition::part_records() to take a proper partition_element* argument, not void* ? > 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); > file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_OPEN); > part_recs+= file->stats.records; > } > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index e9dbf2b49c3..72312ee7ac4 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -814,10 +814,16 @@ bool partition_info::has_unique_name(partition_element *element) > vers_info->interval Limit by fixed time interval > vers_info->hist_part (out) Working history partition > */ > -void partition_info::vers_set_hist_part(THD *thd) > +uint partition_info::vers_set_hist_part(THD *thd, bool auto_inc) let's not all it auto_inc. it'll add noise to every grep for auto_inc issues. What about auto_add ? > { > + DBUG_ASSERT(!thd->lex->last_table()->vers_conditions.delete_history); > + > + uint create_count= 0; > + auto_inc= auto_inc && vers_info->auto_inc; > + > if (vers_info->limit) > { > + DBUG_ASSERT(!vers_info->interval.is_set()); > ha_partition *hp= (ha_partition*)(table->file); > partition_element *next= NULL; > List_iterator<partition_element> it(partitions); > @@ -836,22 +842,26 @@ void partition_info::vers_set_hist_part(THD *thd) > { > if (next == vers_info->now_part) > { > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name, "LIMIT"); > + if (auto_inc) > + create_count= 1; > + else > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "LIMIT"); > } > else > vers_info->hist_part= next; > } > - return; > + // reserve at least one history partition > + if (auto_inc && create_count == 0 && > + vers_info->hist_part->id + 1 == vers_info->now_part->id) > + create_count= 1; Questionable. What does it solve? > } > - > - if (vers_info->interval.is_set()) > + else if (vers_info->interval.is_set() && > + vers_info->hist_part->range_value <= thd->query_start()) > { > - if (vers_info->hist_part->range_value > thd->query_start()) > - return; > - > partition_element *next= NULL; > + bool error= true; > List_iterator<partition_element> it(partitions); > while (next != vers_info->hist_part) > next= it++; > @@ -860,12 +870,200 @@ void partition_info::vers_set_hist_part(THD *thd) > { > vers_info->hist_part= next; > if (next->range_value > thd->query_start()) > - return; > + { > + error= false; > + break; but here you don't "reserve at least one history partition" Why INTERVAL is different? > + } > + } > + if (error) > + { > + if (auto_inc) > + { > + DBUG_ASSERT(thd->query_start() >= vers_info->hist_part->range_value); > + my_time_t diff= thd->query_start() - vers_info->hist_part->range_value; > + if (diff > 0) > + { > + size_t delta= vers_info->interval.seconds(); > + create_count= diff / delta + 1; > + if (diff % delta) > + create_count++; > + } > + else > + create_count= 1; > + } > + else > + { > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "INTERVAL"); > + } > } > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name, "INTERVAL"); > } > + > + return create_count; > +} > + > + > +/** > + @brief Run fast_alter_partition_table() to add new history partitions > + for tables requiring them. > +*/ > +void vers_add_auto_parts(THD *thd, TABLE_LIST* tl, uint num_parts) > +{ > + HA_CREATE_INFO create_info; > + Alter_info alter_info; > + String query; > + partition_info *save_part_info= thd->work_part_info; > + Query_tables_list save_query_tables; > + Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer; > + Diagnostics_area new_stmt_da(thd->query_id, false, true); > + Diagnostics_area *save_stmt_da= thd->get_stmt_da(); > + bool save_no_write_to_binlog= thd->lex->no_write_to_binlog; > + const CSET_STRING save_query= thd->query_string; > + thd->m_reprepare_observer= NULL; > + thd->lex->reset_n_backup_query_tables_list(&save_query_tables); > + thd->in_sub_stmt|= SUB_STMT_AUTO_HIST; > + thd->lex->no_write_to_binlog= !thd->is_current_stmt_binlog_format_row(); > + TABLE *table= tl->table; > + > + DBUG_ASSERT(!thd->is_error()); > + /* NB: we have to preserve m_affected_rows, m_row_count_func, m_last_insert_id, etc */ > + thd->set_stmt_da(&new_stmt_da); > + new_stmt_da.set_overwrite_status(true); Is it needed? You've just started using a new Diagnostics_area, the status should be DA_EMPTY here, nothing to overwrite. > + > + { > + DBUG_ASSERT(table->s->get_table_ref_type() == TABLE_REF_BASE_TABLE); > + DBUG_ASSERT(table->versioned()); > + DBUG_ASSERT(table->part_info); > + DBUG_ASSERT(table->part_info->vers_info); > + alter_info.reset(); > + alter_info.partition_flags= ALTER_PARTITION_ADD|ALTER_PARTITION_AUTO_HIST; > + create_info.init(); > + create_info.alter_info= &alter_info; > + Alter_table_ctx alter_ctx(thd, tl, 1, &table->s->db, &table->s->table_name); > + > + MDL_REQUEST_INIT(&tl->mdl_request, MDL_key::TABLE, tl->db.str, > + tl->table_name.str, MDL_SHARED_NO_WRITE, MDL_TRANSACTION); > + if (thd->mdl_context.acquire_lock(&tl->mdl_request, > + thd->variables.lock_wait_timeout)) > + goto exit; > + table->mdl_ticket= tl->mdl_request.ticket; > + > + create_info.db_type= table->s->db_type(); > + create_info.options|= HA_VERSIONED_TABLE; > + DBUG_ASSERT(create_info.db_type); > + > + create_info.vers_info.set_start(table->s->vers_start_field()->field_name); > + create_info.vers_info.set_end(table->s->vers_end_field()->field_name); > + > + partition_info *part_info= new partition_info(); > + if (unlikely(!part_info)) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + goto exit; > + } > + part_info->use_default_num_partitions= false; > + part_info->use_default_num_subpartitions= false; > + part_info->num_parts= num_parts; > + part_info->num_subparts= table->part_info->num_subparts; > + part_info->subpart_type= table->part_info->subpart_type; > + if (unlikely(part_info->vers_init_info(thd))) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + goto exit; > + } > + > + // NB: set_ok_status() requires DA_EMPTY > + thd->get_stmt_da()->reset_diagnostics_area(); Is it needed? You've just started using a new Diagnostics_area, the status should be DA_EMPTY here. > + > + thd->work_part_info= part_info; > + if (part_info->set_up_defaults_for_partitioning(thd, table->file, NULL, > + table->part_info->next_part_no(num_parts))) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "setting up defaults failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + bool partition_changed= false; > + bool fast_alter_partition= false; > + if (prep_alter_part_table(thd, table, &alter_info, &create_info, > + &partition_changed, &fast_alter_partition)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter partitition prepare failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + if (!fast_alter_partition) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "fast alter partitition is not possible"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + DBUG_ASSERT(partition_changed); > + if (mysql_prepare_alter_table(thd, table, &create_info, &alter_info, > + &alter_ctx)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter prepare failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + > + // Forge query string for rpl logging > + if (!thd->lex->no_write_to_binlog) > + { > + query.set(STRING_WITH_LEN("ALTER TABLE `"), &my_charset_latin1); better use StringBuffer<...> query; because you're doing lots of reallocs now. > + > + if (query.append(table->s->db) || > + query.append(STRING_WITH_LEN("`.`")) || > + query.append(table->s->table_name) || this is wrong, the table and db names can have backticks inside. use append_identifier() instead. > + query.append("` ADD PARTITION PARTITIONS ") || > + query.append_ulonglong(part_info->num_parts) || > + query.append(" AUTO")) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(ME_ERROR_LOG)); > + goto exit; > + } > + CSET_STRING qs(query.c_ptr(), query.length(), &my_charset_latin1); > + thd->set_query(qs); > + } Should it be binlogged at all? May be just leave it to the slave to auto-add the partition as needed? Looks a bit suspicious now, you log an ALTER TABLE possibly in the middle of a transaction. It will be replayed differently, not as auto-adding, in particular, it will commit. So, possibly different results on the slave, perhaps different gtid. Do clear it out with Andrei please. Or don't binlog auto-adding at all. > + > + if (fast_alter_partition_table(thd, table, &alter_info, &create_info, > + tl, &table->s->db, &table->s->table_name)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter partition table failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + } > + } > + > + if (!thd->transaction.stmt.is_empty()) > + trans_commit_stmt(thd); 1. why would this operation register a transaction? 2. you amended a couple of checks like - if (thd->in_sub_stmt) + if (thd->in_sub_stmt & ~SUB_STMT_AUTO_HIST) but what will happen if it's, for example, an insert inside a trigger? So a real sub_stmt and SUB_STMT_AUTO_HIST? > + > +exit: > + thd->work_part_info= save_part_info; > + thd->m_reprepare_observer= save_reprepare_observer; > + thd->lex->restore_backup_query_tables_list(&save_query_tables); > + thd->in_sub_stmt&= ~SUB_STMT_AUTO_HIST; > + if (!new_stmt_da.is_warning_info_empty()) > + save_stmt_da->copy_sql_conditions_from_wi(thd, new_stmt_da.get_warning_info()); > + thd->set_stmt_da(save_stmt_da); > + thd->lex->no_write_to_binlog= save_no_write_to_binlog; > + thd->set_query(save_query); > } > > > diff --git a/sql/partition_info.h b/sql/partition_info.h > index 7ae2d168068..ca68e61cbe2 100644 > --- a/sql/partition_info.h > +++ b/sql/partition_info.h > @@ -72,9 +73,34 @@ struct Vers_part_info : public Sql_alloc > my_time_t start; > INTERVAL step; > enum interval_type type; > - bool is_set() { return type < INTERVAL_LAST; } > + bool is_set() const { return type < INTERVAL_LAST; } > + size_t seconds() const > + { > + if (step.second) > + return step.second; > + if (step.minute) > + return step.minute * 60; > + if (step.hour) > + return step.hour * 3600; > + if (step.day) > + return step.day * 3600 * 24; > + // comparison is used in rough estimates, it doesn't need to be calendar-correct Are you sure the approximate value is enough here? You use it to estimate how many partitions to create. It's not a big deal if you create more than necessary (although it's not nice and we'll definitely get bug reports if that will happen). But you surely don't want to create less. > + if (step.month) > + return step.month * 3600 * 24 * 30; > + DBUG_ASSERT(step.year); > + return step.year * 86400 * 30 * 365; > + } > + bool lt(size_t secs) const > + { > + return seconds() < secs; > + } > + bool ge(size_t seconds) const > + { > + return !(this->lt(seconds)); > + } lt() and ge() don't seem to be used anywhere. > } interval; > ulonglong limit; > + bool auto_inc; > partition_element *now_part; > partition_element *hist_part; > }; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index b89be77f282..5583d70f8eb 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1862,6 +1862,25 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > DBUG_PRINT("info",("Using locked table")); > #ifdef WITH_PARTITION_STORAGE_ENGINE > part_names_error= set_partitions_as_used(table_list, table); > + if (table->part_info && > + table->part_info->part_type == VERSIONING_PARTITION && > + !table_list->vers_conditions.delete_history && > + table_list->lock_type >= TL_WRITE_ALLOW_WRITE && > + table_list->mdl_request.type == MDL_SHARED_WRITE) > + { > + switch (thd->lex->sql_command) > + { > + case SQLCOM_DELETE: > + case SQLCOM_UPDATE: > + case SQLCOM_REPLACE: > + case SQLCOM_REPLACE_SELECT: > + case SQLCOM_DELETE_MULTI: > + case SQLCOM_UPDATE_MULTI: > + /* Rotation is still needed under LOCK TABLES */ > + table->part_info->vers_set_hist_part(thd, false); ALTER TABLE works under LOCK TABLES, so this should be possible too, but let's have it as a separate task. > + default:; > + } > + } > #endif > goto reset; > } > @@ -2104,6 +2123,37 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > tc_add_table(thd, table); > } > > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (table->part_info && > + table->part_info->part_type == VERSIONING_PARTITION && > + !table_list->vers_conditions.delete_history && > + table_list->lock_type >= TL_WRITE_ALLOW_WRITE && > + table_list->mdl_request.type >= MDL_SHARED_WRITE && > + table_list->mdl_request.type < MDL_EXCLUSIVE) > + { > + switch (thd->lex->sql_command) > + { > + 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: this is quite complex condition, better not to duplicate it. May be something like bool need_set_hist_part(TABLE_LIST *table_list, enum_sql_command sql_command) { if (table_list->table->part_info && table_list->table->part_info->part_type == VERSIONING_PARTITION && !table_list->vers_conditions.delete_history && table_list->lock_type >= TL_WRITE_ALLOW_WRITE && table_list->mdl_request.type >= MDL_SHARED_WRITE && table_list->mdl_request.type < MDL_EXCLUSIVE) { switch(sql_command) { 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: return true; } default:; } return false; } > + ot_ctx->vers_create_count= table->part_info->vers_set_hist_part(thd, true); > + if (ot_ctx->vers_create_count) > + { > + ot_ctx->request_backoff_action(Open_table_context::OT_ADD_HISTORY_PARTITION, > + table_list); > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); There's no MYSQL_UNBIND_TABLE/tc_release_table after other request_backoff_action invocations. Why this one is special? > + DBUG_RETURN(TRUE); > + } > + default:; > + } > + } > +#endif > + > if (!(flags & MYSQL_OPEN_HAS_MDL_LOCK) && > table->s->table_category < TABLE_CATEGORY_INFORMATION) > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 14516811262..145ac5c5f64 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3346,6 +3347,10 @@ class THD: public THD_count, /* this must be first */ > > #ifdef WITH_PARTITION_STORAGE_ENGINE > partition_info *work_part_info; > + /** > + List of tables requiring new history partition. > + */ > + List<TABLE_SHARE> vers_auto_part_tables; this doesn't seem to be used anywhere > #endif > > #ifndef EMBEDDED_LIBRARY > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 7cc1faea79b..3f6c1793432 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -7521,14 +7529,17 @@ add_partition_rule: > > add_part_extra: > /* empty */ > - | '(' part_def_list ')' > + | '(' part_def_list ')' opt_vers_auto_inc > { > - LEX *lex= Lex; > - lex->part_info->num_parts= lex->part_info->partitions.elements; > + Lex->part_info->num_parts= Lex->part_info->partitions.elements; > + if ($4) > + Lex->alter_info.partition_flags|= ALTER_PARTITION_AUTO_HIST; > } > - | PARTITIONS_SYM real_ulong_num > + | PARTITIONS_SYM real_ulong_num opt_vers_auto_inc > { > Lex->part_info->num_parts= $2; > + if ($3) > + Lex->alter_info.partition_flags|= ALTER_PARTITION_AUTO_HIST; I'm confused. I thought that ALTER_PARTITION_AUTO_HIST is what you set in vers_add_auto_parts() to mark auto-adding of a new partition. But here you set it in the parser, when a user specifies AUTO in partition specifications. So it seems you're using this flag for two very different purposes. How do you distinguish between them? And why did you decide to do it this way? > } > ; > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index fbed614489d..25017ee5425 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -4825,6 +4829,13 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info, > DBUG_RETURN(TRUE); > } > > + if (alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST && > + (!table->part_info || !table->part_info->vers_info)) > + { > + my_error(ER_SYNTAX_ERROR, MYF(0)); can it even happen? Or should it be DBUG_ASSERT(0) here? > + DBUG_RETURN(TRUE); > + } > + > partition_info *alt_part_info= thd->lex->part_info; > /* > This variable is TRUE in very special case when we add only DEFAULT > @@ -5312,7 +5323,9 @@ that are reorganised. > now_part= el; > } > } > - if (*fast_alter_table && tab_part_info->vers_info->interval.is_set()) > + if (*fast_alter_table && > + !(alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST) && > + tab_part_info->vers_info->interval.is_set()) this !ALTER_PARTITION_AUTO_HIST - do you mean not vers_add_auto_parts() or not sql statement that uses AUTO ? > { > partition_element *hist_part= tab_part_info->vers_info->hist_part; > if (hist_part->range_value <= thd->query_start()) > @@ -5347,7 +5360,8 @@ that are reorganised. > */ > if (!(alter_info->partition_flags & ALTER_PARTITION_TABLE_REORG)) > { > - if (!alt_part_info->use_default_partitions) > + if (!alt_part_info->use_default_partitions && > + !(alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST)) Sorry, I don't understand that if() at all. What does it do? > { > DBUG_PRINT("info", ("part_info: %p", tab_part_info)); > tab_part_info->use_default_partitions= FALSE; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

[Maria-developers] MDEV-21287 dev community opinion wanted
by Pierre-Jean Clement 02 Dec '20

02 Dec '20

Hi all, I created the Jira ticket MDEV-21287 10 months ago and didn't get much feedback. I tried to take a shot at it myself, but found out that the resolution MDEV-20403 introduced this behavior by purposefully evaluating 'update functions' before the BEFORE UPDATE trigger. I couldn't really understand why this was necessary, and I'd like to get feedback from the dev community to try and understand what is really supposed to happen on update. I kinda hate getting my trigger called with a new value that's not going to be persisted anyway, what do you think about that? Regards, Pierre-Jean

2 3

Re: [Maria-developers] 0cd5377e9ff: MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
by Sergei Golubchik 02 Nov '20

02 Nov '20

Hi, Aleksey! On Oct 20, Aleksey Midenkov wrote: > revision-id: 0cd5377e9ff (mariadb-10.2.31-357-g0cd5377e9ff) > parent(s): dc716da4571 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-08-17 00:52:35 +0300 > message: > > MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > > update_virtual_fields() may reallocate Field_blob::value buffer due to > different reasons (f.ex. different types, see > Field_str::memcpy_field_possible()). But the buffer address may be > already in m_ordered_rec_buffer which is then used in ordered index > scan queue. > > The patch updates blob virtual fields when the ordered index scan > queue is sorted as well as the top record is restored from the queue. > > New VCOL_UPDATE_BLOBS mode is introduced to update only blobs. Swap is > not needed as it was already done if it was needed. > > Related to ea1b2504 No, I think we need to go deeper here. The bug is not only not caused by system versioning, it is also not caused by virtual columns. The problem is that partitioning copies record[0] out and then back to save and restore values. This does not work for blobs, because record[0] may store the pointer to the memory that Field_blob owns and can free or reallocate as needed. Normally it's not the problem because typically after row reads the engine owns the memory, not Field_blob. When a virtual column is set to a calculated value - then Field_blob owns the memory and we have this bug. But there are engines that use field->store() to set values - in this case Field_blob will own the memory too. Here's a test case with FederatedX: ============================================================= source include/have_partition.inc; source include/have_sequence.inc; source have_federatedx.inc; source include/federated.inc; 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; 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 select seq, seq from seq_1_to_35; insert t1 select seq, repeat(seq, 100) from seq_50_to_90; flush tables; select x, b from t1 where x > 30 and x < 60 order by x; select x, b from t1 where x > 30 and x < 60 order by b; drop table t1; connection slave; drop table t1_1, t1_2; source include/federated_cleanup.inc; ============================================================= first select doesn't trigger ASAN error, but it produces incorrect results (I'm showing it here, so that you could create a non-ASAN test that could be run on all builders). the second select gets the same ASAN error as your test below. > diff --git a/mysql-test/suite/vcol/r/partition.result b/mysql-test/suite/vcol/r/partition.result > index bd1353fa145..74c3c3394dc 100644 > --- a/mysql-test/suite/vcol/r/partition.result > +++ b/mysql-test/suite/vcol/r/partition.result > @@ -28,3 +28,26 @@ set statement sql_mode= '' for update t1 set i= 1, v= 2; > Warnings: > Warning 1906 The value specified for generated column 'v' in table 't1' has been ignored > drop table t1; > +# > +# MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > +# > +# 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 above doesn't cause ASAN failures for me without your fix. the test below fails all right. > +# 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; > +drop table t1; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 7807d9b6939: MDEV-22974: mysql_native_password make "invalid" valid
by Sergei Golubchik 30 Oct '20

30 Oct '20

Hi, Daniel! On Oct 22, Daniel Black wrote: > revision-id: 7807d9b6939 (mariadb-10.5.4-62-g7807d9b6939) > parent(s): 054f10365c4 > author: Daniel Black <daniel(a)mariadb.org> > committer: Daniel Black <daniel(a)mariadb.org> > timestamp: 2020-07-16 17:06:36 +1000 > message: > > MDEV-22974: mysql_native_password make "invalid" valid > > Per b9f3f06857ac, mysql_system_tables_data.sql creates > a mysql_native_password with a salted hash of "invalid" so that `set password` > will detect a native password can be pplied:. "applied" > > SHOW CREATE USER; dilignently uses this value in its output > generating the SQL: > > MariaDB [(none)]> show create user; > > +---------------------------------------------------------------------------------------------------+ > | CREATE USER for dan@localhost | > +---------------------------------------------------------------------------------------------------+ > | CREATE USER `dan`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket | > +---------------------------------------------------------------------------------------------------+ > > Attempting to execute this before this patch resutls in: > > MariaDB [(none)]> CREATE USER `dan2`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket; > ERROR 1372 (HY000): Password hash should be a 41-digit hexadecimal number > > As such deep the the implementation of mysql_native_password we make > "invalid" valid (pun intended) such that the above create user will > succeed. > > native_password_get_salt is only called in the context of > set_user_salt, so all setting of native passwords to hashed content of > 'invalid', quite literally create an invalid password. > > So other forms of "invalid" are valid SQL in creating invalid passwords: > > MariaDB [(none)]> set password = 'invalid'; > Query OK, 0 rows affected (0.001 sec) > > MariaDB [(none)]> alter user dan@localhost IDENTIFIED BY PASSWORD 'invalid'; > Query OK, 0 rows affected (0.000 sec) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index d94016b7815..3cd7a67ae1a 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length, > > if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) > { > + if (hash_length == 7 && strcmp(hash, "invalid") == 0) > + { > + memcpy(out, "invalid", 7); > + *out_length= 7; > + return 0; > + } okay. After you said ASAN, I think I can see why this could be problematic. You can, of course, pad it to SCRAMBLED_PASSWORD_CHAR_LENGTH, but then you'll create a *valid* scramble that would correspond to some actual password. One option would be to allow "invalid" literal in set_user_auth(), before even any plugin checks. But I'm unsure of the implications. > my_error(ER_PASSWD_LENGTH, MYF(0), SCRAMBLED_PASSWORD_CHAR_LENGTH); > return 1; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f112c4a07475: MDEV-20787: Script dgcov.pl does not work
by Sergei Golubchik 30 Oct '20

30 Oct '20

Hi, Anel! On Oct 30, Anel Husakovic wrote: > revision-id: f112c4a07475 (mariadb-10.2.31-541-gf112c4a07475) > parent(s): 784473b98662 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-26 14:25:41 +0100 > message: > > MDEV-20787: Script dgcov.pl does not work > > - Patch is changing CMakeList with `--coverage` flag as an alias for > `-fprofile-arcs -ftest-coverage -lgcov` in addition. > > - About the usage of `dgcov`: > When the server is compiled with `-DENABLE_GCOV=ON`, from object files are generated > `.gcno` and `.gcda` files. > `./mtr --gcov is_check_constraint` is invoking the following script calls: > - `./dgcov.pl --purge`, `./mtr is_check_constraint` and > - `./dgcov.pl --generate>/var/last_changes.dgcov`. > The `purge` flag is clearing `.gcda` files (and others extensions), > while running the test (which follows after the purge) new `.gcda` files are generated. > With dgcov `generate` flag, `gcov -i` (`intermediate format`) is called > on generated `<source-files-name>.gcda` files (`dbug.c.gcda` e.g.). > The patch is tested on `gcov 6.3` and `gcov 7.4` versions > and can be seen that resulting `.gcov` file for `6.3` creates > `<full path>.gcov` (`dbug.c.gcda.gcov` e.g.) file, > where `gcov 7.4+` is still creating `source-file-names.gcov`(`dbug.c.gcov`) files > as `gcov` in general is doing. > Results are stored in `./mysql-test/var/last_changes.dgcov`. > Note: for the newer versions of `gcov` intermediate format is > deprecated `-i` and should be used `--json-format`. does it mean that eventually we'll have to rewrite dgcov to support json format? > > - This patch doesn't take gcov versions in account since is extracting > filename of a `.gcov` file, which is generated with `gcov -i > <source-file>.gcda` during `dgcov.pl --generate`. > > - There is a check if file exists. > Reason for that is because some generated `gcov` files are outliers from above > rule (that a file generated in intermediate format(gcov) has the same source name). > Example 1: `aestables.cpp.gcda` is not generating `aestables.cpp.gcov`, > but only the files used in #include `modes.hpp.gcov runtime.hpp.gcov` > Example 2: `my_sha256.cc.gcda` is generating `my_sha.ic.gcov` > > - Gcov with out-of-source is not working, this patch will solve that > (probably?) > Make sure to test `MTR_BINDIR` which gets set during out-of-source > build and generate an error when running `./mysql-test/mtr --gcov alias`. > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index fbc5540e6973..f2bbdad4d20e 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -161,8 +161,14 @@ sub gcov_one_file { > system($cmd)==0 or die "system($cmd): $? $!"; > } I don't see any changes since the last review. Is it the correct commit? > - # now, read the generated file > - open FH, '<', "$_.gcov" or die "open(<$_.gcov): $!"; > + (my $filename = $_)=~ s/\.[^.]+$//; # remove extension I think this contradicts your explanation in the commit comment. You wrote that the file name can be either dbug.c.gcda.gcov or dbug.c.gcov. Here $_ is dbug.c.gcda, you unconditionally remove .gcda and then below you open $filename.gcov, that is dbug.c.gcov Which means this won't work with an older gcov. Is this your intention? I think it's fine not to support old gcov. But this should be a conscious decision, e.g. you can write in the commit comment "Now dgcov.pl only supports gcov 7.4+" - if that's indeed what you want. Also, if you do it this way, then there's no need to use s///, it's simpler to do, like sub gcov_one_file { return unless /\.gcda$/; + my $filename= $`; unless ($opt_skip_gcov) { or, may be even sub gcov_one_file { return unless /\.gcda$/; + my $gcov_file_path= "$File::Find::dir$`.gcov"; unless ($opt_skip_gcov) { > + my $gcov_file_path= $File::Find::dir."/$filename.gcov"; still an extra slash > + if (! -f $gcov_file_path) > + { > + return; > + } still not return unless -f $gcov_file_path; > + open FH, '<', "$gcov_file_path" or die "open(<$gcov_file_path): $!"; still extra quotes > + > my $fname; > while (<FH>) { > chomp; > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 900ef736a455..2189529d82a7 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -1791,7 +1791,7 @@ sub command_line_setup { > # -------------------------------------------------------------------------- > # Gcov flag > # -------------------------------------------------------------------------- > - if ( ($opt_gcov or $opt_gprof) and ! $source_dist ) > + if ( ($opt_gcov or $opt_gprof) and (! $source_dist or -d $ENV{MTR_BINDIR})) > { > mtr_error("Coverage test needs the source - please use source dist"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-24015: SQL Error (1038): Out of sort memory when enough memory for the sort buffer is provided
by Sergey Petrunia 27 Oct '20

27 Oct '20

Hi Varun, I have only cosmetic comments, please find them below. Ok to push after they are addressed. > The issue here was with the re-execution of a subquey, after few executions > the subquery execution returned error that not enough space was provided inside the sort > buffer. > This is happening because with the introduction of packed sort keys, during the phase > where sort length is calculated the parameters of the sort keys are not reset and > we keep up adding the sort length to the previous calculated sort length for the > earlier executions of the subquery. This happens because the sort keys structure is > allocated only once and so if their is re-execution of a subquery involving sorting > then we should be resetting the parameters of the sort keys. The comment is hard to read. How about being more specific and using shorter sentences: The error was: filesort was done in a subquery which was executed multiple times. During each execution, sortlength() computed total sort key length in Sort_keys::sort_length, without resetting it first. Eventually Sort_keys::sort_length got larger than @@sort_buffer_size, which caused filesort() to be aborted with error. Fixed by making sortlength() to compute lengths only during the first invocation. Subsequent invocations return pre-computed values. > @@ -2518,7 +2526,7 @@ void Sort_param::try_to_pack_sortkeys() > return; > > const uint sz= Sort_keys::size_of_length_field; > - uint sort_len= sort_keys->get_sort_length(); > + uint sort_len= sort_keys->get_sort_length_with_original_values(); > > /* > Heuristic introduced, skip packing sort keys if saving less than 128 bytes > diff --git a/sql/sql_sort.h b/sql/sql_sort.h > index 40f0c5ede5f..6951d4d190a 100644 > --- a/sql/sql_sort.h > +++ b/sql/sql_sort.h > @@ -258,7 +258,9 @@ class Sort_keys :public Sql_alloc, > Sort_keys_array(arr, count), > m_using_packed_sortkeys(false), > size_of_packable_fields(0), > - sort_length(0) > + sort_length_with_original_values(0), > + sort_length_with_memcmp_values(0), > + first_execution(true) This violates the coding style. Please ident the member initialization (see e.g. item.h for examples) > { > DBUG_ASSERT(!is_null()); > }a ... > @@ -307,9 +319,12 @@ class Sort_keys :public Sql_alloc, > > void increment_original_sort_length(uint len) > { > - sort_length+= len; > + sort_length_with_original_values+= len; > } > > + bool is_first_execution() { return first_execution; } > + void set_first_execution(bool val) { first_execution= val; } A comment just about the naming: I would not drag "execution" into here. "Sort_keys" are not really executed. How about changing the functions to be bool is_parameters_computed(); bool set_parameters_computed(); and the variable to be "bool parameters_computed" ? > + > static const uint size_of_length_field= 4; > > private: BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 5150dfd6ab3: MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! On Apr 27, Nikita Malyavin wrote: > revision-id: 5150dfd6ab3 (mariadb-10.3.12-230-g5150dfd6ab3) > parent(s): cf78b8c699d > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-06-12 22:11:45 +1000 > message: > > MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table > > * set modified_non_trans_table in one missed place This commit needs way more text than that. This is just too misterious now. What asserts fails? What has the test case to do with the fix? Why modified_non_trans_table even matters without replication? Why it's specific to versioning and partitioning? Where trans_safe becomes false? > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index 3fcb59bdb40..99a297417d5 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -541,6 +541,29 @@ t1 CREATE TABLE `t1` ( > PARTITION BY SYSTEM_TIME INTERVAL 7 SECOND > (PARTITION `ver_p1` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `ver_pn` CURRENT ENGINE = DEFAULT_ENGINE) > +set timestamp= default; > +# MDEV-17891 Assertion failures in select_insert::abort_result_set and > +# mysql_load upon attempt to replace into a full table > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > +pk integer auto_increment, > +primary key (pk), > +f varchar(45000) > +) with system versioning engine=memory > +partition by system_time interval 1 year (partition p1 history, > +partition pn current); > +# fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +insert into t1 (f) select f from t1; > +ERROR HY000: The table 't1' is full > +# leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +# copy all data into history partition > +replace into t1 select * from t1; > +replace into t1 select * from t1; > +ERROR HY000: The table 't1' is full > +drop table t1; > +set @@max_heap_table_size= 1048576; > # Test cleanup > drop database test; > create database test; > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index d5c83b4d3bb..23768836efc 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -474,6 +474,7 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +set timestamp= default; > > --echo # > --echo # MDEV-18136 Server crashes in Item_func_dyncol_create::prepare_arguments > @@ -489,6 +490,34 @@ partition by system_time interval column_get(column_create(7,7), 7 as int) secon > --replace_result $default_engine DEFAULT_ENGINE > show create table t1; > > +--echo # MDEV-17891 Assertion failures in select_insert::abort_result_set and > +--echo # mysql_load upon attempt to replace into a full table > + > +--let $max_heap_table_size_orig= `select @@max_heap_table_size;` > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > + pk integer auto_increment, > + primary key (pk), > + f varchar(45000) > +) with system versioning engine=memory > + partition by system_time interval 1 year (partition p1 history, > + partition pn current); > + > +--echo # fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +--error ER_RECORD_FILE_FULL > +insert into t1 (f) select f from t1; > +--echo # leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +--echo # copy all data into history partition > +replace into t1 select * from t1; > +--error ER_RECORD_FILE_FULL > +replace into t1 select * from t1; > + > +# cleanup > +drop table t1; > +eval set @@max_heap_table_size= $max_heap_table_size_orig; > + > --echo # Test cleanup > drop database test; > create database test; > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 1f3a70721fc..44502ea6704 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1953,6 +1953,8 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > if (likely(!error)) > { > info->deleted++; > + if (!table->file->has_transactions()) > + thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->versioned(VERS_TIMESTAMP)) > { > store_record(table, record[2]); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 05968211699: MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! Okay. This reverts my "ok to push" on MDEV-16481 :) If you want to store the value in unix timestamp, you don't need MYSQL_TIME in save_result. You should calculate the value as a unix timestamp in ::check and store that in save_result. Now you still have timezone conversion in ::update and that can fail. Everything that can fail should be done in ::check, that's the contract. May be also I reapplied your patches to 10.3 incorrectly when I was reviewing. Perhaps you could rebase them yourself - to be sure I'm looking at the correct patch? On Oct 22, Nikita Malyavin wrote: > revision-id: 05968211699 (mariadb-10.4.11-291-g05968211699) > parent(s): efb1023b6f4 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 17:07:03 +0200 > message: > > MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone > > * store `system_versioning_asof` in unix time; > * both session and global vars are processed in session timezone; > * setting `default` does not copy global varibale abymore. Instead, it sets system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time is applied and `now()` can be assumed > As a regression, we cannot assign values below 1970 anymore > > --- > mysql-test/suite/versioning/r/sysvars.result | 67 +++++++++++++++++++++++ > mysql-test/suite/versioning/t/sysvars.test | 80 ++++++++++++++++++++++++---- > sql/mysqld.h | 3 +- > sql/sql_select.cc | 6 ++- > sql/sys_vars.ic | 27 ++++++---- > 5 files changed, 162 insertions(+), 21 deletions(-) > > diff --git a/sql/mysqld.h b/sql/mysqld.h > index bd45ff7b798..884e5a06066 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -194,7 +194,8 @@ enum vers_system_time_t > struct vers_asof_timestamp_t > { > ulong type; > - MYSQL_TIME ltime; > + my_time_t unix_time; > + ulong second_part; > }; > > enum vers_alter_history_enum > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d0bb0c816ec..4d62a9829eb 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -722,8 +722,12 @@ bool vers_select_conds_t::init_from_sysvar(THD *thd) > if (type != SYSTEM_TIME_UNSPECIFIED && type != SYSTEM_TIME_ALL) > { > DBUG_ASSERT(type == SYSTEM_TIME_AS_OF); > + MYSQL_TIME ltime; > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, in.unix_time); > + ltime.second_part = in.second_part; > + > start.item= new (thd->mem_root) > - Item_datetime_literal(thd, &in.ltime, TIME_SECOND_PART_DIGITS); > + Item_datetime_literal(thd, &ltime, TIME_SECOND_PART_DIGITS); > if (!start.item) > return true; > } > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index f33f469b160..417fa7842c2 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic > @@ -2646,9 +2646,11 @@ class Sys_var_vers_asof: public Sys_var_enum > } > > private: > - bool update(set_var *var, vers_asof_timestamp_t &out) > + bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool) > { > bool res= false; > + uint error; > + MYSQL_TIME ltime; > out.type= static_cast<enum_var_type>(var->save_result.ulonglong_value); > if (out.type == SYSTEM_TIME_AS_OF) > { > @@ -2658,11 +2660,14 @@ class Sys_var_vers_asof: public Sys_var_enum > Datetime::Options opt(TIME_CONV_NONE | > TIME_NO_ZERO_IN_DATE | > TIME_NO_ZERO_DATE, thd); > - res= var->value->get_date(thd, &out.ltime, opt); > + res= var->value->get_date(thd, &ltime, opt); > + out.unix_time= pool.time_zone->TIME_to_gmt_sec(&ltime, &error); > + out.second_part= ltime.second_part; > + res|= (error != 0); > } > else // set DEFAULT from global var > { > - out= global_var(vers_asof_timestamp_t); > + out.type= SYSTEM_TIME_UNSPECIFIED; > res= false; > } > } > @@ -2672,11 +2677,11 @@ class Sys_var_vers_asof: public Sys_var_enum > public: > virtual bool global_update(THD *thd, set_var *var) > { > - return update(var, global_var(vers_asof_timestamp_t)); > + return update(var, global_var(vers_asof_timestamp_t), thd->variables); > } > virtual bool session_update(THD *thd, set_var *var) > { > - return update(var, session_var(thd, vers_asof_timestamp_t)); > + return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables); > } > > private: > @@ -2690,10 +2695,14 @@ class Sys_var_vers_asof: public Sys_var_enum > case SYSTEM_TIME_AS_OF: > { > uchar *buf= (uchar*) thd->alloc(MAX_DATE_STRING_REP_LENGTH); > - if (buf &&!my_datetime_to_str(&val.ltime, (char*) buf, 6)) > + MYSQL_TIME ltime; > + > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, val.unix_time); > + ltime.second_part= val.second_part; > + > + if (buf && !my_datetime_to_str(&ltime, (char*) buf, 6)) > { > - // TODO: figure out variable name > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong datetime)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong datetime)"); > return (uchar*) thd->strdup("Error: wrong datetime"); > } > return buf; > @@ -2701,7 +2710,7 @@ class Sys_var_vers_asof: public Sys_var_enum > default: > break; > } > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong range type)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong range type)"); > return (uchar*) thd->strdup("Error: wrong range type"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] caec6b4b3d7: MDEV-17573 Assertion in federatedx on multi-update
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Aleksey! Looks reasonable to me. MYSQL_RES is stored in the handler. Of course, MYSQL_ROWS *current, which is a pointer into MYSQL_RES, has to be in the handler, it cannot be shared. Ok to push, thanks! On Oct 23, Aleksey Midenkov wrote: > revision-id: caec6b4b3d7 (mariadb-10.3.21-250-gcaec6b4b3d7) > parent(s): a00c668ddf2 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-10-21 13:38:37 +0300 > message: > > MDEV-17573 Assertion in federatedx on multi-update > > Cause: shared federatedx_io cannot store table-specific data. > > Fix: move current row reference `federatedx_io_mysql::current` to > ha_federatedx. > > FederatedX connection (represented by federatedx_io) is stored into > federatedx_txn::txn_list of per-server connections (see > federatedx_txn::acquire()). federatedx_txn object is stored into THD > (see ha_federatedx::external_lock()). When multiple handlers acquire > FederatedX connection they get single federatedx_io instance. Multiple > handlers do their operation via federatedx_io_mysql::mark_position() > and federatedx_io_mysql::fetch_row() in arbitrarty manner. They access > the same federatedx_io_mysql instance and same MYSQL_ROWS *current > pointer, so one handler disrupts the work of the other. > > Related to "MDEV-14551 Can't find record in table on multi-table update > with ORDER BY". Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2e0bac4688a: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! On Oct 23, Anel Husakovic wrote: > revision-id: 2e0bac4688a (mariadb-10.2.31-517-g2e0bac4688a) > parent(s): 985ede92034 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 12:38:11 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/r/grant5.result b/mysql-test/r/grant5.result > index 338bbd36e14..a69070e8399 100644 > --- a/mysql-test/r/grant5.result > +++ b/mysql-test/r/grant5.result ... > +SHOW GRANTS; > +Grants for root@localhost > +GRANT test_role TO 'root'@'localhost' WITH ADMIN OPTION > +GRANT ALL PRIVILEGES ON *.* TO 'root'@'localhost' WITH GRANT OPTION > +GRANT PROXY ON ''@'%' TO 'root'@'localhost' WITH GRANT OPTION > +GRANT USAGE ON *.* TO 'test_role' > +SET DEFAULT ROLE test_role FOR 'root'@'localhost' ... Yes, that's what I mean. I don't have a strong opinion on that, it seems that I very slightly prefer to have SET DEFAULT ROLE after GRANT PROXY, that is to have all statements for root, and then statements for roles. But having SET DEFAULT ROLE at the end makes sense too - all GRANTs first, then SET DEFAULT ROLE. Just make it the way you want. But either way it's good to have a test for it, a test that shows we've considered this issue and intentionally implemented that particular order. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6d881a271a9: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Rucha! On Oct 23, Rucha Deodhar wrote: > revision-id: 6d881a271a9 (mariadb-10.5.4-108-g6d881a271a9) > parent(s): bbd70fcc43c > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-10-23 12:57:02 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45ce4be3eb5..4ce7b034029 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -7959,7 +7959,7 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > bool any_privileges, uint *hidden_bit_fields) > { > Field_iterator_table_ref field_iterator; > - bool found; > + bool found, is_insert_or_replace_returning= false; > char name_buff[SAFE_NAME_LEN+1]; > DBUG_ENTER("insert_fields"); > DBUG_PRINT("arena", ("stmt arena: %p",thd->stmt_arena)); > @@ -7978,13 +7978,31 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > > found= FALSE; > > + if ((thd->lex->sql_command == SQLCOM_INSERT_SELECT || > + thd->lex->sql_command == SQLCOM_REPLACE_SELECT || > + thd->lex->sql_command == SQLCOM_INSERT || > + thd->lex->sql_command == SQLCOM_REPLACE) && > + thd->lex->has_returning()) > + is_insert_or_replace_returning= true; > + > /* > 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. > + Also, When we have INSERT/REPLACE...RETURNING and have qualified asterisk, > + table_name is not NULL and context->table_list is either NULL or has > + incorrect reference because context->table_list has tables from the > + FROM clause. > + context->table_list has incorrect reference (has table from the FROM clause > + instead of table we are inserting into) for > + INSERT/REPLACE...SELECT...RETURNING because we have a FROM clause from the > + SELECT statement. For INSERT/REPLACE...RETURNING it is NULL because > + there is no FROM clause. > */ > - for (TABLE_LIST *tables= (table_name ? context->table_list : > - context->first_name_resolution_table); > + for (TABLE_LIST *tables= (table_name ? (is_insert_or_replace_returning ? > + context->first_name_resolution_table : > + context->table_list) : > + (context->first_name_resolution_table)); ok, I've compiled it to run few queries, now I see that this is wrong. you need to understand the old code and the old comment before modifying them. one way to do it is to change the condition from table_name ? context->table_list : context->first_name_resolution_table to simply context->first_name_resolution_table and see what breaks. You'll see that main.type_varchar will break with At line 319: query 'SELECT t1.* FROM t1 LEFT JOIN t2 USING (c1)' failed: 1051: Unknown table 'test.t1' indeed, t1 is only present in context->table_list, but not in context->first_name_resolution_table. So after your fix the following statement will no longer work: INSERT INTO t2 SELECT t1.* FROM t1 JOIN t1 AS t USING (id1) WHERE id1=1 RETURNING *; it's because you set is_insert_or_replace_returning both for the SELECT and for the RETURNING clauses. > tables; > tables= (table_name ? tables->next_local : > tables->next_name_resolution_table) also this looks suspicious. Normally one starts from table_list and follows the next_local pointer or one starts from first_name_resolution_table and follows the next_name_resolution_table pointer. Your change creates a strange mix of both, it looks like it would be wrong. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 581857bd999: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! Looks good, thanks! A couple of small comments below. On Oct 23, Anel Husakovic wrote: > revision-id: 581857bd999 (mariadb-10.1.43-314-g581857bd999) let's fix it in 10.2 and up, we're doing the very last 10.1 release, so better be extra-super-conservative about it. > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 00:20:53 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 74a69952124..bf97a930097 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -52,6 +52,23 @@ disconnect u1; > drop user u1@localhost; > drop database mysqltest1; > > +# > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE test_role; oops, sorry, my mistake. Please, add `SET ROLE test_role` before SHOW GRANTS. The goal is to see that SET DEFAULT ROLE is shown *after* all grants to root@localhost, but before any grants to test_role. > +SHOW GRANTS; > +SET DEFAULT ROLE NONE; > +SHOW GRANTS; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > # End of 10.1 tests > # > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..6c852976061 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,15 +8007,44 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *hostname, > + ACL_USER *acl_entry, char *buff, size_t buffsize) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING def_rolename= acl_entry->default_rolename; > + > + if (def_rolename.length) > + { > + String def_str(buff, buffsize, system_charset_info); > + def_str.length(0); > + def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(acl_entry->host.hostname, acl_entry->hostname_length, > + system_charset_info); > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; looks good. The hostname argument seems to be unused now, doesn't it? > +} > + > -static bool show_role_grants(THD *thd, const char *username, > - const char *hostname, ACL_USER_BASE *acl_entry, > +static bool show_role_grants(THD *thd, const char *hostname, > + ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > { > uint counter; > Protocol *protocol= thd->protocol; > LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > > - String grant(buff,sizeof(buff),system_charset_info); > + String grant(buff, buffsize, system_charset_info); ouch, indeed. thanks! > for (counter= 0; counter < acl_entry->role_grants.elements; counter++) > { > grant.length(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 01612f594b3: MDEV-23769: MTR can abort before it prints the test result summary
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thanks for the review. > > Having warning/error is a necessary condition. > Sufficient condition is to call at least 2 tests. > Incorrect behaviour can be seen if: > - Both tests are correct but there is error generated (incorrect option > passed): > So this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st` will > generate and write statistic at the end, > but this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st 1st > --force` will not generate the statistic. > Note: warning is generated (--mysqld=--lock-wait-timeout=-1) statistic > will be generated Okay, thanks. This would be useful to show in the commit comment, that one can use ./mtr --mysqld=--lock-wait-timeout=-xx 1st 1st --force --parallel 1 to repeat the error. But then I don't understand why this happens. Why one failure is not enough? Why --parallel 2 destroys the effect? Apparently, it's check_warnings_post_shutdown() that causes that, not exit(1). One can change to exit(0) or remove exit(1) completely - the bug will still happen. Why check_warnings_post_shutdown() writing to the master server causes this bug? It looks rather fishy that in your fix it's the worker process that prints the statistics, not the master. Btw, note that check_warnings_post_shutdown() doesn't have any return statement, it very implicitly return "number of bytes sent" - it would be easier to read if it'd return something explicitly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 99c14476251: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > revision-id: 99c14476251 (mariadb-10.1.43-314-g99c14476251) > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-22 16:49:28 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 74a69952124..8756648c3e1 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -53,5 +53,19 @@ drop user u1@localhost; > drop database mysqltest1; > > # > -# End of 10.1 tests > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; > +SHOW GRANTS; The point is to run `SHOW GRANTS` for a user with a default role. E.g. SET DEFAULT ROLE test_role; SHOW GRANTS; SET DEFAULT ROLE NONE; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > +# End of 10.1 tests > +# > \ No newline at end of file new line at the end of file? > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..f42f7bdcfaf 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,6 +8007,36 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *hostname, > + ACL_USER *acl_entry, char *buff) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > + > + LEX_STRING def_rolename= acl_entry->default_rolename; > + if (def_rolename.length) > + { > + String def_str(buff,sizeof(buff),system_charset_info); eh? sizeof(buff) is sizeof(char*) that is usually 8 on 64-bit, 4 on 32-bit architecture. You want the size of the buffer here, not the size of the pointer to the buffer. > + def_str.length(0); > + def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(acl_entry->user.str, acl_entry->user.length, > + system_charset_info); again, just as above, you can use def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(&host); why not to use def_str.append(acl_entry->host.hostname, acl_entry->hostname_length, system_charset_info); > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6729bf5c031: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thank you for your review. > How is easier for you (or to ask a general question: how does the review > process go?) regarding the commits? > Is it better to add a new commit as a review commit and if the patch gets > accepted to squash after, or to rebase commits and send only a single > commit again for the review? One single commit is better, thanks. At least for small patches. > Addressed in the new patch: > https://github.com/MariaDB/server/commit/99c144762514d4dea3b6ec395de96aaf10… Thanks! I'll reply to the commit directly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 83614913d9a: MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Aleksey! On Oct 22, Aleksey Midenkov wrote: > revision-id: 83614913d9a (mariadb-10.4.11-291-g83614913d9a) > parent(s): efb1023b6f4 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 16:44:13 +0200 > message: > > MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 8989a918c0c..f287cfddcd2 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -879,9 +879,12 @@ void partition_info::vers_set_hist_part(THD *thd) > } > return; > warn: > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name); > + if (vers_info->hist_part->warn_vers_part_full) > + { > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name); > + } would it make sense to put warn_vers_part_full into vers_info, instead of storing it per-partition? > } > > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2830b9c290e: MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Dmitry! On Oct 22, Dmitry Shulga wrote: > revision-id: 2830b9c290e (mariadb-10.2.31-489-g2830b9c290e) > parent(s): 8984d779103 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-22 00:42:10 +0700 > message: > > MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS > > Compiler warnings like one listed below are generated during server build on MacOS: > > > --- > plugin/auth_pam/CMakeLists.txt | 26 ++++++++++++++++++++++++++ > plugin/auth_pam/config.h.cmake | 1 + > plugin/auth_pam/mapper/pam_user_map.c | 30 +++++++++++++++++++----------- > 3 files changed, 46 insertions(+), 11 deletions(-) Nice! Could you also - in a second follow-up commit, not to confuse with the pure bug fix - do a small cleanup and move all other checks from plugin/auth_pam/CMakeLists.txt into your new config.h.cmake ? Currently they're added to the command line, like IF(HAVE_PAM_EXT_H) ADD_DEFINITIONS(-DHAVE_PAM_EXT_H) ENDIF() but as you've introduced config.h.cmake, there's no need to put them all on the command line anymore. You'll need to include config_auth_pam.h into more source files. And, I suppose. add config_auth_pam.h to .gitignore too. ok to push these two commits when done. Thanks! Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 65af1d07771: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Rucha! On Oct 22, Rucha Deodhar wrote: > revision-id: 65af1d07771 (mariadb-10.5.4-108-g65af1d07771) > parent(s): bbd70fcc43c > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-08-06 17:23:49 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Anaylsis: When we have INSERT/REPLACE...RETURNING and have qualified asterisk, > table_name is not NULL and context->table_list is either NULL or has > incorrect reference because context->table_list has tables from the FROM clause. > context->table_list has incorrect reference (has table from the FROM clause > instead of table we are inserting into) for INSERT/REPLACE...SELECT...RETURNING > because we have a FROM clause from the SELECT statement. > For INSERT/REPLACE...RETURNING it is NULL because there is no FROM clause. > Fix: If table_name is not null, check if we have INSERT/REPLACE...RETURNING. > If so, the reference should be the table we are inserting into and not > the table in the FROM clause. > > diff --git a/mysql-test/main/insert_returning.result b/mysql-test/main/insert_returning.result > index e664e02bedc..e1bab6f0329 100644 > --- a/mysql-test/main/insert_returning.result > +++ b/mysql-test/main/insert_returning.result > @@ -89,6 +89,9 @@ total val1 id1 && id1 id1 UPPER(val1) f(id1) good tests! > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45ce4be3eb5..4ce7b034029 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -7959,7 +7959,7 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > bool any_privileges, uint *hidden_bit_fields) > { > Field_iterator_table_ref field_iterator; > - bool found; > + bool found, is_insert_or_replace_returning= false; > char name_buff[SAFE_NAME_LEN+1]; > DBUG_ENTER("insert_fields"); > DBUG_PRINT("arena", ("stmt arena: %p",thd->stmt_arena)); > @@ -7978,13 +7978,31 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > > found= FALSE; > > + if ((thd->lex->sql_command == SQLCOM_INSERT_SELECT || > + thd->lex->sql_command == SQLCOM_REPLACE_SELECT || > + thd->lex->sql_command == SQLCOM_INSERT || > + thd->lex->sql_command == SQLCOM_REPLACE) && > + thd->lex->has_returning()) > + is_insert_or_replace_returning= true; What about INSERT t1 SELECT t2.* FROM t2 RETURNING t1.*; can your is_insert_or_replace_returning distinguish between the first and the second asterisk? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 01612f594b3: MDEV-23769: MTR can abort before it prints the test result summary
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > revision-id: 01612f594b3 (mariadb-10.1.43-300-g01612f594b3) > parent(s): 82301aea4f2 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-03 02:34:03 +0200 > message: > > MDEV-23769: MTR can abort before it prints the test result summary > > - Patch is solving generating report on warning > - Patch is doing cosmetic fix of `current_test` log file which holds the old log value of test `CURRENT TEST:..` in `mark_log()` in case of `unknown option` and as such > the logic which is using it's content doesn't output valid log content and doesn't generate valid `$test->{'comment'}` message. more is not always better. please remove everything in the commit comment below this line - 400 lines that don't explain anything beyond what your *one line* of the comment already did. And now you have to press PageDown many times to get to my main comment about the fix :) > Here are the test cases observed. Note that only in 6. scenario there is `print $child "HELLO\n";` problem in printing (to delete?). > > 1. Valid test case, but warning in server log > ``` > $ ./mtr MDEV-23769 --force > Logging: ./mtr MDEV-23769 --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > main.MDEV-23769 [ fail ] Found warnings/errors in server log file! > Test ended at 2020-10-03 02:13:00 > line > 2020-10-03 2:13:00 140309996242496 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > ^ Found warnings in /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/mysqld.1.err > ok > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > -------------------------------------------------------------------------- > The servers were restarted 0 times > Spent 0.000 of 4 seconds executing testcases > mysql-test-run: WARNING: Got errors/warnings while running tests, please examine '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/warnings' for details. > > Completed: Failed 1/1 tests, 0.00% were successful. > > Failing test(s): main.MDEV-23769 > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > mysql-test-run: *** ERROR: there were failing test cases > ``` > > 2. Valid test cases (same), but warning in server log (Note reported single test case) > ``` > $ ./mtr MDEV-23769 MDEV-23769 --force > Logging: ./mtr MDEV-23769 MDEV-23769 --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > main.MDEV-23769 [ fail ] Found warnings/errors in server log file! > Test ended at 2020-10-03 02:15:05 > line > 2020-10-03 2:15:04 139868136609344 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > ^ Found warnings in /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/mysqld.1.err > ok > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > main.MDEV-23769 [ fail ] Found warnings/errors in server log file! > Test ended at 2020-10-03 02:15:05 > line > 2020-10-03 2:15:05 140513782085184 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > ^ Found warnings in /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/mysqld.1.err > ok > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > -------------------------------------------------------------------------- > The servers were restarted 1 times > Spent 0.000 of 5 seconds executing testcases > mysql-test-run: WARNING: Got errors/warnings while running tests, please examine '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/warnings' for details. > > Completed: Failed 2/2 tests, 0.00% were successful. > > Failing test(s): main.MDEV-23769 > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > mysql-test-run: *** ERROR: there were failing test cases > ``` > > 3. Valid test cases (different test cases), but warning in server log (Note one passed one failed) > > ``` > $ ./mtr MDEV-23769 1st --force > Logging: ./mtr MDEV-23769 1st --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > main.MDEV-23769 [ fail ] Found warnings/errors in server log file! > Test ended at 2020-10-03 02:17:09 > line > 2020-10-03 2:17:08 140658805515840 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > ^ Found warnings in /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/mysqld.1.err > ok > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > main.1st [ pass ] 1 > -------------------------------------------------------------------------- > The servers were restarted 1 times > Spent 0.001 of 4 seconds executing testcases > mysql-test-run: WARNING: Got errors/warnings while running tests, please examine '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/warnings' for details. > > Completed: Failed 1/2 tests, 50.00% were successful. > > Failing test(s): main.MDEV-23769 > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > mysql-test-run: *** ERROR: there were failing test cases > > ``` > > 4. Now for example 3. create artificial failure with warnings in server log (note, both test failed) > > ``` > > $ ./mtr MDEV-23769 1st --force > Logging: ./mtr MDEV-23769 1st --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > main.MDEV-23769 [ fail ] Found warnings/errors in server log file! > Test ended at 2020-10-03 02:17:52 > line > 2020-10-03 2:17:51 140260493149760 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > ^ Found warnings in /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/mysqld.1.err > ok > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > main.1st [ fail ] > Test ended at 2020-10-03 02:17:52 > > CURRENT_TEST: main.1st > mysqltest: At line 5: End of line junk detected: "show tables in mysql > " > > The result from queries just before the failure was: > show databases; > Database > information_schema > mtr > mysql > performance_schema > test > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.1st/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.1st/' > -------------------------------------------------------------------------- > The servers were restarted 1 times > Spent 0.000 of 4 seconds executing testcases > mysql-test-run: WARNING: Got errors/warnings while running tests, please examine '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/warnings' for details. > > Completed: Failed 2/2 tests, 0.00% were successful. > > Failing test(s): main.MDEV-23769 main.1st > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > mysql-test-run: *** ERROR: there were failing test cases > > ``` > > 5. Generate unknown option, for the same example as 3. (note: both failed) > ``` > > $ ./mtr MDEV-23769 1st --mysqld=--xx --force > Logging: ./mtr MDEV-23769 1st --mysqld=--xx --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > worker[1] mysql-test-run: WARNING: Process [mysqld.1 - pid: 52531, winpid: 52531, exit: 512] died after mysql-test-run waited 0.3 seconds for /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/run/mysqld.1.pid to be created. > main.MDEV-23769 [ fail ] > Test ended at 2020-10-03 02:19:01 > > CURRENT_TEST: main.MDEV-23769 > > Failed to start mysqld.1 > mysqltest failed but provided no output > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > ***Warnings generated in error logs during shutdown after running tests: main.MDEV-23769 > > 2020-10-03 2:19:01 140238937720384 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > 2020-10-03 2:19:01 140238937720384 [ERROR] /home/anel/mariadb/10.1-mtr-in-source/sql/mysqld: unknown option '--xx' > 2020-10-03 2:19:01 140238937720384 [ERROR] Aborting > > worker[1] mysql-test-run: WARNING: Process [mysqld.1 - pid: 52537, winpid: 52537, exit: 512] died after mysql-test-run waited 0.3 seconds for /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/run/mysqld.1.pid to be created. > main.1st [ fail ] > Test ended at 2020-10-03 02:19:02 > > CURRENT_TEST: main.1st > > Failed to start mysqld.1 > mysqltest failed but provided no output > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.1st/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.1st/' > ***Warnings generated in error logs during shutdown after running tests: main.1st > > 2020-10-03 2:19:02 140048201700928 [ERROR] /home/anel/mariadb/10.1-mtr-in-source/sql/mysqld: unknown option '--xx' > 2020-10-03 2:19:02 140048201700928 [ERROR] Aborting > > Can't use an undefined value as a symbol reference at ./mtr line 657. > -------------------------------------------------------------------------- > The servers were restarted 1 times > Spent 0.000 of 5 seconds executing testcases > > Completed: Failed 2/2 tests, 0.00% were successful. > > Failing test(s): main.MDEV-23769 main.1st > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > Errors/warnings were found in logfiles during server shutdown after running the > following sequence(s) of tests: > main.MDEV-23769 > main.1st > mysql-test-run: *** ERROR: there were failing test cases > > ``` > > 6. Again the same option, but with 2 same tests -> not perl error: `Can't use an undefined value as a symbol reference at ./mtr line 657. > ` ` print $child "HELLO\n";` (solution delete the line). > > ``` > $ ./mtr MDEV-23769 MDEV-23769 --mysqld=--xx --force > Logging: ./mtr MDEV-23769 MDEV-23769 --mysqld=--xx --force > vardir: /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var > Checking leftover processes... > Removing old var directory... > Creating var directory '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var'... > Checking supported features... > MariaDB Version 10.1.47-MariaDB-debug > - SSL connections supported > - binaries are debug compiled > Sphinx 'indexer' binary not found, sphinx suite will be skipped > Collecting tests... > Installing system database... > > ============================================================================== > > TEST RESULT TIME (ms) or COMMENT > -------------------------------------------------------------------------- > > worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 16000..16019 > worker[1] mysql-test-run: WARNING: Process [mysqld.1 - pid: 52595, winpid: 52595, exit: 512] died after mysql-test-run waited 0.3 seconds for /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/run/mysqld.1.pid to be created. > main.MDEV-23769 [ fail ] > Test ended at 2020-10-03 02:20:12 > > CURRENT_TEST: main.MDEV-23769 > > Failed to start mysqld.1 > mysqltest failed but provided no output > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > ***Warnings generated in error logs during shutdown after running tests: main.MDEV-23769 > > 2020-10-03 2:20:12 139815724283456 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > 2020-10-03 2:20:12 139815724283456 [ERROR] /home/anel/mariadb/10.1-mtr-in-source/sql/mysqld: unknown option '--xx' > 2020-10-03 2:20:12 139815724283456 [ERROR] Aborting > > worker[1] mysql-test-run: WARNING: Process [mysqld.1 - pid: 52601, winpid: 52601, exit: 512] died after mysql-test-run waited 0.3 seconds for /home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/run/mysqld.1.pid to be created. > main.MDEV-23769 [ fail ] > Test ended at 2020-10-03 02:20:12 > > CURRENT_TEST: main.MDEV-23769 > > Failed to start mysqld.1 > mysqltest failed but provided no output > > - saving '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' to '/home/anel/mariadb/10.1-mtr-in-source/mysql-test/var/log/main.MDEV-23769/' > ***Warnings generated in error logs during shutdown after running tests: main.MDEV-23769 > > 2020-10-03 2:20:12 140650797745728 [Warning] option 'lock_wait_timeout': unsigned value 18446744073709551615 adjusted to 31536000 > 2020-10-03 2:20:12 140650797745728 [ERROR] /home/anel/mariadb/10.1-mtr-in-source/sql/mysqld: unknown option '--xx' > 2020-10-03 2:20:12 140650797745728 [ERROR] Aborting > > Can't use an undefined value as a symbol reference at ./mtr line 657. > -------------------------------------------------------------------------- > The servers were restarted 1 times > Spent 0.000 of 5 seconds executing testcases > > Completed: Failed 2/2 tests, 0.00% were successful. > > Failing test(s): main.MDEV-23769 > > The log files in var/log may give you some hint of what went wrong. > > If you want to report this error, please read first the documentation > at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html > > Errors/warnings were found in logfiles during server shutdown after running the > following sequence(s) of tests: > main.MDEV-23769 > main.MDEV-23769 > mysql-test-run: *** ERROR: there were failing test cases > > ``` > > diff --git a/mysql-test/disabled.def b/mysql-test/disabled.def > index ee30c4f4d3c..fdb62a7e5f5 100644 > --- a/mysql-test/disabled.def > +++ b/mysql-test/disabled.def > @@ -20,3 +20,4 @@ ssl_crl_clrpath : broken upstream > file_contents : MDEV-6526 these files are not installed anymore > max_statement_time : cannot possibly work, depends on timing > partition_open_files_limit : open_files_limit check broken by MDEV-18360 > +MDEV-23769 : Intentionally disabled, for validating purposes. I don't know if it makes any sense. A dead test that nobody will ever run. May be it's better not to add it at all? In the past I've added tests like that too, the "mtr" and "mtr2" suites. I regret it now :) Tests only help if they're run. > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 52920530132..8d761fd70cf 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -1005,6 +1005,13 @@ sub run_worker ($) { > stop_servers(reverse all_servers()); > if(check_warnings_post_shutdown($server)) { > # Warnings appeared in log file(s) during final server shutdown. > + # Make sure first to run statistics > + my $tests= collect_test_cases($opt_reorder, $opt_suites, \@opt_cases, \@opt_skip_test_list); > + my ($prefix, $fail, $completed, $extra_warnings)= > + run_test_server($server, $tests, $opt_parallel); > + mtr_print_line(); > + print_total_times($opt_parallel) if $opt_report_times; > + mtr_report_stats($prefix, $fail, $completed, $extra_warnings); I don't understand the fix. You're saying that if a warning happens on shutdown then mtr won't print the result summary. This doesn't seem to be the case, I've tried few times to repeat it - modified mtr to always pretend there's a warning on shutdown, and modified mysqld to always issue a warning on shutdown - and in all cases I saw the result summary all right. > exit(1); > } > } > @@ -3971,6 +3978,7 @@ sub run_testcase ($$) { > if (start_servers($tinfo)) > { > report_failure_and_restart($tinfo); > + unlink $path_current_testlog; > return 1; > } > } > diff --git a/mysql-test/t/MDEV-23769.opt b/mysql-test/t/MDEV-23769.opt > new file mode 100644 > index 00000000000..fd8dbcab53f > --- /dev/null > +++ b/mysql-test/t/MDEV-23769.opt > @@ -0,0 +1 @@ > +--lock-wait-timeout=-1 > diff --git a/mysql-test/t/MDEV-23769.test b/mysql-test/t/MDEV-23769.test > new file mode 100644 > index 00000000000..9938c7cf644 > --- /dev/null > +++ b/mysql-test/t/MDEV-23769.test > @@ -0,0 +1,4 @@ > +--echo # > +--echo # MDEV-23769: MTR can abort before it prints the test result summary > +--echo # > +show databases; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8c745467560: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Rucha! what happens in this bug? Why does the server need to open_table() after my_ok()? On Oct 21, Rucha Deodhar wrote: > revision-id: 8c745467560 (mariadb-10.5.4-226-g8c745467560) > parent(s): 308f8350c7b > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-10-20 11:53:22 +0530 > message: > > MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in > Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK) > > Analysis: Assertion failure happens because we don't ignore kill query > even if 'OK' is already sent. > Fix: Check if we already sent 'OK'. If sent, ignore kill query. > > diff --git a/mysql-test/main/alter_table.result b/mysql-test/main/alter_table.result > index 197e52b35ca..204bfa5249c 100644 > --- a/mysql-test/main/alter_table.result > +++ b/mysql-test/main/alter_table.result > @@ -3322,5 +3322,23 @@ Note 1176 Key 'x' doesn't exist in table 't1' > unlock tables; > drop table t1; > # > +# MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in > +# Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK) > +# > +set @max_session_mem_used_save= @@max_session_mem_used; > +CREATE TABLE t1 (a INT); > +SELECT * FROM t1; > +a > +ALTER TABLE x MODIFY xx INT; > +ERROR 42S02: Table 'test.x' doesn't exist > +SET SESSION max_session_mem_used= 8192; > +LOCK TABLE t1 WRITE; > +ALTER TABLE t1 CHANGE COLUMN IF EXISTS b c INT; > +Warnings: > +Note 1054 Unknown column 'b' in 't1' > +set SESSION max_session_mem_used = @max_session_mem_used_save; > +UNLOCK TABLES; > +DROP TABLE t1; > +# > # End of 10.5 tests > # > diff --git a/mysql-test/main/alter_table.test b/mysql-test/main/alter_table.test > index 6fe61a3222a..d09c4378339 100644 > --- a/mysql-test/main/alter_table.test > +++ b/mysql-test/main/alter_table.test > @@ -2529,6 +2529,27 @@ alter table t1 rename key if exists x to xx; > unlock tables; > drop table t1; > > +--echo # > +--echo # MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in > +--echo # Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK) > +--echo # > + > +set @max_session_mem_used_save= @@max_session_mem_used; > + > +CREATE TABLE t1 (a INT); > +SELECT * FROM t1; > + > +--error ER_NO_SUCH_TABLE > +ALTER TABLE x MODIFY xx INT; > + > +SET SESSION max_session_mem_used= 8192; > +LOCK TABLE t1 WRITE; > +ALTER TABLE t1 CHANGE COLUMN IF EXISTS b c INT; > + > +set SESSION max_session_mem_used = @max_session_mem_used_save; > +UNLOCK TABLES; > +DROP TABLE t1; > + > --echo # > --echo # End of 10.5 tests > --echo # > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index e6d0ab13e75..e8d39917ba6 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1682,7 +1682,9 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > > if (!(flags & MYSQL_OPEN_IGNORE_KILLED) && thd->killed) > { > - thd->send_kill_message(); > + /* If we already sent 'ok', we can ignore any kill query statements */ > + if (! thd->get_stmt_da()->is_set()) > + thd->send_kill_message(); > DBUG_RETURN(TRUE); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1060cc4235a: MDEV-23500: Assertion `!is_set() || (m_status == DA_OK_BULK && is_bulk_op())'
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Rucha! On Oct 21, Rucha Deodhar wrote: > revision-id: 1060cc4235a (mariadb-10.2.31-438-g1060cc4235a) > parent(s): 80075ba011f > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-09-22 17:12:57 +0530 > message: > > MDEV-23500: Assertion `!is_set() || (m_status == DA_OK_BULK && is_bulk_op())' > failed in Diagnostics_area::set_ok_status upon multi-table update > > Analysis: When NULL field is copied to NOT-NULL so there is error but > this error is not reported so we get the failure. > Fix: Return the error state. > > diff --git a/mysql-test/r/update.result b/mysql-test/r/update.result > index 9e19abc4e9c..c1d37fc36e7 100644 > --- a/mysql-test/r/update.result > +++ b/mysql-test/r/update.result > @@ -731,4 +731,14 @@ INSERT INTO t2 VALUES > UPDATE t1,t2 SET t1.i1 = -39 WHERE t2.d1 <> t1.i1 AND t2.d1 = t1.d2; > ERROR 22007: Incorrect datetime value: '19' for column `test`.`t1`.`i1` at row 1 > DROP TABLE t1,t2; > +# > +# MDEV-23500: Assertion `!is_set() || (m_status == DA_OK_BULK && > +# is_bulk_op())' failed in Diagnostics_area::set_ok_status upon > +# multi-table update > +# > +CREATE TABLE t (f INT NOT NULL); > +INSERT INTO t VALUES (0),(0); > +UPDATE t AS t1 LEFT JOIN t AS t2 ON t1.f <=> t2.f SET t2.f = NULL; > +ERROR 01000: Data truncated for column 'f' at row 4 with a single-table update I get MariaDB [test]> update t set f=NULL; ERROR 1048 (23000): Column 'f' cannot be null should multi-table update produce the same error? > +DROP TABLE t; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9c2570e6c42: MDEV-22422: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Rucha! On Oct 21, Rucha Deodhar wrote: > revision-id: 9c2570e6c42 (mariadb-10.1.43-279-g9c2570e6c42) > parent(s): 5768f57d243 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-09-22 17:18:37 +0530 > message: > > MDEV-22422: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status > > Analysis: The error is not returned when the statement can't be used. > And so we still go on to search for keywords. > Fix: Return the error state. > > diff --git a/mysql-test/t/errors.test b/mysql-test/t/errors.test > index 55461002fd4..e2b10559299 100644 > --- a/mysql-test/t/errors.test > +++ b/mysql-test/t/errors.test > @@ -213,3 +213,22 @@ select * from seq_1_to_1000; > --enable_result_log > # We may not be able to execute any more queries with this connection > # because of too little memory# > + > +--echo # > +--echo # 10.1 Test > +--echo # > +--echo # MDEV-22422: Assertion `! is_set()' failed in > +--echo # Diagnostics_area::set_eof_status > +--echo # > + > +SET @saved_max_session_mem_used = @@max_session_mem_used; > + > +SET max_session_mem_used = 50000; > +--error ER_OPTION_PREVENTS_STATEMENT > +help 'it is going to crash'; > + > +SET @@max_session_mem_used=@saved_max_session_mem_used; > + > +--echo # > +--echo # End of 10.1 Test > +--echo # > diff --git a/sql/sql_help.cc b/sql/sql_help.cc > index cd9da566e51..654a3915b29 100644 > --- a/sql/sql_help.cc > +++ b/sql/sql_help.cc > @@ -736,6 +736,9 @@ static bool mysqld_help_internal(THD *thd, const char *mask) > &name, &description, &example); > delete select; > > + if (thd->is_error()) > + goto error; does the error happen during the first count_topics= search_topics() ? There are lots of places below that look kind of fragile too. May be they're safe. Or may be by increasing max_session_mem_used somewhat you can get mysqld_help_internal to crash below again and again? Could you look into that please? > if (count_topics == 0) > { > int UNINIT_VAR(key_id); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6729bf5c031: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Anel! On Oct 21, Anel Husakovic wrote: > revision-id: 6729bf5c031 (mariadb-10.1.43-112-g6729bf5c031) > parent(s): ad4b70562bb > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-04-22 20:48:55 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 649bba7d1ca..5eefd9b9185 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -33,3 +33,16 @@ REVOKE EXECUTE ON PROCEDURE sp FROM u; > --error ER_TABLE_NOT_LOCKED > REVOKE PROCESS ON *.* FROM u; > DROP TABLE t1; > + > +# > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; add a test for SHOW GRANTS without FOR username > +DROP USER test_user; > +DROP ROLE test_role; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index b6a6f806e50..90dd552ec43 100644 > --- a/sql/sql_acl.cc > @@ -7891,6 +7893,10 @@ bool mysql_show_grants(THD *thd, LEX_USER *lex_user) > if (show_role_grants(thd, username, hostname, acl_user, buff, sizeof(buff))) > goto end; > > + /* Show default role to acl_user */ > + if (show_default_role(thd, username, hostname, acl_user, buff, sizeof(buff))) > + goto end; Please, print SET DEFAULT ROLE after all grants for this user. > + > /* Add first global access grants */ > if (show_global_privileges(thd, acl_user, FALSE, buff, sizeof(buff))) > goto end; > @@ -7963,6 +7969,42 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *username, > + const char *hostname, ACL_USER_BASE *acl_entry, why did you cast acl_entry (which you know is ACL_USER) to the base class ACL_USER_BASE? why did you pass the username if you don't use it? > + char *buff, size_t buffsize) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; This doesn't make much sense. You only create this LEX_STRING to use it for String::append(LEX_STRING *). But you can directly use String::append(const char*) and you won't need any LEX_STRING here. Furthermore, you can print acl_entry->host.hostname just as you print acl_entry->user, so you don't need the above hostname argument at all. > + String def_role(buff,sizeof(buff),system_charset_info); > + def_role.length(0); you don't use def_role anywhere. > + LEX_STRING def_rolename= ((ACL_USER *)acl_entry)->default_rolename; > + if (def_rolename.length) > + { > + String def_str(buff,sizeof(buff),system_charset_info); > + def_str.length(0); > + def_str.append("SET DEFAULT ROLE "); prefer to use STRING_WITH_LEN() for string literals. E.g. here it should be def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(def_rolename.str, def_rolename.length, > + system_charset_info); here you can do def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(acl_entry->user.str, acl_entry->user.length, > + system_charset_info); > + if (!(acl_entry->flags & IS_ROLE)) better put an assert here not an if > + { > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(&host); > + } > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; > +} > + > static bool show_role_grants(THD *thd, const char *username, > const char *hostname, ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8c34a242aa2: MDEV-20787: Script dgcov.pl does not work
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Anel! On Oct 21, Anel Husakovic wrote: > revision-id: 8c34a242aa2 (mariadb-10.2.31-478-g8c34a242aa2) > parent(s): 01ffccd6a4e > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-06 00:00:58 +0200 > message: > > MDEV-20787: Script dgcov.pl does not work > > - This patch is done after doing test on 00f9df29224ada9cdbb796c99f4 > where difference was found between versions of gcov. don't refer to commits that are not pushed and are not going to be pushed. Because if you push this one you'll have a commit forever in the history that refers to an non-existing commit. It isn't helpful. > - This patch doesn't take gcov versions in account since is extracting > filename of a `.gcov` file, which is generated with `gcov -i > <source-file>.gcda` during `dgcov.pl --generate`. > - There is a check if file exists. > Reason for that is because some generated `gcov` files are outliers from above > rule (that a file generated in intermediate format(gcov) has the same source name). > Example 1: `aestables.cpp.gcda` is not generating `aestables.cpp.gcov`, > but only the files used in #include `modes.hpp.gcov runtime.hpp.gcov` > Example 2: `my_sha256.cc.gcda` is generating `my_sha.ic.gcov` > - Make sure to test `MTR_BINDIR` which gets set during out-of-source > build and generate error when running `./mysql-test/mtr --gcov alias` What is a problem with out of source builds? If they don't work, perhaps it would be better to fix dgcov to work with out of source builds? > - During the testing: > 10.1 results are stored in > - `<basedir>//mysql-test-gcov.msg` and errors in > - `<basedir>//mysql-test-gcov.err` > 10.2 results should be stored in > `./mysql-test/var/last_changes.dgcov`, but only empty results are > obtained (tests `1st` and `alias`). I don't understand this ^^^ paragraph. And also because your patch is 10.2+ so who cares what it does on 10.1? > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index fbc5540e697..f2bbdad4d20 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -161,8 +161,14 @@ sub gcov_one_file { > system($cmd)==0 or die "system($cmd): $? $!"; > } > > - # now, read the generated file > - open FH, '<', "$_.gcov" or die "open(<$_.gcov): $!"; > + (my $filename = $_)=~ s/\.[^.]+$//; # remove extension In your previous patch you either removed the extension or not based on gcov version. Why did you abandon this logic? > + my $gcov_file_path= $File::Find::dir."/$filename.gcov"; File::Find documentation seems to be saying that $File::Find::dir ends with a slash (/) so you don't need to prefix $filename.gcov with it. > + if (! -f $gcov_file_path) > + { > + return; > + } may be return unless -f $gcov_file_path it's perl after all :) > + open FH, '<', "$gcov_file_path" or die "open(<$gcov_file_path): $!"; you don't need double quotes around "$gcov_file_path" - they don't serve any purpose. > + > my $fname; > while (<FH>) { > chomp; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f83c1615d97: MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Rucha! On Oct 21, Rucha Deodhar wrote: > revision-id: f83c1615d97 (mariadb-10.1.43-240-gf83c1615d97) > parent(s): 7e9ffc69ecd > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-08-05 00:33:03 +0530 > message: > > MDEV-20207: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status > upon HANDLER READ > > Analysis: The error state is not stored while checking condition and key name. > Fix: Return true while checking condition and key name if error is reported > because geometry object can't be created from the data in the index value for > HANDLER READ. > > diff --git a/mysql-test/suite/handler/aria.test b/mysql-test/suite/handler/aria.test > index 1913d2b791c..50ef43ce5b2 100644 > --- a/mysql-test/suite/handler/aria.test > +++ b/mysql-test/suite/handler/aria.test > @@ -80,3 +80,23 @@ HANDLER t1 CLOSE; > DROP TABLE t1; > > --echo End of 5.1 tests > + > +--echo # > +--echo # 10.1 Test > +--echo # > +--echo # MDEV-20207: Assertion `! is_set()' failed in > +--echo # Diagnostics_area::set_eof_status upon HANDLER READ > +--echo # > + > +DROP TABLE IF EXISTS t1; there's no need to start the test from DROP TABLE IF EXISTS t1; historically people customarily did that. before mtr had after-test checks. but now all tests are supposed to clean up after itself, you do not need to worry about tests leaving tables around. > + > +CREATE TABLE t1 (a POINT, KEY(a)); > +HANDLER t1 OPEN h; > + > +--error ER_CANT_CREATE_GEOMETRY_OBJECT > +HANDLER h READ a = (0); I don't think it should be an error. SELECT * FROM t1 WHERE a=(0) -- this does not fail, it just doesn't find anything. I think HANDLER should do the same. > + > +HANDLER h CLOSE; > +DROP TABLE t1; note that all handler test files source handler.inc. that is, if you want a test to run for all engines, you do not need to add it to all individual test files, add it to handler.inc Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e3f45b2f9ea: MDEV-10267 Add ngram fulltext parser plugin
by Sergei Golubchik 21 Oct '20

21 Oct '20

Hi, Rinat! On Oct 21, Rinat Ibragimov wrote: > > > + > > > + // Avoid creating partial n-grams by stopping at word boundaries. > > > + // Underscore is treated as a word character too, since identifiers in > > > + // programming languages often contain them. > > > + if (!is_alnum(char_type) && !is_underscore(cs, next, end)) { > > > + start= next + char_len; > > > + next= start; > > > + n_chars= 0; > > > + continue; > > > > I don't think it's a good idea. On the opposite, I would rather create > > ngrams over word boundaries, so that, say, I'd split "n-gram plugin" to > > > > "n-g", "-gr", "gra", "ram", "am ", "m p", " pl", "plu", "lug", "ugi", "gin". > > I see how this can increase index size: more different strings are > included. Implementation will become a tiny bit simpler. But can it > make searching better somehow? Yes, the idea is that it'll rank "One can use n-gram plugin in MariaDB fulltext search" higher than "Plugins are useful for extending functionality of a program" because some ngrams encode that "n-gram" and "plugin" words must be together as a phrase. > I can't decide. From my point of view, the current approach is fine. > Please pick a variant, and I'll try to implement that. No, I cannot guess which approach will produce more relevant searches. Implement something and then we test what works better > > > + } > > > + > > > + next += char_len; > > > + n_chars++; > > > + > > > + if (n_chars == ngram_token_size) { > > > + bool_info->position= static_cast<uint>(start - doc); > > > > nope, we don't have MYSQL_FTPARSER_FULL_BOOLEAN_INFO::position and this > > plugin is definitely not the reason to break the API and add it. > > There is a patch in the patchset that adds the "position" field. Without > that field, > it's possible to trigger an assertion in InnoDB's FTS code by executing: > > INSTALL PLUGIN simple_parser SONAME 'mypluglib'; > CREATE TABLE articles( > a TEXT DEFAULT NULL, > b TEXT DEFAULT NULL, > FULLTEXT (a, b) WITH PARSER simple_parser > ) ENGINE=InnoDB; > INSERT INTO articles VALUES ('111', '1234 1234 1234'); > DROP TABLE articles; > UNINSTALL PLUGIN simple_parser; > > Same assertion may be triggered by any fts plugin. Are you sure you > want that patch to be removed? I don't think InnoDB assert is a reason to extend the fulltext plugin API. What about this fix instead: diff --git a/storage/innobase/fts/fts0fts.cc b/storage/innobase/fts/fts0fts.cc --- a/storage/innobase/fts/fts0fts.cc +++ b/storage/innobase/fts/fts0fts.cc @@ -4669,7 +4669,7 @@ fts_tokenize_add_word_for_parser( ut_ad(boolean_info->position >= 0); position = boolean_info->position + fts_param->add_pos; */ - position = fts_param->add_pos; + position = fts_param->add_pos++; fts_add_token(result_doc, str, position); this fixed the crash for me. > > > + n_chars= ngram_token_size - 1; > > > + is_first= false; > > > + } > > > + } > > > + > > > + // If nothing was emitted yet, emit the remainder. > > > > what reminder, what is it for? > > It's "remainder", with "a" before "i". Remainder of the processed > string. That way strings that are too short for having even a single > n-gram, will still generate some index entries and will be > discoverable by exact queries. Perhaps the comment should say it? That if the string too short, it'll be emitted here? > > > + case MYSQL_FTPARSER_FULL_BOOLEAN_INFO: > > > + // Reusing InnoDB's boolean mode parser to handle all special characters. > > > + while (fts_get_word(cs, &start, end, &word, &bool_info)) { > > > > nope. InnoDB might not even be compiled it. Use param->mysql_parse() > > Copied fts_get_word() implementation into the plugin code. > > As far as I understand, param->mysql_parse() cannot be used here as it > splits a string into words like default fulltext search does. This > plugin however must generate n-grams. Of course, it can. Note that fts_get_word() doesn't generate n-grams either, it gets the whole word and the n-gram plugin later splits it into n-grams. Similarly param->mysql_parse() will extract words for you and you'll split them into n-grams. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Please assign PRs that modify debian/ contents for Otto to review
by Otto Kekäläinen 20 Oct '20

20 Oct '20

Hello! While working on putting MariaDB 10.5.5 into Debian I noticed that there were a couple of commits that landed more or less on the same day as the release was made and "broke" it just before release. Now there are surprises in a stable release I need to fix and the situation puts extra strain on me. It is good that many people participate in maintaining the contents of debian/ in the MariaDB Server, because I don't have time to do everything. I would however appreciate that when changes are made, I would be given a chance to review them. I will review them post-release anyway and it would save a lot of trouble, if we can fix stuff during the development cycle and not having to patch a post-release in order to be able to put it forward to Debian/Ubuntu. So, please make PRs of any debian/ changes you make, and feel free to add me as the reviewer (or one of the reviewers). This policy can also be automated using a .github/CODEOWNERS file, so I made a PR about it at https://github.com/MariaDB/server/pull/1650 Thanks! PS. I will deliver a MariaDB 10.5 and MariaDB in Debian status talk at https://debconf20.debconf.org/talks/98-whats-new-in-mariadb-server-105-and-… in case anybody is interested to listen in. -- Follow me at @ottokekalainen

2 3

Re: [Maria-developers] db1fbafbead: MDEV-23935: Fix warnings generated during compilation of plugin/auth_pam/testing/pam_mariadb_mtr.c on MacOS
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Dmitry! ok to push On Oct 19, Dmitry Shulga wrote: > revision-id: db1fbafbead (mariadb-10.4.11-420-gdb1fbafbead) > parent(s): c2ac0ce1f02 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-11 21:51:45 +0700 > message: > > MDEV-23935: Fix warnings generated during compilation of plugin/auth_pam/testing/pam_mariadb_mtr.c on MacOS > > During build server 10.4 on MacOS warnings like the following one > are generated on compiling the file plugin/auth_pam/testing/pam_mariadb_mtr.c > > server-10.4/plugin/auth_pam/testing/pam_mariadb_mtr.c:25:22: error: > initializing 'char *' with an expression of type 'const char [23]' > discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] > { PAM_TEXT_INFO, "Challenge input first." } > > The reason of the warnings is that the data member pam_message::msg is declared > as 'char *' on MacOS but initializer of the data member is 'const char *'. > > To eliminate warnings the compiler option > -Wno-incompatible-pointer-types-discards-qualifiers > has been added to the compiler flags used for compiling the file > pam_mariadb_mtr.c. > > --- > plugin/auth_pam/testing/CMakeLists.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/plugin/auth_pam/testing/CMakeLists.txt b/plugin/auth_pam/testing/CMakeLists.txt > index 889cb524402..c8d2e3cbb97 100644 > --- a/plugin/auth_pam/testing/CMakeLists.txt > +++ b/plugin/auth_pam/testing/CMakeLists.txt > @@ -4,6 +4,12 @@ ADD_LIBRARY(pam_mariadb_mtr MODULE pam_mariadb_mtr.c) > SET_TARGET_PROPERTIES (pam_mariadb_mtr PROPERTIES PREFIX "") > TARGET_LINK_LIBRARIES(pam_mariadb_mtr pam) > > +IF(APPLE) > + SET_SOURCE_FILES_PROPERTIES( > + pam_mariadb_mtr.c > + PROPERTY COMPILE_FLAGS "-Wno-incompatible-pointer-types-discards-qualifiers") > +ENDIF() > + > SET(dest DESTINATION "${INSTALL_MYSQLTESTDIR}/suite/plugins/pam" COMPONENT Test) > INSTALL(TARGETS pam_mariadb_mtr ${dest}) > INSTALL(FILES mariadb_mtr.conf RENAME mariadb_mtr ${dest}) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 727d97feb99: MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Dmitry! plugin code should stay in the plugin directory. please move the check to plugin/auth_pam/CMakeLists.txt Also, what about an alternative fix: #ifdef HAVE_POSIX_GETGROUPLIST typedef gid_t my_gid_t; #else typedef int my_gid_t; #end would that help but with less ifdefs? On Oct 19, Dmitry Shulga wrote: > revision-id: 727d97feb99 (mariadb-10.2.31-489-g727d97feb99) > parent(s): 8984d779103 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-09 20:35:48 +0700 > message: > > MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS > > Compiler warnings listed below are generated during server build on MacOS: > > 88%] Building C object plugin/auth_pam/CMakeFiles/pam_user_map.dir/mapper/pam_user_map.c.o > /Users/shulga/projects/mariadb/server-10.2/plugin/auth_pam/mapper/pam_user_map.c:87:41: error: passing > 'gid_t *' (aka 'unsigned int *') to parameter of type 'int *' converts between pointers to integer types > with different sign [-Werror,-Wpointer-sign] > if (getgrouplist(user, user_group_id, loc_groups, &ng) < 0) > ^~~~~~~~~~ > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/unistd.h:650:43: note: > passing argument to parameter here > int getgrouplist(const char *, int, int *, int *); > ^ > /Users/shulga/projects/mariadb/server-10.2/plugin/auth_pam/mapper/pam_user_map.c:95:46: error: passing > 'gid_t *' (aka 'unsigned int *') to parameter of type 'int *' converts between pointers to integer types > with different sign [-Werror,-Wpointer-sign] > (void) getgrouplist(user, user_group_id, loc_groups, &ng); > ^~~~~~~~~~ > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/includ > e/unistd.h:650:43: note: > passing argument to parameter here > int getgrouplist(const char *, int, int *, int *); > ^ > 2 errors generated. > > In case MariaDB serer is build with -DCMAKE_BUILD_TYPE=Debug it results in > uild error. > > The reason of compiler warnings is that declaration of the Posix C API function > getgrouplist() on MacOS differs from declration of getgrouplist() proposed > by Posix. > > To suppress this compiler warning cmake configure was adapted to detect what > kind of getgrouplist() function is declared on the building platform and > set the macros HAVE_POSIX_GETGROUPLIST in case the building platform suppors > Posix compatible interface of the getgrouplist() function. Depending on > whether this macros is set the compatible type of arguments is used to pass > parameter values to the function. > > --- > config.h.cmake | 1 + > configure.cmake | 22 ++++++++++++++++++++++ > plugin/auth_pam/mapper/pam_user_map.c | 13 ++++++++++++- > 3 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/config.h.cmake b/config.h.cmake > index 0e19dd44694..07e4647f0bf 100644 > --- a/config.h.cmake > +++ b/config.h.cmake > @@ -188,6 +188,7 @@ > #cmakedefine HAVE_POSIX_FALLOCATE 1 > #cmakedefine HAVE_LINUX_FALLOC_H 1 > #cmakedefine HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE 1 > +#cmakedefine HAVE_POSIX_GETGROUPLIST 1 > #cmakedefine HAVE_PREAD 1 > #cmakedefine HAVE_PAUSE_INSTRUCTION 1 > #cmakedefine HAVE_FAKE_PAUSE_INSTRUCTION 1 > diff --git a/configure.cmake b/configure.cmake > index 58e1111bfc7..c6db3ecb912 100644 > --- a/configure.cmake > +++ b/configure.cmake > @@ -1031,3 +1031,25 @@ IF(NOT MSVC) > HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE > ) > ENDIF() > + > +# Check whether getgrouplist uses git_t for second and third arguments. > +SET(CMAKE_REQUIRED_FLAGS -Werror) > +CHECK_C_SOURCE_COMPILES( > +" > +#ifdef HAVE_GRP_H > +#include <grp.h> > +#endif > +#ifdef HAVE_UNISTD_H > +#include <unistd.h> > +#endif > +int main() { > + char *arg_1; > + gid_t arg_2, arg_3; > + int arg_4; > + (void)getgrouplist(arg_1,arg_2,&arg_3,arg_4); > + return 0; > +} > +" > +HAVE_POSIX_GETGROUPLIST > +) > +SET(CMAKE_REQUIRED_FLAGS) > diff --git a/plugin/auth_pam/mapper/pam_user_map.c b/plugin/auth_pam/mapper/pam_user_map.c > index 9d7ed53f8b1..b0d2affac5c 100644 > --- a/plugin/auth_pam/mapper/pam_user_map.c > +++ b/plugin/auth_pam/mapper/pam_user_map.c > @@ -31,6 +31,8 @@ These comments are written to the syslog as 'authpriv.debug' > and usually end up in /var/log/secure file. > */ > > +#include <my_config.h> > + > #include <stdlib.h> > #include <stdio.h> > #include <ctype.h> > @@ -72,8 +74,13 @@ static const char debug_keyword[]= "debug"; > > static int populate_user_groups(const char *user, gid_t **groups) > { > +#ifdef HAVE_POSIX_GETGROUPLIST > gid_t user_group_id; > gid_t *loc_groups= *groups; > +#else > + int user_group_id; > + int *loc_groups= (int*)*groups; > +#endif > int ng; > > { > @@ -88,12 +95,16 @@ static int populate_user_groups(const char *user, gid_t **groups) > { > /* The rare case when the user is present in more than */ > /* GROUP_BUFFER_SIZE groups. */ > +#ifdef HAVE_POSIX_GETGROUPLIST > loc_groups= (gid_t *) malloc(ng * sizeof (gid_t)); > +#else > + loc_groups= (int *) malloc(ng * sizeof (gid_t)); > +#endif > if (!loc_groups) > return 0; > > (void) getgrouplist(user, user_group_id, loc_groups, &ng); > - *groups= loc_groups; > + *groups= (gid_t*)loc_groups; > } > > return ng; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5e71a3e0df7: MDEV-23925: Fix warnings generated during compilation of mysys_ssl/openssl.c on MacOS
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Dmitry! ok to push (again, comments below) On Oct 19, Dmitry Shulga wrote: > revision-id: 5e71a3e0df7 (mariadb-10.2.31-489-g5e71a3e0df7) > parent(s): 8984d779103 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-09 20:55:14 +0700 > message: > > MDEV-23925: Fix warnings generated during compilation of mysys_ssl/openssl.c on MacOS > > Compiler warnings listed below are generated during server build on MacOS: > In file included from server-10.2-MDEV-23564/mysys_ssl/openssl.c:33: > In file included from /usr/local/include/openssl/evp.h:16: > In file included from /usr/local/include/openssl/bio.h:20: > /usr/local/include/openssl/crypto.h:206:10: warning: 'CRYPTO_cleanup_all_ex_data' macro redefined [-Wmacro-redefined] > ^ > /mariadb/server-10.2-MDEV-23564/include/ssl_compat.h:46:9: note: previous definition is here > ^ > In file included from /mariadb/server-10.2-MDEV-23564/mysys_ssl/openssl.c:33: > /usr/local/include/openssl/evp.h:501:10: warning: 'EVP_MD_CTX_init' macro redefined [-Wmacro-redefined] > ^ > /mariadb/server-10.2-MDEV-23564/include/ssl_compat.h:33:9: note: previous definition is here > ^ > In file included from /mariadb/server-10.2-MDEV-23564/mysys_ssl/openssl.c:33: > /usr/local/include/openssl/evp.h:627:11: warning: 'EVP_CIPHER_CTX_init' macro redefined [-Wmacro-redefined] > ^ > /mariadb/server-10.2-MDEV-23564/include/ssl_compat.h:35:9: note: previous definition is here > ^ > In file included from /mariadb/server-10.2-MDEV-23564/mysys_ssl/openssl.c:33: > /usr/local/include/openssl/evp.h:866:11: warning: 'EVP_cleanup' macro redefined [-Wmacro-redefined] > ^ > /mariadb/server-10.2-MDEV-23564/include/ssl_compat.h:44:9: note: previous definition is here > ^ > 4 warnings generated. generally these long commit comments are not any more clear than if you'd only listed one compilation failure. > > In case MariaDB serer is build with -DCMAKE_BUILD_TYPE=Debug it results in > build error. > > The reason of compiler warnings is that header file <ssl_compat.h> > included before the openssl system header files. File ssl_compat.h > contains some macros with the name as SSL API functions declared > in the openssl system header files. It resulted in duplicate > symbols thats produces compiler warnings. > > To fix the issue the header file ssl_compat.h should be included > after a line where openssl system header is included. > > --- > mysys_ssl/openssl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mysys_ssl/openssl.c b/mysys_ssl/openssl.c > index f3dc8f4277c..a11db113b14 100644 > --- a/mysys_ssl/openssl.c > +++ b/mysys_ssl/openssl.c > @@ -15,7 +15,6 @@ > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > > #include <my_global.h> > -#include <ssl_compat.h> > > /* > The check is only done for OpenSSL 1.1.x. > @@ -25,13 +24,14 @@ > */ > > #ifndef HAVE_OPENSSL11 > +#include <ssl_compat.h> > int check_openssl_compatibility() > { > return 0; > } > #else > #include <openssl/evp.h> > - > +#include <ssl_compat.h> > static uint testing, alloc_size, alloc_count; > > static void *coc_malloc(size_t size, const char *f __attribute__((unused)), > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b92597c220f: MDEV-23564: CMAKE failing due to deprecated Apple GSS method
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Dmitry! ok to push. but see comments below On Oct 19, Dmitry Shulga wrote: > revision-id: b92597c220f (mariadb-10.2.31-479-gb92597c220f) > parent(s): 577c61e8be0 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-08 00:48:47 +0700 > message: > > MDEV-23564: CMAKE failing due to deprecated Apple GSS method > > Some GSS-API functions like gss_import_name(), gss_release_buffer() > used in plugin/auth_gssapi and libmariadb/plugins/auth are marked > as deprecated in MacOS starting from version 10.14+. It results in > extra warnings output on server building. > > To eliminate extra warnings the flag '-Wno-deprecated-declarations' > has been added to compiler invocation string for those source > files that invoke deprecated GSS-API functions. > > diff --git a/libmariadb b/libmariadb > index a746c3af449..fd0dc672309 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit a746c3af449a8754e78ad7971e59e79af7957cdb > +Subproject commit fd0dc6723095f066648a7c3b2a48523b3002e392 be careful when pushing this commit. you'd need something like 1. rebase your libmariadb fix on top of 3.1, push into 3.1 2. rebase your server fix on top of 10.2 2. update submodule reference in the server repo 3. amend your commit fix and push > diff --git a/plugin/auth_gssapi/CMakeLists.txt b/plugin/auth_gssapi/CMakeLists.txt > index bca4f5af3a1..7a3c32a00b1 100644 > --- a/plugin/auth_gssapi/CMakeLists.txt > +++ b/plugin/auth_gssapi/CMakeLists.txt > @@ -18,6 +18,13 @@ ELSE() > SET(GSSAPI_SERVER gssapi_server.cc) > SET(GSSAPI_ERRMSG gssapi_errmsg.cc) > > + IF(APPLE) > + SET_SOURCE_FILES_PROPERTIES( > + ${GSSAPI_CLIENT} ${GSSAPI_SERVER} ${GSSAPI_ERRMSG} > + PROPERTY COMPILE_FLAGS "-Wno-deprecated-declarations") > +# SET_SOURCE_FILES_PROPERTIES(${GSSAPI_SERVER} PROPERTY COMPILE_FLAGS "-Wno-deprecated-declarations") > +# SET_SOURCE_FILES_PROPERTIES(${GSSAPI_ERRMSG} PROPERTY COMPILE_FLAGS "-Wno-deprecated-declarations") you don't need these commented lines anymore, do you? Is it possible to use a compiler on APPLE that doesn't understand -Wno-deprecated-declarations? If not - your fix it ok. If it is - better use MY_CHECK_AND_SET_COMPILER_FLAG > + ENDIF() > SET(CMAKE_REQUIRED_INCLUDES ${GSSAPI_INCS}) > SET(CMAKE_REQUIRED_LIBRARIES ${GSSAPI_LIBS}) > INCLUDE(CheckCXXSymbolExists) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2d73406dad8: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Nikita! ok to push (better late than never :) On Oct 19, Nikita Malyavin wrote: > revision-id: 2d73406dad8 (mariadb-10.3.16-39-g2d73406dad8) > parent(s): 609f358a9bd > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-19 17:37:04 +0200 > message: > > MDEV-16481: set global system_versioning_asof=sf() crashes in specific case > > * sys_vars.h: add `MYSQL_TIME` field to `set_var::save_result` > * sys_vars.ic: get rid of calling `var->value->get_date()` from `Sys_var_vers_asof::update()` > * versioning.sysvars: add test; remove double warning > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e3f45b2f9ea: MDEV-10267 Add ngram fulltext parser plugin
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Rinat! On Oct 19, Rinat Ibragimov wrote: > revision-id: e3f45b2f9ea (mariadb-10.2.31-368-ge3f45b2f9ea) > parent(s): 44c48cefdbe > author: Rinat Ibragimov <ri(a)tempesta-tech.com> > committer: Rinat Ibragimov <ri(a)tempesta-tech.com> > timestamp: 2020-08-25 01:59:06 +0300 > message: > > MDEV-10267 Add ngram fulltext parser plugin > > Ngram is a fulltext parser plugin that splits words into overlapping > segments of fixed lengths. For example, if 3-grams are in use, string > "abcdef" is split into "abc", "bcd", "cde", "def", thus allowing an > efficient substring search for CJK texts where splitting by words in not > feasible. Only word characters are going into n-grams. Spaces and > punctuation characters are treated as separators. > > diff --git a/plugin/fulltext/CMakeLists.txt b/plugin/fulltext/CMakeLists.txt > index 3c37666cb3e..9704733a9aa 100644 > --- a/plugin/fulltext/CMakeLists.txt > +++ b/plugin/fulltext/CMakeLists.txt > @@ -18,3 +18,7 @@ set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -DNDEBUG") > > MYSQL_ADD_PLUGIN(ftexample plugin_example.c > MODULE_ONLY MODULE_OUTPUT_NAME "mypluglib" COMPONENT Test) > + > +# n-gram parser > +MYSQL_ADD_PLUGIN(ftngram ngram_parser/plugin_ngram.cc > + MODULE_ONLY MODULE_OUTPUT_NAME "fulltext_ngram") why to put the plugin under plugin/fulltext/ngram_parser/ ? it's only one file, just put it in plugin/fulltext/ > diff --git a/plugin/fulltext/mysql-test/ngram/disabled.def b/plugin/fulltext/mysql-test/ngram/disabled.def > new file mode 100644 > index 00000000000..0ca9dd33ef7 > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/disabled.def > @@ -0,0 +1 @@ > +ngram_gb18030 : GB18030 character set is not supported yet. I don't think there's any need to add tests for unsupported charsets. The plugin is trivial, no need to test it for every possible charset that we have and don't have. > diff --git a/plugin/fulltext/mysql-test/ngram/ngram_basic.inc b/plugin/fulltext/mysql-test/ngram/ngram_basic.inc > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/ngram_basic.inc > @@ -0,0 +1,25 @@ > +--echo > +--eval CREATE TABLE t (lang TEXT, str TEXT, FULLTEXT(str) WITH PARSER ngram) ENGINE=$engine CHARSET $charset > + > +# Some character are not possible to represent in some encodings. Ignore them. > +--disable_warnings > +--disable_query_log > +# http://www.gerolf.org/doc/polyglot/polyglot.htm. > +INSERT IGNORE INTO t VALUES > + ('Chinese', '他们为什么不说中文？'), > + ('English', 'Why can''t they just speak English?'), > + ('Japanese', 'なぜ、みんな日本語を話してくれないのか？'), > + ('Korean', '세계의 모든 사람들이 한국어를 이해한다면 얼마나 좋을까?'); > +--enable_query_log > +--enable_warnings > + > +SELECT * FROM t ORDER BY lang; > + > +SELECT * FROM t WHERE MATCH(str) AGAINST('之一' IN BOOLEAN MODE) ORDER BY lang; > +SELECT * FROM t WHERE MATCH(str) AGAINST('数据' IN BOOLEAN MODE) ORDER BY lang; > +SELECT * FROM t WHERE MATCH(str) AGAINST('zzz' IN BOOLEAN MODE) ORDER BY lang; > +SELECT * FROM t WHERE MATCH(str) AGAINST('gli' IN BOOLEAN MODE) ORDER BY lang; > +SELECT * FROM t WHERE MATCH(str) AGAINST('ない' IN BOOLEAN MODE) ORDER BY lang; > +SELECT * FROM t WHERE MATCH(str) AGAINST('람들' IN BOOLEAN MODE) ORDER BY lang; > + > +DROP TABLE t; > diff --git a/plugin/fulltext/mysql-test/ngram/ngram_basic.test b/plugin/fulltext/mysql-test/ngram/ngram_basic.test > new file mode 100644 > index 00000000000..eaa8829c3ee > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/ngram_basic.test > @@ -0,0 +1,25 @@ > +# Ngram fulltext parser basic functionality. > + > +SET NAMES utf8mb4; > +SELECT @@ngram_token_size; > + > +let $engine_index= 3; > +while ($engine_index) { > + if ($engine_index == 3) { let $engine= InnoDB; } > + if ($engine_index == 2) { let $engine= MyISAM; } > + if ($engine_index == 1) { let $engine= Aria; } > + > + let $charset_index= 6; > + while ($charset_index) { > + if ($charset_index == 6) { let $charset= utf8mb4; } > + if ($charset_index == 5) { let $charset= big5; } > + if ($charset_index == 4) { let $charset= gbk; } > + if ($charset_index == 3) { let $charset= gb2312; } > + if ($charset_index == 2) { let $charset= sjis; } > + if ($charset_index == 1) { let $charset= ujis; } > + > + source ngram_basic.inc; > + dec $charset_index; > + } eh. unroll the loop please, write let $engine= InnoDB; let $charset= utf8mb4; source ngram_basic.inc; let $charset= big5; source ngram_basic.inc; ... Or, better: CREATE TABLE t (lang TEXT, str TEXT, FULLTEXT(str) WITH PARSER ngram) ENGINE=InnoDB CHARSET utf8mb4; source ngram_basic.inc; CREATE TABLE t (lang TEXT, str TEXT, FULLTEXT(str) WITH PARSER ngram) ENGINE=InnoDB CHARSET big5; source ngram_basic.inc; ... > + dec $engine_index; > +} > diff --git a/plugin/fulltext/mysql-test/ngram/ngram_missing_plugin.result b/plugin/fulltext/mysql-test/ngram/ngram_missing_plugin.result > new file mode 100644 > index 00000000000..aa17b1d6053 > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/ngram_missing_plugin.result > @@ -0,0 +1,23 @@ there are tests for the missing fulltest parser plugin. they don't depend on the individial plugin (that is, btw, *not installed*). No need to repeat them for every plugin, remove this test. > +SET NAMES utf8mb4; > +CREATE TABLE t (str TEXT, FULLTEXT(str) WITH PARSER ngram) > +ENGINE=InnoDB; > +INSERT INTO t VALUES ('Hello World'), ('abcdef ghijklm'); > +# Restart with ngram plugin disabled. > +SET NAMES utf8mb4; > +SELECT * FROM t WHERE MATCH(str) AGAINST('bcd') ORDER BY str; > +ERROR HY000: Plugin 'ngram' is not loaded > +SELECT * FROM t ORDER BY str; > +ERROR HY000: Plugin 'ngram' is not loaded > +INSERT INTO t VALUES ('something'); > +ERROR HY000: Plugin 'ngram' is not loaded > +# Restart with ngram plugin enabled again. > +SET NAMES utf8mb4; > +SELECT * FROM t WHERE MATCH(str) AGAINST('bcd') ORDER BY str; > +str > +abcdef ghijklm > +SELECT * FROM t ORDER BY str; > +str > +abcdef ghijklm > +Hello World > +INSERT INTO t VALUES ('something'); > +DROP TABLE t; > diff --git a/plugin/fulltext/mysql-test/ngram/ngram_token_size.result b/plugin/fulltext/mysql-test/ngram/ngram_token_size.result > new file mode 100644 > index 00000000000..7043e147d5b > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/ngram_token_size.result > @@ -0,0 +1,109 @@ > +# Check 'ngram_token_size' variable. > +# > +# It's a global variable. > +SELECT @@global.ngram_token_size; > +@@global.ngram_token_size > +2 > +SHOW GLOBAL VARIABLES LIKE 'ngram_token_size'; > +Variable_name Value > +ngram_token_size 2 > +SELECT * FROM information_schema.global_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 2 > +# > +# Not a session variable. > +SELECT @@session.ngram_token_size; > +ERROR HY000: Variable 'ngram_token_size' is a GLOBAL variable > +SHOW SESSION VARIABLES LIKE 'ngram_token_size'; > +Variable_name Value > +ngram_token_size 2 > +SELECT * FROM information_schema.session_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 2 > +# > +# 0 is outside allowed range [1, 10], so it's changed to 1. > +SET GLOBAL ngram_token_size= 0; > +Warnings: > +Warning 1292 Truncated incorrect ngram_token_size value: '0' > +SELECT * FROM information_schema.global_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 1 > +# > +# 10 is an acceptable value. > +SET GLOBAL ngram_token_size= 10; > +SELECT * FROM information_schema.global_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 10 > +# > +# 1 is an acceptable value. > +SET GLOBAL ngram_token_size= 1; > +SELECT * FROM information_schema.global_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 1 > +# > +# 11 is outside allowed range [1, 10], so it's changed to 10. > +SET GLOBAL ngram_token_size= 11; > +Warnings: > +Warning 1292 Truncated incorrect ngram_token_size value: '11' > +SELECT * FROM information_schema.global_variables WHERE variable_name='ngram_token_size'; > +VARIABLE_NAME VARIABLE_VALUE > +NGRAM_TOKEN_SIZE 10 ^^^^ all tests above aren't needed, it's enough to do query_vertical select * from information_schema.system_variables where variable_name='ngram_token_size'; all the behavior above is implemented outside of the plugin, no need to test it. > +# > +# Try 3-grams. > +SET GLOBAL ngram_token_size= 3; > +CREATE TABLE t (str TEXT, FULLTEXT INDEX (str) WITH PARSER ngram) Engine=InnoDB; > +INSERT INTO t VALUES > +('Lorem ipsum dolor sit amet, consectetur adipiscing elit.'), > +('Maecenas non pellentesque odio.'), > +('Sed fermentum egestas sapien vel consequat.'); > +# Only one row contains substring 'api'. > +SELECT str FROM t WHERE MATCH(str) AGAINST('api'); > +str > +Sed fermentum egestas sapien vel consequat. > +# 'lent' is parsed into ('len', 'ent') list. Two rows contain at least one of the tokens. > +SELECT str FROM t WHERE MATCH(str) AGAINST('lent*'); > +str > +Maecenas non pellentesque odio. > +Sed fermentum egestas sapien vel consequat. > +# 'zzz' is not present anywhere, so result is empty. > +SELECT str FROM t WHERE MATCH(str) AGAINST('zzzz'); > +str > +# 'piz' cannot be found anywhere, but 'api' is in one of the rows. > +SELECT str FROM t WHERE MATCH(str) AGAINST('apiz'); > +str > +Sed fermentum egestas sapien vel consequat. > +# > +# Try switching to 2-grams while keeping the index. > +SET GLOBAL ngram_token_size= 2; > +# 'api' is now parsed into ('ap', 'pi'). Since index only contains 3-grams, nothing is found. > +SELECT str FROM t WHERE MATCH(str) AGAINST('api'); > +str > +# > +# Add some data while ngram size is set to 2. > +INSERT INTO t VALUES('1234 56789'); > +# > +# Now index contains 3-grams from the rows added before and a number of > +# 2-grams from the just added row. > +# Querying for 'api' still returns nothing, but line with '678' is found. > +SELECT str from t WHERE MATCH(str) AGAINST('api'); > +str > +SELECT str FROM t WHERE MATCH(str) AGAINST('678'); > +str > +1234 56789 > +# > +# Switching back to 3-grams make intitially added rows searchable. > +SET GLOBAL ngram_token_size= 3; > +SELECT str FROM t WHERE MATCH(str) AGAINST('api'); > +str > +Sed fermentum egestas sapien vel consequat. > +# > +# But row with '678' in it cannot be found, as it was added with 2-grams. > +SELECT str FROM t WHERE MATCH(str) AGAINST('678'); > +str > +# > +# Try matching in boolean mode. > +SELECT str FROM t WHERE MATCH(str) AGAINST('+ent -api' IN BOOLEAN MODE); > +str > +Maecenas non pellentesque odio. > +# > +DROP TABLE t; > diff --git a/plugin/fulltext/mysql-test/ngram/ngram_ucs2.result b/plugin/fulltext/mysql-test/ngram/ngram_ucs2.result > new file mode 100644 > index 00000000000..9d82743bd24 > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/ngram_ucs2.result > @@ -0,0 +1,8 @@ > +CREATE TABLE t (str TEXT, FULLTEXT INDEX (str) WITH PARSER ngram) Engine=InnoDB CHARSET=UCS2; > +ERROR HY000: Column 'str' cannot be part of FULLTEXT index > +CREATE TABLE t (str TEXT, FULLTEXT INDEX (str) WITH PARSER ngram) Engine=InnoDB CHARSET=UTF16; > +ERROR HY000: Column 'str' cannot be part of FULLTEXT index > +CREATE TABLE t (str TEXT, FULLTEXT INDEX (str) WITH PARSER ngram) Engine=InnoDB CHARSET=UTF16LE; > +ERROR HY000: Column 'str' cannot be part of FULLTEXT index > +CREATE TABLE t (str TEXT, FULLTEXT INDEX (str) WITH PARSER ngram) Engine=InnoDB CHARSET=UTF32; > +ERROR HY000: Column 'str' cannot be part of FULLTEXT index same as above. This is fulltext index limitation, it does not depend on the parser plugin. The plugin doesn't need to test for it. > diff --git a/plugin/fulltext/mysql-test/ngram/suite.opt b/plugin/fulltext/mysql-test/ngram/suite.opt > new file mode 100644 > index 00000000000..ab37cea996e > --- /dev/null > +++ b/plugin/fulltext/mysql-test/ngram/suite.opt > @@ -0,0 +1,4 @@ > +--plugin-load-add=$FULLTEXT_NGRAM_SO > +--loose-innodb no need to enable innodb explicitly, `source have_innodb.inc` does that. > +--loose-enable-innodb_ft_index_table > +--loose-innodb_ft_index_cache why did you enable these two plugins if you don't use them in your tests? > diff --git a/plugin/fulltext/ngram_parser/plugin_ngram.cc b/plugin/fulltext/ngram_parser/plugin_ngram.cc > new file mode 100644 > index 00000000000..80498c3299b > --- /dev/null > +++ b/plugin/fulltext/ngram_parser/plugin_ngram.cc > @@ -0,0 +1,256 @@ > +/* > + Copyright (c) 2014, 2017, Oracle and/or its affiliates. All rights reserved. > + Copyright (c) 2020, MariaDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License, version 2.0, > + as published by the Free Software Foundation. > + > + This program is also distributed with certain software (including > + but not limited to OpenSSL) that is licensed under separate terms, > + as designated in a particular file or component or in included license > + documentation. The authors of MySQL hereby grant you an additional > + permission to link the program and your derivative works with the > + separately licensed software that they have included with MySQL. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License, version 2.0, for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > + > +#include <stddef.h> > +#include <limits.h> > +#include <my_attribute.h> > +#include <my_global.h> > +#include "../storage/innobase/include/fts0tokenize.h" No, the plugin should be self-contained and not include anything from other plugins. At least if a plugin is as trivial as that one. > + > +#define U_LOW_LINE 0x5f // Code point for underscore '_'. > +#define is_alnum(ct) ((ct) & (_MY_U | _MY_L | _MY_NMR)) > + > +static int ngram_token_size; > + > +static MYSQL_SYSVAR_INT(token_size, ngram_token_size, > + 0, > + "Ngram full text plugin parser token size in characters", > + NULL, NULL, 2, 1, 10, 0); > + > +static int my_ci_charlen(CHARSET_INFO *cs, const uchar *str, const uchar *end) > +{ > + return my_charlen(cs, reinterpret_cast<const char *>(str), > + reinterpret_cast<const char *>(end)); > +} > + > +static int my_ci_ctype(CHARSET_INFO *cs, int *char_type, const uchar *str, > + const uchar *end) > +{ > + return cs->cset->ctype(cs, char_type, str, end); > +} > + > +static bool is_underscore(CHARSET_INFO *cs, const uchar *str, const uchar *end) > +{ > + my_wc_t wc; > + cs->cset->mb_wc(cs, &wc, str, end); > + return wc == U_LOW_LINE; > +} > + > +// Splits a string into ngrams and emits them. > +static int split_into_ngrams(MYSQL_FTPARSER_PARAM *param, const uchar *doc, > + int len, MYSQL_FTPARSER_BOOLEAN_INFO *bool_info) > +{ > + const CHARSET_INFO *cs= param->cs; > + const uchar *start= doc; > + const uchar *end= doc + len; > + const uchar *next= start; > + int n_chars= 0; > + int ret= 0; > + bool is_first= true; > + > + while (next < end) { > + int char_type; > + int char_len= my_ci_ctype(cs, &char_type, next, end); > + > + // Broken data? > + if (next + char_len > end || char_len == 0) > + break; > + > + // Avoid creating partial n-grams by stopping at word boundaries. > + // Underscore is treated as a word character too, since identifiers in > + // programming languages often contain them. > + if (!is_alnum(char_type) && !is_underscore(cs, next, end)) { > + start= next + char_len; > + next= start; > + n_chars= 0; > + continue; I don't think it's a good idea. On the opposite, I would rather create ngrams over word boundaries, so that, say, I'd split "n-gram plugin" to "n-g", "-gr", "gra", "ram", "am ", "m p", " pl", "plu", "lug", "ugi", "gin". May be better would be to split it into "n-gr", "gra", "ram", "am p", "m pl", "plu", "lug", "ugi", "gin". (that is, always three letters in an n-gram, not three characters). This needs testing to find out what approach works better. > + } > + > + next += char_len; > + n_chars++; > + > + if (n_chars == ngram_token_size) { > + bool_info->position= static_cast<uint>(start - doc); nope, we don't have MYSQL_FTPARSER_FULL_BOOLEAN_INFO::position and this plugin is definitely not the reason to break the API and add it. > + ret= param->mysql_add_word(param, reinterpret_cast<const char *>(start), > + static_cast<int>(next - start), bool_info); > + if (ret != 0) > + return ret; > + > + start += my_ci_charlen(cs, start, end); why do you scan the string again here, you've already counted character by character above? > + n_chars= ngram_token_size - 1; > + is_first= false; > + } > + } > + > + // If nothing was emitted yet, emit the remainder. what reminder, what is it for? > + switch (param->mode) { > + case MYSQL_FTPARSER_FULL_BOOLEAN_INFO: > + case MYSQL_FTPARSER_WITH_STOPWORDS: > + if (n_chars > 0 && is_first) { > + assert(next > start); > + assert(n_chars < ngram_token_size); > + > + bool_info->position= static_cast<uint>(start - doc); > + ret= param->mysql_add_word(param, reinterpret_cast<const char *>(start), > + static_cast<int>(next - start), bool_info); > + } > + break; > + > + default: > + break; > + } > + > + return ret; > +} > + > +static int number_of_chars(const CHARSET_INFO *cs, const uchar *buf, int len) > +{ > + const uchar *ptr= buf; > + const uchar *end= buf + len; > + int size= 0; > + > + while (ptr < end) { > + ptr += my_ci_charlen(cs, ptr, end); > + size += 1; > + } > + > + return size; > +} > + > +// Convert term into phrase and handle wildcard. > +static int ngram_term_convert(MYSQL_FTPARSER_PARAM *param, const uchar *token, > + int len, MYSQL_FTPARSER_BOOLEAN_INFO *bool_info) > +{ > + MYSQL_FTPARSER_BOOLEAN_INFO token_info= > + { FT_TOKEN_WORD, 0, 0, 0, 0, 0, ' ', 0 }; > + const CHARSET_INFO *cs= param->cs; > + int ret= 0; > + > + assert(bool_info->type == FT_TOKEN_WORD); > + assert(bool_info->quot == NULL); > + > + /* Convert rules: > + * 1. if term with wildcard and term length is less than ngram_token_size, > + * we keep it as normal term search. > + * 2. otherwise, term is converted to phrase and wildcard is ignored. > + * e.g. 'abc' and 'abc*' are both equivalent to '"ab bc"'. > + */ > + > + if (bool_info->trunc && number_of_chars(cs, token, len) < ngram_token_size) { > + ret= param->mysql_add_word(param, reinterpret_cast<const char *>(token), > + len, bool_info); > + } else { > + bool_info->type= FT_TOKEN_LEFT_PAREN; > + bool_info->quot= reinterpret_cast<char*>(1); > + > + ret= param->mysql_add_word(param, NULL, 0, bool_info); > + if (ret != 0) > + return ret; > + > + ret= split_into_ngrams(param, token, len, &token_info); > + if (ret != 0) > + return ret; > + > + bool_info->type= FT_TOKEN_RIGHT_PAREN; > + ret= param->mysql_add_word(param, NULL, 0, bool_info); > + > + assert(bool_info->quot == NULL); > + bool_info->type= FT_TOKEN_WORD; > + } > + > + return ret; > +} > + > +static int ngram_parser_parse(MYSQL_FTPARSER_PARAM *param) > +{ > + MYSQL_FTPARSER_BOOLEAN_INFO bool_info= > + { FT_TOKEN_WORD, 0, 0, 0, 0, 0, ' ', 0 }; > + const CHARSET_INFO *cs= param->cs; > + uchar *start= reinterpret_cast<uchar *>(const_cast<char *>(param->doc)); > + uchar *end= start + param->length; > + FT_WORD word= {NULL, 0, 0}; > + int ret= 0; > + > + switch (param->mode) { > + case MYSQL_FTPARSER_SIMPLE_MODE: > + case MYSQL_FTPARSER_WITH_STOPWORDS: > + ret= split_into_ngrams(param, start, static_cast<int>(end - start), > + &bool_info); > + break; > + > + case MYSQL_FTPARSER_FULL_BOOLEAN_INFO: > + // Reusing InnoDB's boolean mode parser to handle all special characters. > + while (fts_get_word(cs, &start, end, &word, &bool_info)) { nope. InnoDB might not even be compiled it. Use param->mysql_parse() > + if (bool_info.type == FT_TOKEN_WORD) { > + if (bool_info.quot != NULL) { > + ret= split_into_ngrams(param, word.pos, word.len, &bool_info); > + } else { > + ret= ngram_term_convert(param, word.pos, word.len, &bool_info); > + } > + } else { > + ret= param->mysql_add_word(param, reinterpret_cast<char*>(word.pos), > + word.len, &bool_info); > + } > + > + if (ret != 0) > + return ret; > + } > + > + break; > + } > + > + return ret; > +} > + > +static struct st_mysql_ftparser ngram_parser_descriptor= > +{ > + MYSQL_FTPARSER_INTERFACE_VERSION, > + ngram_parser_parse, > + NULL, > + NULL, > +}; > + > +static struct st_mysql_sys_var *ngram_system_variables[]= > +{ > + MYSQL_SYSVAR(token_size), > + NULL > +}; > + > +maria_declare_plugin(ngram_parser) > +{ > + MYSQL_FTPARSER_PLUGIN, // Type. > + &ngram_parser_descriptor, // Descriptor. > + "ngram", // Name. > + "Oracle Corp", // Author. > + "Ngram Full-Text Parser", // Description. > + PLUGIN_LICENSE_GPL, // License. > + NULL, // Initialization function. > + NULL, // Deinitialization function. > + 0x0100, // Numeric version. > + NULL, // Status variables. > + ngram_system_variables, // System variables. > + "1.0", // String version representation. > + MariaDB_PLUGIN_MATURITY_EXPERIMENTAL, // Maturity. > +} > +maria_declare_plugin_end; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 12d5c4f2813: Fixed problems with S3 after "DROP TABLE FORCE" changes
by Sergei Golubchik 19 Oct '20

19 Oct '20

Hi, Monty! On Oct 16, Michael Widenius wrote: > revision-id: 12d5c4f2813 (mariadb-10.5.4-190-g12d5c4f2813) > parent(s): afa5f735f30 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-13 17:48:15 +0300 > message: > > Fixed problems with S3 after "DROP TABLE FORCE" changes > > MDEV-23691 S3 storage engine: delayed slave can drop the table please put the MDEV on the first line of the commit comment > > This commit also fixes all failing replication S3 tests. > > A slave is delayed if it is trying to execute replicated queries on a > table that is already converted to S3 by the master. What do you mean, a slave is delayed? > > Fixes for replication events on S3 tables for delayed slaves: > - INSERT and INSERT ... SELECT and CREATE TABLE are ignored but written > to the binary log. UPDATE & DELETE will be fixed in a future commit. > > Other things: > - On slaves with --s3-slave-ignore-updates set, allow S3 tables to be > opened in read-write mode. This was done to be able to > ignore-but-replicate queries like insert. Without this change any > open of an S3 table failed with 'Table is read only' which is too > early to be able to replicate the original query. > - Errors are now printed if handler::extra() call fails in > wait_while_tables_are_used() > - Error message for row changes are changed from HA_ERR_WRONG_COMMAND > to HA_ERR_TABLE_READONLY > - Disable some maria_extra() calls for S3 tables. This could cause > S3 tables to fail in some cases. > - Added missing thr_lock_delete() to ma_open() in case of failure > > diff --git a/sql/log.cc b/sql/log.cc > index 2a887a68606..9c656387e90 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2151,6 +2151,12 @@ static int binlog_commit(handlerton *hton, THD *thd, bool all) > > DBUG_RETURN(0); > } > + /* > + This is true if we are doing an alter table that is replicated as > + CREATE TABLE ... SELECT > + */ > + if (thd->variables.option_bits & OPTION_BIN_COMMIT_OFF) > + DBUG_RETURN(0); why binlog_commit is even called here? > > DBUG_PRINT("debug", > ("all: %d, in_transaction: %s, all.modified_non_trans_table: %s, stmt.modified_non_trans_table: %s", > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index f4cbda5490c..7c103756566 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7476,6 +7476,34 @@ int THD::binlog_query(THD::enum_binlog_query_type qtype, char const *query_arg, > DBUG_RETURN(0); > } > > + > +/** > + Binlog current query as a statement if binary log is open > + > + @retval false ok > + @retval true error > +*/ > + > + > +bool THD::binlog_current_query() > +{ > + if (!mysql_bin_log.is_open()) > + return 0; > + > + /* > + The following is needed if the query was "ignored" by a shared distributed > + engine, in which case decide_logging_format() will set the binlog filter > + */ > + reset_binlog_local_stmt_filter(); > + clear_binlog_local_stmt_filter(); > + return binlog_query(THD::STMT_QUERY_TYPE, query(), query_length(), > + /* is_trans */ FALSE, > + /* direct */ FALSE, > + /* suppress_use */ FALSE, > + /* Error */ 0) > 0; > +} you created a generally looking function with a rather special behavior. Please rename it to make sure it's not a general purpose helper. For example, call it binlog_current_query_unfiltered() or force_binlog_current_query(). And the function comment should say Force-binlog current query as a statement ignoring the binlog filter setting > + > + > void > THD::wait_for_wakeup_ready() > { > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 4ad4c478937..5f59e064a35 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -724,6 +724,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > Item *unused_conds= 0; > DBUG_ENTER("mysql_insert"); > > + bzero((char*) &info,sizeof(info)); why did you move it up? > create_explain_query(thd->lex, thd->mem_root); > /* > Upgrade lock type if the requested lock is incompatible with > @@ -764,16 +765,27 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, > DBUG_RETURN(TRUE); > value_count= values->elements; > > - if (mysql_prepare_insert(thd, table_list, table, fields, values, > - update_fields, update_values, duplic, > - &unused_conds, FALSE)) > + if ((res= mysql_prepare_insert(thd, table_list, table, fields, values, > + update_fields, update_values, duplic, > + &unused_conds, FALSE))) > + { > + retval= thd->is_error(); > + if (res < 0) > + { > + /* > + Insert should be ignored but we have to log the query in statement > + format in the binary log > + */ > + res= thd->binlog_current_query(); abort label doesn't look at `res` it returns `retval`. So I don't think you need `retval= thd->is_error()` at all, and instead you need if (res < 0) retval= thd->binlog_current_query(); > + } > goto abort; > + } > + /* mysql_prepare_insert sets table_list->table if it was not set */ > + table= table_list->table; > > /* Prepares LEX::returing_list if it is not empty */ > if (returning) > result->prepare(returning->item_list, NULL); > - /* mysql_prepare_insert sets table_list->table if it was not set */ > - table= table_list->table; why did you move this up? > > context= &thd->lex->first_select_lex()->context; > /* > @@ -1575,21 +1587,29 @@ bool mysql_prepare_insert(THD *thd, TABLE_LIST *table_list, > DBUG_ASSERT (!select_insert || !values); > > if (mysql_handle_derived(thd->lex, DT_INIT)) > DBUG_RETURN(1); > if (table_list->handle_derived(thd->lex, DT_MERGE_FOR_INSERT)) > DBUG_RETURN(1); > if (thd->lex->handle_list_of_derived(table_list, DT_PREPARE)) > DBUG_RETURN(1); > > if (duplic == DUP_UPDATE) > { > /* it should be allocated before Item::fix_fields() */ > if (table_list->set_insert_values(thd->mem_root)) > DBUG_RETURN(1); > + } > + > + if (!table) > + table= table_list->table; As far as I can see, table in mysql_prepare_insert is *always* either table_list->table or NULL. At least in 10.5. So I'd rather remove the table argument completely and let mysql_prepare_insert always use table_list->table. > + > + if (table->file->check_if_updates_are_ignored("INSERT")) > + { > + DBUG_RETURN(-1); > } > > if (mysql_prepare_insert_check_table(thd, table_list, fields, select_insert)) > DBUG_RETURN(1); > > /* Prepare the fields in the statement. */ > if (values) > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 15d190c3139..2843496f783 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -5206,7 +5215,19 @@ int create_table_impl(THD *thd, const LEX_CSTRING &orig_db, > goto err; > } > else if (options.if_not_exists()) > + { > + /* > + We never come here as part of normal create table as table existance > + is checked in open_and_lock_tables(). We may come here as part of > + ALTER TABLE > + */ How so? There is no ALTER IF NOT EXISTS. And this is the old code, so it seems to be for CREATE TABLE IF NOT EXISTS. > + > + /* Log CREATE IF NOT EXISTS on slave for distributed engines */ > + if (thd->slave_thread && (exists_hton && exists_hton->flags & > + HTON_IGNORE_UPDATES)) > + thd->log_current_statement= 1; > goto warn; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] MDEV-21265: IN predicate conversion to IN subquery should be allowed for a broader set of datatype comparison
by Sergey Petrunia 16 Oct '20

16 Oct '20

Hi Varun, > Commit 3e29c5a1620deef2fa9b25a0d42b4c07fe59e96f > > MDEV-21265: IN predicate conversion to IN subquery should be allowed for a broader set of datatype comparison > > Allow materialization strategy when the collation of the columns of the > left side of the IN subquery are not same as the one on the right side > of the IN subquery but are compatible with each other. > This allows conversion from utf8mb3 to utf8mb4 as the former is a subset of the later. Please fix the comment to say what the patch does: When collations on the inner and outer sides are the same, and the character set of the inner side is a proper subset of the charset on the outer. > > diff --git a/mysql-test/main/subselect4.result b/mysql-test/main/subselect4.result > index 020a70642913..f3ae2398d45e 100644 > --- a/mysql-test/main/subselect4.result > +++ b/mysql-test/main/subselect4.result > @@ -2718,3 +2718,172 @@ Warning 1931 Query execution was interrupted. The query examined at least 3020 r > SET join_cache_level= @save_join_cache_level; > DROP TABLE t1,t2,t3,t4; > # End of 10.2 tests > +# > +# MDEV-21265: IN predicate conversion to IN subquery should be allowed for a broader set of datatype comparison > +# > +CREATE TABLE t1 ( col1 varchar (32) collate utf8_general_ci); > +CREATE TABLE t2 ( col1 varchar (32) collate utf8mb4_general_ci, key(col1)); > +INSERT INTO t1 select seq from seq_1_to_100; > +INSERT INTO t2 values (_utf8mb4 X'F09F9883'), (1),('?'); ... > EXPLAIN FORMAT=JSON SELECT HEX(col1), col1 IN (SELECT col1 FROM t1) FROM t2; Please insert the '?' into the table t1, not t2. The idea of the check is to check for the following: - We get smiley symbol from table t2. - We make the lookup key from it. This results in an error and the '?' as output. - Then, a lookup is made in t1. And we are trying to provoke a wrong result by having the '?' in t1. OK to push into the custom-build tree when the above is addressed. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 0d05c9f8699: Add mytop to Client component for rpm package
by Sergei Golubchik 13 Oct '20

13 Oct '20

Hi, Anel! About the diff to 1.9, I tend to agree that our mytop is a superset. I did have some comments when looking at the diff, couldn't help it :) Here they are: > diff --git a/scripts/mytop.sh b/../mytop-1.9.1-4 > index 321b79537fc..056b7a9a806 100644 > --- b/../mytop-1.9.1-4 > +++ a/scripts/mytop.sh > @@ -1,13 +1,13 @@ > -#!/usr/bin/perl -w > +#!/usr/bin/env perl I thought we're getting rid of "env perl" ? > # > -# $Id: mytop,v 1.91 2012/01/18 16:49:12 mgrennan Exp $ > +# $Id: mytop,v 1.99-maria6 2019/10/22 14:53:51 jweisbuch Exp $ > > =pod > > =head1 NAME > > -mytop - display MySQL server performance info like `top' > +mytop - display MariaDB server performance info like `top' may be "MariaDB/MySQL" here and below? > > =cut > > @@ -242,11 +253,15 @@ my $dsn; > > ## Socket takes precedence. > > -$dsn ="DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; > +if (eval {DBI->install_driver("MariaDB")}) { > + $dsn = "DBI:MariaDB:database=$config{db};mariadb_read_default_group=mytop;"; > +} else { > + $dsn = "DBI:mysql:database=$config{db};mysql_read_default_group=mytop;"; > +} > > if ($config{socket} and -S $config{socket}) > { > - $dsn .= "mysql_socket=$config{socket}"; > + $dsn .= "mariadb_socket=$config{socket}"; if you support both drivers, you need to use the correct option here > } > else > { > @@ -1027,10 +918,15 @@ sub GetData() > } > } > > - open L, "</proc/loadavg"; > - my $l = <L>; > - close L; > - chomp $l; > + my $l; > + if (-e "/proc/loadavg") > + { > + ## To avoid warnings if the OS is not Linux > + open (my $fh, "<", "/proc/loadavg"); > + ## Only the first 3 values are interresting rr? > + $l = join(" ", (split /\s+/, <$fh>)[0..2]); > + close $fh; > + } > > $last_time = $now_time; > > @@ -1960,11 +1694,15 @@ sub PrintHelp() > S - change slow query hightlighting > t - switch to thread view (default) > u - show only a specific user > + V - show variables > : - enter a command (not yet implemented) > ! - Skip an error that has stopped replications (at your own risk) > + L - show full queries (do not strip to terminal width) > + w - adjust the User and DB columns width > + a - toggle the progress column > > -${GREEN}http://jeremy.zawodny.com/mysql/mytop/${RESET} > -${GREEN}http://www.mysqlfanboy.com/mytop-3/${RESET} > +Base version from ${GREEN}http://www.mysqlfanboy.com/mytop-3${RESET} if it's mytop-3, perhaps it should have the version 3.x and not 1.99-maria6 ? > +This version comes as part of the ${GREEN}MariaDB${RESET} distribution. > ]; > > print $help; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 2

Re: [Maria-developers] d19f380d62c: MDEV-21445 Strange/inconsistent behavior of IN condition when mixing numbers and strings
by Sergei Golubchik 12 Oct '20

12 Oct '20

Hi, Alexander! On Oct 12, Alexander Barkov wrote: > revision-id: d19f380d62c (mariadb-10.3.21-213-gd19f380d62c) > parent(s): 90c8d773ed0 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2020-08-18 13:19:21 +0400 > message: > > MDEV-21445 Strange/inconsistent behavior of IN condition when mixing numbers and strings > > Comparison is now performed as DECIMAL for pairs (int,string) and (string,int). Are there tests with strings like '1e100', outside of bigint range ? > Before the patch the comparison was performed in DOUBLE format, > so precision loss happened on huge values, e.g.: > > '97716021308405775' was converted to > 97716021308405770 (notice, the last digit was lost) > > diff --git a/sql/field.h b/sql/field.h > index be3a648617b..c9f5a34a3ae 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -311,7 +311,7 @@ class Value_source > const char *end) > { > Converter_str2my_decimal_with_warn(NULL, Warn_filter_all(), > - E_DEC_FATAL_ERROR & ~E_DEC_BAD_NUM, > + 0, why did you remove warnings (here and elsewhere)? would be good to have it in the commit comment too, not just in email. > cs, cptr, end - cptr, decimal_value); > return decimal_value; > } > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 9d3a47adfa5..7a847c90b8f 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -764,6 +764,11 @@ Type_handler_hybrid_field_type::aggregate_for_comparison(const Type_handler *h) > m_type_handler= &type_handler_datetime; > } > } > + else if ((a == INT_RESULT && b == STRING_RESULT) || > + (b == INT_RESULT && a == INT_RESULT)) looks like a typo, the second line should be b == INT_RESULT && a == STRING_RESULT please add some tests to cover it. > + { > + m_type_handler= &type_handler_newdecimal; > + } > else if ((a == INT_RESULT || a == DECIMAL_RESULT) && > (b == INT_RESULT || b == DECIMAL_RESULT)) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2792940391a: MDEV-23327 Can't uninstall UDF if the implementation library file doesn't exist
by Sergei Golubchik 11 Oct '20

11 Oct '20

Hi, Oleksandr! Mostly ok, one problem below: On Oct 10, Oleksandr Byelkin wrote: > revision-id: 2792940391a (mariadb-10.2.31-343-g2792940391a) > parent(s): 2107e3bb9c0 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-07-29 21:54:24 +0200 > message: > > MDEV-23327 Can't uninstall UDF if the implementation library file doesn't exist > > Made cleanup of DROP (udf) FUNCTION procedure and also check of mysql.func (not only loaded udf). > > @@ -606,43 +648,54 @@ int mysql_create_function(THD *thd,udf_func *udf) > } > > > -int mysql_drop_function(THD *thd,const LEX_STRING *udf_name) > +enum drop_udf_result mysql_drop_function(THD *thd, const LEX_STRING *udf_name) > { > TABLE *table; > - TABLE_LIST tables; > udf_func *udf; > DBUG_ENTER("mysql_drop_function"); > > + if (!(table= open_udf_func_table(thd))) > + DBUG_RETURN(UDF_DEL_RESULT_ERROR); > + > + // Pre-check without protection > + if (!find_udf_everywhere(thd, udf_name->str, udf_name->length, table)) you cannot check the udf_hash without a lock > + { > + close_mysql_tables(thd); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Unsubscribe
by TINKU BANSAL 11 Oct '20

11 Oct '20

Hi there, Please Unsubscribe me to this mailing list. Thanks

2 1

Re: [Maria-developers] 62c35fe93e1: MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT
by Sergei Golubchik 09 Oct '20

09 Oct '20

Hi, Oleksandr! On Oct 09, Oleksandr Byelkin wrote: > revision-id: 62c35fe93e1 (mariadb-10.2.31-487-g62c35fe93e1) > parent(s): f584d567dbb > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-10-07 13:43:39 +0200 > message: > > MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT > > part 2: > > - check that expressions are evaluable for > making empty row and assigning PS variable > > - correctly handling writing to a temporary tabe during multi-update > by setting associated field > > - Item::raise_error_not_evaluable() bakported from 10.4 and made vitrual > > - Item::is_evaluable_expression() bakported from 10.4 > > - Item::check_is_evaluable_expression_or_error() bakported from 10.4 I still don't quite understand all details of your patch. But here're more questions and comments below: > diff --git a/sql/field.cc b/sql/field.cc > index bdaaecc2026..8ce412a207d 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -11183,6 +11205,16 @@ bool Field::save_in_field_default_value(bool view_error_processing) > { > THD *thd= table->in_use; > > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } I'd still think it'd be better to do a series of asserts. a complex condition to check for something that cannot ever happen? > if (flags & NO_DEFAULT_VALUE_FLAG && > real_type() != MYSQL_TYPE_ENUM) > { > @@ -11221,13 +11253,30 @@ bool Field::save_in_field_default_value(bool view_error_processing) > > bool Field::save_in_field_ignore_value(bool view_error_processing) > { > - enum_sql_command com= table->in_use->lex->sql_command; > - // All insert-like commands > - if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE || > - com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT || > - com == SQLCOM_LOAD) > - return save_in_field_default_value(view_error_processing); > - return 0; // ignore > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } ditto > + switch (find_ignore_reaction(table->in_use)) > + { > + case IGNORE_MEANS_DEFAULT: > + return save_in_field_default_value(view_error_processing); > + case IGNORE_MEANS_FIELD_VALUE: > + return 0; // ignore > + default: > + ; // fall through to error > + } > + > + // unexpected command > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > } > > > diff --git a/sql/item.cc b/sql/item.cc > index 994d45a9dc3..401ee5d6be2 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -84,6 +85,17 @@ void item_init(void) > } > > > +void Item::raise_error_not_evaluable() > +{ > + String tmp; > + this->print(&tmp, QT_ORDINARY); > + // TODO-10.5: add an error message to errmsg-utf8.txt > + my_printf_error(ER_UNKNOWN_ERROR, > + "'%s' is not allowed in this context", MYF(0), tmp.ptr()); > + DBUG_ASSERT(0); // cannot be happen before 10.5 > +} > + > + better remove it, this dead code won't simplify merges. In 10.3 it'll be added with a merge normally. having check_is_evaluable_expression_or_error() makes sense though, it is used and copying it from 10.3 will help to merge indeed or, may be, just backport the patch from 10.3 to 10.2? > void Item::push_note_converted_to_negative_complement(THD *thd) > { > push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR, > @@ -8968,18 +9084,26 @@ bool Item_default_value::fix_fields(THD *thd, Item **items) > def_field->move_field_offset((my_ptrdiff_t) > (def_field->table->s->default_values - > def_field->table->record[0])); > - set_field(def_field); > - return FALSE; > + return def_field; > +} > > -error: > - context->process_error(thd); > - return TRUE; > +bool Item_default_value::associate_with_target_field(THD *thd, > + Item_field *field) > +{ > + associated= true; > + arg= field; iirc, `arg` used to tell the difference between DEFAULT and DEFAULT(x). how are you doing it now? > + return tie_field(thd); > } > > void Item_default_value::cleanup() > { > delete cached_field; // Free cached blob data > cached_field= 0; > + if (associated) > + { > + arg= NULL; > + associated= false; > + } > Item_field::cleanup(); > } > > @@ -9102,8 +9226,54 @@ void Item_ignore_value::print(String *str, enum_query_type query_type) > str->append(STRING_WITH_LEN("ignore")); > } > > + > +bool Item_ignore_value::associate_with_target_field(THD *thd, > + Item_field *field) > +{ > + bitmap_set_bit(field->field->table->read_set, field->field->field_index); > + return Item_default_value::associate_with_target_field(thd, field); > +} > + > + > int Item_ignore_value::save_in_field(Field *field_arg, bool no_conversions) > { > + if (arg) how is IGNORE with arg!=0 differs from IGNORE with arg==0? when happens what? > + { > + switch (find_ignore_reaction(field->table->in_use)) > + { > + case IGNORE_MEANS_DEFAULT: > + DBUG_ASSERT(0); // impossible now, but fully working code if needed > + /* > + save default value > + Note: (((Field*)(arg()->real_item()) is Item_field* (checked in > + tie_field(). > + */ > + if ((((Item_field*)(arg->real_item()))->field->flags & > + NO_DEFAULT_VALUE_FLAG)) > + { > + my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0), > + ((Item_field*)(arg->real_item()))->field->field_name); > + return true; > + } > + calculate(); > + return Item_field::save_in_field(field_arg, no_conversions); > + case IGNORE_MEANS_FIELD_VALUE: > + // checked on tie_field > + DBUG_ASSERT(arg->real_item()->type() == FIELD_ITEM); > + /* > + save original field (Item::save_in_field is not applicable because > + result_field for the referenced field set to this temporary table). > + */ > + arg->save_val(field_arg); > + return false; > + default: > + ; // fall through to error > + } > + DBUG_ASSERT(0); //impossible > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return true; > + } > + > return field_arg->save_in_field_ignore_value(context->error_processor == > &view_error_processor); > } > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index 6a650183fb8..41a27a3aaaf 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -418,6 +418,9 @@ sp_eval_expr(THD *thd, Field *result_field, Item **expr_item_ptr) > if (!(expr_item= sp_prepare_func_item(thd, expr_item_ptr))) > goto error; > > + if (expr_item->check_is_evaluable_expression_or_error()) > + goto error; for CALL(?) ? > + > /* > Set THD flags to emit warnings/errors in case of overflow/type errors > during saving the item into the field. > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index cc77b58cb3e..b52d6f96a84 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8345,6 +8346,9 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values, > field->field_name, table->s->table_name.str); > } > > + if(evaluable && value->check_is_evaluable_expression_or_error()) > + goto err; Is it a good idea to do this check constantly for every user and for every row in selects, insert/update, etc? Could it be done inside IGNORE/DEFAULT? For example, by checking if field->table->tmp_table == INTERNAL_TMP_TABLE. > + > if (use_value) > value->save_val(field); > else > diff --git a/sql/unireg.cc b/sql/unireg.cc > index 083960523c1..91f9073c7ef 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -1002,6 +1002,12 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint table_options, > > int res= !expr->fixed && // may be already fixed if ALTER TABLE > expr->fix_fields(thd, &expr); > + if (!res && expr->check_is_evaluable_expression_or_error()) is this for CREATE TABLE case? > + { > + error= 1; > + delete regfield; //To avoid memory leak > + goto err; > + } > if (!res) > res= expr->save_in_field(regfield, 1); > if (!res && (field->flags & BLOB_FLAG)) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 56f2b3d12b6: MDEV-23264 Unique blobs allow duplicate values upon UPDATE
by Sergei Golubchik 08 Oct '20

08 Oct '20

Hi, Sachin! I suspect is_null() could be wrong here. Please, add a test case where a table has a BEFORE UPDATE trigger and a long UNIQUE over a NOT NULL field. And see how cmp_binary_offset() is used in compare_record(), for example. On Oct 08, Sachin Setiya wrote: > revision-id: 56f2b3d12b6 (mariadb-10.4.11-385-g56f2b3d12b6) > parent(s): af834c218a5 > author: Sachin Setiya <sachin.setiya(a)mariadb.com> > committer: Sachin Setiya <sachin.setiya(a)mariadb.com> > timestamp: 2020-09-14 20:08:18 +0100 > message: > > MDEV-23264 Unique blobs allow duplicate values upon UPDATE > > Problem:- > We are able to insert duplicate value in table because cmp_binary_offset > is not able to differentiate between NULL and empty string. So > check_duplicate_long_entry_key is never called and we don't check for > duplicate. > Solution > Added a if condition with is_null() on field which can differentiate > between NULL and empty string. > > diff --git a/sql/handler.cc b/sql/handler.cc > index 40dea349272..71e6a904f23 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6699,8 +6699,13 @@ static int check_duplicate_long_entries_update(TABLE *table, handler *h, uchar * > for (uint j= 0; j < key_parts; j++, keypart++) > { > field= keypart->field; > - /* Compare fields if they are different then check for duplicates*/ > - if(field->cmp_binary_offset(reclength)) > + /* > + Compare fields if they are different then check for duplicates > + cmp_binary_offset cannot differentiate between null and empty string > + So also check for that too > + */ > + if((field->is_null(0) != field->is_null(reclength)) || > + field->cmp_binary_offset(reclength)) > { > if((error= check_duplicate_long_entry_key(table, table->update_handler, > new_rec, i))) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Review of MDEV-4851
by Vicențiu Ciorbaru 07 Oct '20

07 Oct '20

Hi Daniel! The patch looks good, some comments about the test cases. About the commit message, we have --write-binlog not just --binlog. Also, check for typos in the last commit message and also, I would squash the two test cases commits into just one. > diff --git a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > index 1931e2eab2a..b836e889429 100644 > --- a/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > +++ b/mysql-test/suite/rpl/t/rpl_mysql_upgrade.test > @@ -14,8 +14,8 @@ call mtr.add_suppression("table or database name 'mysqltest-1'"); > > connection master; > --disable_warnings > -DROP DATABASE IF EXISTS `#mysql50#mysqltest-1`; > -CREATE DATABASE `#mysql50#mysqltest-1`; > +DROP DATABASE IF EXISTS `mysqltest-1`; > +CREATE DATABASE `mysqltest-1`; Since you are doing cleanup here anyway, let's get rid of the DROP statements a test case should not leave any dirty context behind and if we drop things just to be sure, we are masking potential problems. > --enable_warnings > sync_slave_with_master; > > @@ -34,13 +34,20 @@ if ($before_position == $after_position) > echo Master position is not changed; > } > > -#Some log events of the mysql_upgrade's will cause errors on slave. > +# Some log events of the mysql_upgrade previously caused errors on slave, > +# however with MDEV-4851 this should be ok, so we test it: > connection slave; > -STOP SLAVE SQL_THREAD; > -source include/wait_for_slave_sql_to_stop.inc; > +SET @old_general_log_state = @@global.general_log; > +SET @old_slow_log_state = @@global.slow_query_log; > +SET @old_log_output = @@global.log_output; > +SET GLOBAL general_log = 'ON'; > +SET GLOBAL slow_query_log = 'ON'; > +SET GLOBAL log_output = 'FILE'; > > connection master; > #With '--force' option, mysql_upgrade always executes all sql statements for upgrading. > +ALTER TABLE mysql.slow_log DROP COLUMN thread_id, DROP COLUMN rows_affected; > +DROP DATABASE IF EXISTS `mysqltest-1`; Why IF EXISTS? It *should* be there already. Can you add some syncs between master and slave and then show from the slave connection that the log table table is missing those columns and also that they get applied *after* mysql_upgrade is run? > --exec $MYSQL_UPGRADE --skip-verbose --write-binlog --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade.log 2>&1 > > let $datadir= `select @@datadir`; > @@ -55,8 +62,14 @@ if ($before_position != $after_position) > echo Master position has been changed; > } > > -DROP DATABASE `mysqltest-1`; Notice that the original code didn't use "IF EXISTS". > +sync_slave_with_master; > connection slave; > -DROP DATABASE `#mysql50#mysqltest-1`; > ---let $rpl_only_running_threads= 1 > +SET GLOBAL general_log = 'OFF'; > +SET GLOBAL slow_query_log = 'OFF'; > +truncate mysql.slow_log; > +truncate mysql.general_log; Why do you need to clear the tables? I tested without clearing and it still needs to pass. > +SET GLOBAL general_log = @old_general_log_state; > +SET GLOBAL slow_query_log = @old_slow_log_state; > +SET GLOBAL log_output = @old_log_output; > + > --source include/rpl_end.inc > diff --git a/mysql-test/t/log_tables.test b/mysql-test/t/log_tables.test > index f822ec8d758..0196e62f12f 100644 > --- a/mysql-test/t/log_tables.test > +++ b/mysql-test/t/log_tables.test

1 0

Re: [Maria-developers] Fwd: [Commits] 244f5bd: MDEV-23811: With large number of indexes optimizer chooses an inefficient plan
by Sergey Petrunia 06 Oct '20

06 Oct '20

Hello Igor, On Mon, Oct 05, 2020 at 10:13:17PM -0700, Igor Babaev wrote: > > I've accepted all your changes for my original fix. > This is a new patch for 10.2. > ... > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index e933d2a..9f1fb9a 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -8899,11 +8902,10 @@ tree_or(RANGE_OPT_PARAM *param,SEL_TREE > *tree1,SEL_TREE *tree2) > { > SEL_ARG *key1= tree1->keys[key_no]; > SEL_ARG *key2= tree2->keys[key_no]; > - if (!must_be_ored) > - { > + if (!must_be_ored || no_merges1) > key1->incr_refs(); > + if (!must_be_ored || no_merges2) > key2->incr_refs(); > - } > if ((result->keys[key_no]= key_or(param, key1, key2))) > result->keys_map.set_bit(key_no); > } What is the purpose of this change? As far as I understand, the bug is fixed without it, too? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Test failure in main.flush_logs_not_windows on x32
by Otto Kekäläinen 06 Oct '20

06 Oct '20

Hello! After uploading MariaDB 10.5.5 to Debian I came across that the test main.mysqld--help is failing on arch x32: ****************** main.flush_logs_not_windows w2 [ fail ] Test ended at 2020-09-28 19:25:55 CURRENT_TEST: main.flush_logs_not_windows mysqltest: At line 10: query 'flush logs' succeeded - should have failed with errno 1004... ****************** Full log: https://people.debian.org/~jrtc27/mariadb-10.5_10.5.5-1+jrtc27.1_x32-2020-0… Any tips on what to do about this one?

1 0

[Maria-developers] Test failure in main.mysqld--help on x32
by Otto Kekäläinen 05 Oct '20

05 Oct '20

Hello! After uploading MariaDB 10.5.5 to Debian I came across that the test main.mysqld--help is failing on arch x32: ****************** main.mysqld--help 'unix' w1 [ fail ] Test ended at 2020-09-28 19:24:57 CURRENT_TEST: main.mysqld--help --- /<<PKGBUILDDIR>>/mysql-test/main/mysqld--help.result 2020-08-07 12:57:35.000000000 +0000 +++ /<<PKGBUILDDIR>>/mysql-test/main/mysqld--help.reject 2020-09-28 19:24:57.628353544 +0000 @@ -1797,10 +1797,10 @@ sync-relay-log-info 10000 sysdate-is-now FALSE system-versioning-alter-history ERROR -table-cache 421 +table-cache 2000 table-definition-cache 400 -table-open-cache 421 -table-open-cache-instances 1 +table-open-cache 2000 +table-open-cache-instances 8 tc-heuristic-recover OFF tcp-keepalive-interval 0 tcp-keepalive-probes 0 main.flush_logs_not_windows w2 [ fail ] Test ended at 2020-09-28 19:25:55 CURRENT_TEST: main.flush_logs_not_windows mysqltest: At line 10: query 'flush logs' succeeded - should have failed with errno 1004... ****************** Any tips on what to do about this? On Zulip I was tipped about https://github.com/MariaDB/server/commit/deb365581b96527c5c96b3f5ed1235e105… but it does not apply on 10.5.

2 2

[Maria-developers] Test failure in main.index_intersect on hppa
by Otto Kekäläinen 04 Oct '20

04 Oct '20

Hello! I already asked this on Zulip but did not get anywhere, so re-posting here on the mailing list. The basic test suite run directly after the build fails with the following test on hppa on Debian: https://buildd.debian.org/status/fetch.php?pkg=mariadb-10.5&arch=hppa&ver=1… ****************** main.index_intersect w3 [ fail ] Test ended at 2020-09-27 16:41:38 CURRENT_TEST: main.index_intersect --- /<<PKGBUILDDIR>>/mysql-test/main/index_intersect.result 2020-08-07 12:57:35.000000000 +0000 +++ /<<PKGBUILDDIR>>/mysql-test/main/index_intersect.reject 2020-09-27 16:41:38.035115473 +0000 @@ -715,7 +715,7 @@ SELECT * FROM City WHERE Name BETWEEN 'G' AND 'J' AND Population > 500000 AND Country LIKE 'C%'; id select_type table type possible_keys key key_len ref rows Extra -1 SIMPLE City range Population,Country,Name Name 35 NULL # Using index condition; Using where +1 SIMPLE City index_merge Population,Country,Name Name,Country 35,3 NULL # Using sort_intersect(Name,Country); Using where EXPLAIN SELECT * FROM City WHERE ID BETWEEN 1 AND 500 AND Population > 700000 AND Country LIKE 'C%'; mysqltest: Result l ****************** Any suggestions what this 'range' -> 'index_merge' word change in the result might step from?

1 0

Re: [Maria-developers] 19bdb03e97d: MDEV-21211 LOCK_plugin optimization.
by Sergei Golubchik 02 Oct '20

02 Oct '20

Hi, Alexey! Thanks, this is great! Comments are below, there aren't many. The main concern - you only do RCU when installingplugins, but not when uninstalling. I'm not sure how it's supposed to work. On Oct 02, Alexey Botchkov wrote: > revision-id: 19bdb03e97d (mariadb-10.4.11-337-g19bdb03e97d) > parent(s): 0041dacc1b8 > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2020-08-11 00:01:53 +0400 > message: > > MDEV-21211 LOCK_plugin optimization. > > Use Apc_target calls to 'notify' threads about changes in plugins. > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index bf33148811e..1f38c55b922 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1348,10 +1348,10 @@ bool do_command(THD *thd) > my_net_set_read_timeout(net, thd->variables.net_read_timeout); > > DBUG_ASSERT(packet_length); > - DBUG_ASSERT(!thd->apc_target.is_enabled()); > + thd->apc_target.enable(); > return_value= dispatch_command(command, thd, packet+1, > (uint) (packet_length-1), FALSE, FALSE); > - DBUG_ASSERT(!thd->apc_target.is_enabled()); > + thd->apc_target.disable(); is this because you don't want to post apc requests to threads that wait for the client? > > out: > thd->lex->restore_set_statement_var(); > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 981420f03ae..84438e3ff70 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -217,17 +217,28 @@ static struct > #include "sql_plugin_services.ic" > > /* > - A mutex LOCK_plugin must be acquired before accessing the > - following variables/structures. > - We are always manipulating ref count, so a rwlock here is unneccessary. > + A mutex LOCK_plugin must be acquired when we change the > + new_plugins_state content and when we swap the global_plugins_state > + and the new_plugins_state. > */ > mysql_mutex_t LOCK_plugin; > +struct st_plugins_state > +{ > + DYNAMIC_ARRAY m_array; > + HASH m_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; > +#ifndef DBUG_OOF DBUG_OFF > + uint m_ref_counter; > +#endif > +}; > + > static DYNAMIC_ARRAY plugin_dl_array; > -static DYNAMIC_ARRAY plugin_array; > -static HASH plugin_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; > static MEM_ROOT plugin_mem_root; > static bool reap_needed= false; > volatile int global_plugin_version= 1; > +static st_plugins_state plugins_states[2]; > +st_plugins_state *global_plugins_state= plugins_states; it seems this can be static too, you don't use it outside of sql_plugin.cc (and making it global_plugins_state= &plugins_states[0], new_plugins_state= &plugins_state[1]; would be a bit more explicit) > +static st_plugins_state *new_plugins_state= plugins_states + 1; > + > > static bool initialized= 0; > ulong dlopen_count; > @@ -871,7 +882,8 @@ static void plugin_dl_del(struct st_plugin_dl *plugin_dl) > } > > > -static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, > +static struct st_plugin_int *plugin_find_internal(st_plugins_state *st, shouldn't `st` be const here too? > + const LEX_CSTRING *name, > int type) > { > uint i; > @@ -879,21 +891,19 @@ static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, > if (! initialized) > DBUG_RETURN(0); > > - mysql_mutex_assert_owner(&LOCK_plugin); may be you'd want here something like if (st == new_plugins_state) mysql_mutex_assert_owner(&LOCK_plugin); > - > if (type == MYSQL_ANY_PLUGIN) > { > for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > struct st_plugin_int *plugin= (st_plugin_int *) > - my_hash_search(&plugin_hash[i], (const uchar *)name->str, name->length); > + my_hash_search(&st->m_hash[i], (const uchar *)name->str, name->length); > if (plugin) > DBUG_RETURN(plugin); > } > } > else > DBUG_RETURN((st_plugin_int *) > - my_hash_search(&plugin_hash[type], (const uchar *)name->str, > + my_hash_search(&st->m_hash[type], (const uchar *)name->str, > name->length)); > DBUG_RETURN(0); > } > @@ -1210,13 +1209,6 @@ static void plugin_variables_deinit(struct st_plugin_int *plugin) > > static void plugin_deinitialize(struct st_plugin_int *plugin, bool ref_check) > { > - /* > - we don't want to hold the LOCK_plugin mutex as it may cause > - deinitialization to deadlock if plugins have worker threads > - with plugin locks > - */ > - mysql_mutex_assert_not_owner(&LOCK_plugin); does it mean you *do* keep LOCK_plugin here now? > - > if (plugin->plugin->status_vars) > { > /* > @@ -1280,24 +1273,25 @@ static void plugin_del(struct st_plugin_int *plugin) > DBUG_VOID_RETURN; > } > > + > static void reap_plugins(void) > { > uint count; > struct st_plugin_int *plugin, **reap, **list; > > - mysql_mutex_assert_owner(&LOCK_plugin); > - > if (!reap_needed) > return; > > + mysql_mutex_lock(&LOCK_plugin); > reap_needed= false; > - count= plugin_array.elements; > + count= global_plugins_state->m_array.elements; > reap= (struct st_plugin_int **)my_alloca(sizeof(plugin)*(count+1)); > *(reap++)= NULL; > > for (uint i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > - HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > + HASH *hash= global_plugins_state->m_hash + > + plugin_type_initialization_order[i]; do you need LOCK_plugin when changing plugin states? your comment at the LOCK_plugin declaration doesn't mention it. > for (uint j= 0; j < hash->records; j++) > { > plugin= (struct st_plugin_int *) my_hash_element(hash, j); > @@ -1542,6 +1529,38 @@ static void init_plugin_psi_keys(void) > } > #endif /* HAVE_PSI_INTERFACE */ > > + > +static bool copy_plugins_state(st_plugins_state *dest, st_plugins_state *src) > +{ > + uint i; > + mysql_mutex_assert_owner ? for documentation purposes. > + reset_dynamic(&dest->m_array); > + for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > + my_hash_reset(&dest->m_hash[i]); > + > + for (i= 0; i < src->m_array.elements; i++) > + { > + struct st_plugin_int *ptr; > + ptr= *dynamic_element(&src->m_array, i, struct st_plugin_int **); > + if (insert_dynamic(&dest->m_array, (uchar *) &ptr) || > + (ptr->state != PLUGIN_IS_FREED && > + my_hash_insert(&dest->m_hash[ptr->plugin->type], > + (uchar*) ptr))) > + goto err; > + } > + > + return false; > +err: > + return true; > +} > + > + > +static void set_new_global_plugins_state() > +{ mysql_mutex_assert_owner ? for documentation purposes. > + swap_variables(st_plugins_state *, global_plugins_state, new_plugins_state); > +} > + > + > /* > The logic is that we first load and initialize all compiled in plugins. > From there we load up the dynamic types (assuming we have not been told to > @@ -1815,7 +1834,9 @@ static void plugin_load(MEM_ROOT *tmp_root) > tables.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PLUGIN_NAME, 0, TL_READ); > tables.open_strategy= TABLE_LIST::OPEN_NORMAL; > > + set_new_global_plugins_state(); > result= open_and_lock_tables(new_thd, &tables, FALSE, MYSQL_LOCK_IGNORE_TIMEOUT); > + set_new_global_plugins_state(); why here, why twice? > > table= tables.table; > if (result) > @@ -1851,52 +1872,31 @@ static void plugin_load(MEM_ROOT *tmp_root) > if (!name.length || !dl.length) > continue; > > - /* > - Pre-acquire audit plugins for events that may potentially occur > - during [UN]INSTALL PLUGIN. > - > - When audit event is triggered, audit subsystem acquires interested > - plugins by walking through plugin list. Evidently plugin list > - iterator protects plugin list by acquiring LOCK_plugin, see > - plugin_foreach_with_mask(). > - > - On the other hand plugin_load is acquiring LOCK_plugin > - rather for a long time. > - > - When audit event is triggered during plugin_load plugin > - list iterator acquires the same lock (within the same thread) > - second time. > - > - This hack should be removed when LOCK_plugin is fixed so it > - protects only what it supposed to protect. > - > - See also mysql_install_plugin(), mysql_uninstall_plugin() and > - initialize_audit_plugin() > - */ > - if (mysql_audit_general_enabled()) > - mysql_audit_acquire_plugins(new_thd, event_class_mask); > - \o/ > /* > there're no other threads running yet, so we don't need a mutex. > but plugin_add() before is designed to work in multi-threaded > environment, and it uses mysql_mutex_assert_owner(), so we lock > the mutex here to satisfy the assert > */ > - mysql_mutex_lock(&LOCK_plugin); > plugin_add(tmp_root, false, &name, &dl, MYF(ME_ERROR_LOG)); > free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE)); > - mysql_mutex_unlock(&LOCK_plugin); > } > if (unlikely(error > 0)) > sql_print_error(ER_THD(new_thd, ER_GET_ERRNO), my_errno, > table->file->table_type()); > + set_new_global_plugins_state(); > end_read_record(&read_record_info); > table->mark_table_for_reopen(); > close_mysql_tables(new_thd); > -end: > +err_ret: > new_thd->db= null_clex_str; // Avoid free on thd->db > delete new_thd; > + set_new_global_plugins_state(); > DBUG_VOID_RETURN; > + > +end: > + set_new_global_plugins_state(); > + goto err_ret; eh, really? you swap twice again? > } > > > @@ -2058,13 +2055,8 @@ void plugin_shutdown(void) > plugin_deinitialize(plugins[i], false); > } > > - /* > - It's perfectly safe not to lock LOCK_plugin, as there're no > - concurrent threads anymore. But some functions called from here > - use mysql_mutex_assert_owner(), so we lock the mutex to satisfy it > - */ > - mysql_mutex_lock(&LOCK_plugin); > > + mysql_mutex_lock(&LOCK_plugin); the comment was helpful, I think > /* > We defer checking ref_counts until after all plugins are deinitialized > as some may have worker threads holding on to plugin references. > @@ -2176,6 +2173,44 @@ static bool finalize_install(THD *thd, TABLE *table, const LEX_CSTRING *name, > return 0; > } > > + > +/* > + Request object for ending the new_plugins_state change. > +*/ > + > +class Ping_all_threads_request : public Apc_target::Apc_call > +{ > +public: > + /* Overloaded virtual functions. */ > + void call_in_target_thread(); > +}; > + > + > +void Ping_all_threads_request::call_in_target_thread() > +{ > + /* Do nothing. We just make sure the thread got to this point. */ > +} > + > + > +static my_bool fix_plugins_state_callback(THD *thd, THD *caller_thd) > +{ > + bool timed_out; > + int timeout_sec= 30; > + > + Ping_all_threads_request ping_req; > + > + mysql_mutex_lock(&thd->LOCK_thd_kill); > + if (thd->get_command() == COM_SLEEP) here you also skip threads waiting on the client? > + { > + mysql_mutex_unlock(&thd->LOCK_thd_kill); > + return FALSE; > + } > + thd->apc_target.make_apc_call(caller_thd, &ping_req, > + timeout_sec, &timed_out); > + return FALSE; > +} > + > + > bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name, > const LEX_CSTRING *dl_arg) > { > @@ -2252,12 +2272,19 @@ bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name, > > if (unlikely(error != INSTALL_GOOD)) > { > + mysql_mutex_unlock(&LOCK_plugin); > reap_needed= true; > reap_plugins(); > + goto err; > } > -err: > global_plugin_version++; > + set_new_global_plugins_state(); > + thd->apc_target.disable(); > mysql_mutex_unlock(&LOCK_plugin); > + server_threads.iterate(fix_plugins_state_callback, thd); > + thd->apc_target.enable(); a comment be good here. to explain why you disable apc and why disable/unlock/iterate must happen in this particlar order. > + > +err: > if (argv) > free_defaults(argv); > DBUG_RETURN(error == INSTALL_FAIL_NOT_OK); > @@ -2414,10 +2416,12 @@ bool mysql_uninstall_plugin(THD *thd, const LEX_CSTRING *name, > error|= !MyFlags; > } > } > - reap_plugins(); > - > global_plugin_version++; > + thd->apc_target.disable(); > mysql_mutex_unlock(&LOCK_plugin); > + server_threads.iterate(fix_plugins_state_callback, thd); > + reap_plugins(); > + thd->apc_target.enable(); I don't quite understand this. You don't do copy-update here. I presume because plugins are only marked PLUGIN_IS_FREED but not actually removed from the array or hash? but you modify plugin state under a mutex, and read it without a mutex. how is that supposed to work? > DBUG_RETURN(error); > #ifdef WITH_WSREP > wsrep_error_label: Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6f211b1f01b: MDEV-23518 Syntax error in ond SP results in misleading message on SHOW CREATE PROCEDURE
by Sergei Golubchik 02 Oct '20

02 Oct '20

Hi, Oleksandr! On Oct 01, Oleksandr Byelkin wrote: > revision-id: 6f211b1f01b (mariadb-10.4.11-369-g6f211b1f01b) > parent(s): 571764c04fc > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-08-28 16:05:38 +0200 > message: > > MDEV-23518 Syntax error in ond SP results in misleading message on SHOW CREATE PROCEDURE > > Add info to the error message how to get details about error which happened. > > diff --git a/mysql-test/main/errors.test b/mysql-test/main/errors.test > index f3aedc8e491..a4b95d48e5d 100644 > --- a/mysql-test/main/errors.test > +++ b/mysql-test/main/errors.test > @@ -246,3 +246,39 @@ SELECT UPDATEXML(-73 * -2465717823867977728,@@global.auto_increment_increment,nu > SELECT UPDATEXML(-73 * -2465717823867977728,@@global.long_query_time,null); > > -- echo # End of 10.3 tests > + > +--echo # > +--echo # MDEV-23518: Syntax error in ond SP results in misleading > +--echo # message on SHOW CREATE PROCEDURE > +--echo # > + > +DELIMITER $$; > + > +CREATE PROCEDURE P1 () > +BEGIN NOT ATOMIC > + IF (SELECT 2) THEN > + SELECT 4; > + END IF ; > +END; > +$$ > + > +DELIMITER ;$$ you could've had just CREATE PROCEDURE P1 () SELECT 1; one line, no need to change a delimiter. You overwrite the body below anyway :) > + > +select name,db,body from mysql.proc where name = "P1"; > +update mysql.proc set body_utf8="BEGIN NOT ATOMIC > +IF (SELECT 2) OR foo = 3 THEN > +SELECT 4; > +END IF ; > +END", body="BEGIN NOT ATOMIC > +IF (SELECT 2) OR foo = 3 THEN > +SELECT 4; > +END IF ; > +END"where name = "P1"; > + > +--error ER_SP_PROC_TABLE_CORRUPT > +show create procedure P1; > +show warnings; > + > +drop procedure P1; > + > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index ddc52702e42..582c80e2ad8 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -5714,8 +5714,7 @@ ER_SP_RECURSION_LIMIT > eng "Recursive limit %d (as set by the max_sp_recursion_depth variable) was exceeded for routine %.192s" > ger "Rekursionsgrenze %d (durch Variable max_sp_recursion_depth gegeben) wurde für Routine %.192s überschritten" > ER_SP_PROC_TABLE_CORRUPT > - eng "Failed to load routine %-.192s. The table mysql.proc is missing, corrupt, or contains bad data (internal code %d)" > - ger "Routine %-.192s konnte nicht geladen werden. Die Tabelle mysql.proc fehlt, ist beschädigt, oder enthält fehlerhaften Daten (interner Code: %d)" > + eng "Failed to load routine %-.192s. The table mysql.proc is missing, corrupt, etc. (internal code %d). For more details, run SHOW WARNINGS" Okay, so you just extended the error message. Perhaps "The table mysql.proc is missing, corrupt, etc." is now redundant and shoud go away too? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1