September 2020 - developers - lists.mariadb.org

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

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

[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] New patch on the JSON_TABLE.
by Sergey Petrunia 27 Sep '20

27 Sep '20

Hi Alexey, On Mon, Sep 14, 2020 at 03:38:38PM +0400, Alexey Botchkov wrote: > Hi, Sergei!. > > so the branch name is 'bb-mdef17399-hf'. > It has slightly changed since last patch so please pay attention. > > There i present the patch that resolves issues that you mentioned > in your last comments. > One important is not fixed though. > That is: > CREATE TABLE t1(id INT, f1 JSON); > INSERT INTO t1 VALUES (1, '{\"1\": 1}'), (2, '{\"1\": 2}'), (3, '{\"1\": > 3}'), > (4, '{\"1\": 4}'), (5, '{\"1\": 5}'), (6, '{\"1\": 6}'); > > SELECT * FROM t1 WHERE id IN > (SELECT id FROM t1 as tt2, > JSON_TABLE(f1, "$" COLUMNS (jf FOR ORDINALITY)) AS tbl); > > That SELECT crashes in the optimizer, as the greedy_search() can't find any > satisfying plan. > I got rid of the crash with this line: > @@ -9520,7 +9528,7 @@ best_extension_by_limited_search(JOIN *join, > table_map real_table_bit= s->table->map; > if ((remaining_tables & real_table_bit) && > (allowed_tables & real_table_bit) && > - !(remaining_tables & s->dependent) && > +// !(remaining_tables & s->dependent) && > > But i don't mean it's an acceptable fix. > > It seems to me the problem is in the optimizer, so i'd like to ask your > opinion. > Maybe you just know the answer at once. After some debugging I see that the following happens: 1. Execution reaches Table_function_json_table::setup It computes m_dep_tables= m_json->used_tables(); this is =1 (In the subquery, table tt2 has map=1) 2. Then, subquery is converted into a semi-join. That is, it is merged into its parent subquery. There, we have: table t1: map=1 table tt2: map=2 table JSON_TABLE(...) AS tbl: map=3 3. The subquery was uncorrelated, so SJ-Materialization is a possible option and the join optimizer attempts to construct a join order for the subquery tables. However it can't succeed, as the table "JSON_TABLE(..) AS tbl" is set to be dependent on table with map=1, which is not put into the join order because it is not a part of subquery. As a result, we get assertion failure when the optimizer fails to produce any join orders. The probem here is on step #2. When the subquery's tables are moved to the parent query, they get new values of TABLE::map, and also all their attributes that contain table maps need to be updated. This done here in convert_subq_to_sj() function: ``` /* n. Walk through child's tables and adjust table->map */ List_iterator_fast<TABLE_LIST> si(subq_lex->leaf_tables); while ((tl= si++)) { tl->set_tablenr(table_no); if (tl->is_jtbm()) { tl->jtbm_table_no= table_no; Item *dummy= tl->jtbm_subselect; tl->jtbm_subselect->fix_after_pullout(parent_lex, &dummy, true); ``` In that code one can see: if the subquery has table that has 'is_jtbm()==true', then ... some processing is done to re-compute its attributes after the table has been pulled up into the parent select. The same should be done for TABLE_LIST elements that are JSON_TABLE(..). If you'll introduce a new function, fix_after_pullout() is a good name. It is already used for Item-derived classes. > If not, i'll dig into it. > And you're welcome to observe the new patch. > > Best regards. > HF -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

[Maria-developers] MDEV-21829: Unique class: discussion followup part #2
by Sergey Petrunia 25 Sep '20

25 Sep '20

Hello Igor, Trying to capture the input from the last call. Currently class Unique is using RB-Tree for its in-memory storage. RB-Tree is implemented by struct TREE and tree_XXX() functions (further I'll refer to them both as just "TREE"). It is possible to get Unique to use some other data structure for its in-memory storage. The most obvious candidate is a hash table. (There are currently no plans to do this, though). So, your request was that - An interface is developed for in-memory storage (let's refer to it as Unique::MemStorage) - An implementation of this interface is developed that uses TREE. (We can't change the TREE itself as it is used in multple places). Is this really the intent? Please clarify. My reaction is that the above described change does not seem to have any relevance to MDEV-21829. MDEV-21829 is about supporting variable-length keys, not about changing, or preparing to change Unique::MemStorage. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 1

Re: [Maria-developers] 2564334b4c1: Backported setting of transcation.on=1 in THD::reset_for_reuse()
by Sergei Golubchik 25 Sep '20

25 Sep '20

Hi, Michael! > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7327f270c33..8f6356b15c7 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1640,6 +1640,7 @@ void THD::reset_for_reuse() > abort_on_warning= 0; > free_connection_done= 0; > m_command= COM_CONNECT; > + transaction.on= 1; > #if defined(ENABLED_PROFILING) > profiling.reset(); > #endif This looks risky, to change transaction.on at some random point in time. I understand that it's not a random point and it should be actually safe here. But perhaps you can add an assert to document that it is safe? Like, transaction.on should be already 1 or there should be no active transaction. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] fc07306120f: MDEV-23586 Mariabackup: GTID saved for replication in 10.4.14 is wrong
by Sergei Golubchik 25 Sep '20

25 Sep '20

Hi, Monty! Just a couple of minor comments Note, the below is from the combined diff of this commit and your earlier MDEV-21953 commit, so you can see some lines here that are not part of the fc07306120f. > diff --git a/sql/handler.cc b/sql/handler.cc > index cc7eedd18f2..402db310ed6 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -133,6 +133,23 @@ static plugin_ref ha_default_tmp_plugin(THD *thd) > return ha_default_plugin(thd); > } > > +#if defined(WITH_ARIA_STORAGE_ENGINE) && MYSQL_VERSION_ID < 100500 > +void ha_maria_implicit_commit(THD *thd, bool new_trn) > +{ > + if (ha_maria::has_active_transaction(thd)) > + { > + int error; > + MDL_request mdl_request; > + mdl_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, MDL_EXPLICIT); > + error= thd->mdl_context.acquire_lock(&mdl_request, > + thd->variables.lock_wait_timeout); > + ha_maria::implicit_commit(thd, new_trn); > + if (!error) > + thd->mdl_context.release_lock(mdl_request.ticket); This looks rather fishy. You commit in aria unconditionally? whether you got the lock or not? > + } > +} > +#endif > + > > /** @brief > Return the default storage engine handlerton for thread > @@ -1753,13 +1777,14 @@ int ha_commit_one_phase(THD *thd, bool all) > if ((res= thd->wait_for_prior_commit())) > DBUG_RETURN(res); > } > - res= commit_one_phase_2(thd, all, trans, is_real_trans); > + res= commit_one_phase_2(thd, all, trans, is_real_trans, rw_trans); > DBUG_RETURN(res); > } > > > static int > -commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans) > +commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans, > + bool rw_trans) rw_trans seems to be unused here (also in ha_commit_one_phase()) > { > int error= 0; > uint count= 0; > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 40e606425c5..71b447fb920 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1383,7 +1384,11 @@ void THD::update_all_stats() > void THD::init_for_queries() > { > set_time(); > - ha_enable_transaction(this,TRUE); > + /* > + We don't need to call ha_enable_transaction() as we can't have > + any active transactions that has to be commited > + */ > + transaction.on= TRUE; I just commented on the similar code in another commit of yours. don't just say "can't have any active transactions", add an assert instead, please. > > reset_root_defaults(mem_root, variables.query_alloc_block_size, > variables.query_prealloc_size); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] New patch on the JSON_TABLE.
by Sergey Petrunia 23 Sep '20

23 Sep '20

One more thing: On Mon, Sep 14, 2020 at 03:38:38PM +0400, Alexey Botchkov wrote: > CREATE TABLE t1(id INT, f1 JSON); > INSERT INTO t1 VALUES (1, '{\"1\": 1}'), (2, '{\"1\": 2}'), (3, '{\"1\": > 3}'), > (4, '{\"1\": 4}'), (5, '{\"1\": 5}'), (6, '{\"1\": 6}'); > > SELECT * FROM t1 WHERE id IN > (SELECT id FROM t1 as tt2, > JSON_TABLE(f1, "$" COLUMNS (jf FOR ORDINALITY)) AS tbl); I've noticed that Optimizer Trace is not saved for this query. I get MariaDB [test]> select * from information_schema.optimizer_trace\G *************************** 1. row *************************** QUERY: TRACE: MISSING_BYTES_BEYOND_MAX_MEM_SIZE: 0 INSUFFICIENT_PRIVILEGES: 1 1 row in set (0.001 sec) INSUFFICIENT_PRIVILEGES: 1! This is only supposed to happen when the query uses VIEWs which definition cannot be viewed by the user executing the query. Could you investigate and fix this please? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] On bb-10.6-monty work
by Nikita Malyavin 16 Sep '20

16 Sep '20

Hello, Monty! I've been asked to review your recent work regarding memory footprint optimizations. I went through all the work done, and mostly liked the changes done. So here are only two commits i'd like to highlight > commit bf8bd0573440dc39490eb795b5455735dd63132b > Author: Monty <monty(a)mariadb.org> > Date: Tue Jul 14 18:36:05 2020 +0300 > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - Less and faster code > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's > > diff --git a/sql/item.h b/sql/item.h > index c4fbf8f9c0a..4cdee637415 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -4236,6 +4239,23 @@ class Item_bool :public Item_int > }; > > > +class Item_bool_static :public Item_bool > +{ > +public: > + Item_bool_static(const char *str_arg, longlong i): > + Item_bool(NULL, str_arg, i) {}; > + > + /* Create dummy functions for things that may change the static item */ > + void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO *cs) > + {} > + void fix_char_length(size_t max_char_length_arg) > + {} > + void set_join_tab_idx(uint join_tab_idx_arg) > + { DBUG_ASSERT(0); } > +}; > + > +extern const Item_bool_static Item_false, Item_true; > + > class Item_uint :public Item_int > { > public: Do we really need an additional class for static Item_bool's? I know that we have constructors hidden for Item, so maybe just unhide it in Item_bool, and that's it. Besides, I see fix_char_length is not virtual at all, so reimplementing it has no efect. And why can't we do all that in Item_bool itself? Note that adding new class increases a virtual method table, and it's huge already, for Item hierarchy. We could also add some other frequently used Items, like 0, 1, '', NULL, to a static memory. > commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372 > Author: Monty <monty(a)mariadb.org> > Date: Wed Sep 2 03:13:32 2020 +0300 > > Split item->flags into base_flags and with_flags > > This was done to simplify copying of with_* flags > > Other things: > - Changed Flags to C++ enums, which enables gdb to print > out bit values for the flags. This also enables compiler > errors if one tries to manipulate a non existing bit in > a variable. > - Added set_maybe_null() as a shortcut as setting the > MAYBE_NULL flags was used in a LOT of places. > - Renamed PARAM flag to SP_VAR to ensure it's not confused with persistent > statement parameters. > > diff --git a/sql/item.h b/sql/item.h > index 09618bbb52c..9a794d96982 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -717,28 +717,94 @@ class Item_const > > #define STOP_PTR ((void *) 1) > > -/* Bits used in Item.flags */ > -/* If item may be null */ > -#define ITEM_FLAG_MAYBE_NULL_SHIFT 0 > -#define ITEM_FLAG_MAYBE_NULL (1<<ITEM_FLAG_MAYBE_NULL_SHIFT) > -/* If used in GROUP BY list of a query with ROLLUP */ > -#define ITEM_FLAG_IN_ROLLUP (1<<1) > -/* If Item contains an SP parameter */ > -#define ITEM_FLAG_WITH_PARAM (1<<2) > -/* If item contains a window func */ > -#define ITEM_FLAG_WITH_WINDOW_FUNC (1<<3) > -/* True if any item except Item_sum contains a field. Set during parsing. */ > -#define ITEM_FLAG_WITH_FIELD (1<<4) > -/* If item was fixed with fix_fields */ > -#define ITEM_FLAG_FIXED (1<<5) > -/* Indicates that name of this Item autogenerated or set by user */ > -#define ITEM_FLAG_IS_AUTOGENERATED_NAME (1 << 6) > -/* Indicates that this item is in CYCLE clause of WITH */ > -#define ITEM_FLAG_IS_IN_WITH_CYCLE (1<<7) > -/* True if item contains a sum func */ > -#define ITEM_FLAG_WITH_SUM_FUNC (1<< 8) > -/* True if item containts a sub query */ > -#define ITEM_FLAG_WITH_SUBQUERY (1<< 9) > + > +/* Base flags (including IN) for an item */ > + > + > +typedef uint8 item_flags_t; > + > +enum class item_base_t : item_flags_t > +{ > + NONE= 0, > +#define ITEM_FLAGS_MAYBE_NULL_SHIFT 0 // Must match MAYBE_NULL > + MAYBE_NULL= (1<<0), // May be NULL. > + IN_ROLLUP= (1<<1), // Appears in GROUP BY list > + // of a query with ROLLUP. > + FIXED= (1<<2), // Was fixed with fix_fields(). > + IS_AUTOGENERATED_NAME= (1<<3), // The name if this Item was > + // generated or set by the user. > + IS_IN_WITH_CYCLE= (1<<4) // This item is in CYCLE clause > + // of WITH. > +}; > + > + > +/* Flags that tells us what kind of items the item contains */ > + > +enum class item_with_t : item_flags_t > +{ > + NONE= 0, > + SP_VAR= (1<<0), // If Item contains a stored procedure variable > + WINDOW_FUNC= (1<<1), // If item contains a window func > + FIELD= (1<<2), // If any item except Item_sum contains a field. > + SUM_FUNC= (1<<3), // If item contains a sum func > + SUBQUERY= (1<<4) // If item containts a sub query > +}; > + > + > +/* Make operations in item_base_t and item_with_t work like 'int' */ > +static inline item_base_t operator&(const item_base_t a, const item_base_t b) > +{ > + return (item_base_t) (((item_flags_t) a) & ((item_flags_t) b)); > +} > + > +static inline item_base_t & operator&=(item_base_t &a, item_base_t b) > +{ > + a= (item_base_t) (((item_flags_t) a) & (item_flags_t) b); > + return a; > +} > + > +static inline item_base_t operator|(const item_base_t a, const item_base_t b) > +{ > + return (item_base_t) (((item_flags_t) a) | ((item_flags_t) b)); > +} > + > +static inline item_base_t & operator|=(item_base_t &a, item_base_t b) > +{ > + a= (item_base_t) (((item_flags_t) a) | (item_flags_t) b); > + return a; > +} > + > +static inline item_base_t operator~(const item_base_t a) > +{ > + return (item_base_t) ~(item_flags_t) a; > +} > + > +static inline item_with_t operator&(const item_with_t a, const item_with_t b) > +{ > + return (item_with_t) (((item_flags_t) a) & ((item_flags_t) b)); > +} > + > +static inline item_with_t & operator&=(item_with_t &a, item_with_t b) > +{ > + a= (item_with_t) (((item_flags_t) a) & (item_flags_t) b); > + return a; > +} > + > +static inline item_with_t operator|(const item_with_t a, const item_with_t b) > +{ > + return (item_with_t) (((item_flags_t) a) | ((item_flags_t) b)); > +} > + > +static inline item_with_t & operator|=(item_with_t &a, item_with_t b) > +{ > + a= (item_with_t) (((item_flags_t) a) | (item_flags_t) b); > + return a; > +} > + > +static inline item_with_t operator~(const item_with_t a) > +{ > + return (item_with_t) ~(item_flags_t) a; > +} > > > class Item :public Value_source, > @@ -932,8 +998,8 @@ class Item :public Value_source, > const char *orig_name; > > /* All common bool variables for an Item is stored here */ > - typedef uint16 item_flags_t; > - item_flags_t flags; > + item_base_t base_flags; > + item_with_t with_flags; > > /* Marker is used in some functions to temporary mark an item */ > int16 marker; Nice experimentation! I wanted to suggest using constexprs inside Item, but this is even better, because you cant freely add a bit that not in the enum. Though I think it is too bloaty to specify all the operators each time, so I'd prefer to see duplications metaprogrammed out somehow. There is no sane way in C++ (even in modern one) to specify traits, which extend the class functionality (see Golang's traits), so I'd suggest to use good old preprocessor way, like following: #define T item_base_t #include "setup_bitwise_operators.inc" #define T item_with_t #include "setup_bitwise_operators.inc" and then in setup_bitwise_operators.inc: static inline T operator&(const T a, const T b) { return (T) (((item_flags_t) a) & ((item_flags_t) b)); } And so on. Another variant I've just googled is to make a template operator for all types, but restrict its applicatory type set with std::enable_if<T>: https://www.justsoftwaresolutions.co.uk/cplusplus/using-enum-classes-as-bit… So we would only need to add a structure: template<> struct enable_bitmask_operators<item_base_t>{ static constexpr bool enable=true; }; To work. Thereby, the first approach is: + Good old way + Works fast - not a C++ way, though we work in a C++-only file The second approach is: + modern + C++ way - may compile slow - may produce uncomfortable errors when compiler tries to deduce a function/operator to apply Up to you whic way to use, but I think it should be done, since we have many other flag sets, which are likely to be prettified with the same style. -- Yours truly, Nikita Malyavin

2 3

[Maria-developers] MDEV-21829:Followup: On Unique object and variable-size keys
by Sergey Petrunia 15 Sep '20

15 Sep '20

Hi Varun, I was looking at the code for MDEV-21829, trying to understand whether there are any issues with how Unique object was extended to handle variable-size keys. == TREE object (the RB tree) == TREE (the structure and functions that operate on it) support variable-sized keys, however the keys "are expected to know" their size. tree_insert() has "uint key_size" argument. This way, tree_insert() knows how many bytes to allocate and copy when putting the key into the tree. The rest of the TREE code doesn't seem to care about the size of the keys. When TREE code needs to compare a couple of keys, it will call TREE::compare callack. The callback only receives key pointers as arguments, that is, it is expected to infer key size from the contents of the key. The same happens when when walking the tree. Only "uchar *key" is passed to tree_walk_action callback. The walk function has to figure out they key size on its own. == Unique == Unique class cannot do the same what TREE functions function do. It needs to be aware of the sizes of the values it is storing: - It will need to pass value size to tree_insert() - It will need to know value size to write it into tmp file. - It will need to read it back when merging. How does it know it? Unique::merge() and Unique::walk() with merge_walk() functions do make certain assumptions about the data format being used. == The review points == Is this bad? I think tight coupling between the Unique object and the specifics of the data format it is storing is bad. What is worse is that currently this coupling is implicit, it is not immediately apparent to somebody just looking at the code. Possible ways out: 1. Explicitly declare in Unique class that variable-sized records must use certain data format. 2. Isolate the Unique from the specifics of the data format used. 2A. Let Unique get the size of the key as argument of unique_add() and then keep it together with the key. This creates some RAM/storage overhead. 2B. Pass a read_size() callback function which would get the size from the data? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE
by Sergei Golubchik 13 Sep '20

13 Sep '20

Hi, Michael! This is a good catch! But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's defined as /* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */ #define LONGLONG_BUFFER_SIZE 21 The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses 20*cs->mbmaxlen+1 This +1 is supposed to reserve one byte for '\0'. Which is incorrect, because alloc() does it too, so a better fix would be to remove +1 from String::set_int(). On Sep 08, Michael Widenius wrote: > revision-id: 099596296ce (mariadb-10.5.2-272-g099596296ce) > parent(s): 2de0c74f94c > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Avoid mallocs when using LONGLONG_BUFFER_SIZE > > When using String::alloc() to allocate a string, the String ensures that > there is space for an extra NULL byte in the buffer and if not, reallocates > the string. This is a problem with the String::set_int() that calls > alloc(LONGLONG_BUFFER_SIZE), which forces an extra malloc/free to > happen. > > Fixed by using LONGLONG_BUFFER_SIZE+1 for StringBuffer's. > > diff --git a/sql/field.cc b/sql/field.cc > index ab7254e483d..2be4da8d4a8 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -1085,7 +1085,7 @@ Field_longstr::make_packed_sort_key_part(uchar *buff, > uchar* > Field_longstr::pack_sort_string(uchar *to, const SORT_FIELD_ATTR *sort_field) > { > - StringBuffer<LONGLONG_BUFFER_SIZE> buf; > + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; > val_str(&buf, &buf); > return to + sort_field->pack_sort_string(to, &buf, field_charset()); > } > diff --git a/sql/item.cc b/sql/item.cc > index 4a26cb6c140..0d947749e01 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -3612,7 +3612,7 @@ String *Item_int::val_str(String *str) > > void Item_int::print(String *str, enum_query_type query_type) > { > - StringBuffer<LONGLONG_BUFFER_SIZE> buf; > + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; > // my_charset_bin is good enough for numbers > buf.set_int(value, unsigned_flag, &my_charset_bin); > str->append(buf); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 23814b75241: Report memory leaks from mariadbd if -T or --debug is used
by Sergei Golubchik 11 Sep '20

11 Sep '20

Hi, Michael! ok to push, of course On Sep 08, Michael Widenius wrote: > revision-id: 23814b75241 (mariadb-10.5.2-273-g23814b75241) > parent(s): 099596296ce > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Report memory leaks from mariadbd if -T or --debug is used > > Before memory leaks was only reported if server stopped normally. > This made it harder to find out where the leaks happened when > debugging test cases. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 331684ff728..7468f436a03 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1939,7 +1939,7 @@ static void mysqld_exit(int exit_code) > { > fprintf(stderr, "Warning: Memory not freed: %lld\n", > (longlong) global_status_var.global_memory_used); > - if (exit_code == 0) > + if (exit_code == 0 || opt_endinfo) > SAFEMALLOC_REPORT_MEMORY(0); > } > DBUG_LEAVE; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2de0c74f94c: Optimize Sql_alloc
by Sergei Golubchik 11 Sep '20

11 Sep '20

Hi, Michael! ok to push On Sep 08, Michael Widenius wrote: > revision-id: 2de0c74f94c (mariadb-10.5.2-271-g2de0c74f94c) > parent(s): 21eb8969ce9 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Optimize Sql_alloc > > - Remove 'dummy_for_valgrind' overrun marker as this doesn't help much. > The element also distorts the sizes of objects a bit, which makes it > harder to calculate gain in object sizes when doing size optimizations. > - Avoid one extra call indirection when using thd_get_current_thd(), which > is used by Sql_alloc. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c3ecf0d6243: Optimize size of lex structures
by Sergei Golubchik 09 Sep '20

09 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: c3ecf0d6243 (mariadb-10.5.2-269-gc3ecf0d6243) > parent(s): c79c6f78205 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:33 +0300 > message: > > Optimize size of lex structures > > LEX, st_select_lex, st_select_unit optimized for space: > - Use bit fields for bool variables > - Ensure that all bit fields are initialized (improves > performance for init functions as all bit fields can be > initalized with one memory access) > - Move members around in above structures to remove alignment > gaps > > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index f516219c01a..1f6e50d8b60 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -725,12 +725,15 @@ class st_select_lex_node { > st_select_lex_node *next, **prev, /* neighbor list */ > *master, *slave, /* vertical links */ > *link_next, **link_prev; /* list of whole SELECT_LEX */ > + enum sub_select_type linkage; > > void init_query_common(); > -public: > > +public: > ulonglong options; > - > + uint8 uncacheable; > + bool distinct:1; > + bool no_table_names_allowed:1; /* used for global order by */ I thought you didn't like this approach and preferred flags & DISTINCT instead. Either you forgot to change LEX to flags or you forgot to squash your commits. > /* > result of this query can't be cached, bit field, can be : Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c79c6f78205: Added typedef decimal_digits_t (uint16) for number of decimals
by Sergei Golubchik 09 Sep '20

09 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: c79c6f78205 (mariadb-10.5.2-268-gc79c6f78205) > parent(s): bf8bd057344 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:33 +0300 > message: > > Added typedef decimal_digits_t (uint16) for number of decimals > > For fields and Item's uint8 should be good enough. After > discussions with Alexander Barkov we choose uint16 (for now) > as some format functions may accept +256 digits. > > The reason for this patch was to make the storage of decimal > digits simlar. Before this patch decimals was stored/used as > uint8, int and uint. > > Changed most decimal variables and functions to use the new > typedef. > > diff --git a/include/decimal.h b/include/decimal.h > index cab18f99348..f713409ede1 100644 > --- a/include/decimal.h > +++ b/include/decimal.h > @@ -53,10 +53,11 @@ int decimal2double(const decimal_t *from, double *to); > int double2decimal(double from, decimal_t *to); > int decimal_actual_fraction(const decimal_t *from); > int decimal2bin(const decimal_t *from, uchar *to, int precision, int scale); > -int bin2decimal(const uchar *from, decimal_t *to, int precision, int scale); > +int bin2decimal(const uchar *from, decimal_t *to, int precision, > + decimal_digits_t scale); I don't understand. Is it the type for precision (max number of digits in a decimal number) or for scale (mux number of digits after the dot)? If the former - why did you not change the type of the precision argument? If the latter - why is it called not decimal_scale_t? I personally think the former, using decimal_digits_t for both precision and scale, should be fine. > -int decimal_size(int precision, int scale); > -int decimal_bin_size(int precision, int scale); > +int decimal_size(int precision, decimal_digits_t scale); > +int decimal_bin_size(int precision, decimal_digits_t scale); > int decimal_result_size(decimal_t *from1, decimal_t *from2, char op, > int param); > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 59ac7814aee..34821f76474 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -79,6 +79,7 @@ typedef const struct my_collation_handler_st MY_COLLATION_HANDLER; > typedef const struct unicase_info_st MY_UNICASE_INFO; > typedef const struct uni_ctype_st MY_UNI_CTYPE; > typedef const struct my_uni_idx_st MY_UNI_IDX; > +typedef uint16 decimal_digits_t; A bit strange to see it here and not in decimal.h above. > > typedef struct unicase_info_char_st > { > diff --git a/plugin/type_inet/sql_type_inet.h b/plugin/type_inet/sql_type_inet.h > index 8c42431ccaa..df74b91172a 100644 > --- a/plugin/type_inet/sql_type_inet.h > +++ b/plugin/type_inet/sql_type_inet.h > @@ -402,19 +402,19 @@ class Type_handler_inet6: public Type_handler > bool can_return_time() const override { return false; } > bool convert_to_binary_using_val_native() const override { return true; } > > - uint Item_time_precision(THD *thd, Item *item) const override > + decimal_digits_t Item_time_precision(THD *thd, Item *item) const override here you return the precision using the decimal_digits_t type. So it seems that it should apply to both precision and scale. > { > return 0; > } > - uint Item_datetime_precision(THD *thd, Item *item) const override > +decimal_digits_t Item_datetime_precision(THD *thd, Item *item) const override > { > return 0; > } > - uint Item_decimal_scale(const Item *item) const override > + decimal_digits_t Item_decimal_scale(const Item *item) const override > { > return 0; > } > - uint Item_decimal_precision(const Item *item) const override > + decimal_digits_t Item_decimal_precision(const Item *item) const override > { > /* > This will be needed if we ever allow cast from INET6 to DECIMAL. > diff --git a/sql/field.cc b/sql/field.cc > index f50ddec1c80..a5d00dd6fd5 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -3242,10 +3242,11 @@ Field *Field_decimal::make_new_field(MEM_ROOT *root, TABLE *new_table, > ** Field_new_decimal > ****************************************************************************/ > > -static uint get_decimal_precision(uint len, uint8 dec, bool unsigned_val) > +static decimal_digits_t get_decimal_precision(uint len, decimal_digits_t dec, > + bool unsigned_val) > { > uint precision= my_decimal_length_to_precision(len, dec, unsigned_val); shouldn't my_decimal_length_to_precision() return decimal_digits_t result? > - return MY_MIN(precision, DECIMAL_MAX_PRECISION); > + return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION); > } > > Field_new_decimal::Field_new_decimal(uchar *ptr_arg, > diff --git a/sql/item.h b/sql/item.h > index 4cdee637415..948bb85f253 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1644,14 +1644,14 @@ class Item: public Value_source, > return type_handler()->Item_decimal_precision(this); > } > /* Returns the number of integer part digits only */ > - inline int decimal_int_part() const > - { return my_decimal_int_part(decimal_precision(), decimals); } > + inline decimal_digits_t decimal_int_part() const > + { return (decimal_digits_t) my_decimal_int_part(decimal_precision(), decimals); } again, shouldn't my_decimal_int_part() return decimal_digits_t? > /* > Returns the number of fractional digits only. > NOT_FIXED_DEC is replaced to the maximum possible number > of fractional digits, taking into account the data type. > */ > - uint decimal_scale() const > + decimal_digits_t decimal_scale() const > { > return type_handler()->Item_decimal_scale(this); > } > @@ -4856,7 +4856,7 @@ class Item_temporal_literal :public Item_literal > Item_literal(thd) > { > collation= DTCollation_numeric(); > - decimals= dec_arg; > + decimals= (decimal_digits_t) dec_arg; shouldnt dec_arg be decimal_digits_t? > cached_time= *ltime; > } > > diff --git a/sql/item_func.cc b/sql/item_func.cc > index 8a75d2c9946..805647d2a04 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -2713,7 +2713,7 @@ my_decimal *Item_func_round::decimal_op(my_decimal *decimal_value) > dec= INT_MIN; > > if (!(null_value= (value.is_null() || args[1]->null_value || > - value.round_to(decimal_value, (uint) dec, > + value.round_to(decimal_value, (int) dec, You don't need to cast uint16 to int explicitly > truncate ? TRUNCATE : HALF_UP) > 1))) > return decimal_value; > return 0; > diff --git a/sql/item_func.h b/sql/item_func.h > index 5b4acdce1c6..d5aa1477ff3 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -1369,10 +1369,10 @@ class Item_decimal_typecast :public Item_func > { > my_decimal decimal_value; > public: > - Item_decimal_typecast(THD *thd, Item *a, uint len, uint dec) > + Item_decimal_typecast(THD *thd, Item *a, uint len, decimal_digits_t dec) > :Item_func(thd, a) > { > - decimals= (uint8) dec; > + decimals= (decimal_digits_t) dec; you don't need a cast here > collation= DTCollation_numeric(); > fix_char_length(my_decimal_precision_to_length_no_truncation(len, dec, > unsigned_flag)); > diff --git a/sql/my_decimal.cc b/sql/my_decimal.cc > index edf3e82de40..ac86ff71b64 100644 > --- a/sql/my_decimal.cc > +++ b/sql/my_decimal.cc > @@ -198,7 +198,8 @@ str_set_decimal(uint mask, const my_decimal *val, > E_DEC_OVERFLOW > */ > > -int my_decimal::to_binary(uchar *bin, int prec, int scale, uint mask) const > +int my_decimal::to_binary(uchar *bin, int prec, decimal_digits_t scale, again, shouldn't prec be decimal_digits_t too? > + uint mask) const > { > int err1= E_DEC_OK, err2; > my_decimal rounded; > @@ -329,7 +330,7 @@ my_decimal *date2my_decimal(const MYSQL_TIME *ltime, my_decimal *dec) > } > > > -void my_decimal_trim(ulonglong *precision, uint *scale) > +void my_decimal_trim(ulonglong *precision, decimal_digits_t *scale) and here? > { > if (!(*precision) && !(*scale)) > { > diff --git a/sql/my_decimal.h b/sql/my_decimal.h > index 2db08bf01e3..9f7232f94b4 100644 > --- a/sql/my_decimal.h > +++ b/sql/my_decimal.h > @@ -50,7 +50,7 @@ typedef struct st_mysql_time MYSQL_TIME; > #define DECIMAL_MAX_FIELD_SIZE DECIMAL_MAX_PRECISION > > > -inline uint my_decimal_size(uint precision, uint scale) > +inline uint my_decimal_size(uint precision, decimal_digits_t scale) and here? (and in many places below, that I'll stop commenting on) > { > /* > Always allocate more space to allow library to put decimal point > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 9e0f9b013c0..6569ea1a415 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1197,11 +1198,12 @@ Type_numeric_attributes::aggregate_numeric_attributes_decimal(Item **item, > uint nitems, > bool unsigned_arg) > { > - int max_int_part= find_max_decimal_int_part(item, nitems); > + decimal_digits_t max_int_part= find_max_decimal_int_part(item, nitems); > decimals= find_max_decimals(item, nitems); > - int precision= MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > + decimal_digits_t precision= (decimal_digits_t) why do you need a cast here? > + MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > max_length= my_decimal_precision_to_length_no_truncation(precision, > - (uint8) decimals, > + decimals, > unsigned_flag); > } > > @@ -6910,20 +6912,20 @@ bool Type_handler_string_result:: > > /***************************************************************************/ > > -uint Type_handler::Item_time_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_time_precision(THD *thd, Item *item) const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); note, no cast here (good) > } > > > -uint Type_handler::Item_datetime_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_datetime_precision(THD *thd, Item *item) const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and not here > } > > > -uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > - bool is_time) const > +decimal_digits_t Type_handler_string_result:: > +Item_temporal_precision(THD *thd, Item *item, bool is_time) const > { > StringBuffer<64> buf; > String *tmp; > @@ -6939,34 +6941,34 @@ uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > Datetime(thd, &status, tmp->ptr(), tmp->length(), tmp->charset(), > Datetime::Options(TIME_FUZZY_DATES, TIME_FRAC_TRUNCATE)). > is_valid_datetime())) > - return MY_MIN(status.precision, TIME_SECOND_PART_DIGITS); > - return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(status.precision, TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and here again. why? (and below too, I'll stop commenting on casts now) > } > > diff --git a/sql/sql_type.h b/sql/sql_type.h > index a6d85b5bb47..7b1a4f730db 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -2955,9 +2955,10 @@ class Type_numeric_attributes > class Type_temporal_attributes: public Type_numeric_attributes > { > public: > - Type_temporal_attributes(uint int_part_length, uint dec, bool unsigned_arg) > + Type_temporal_attributes(uint32 int_part_length, decimal_digits_t dec, bool unsigned_arg) > :Type_numeric_attributes(int_part_length + (dec ? 1 : 0), > - MY_MIN(dec, TIME_SECOND_PART_DIGITS), > + MY_MIN(dec, > + (decimal_digits_t) TIME_SECOND_PART_DIGITS), this should not be necessary > unsigned_arg) > { > max_length+= decimals; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] bf8bd057344: MDEV-23001 Precreate static Item_bool() to simplify code
by Sergei Golubchik 08 Sep '20

08 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: bf8bd057344 (mariadb-10.5.2-267-gbf8bd057344) > parent(s): 32a29afea77 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:32 +0300 > message: > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - Less and faster code > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's > > diff --git a/sql/item.cc b/sql/item.cc > index dbf20f31d7a..b24103f2330 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -55,7 +55,8 @@ const char *item_empty_name=""; > const char *item_used_name= "\0"; > > static int save_field_in_field(Field *, bool *, Field *, bool); > - > +const Item_bool_static Item_false("FALSE", 0); > +const Item_bool_static Item_true("TRUE", 1); The maain question, that Bar asked in his email - we have tons of modifiable Item members. How do you know none of them will modified in your Item_bool_static objects? > /** > Compare two Items for List<Item>::add_unique() > @@ -412,7 +413,6 @@ Item::Item(THD *thd): > is_expensive_cache(-1), rsize(0), name(null_clex_str), orig_name(0), > common_flags(IS_AUTO_GENERATED_NAME) > { > - DBUG_ASSERT(thd); > marker= 0; > maybe_null= null_value= with_window_func= with_field= false; > in_rollup= 0; > @@ -421,6 +421,9 @@ Item::Item(THD *thd): > /* Initially this item is not attached to any JOIN_TAB. */ > join_tab_idx= MAX_TABLES; > > + /* thd is NULL in case of static items like Item_true */ > + if (!thd) > + return; I don't like that. thd can be NULL only twice, for statically initialized Item_true and Item_false. For that you removed a useful assert and added an if() that is completely unnecessary for billions of items created runtime. Better to have special contructor just for these two very special items. Or to create a THD temporarily when constructing them. > /* Put item in free list so that we can free all items at end */ > next= thd->free_list; > thd->free_list= this; > @@ -3614,6 +3618,19 @@ void Item_int::print(String *str, enum_query_type query_type) > } > > > +/* > + This function is needed to ensure that Item_bool_static doesn't change > + the value of the member str_value. > +*/ > + > +void Item_bool::print(String *str, enum_query_type query_type) > +{ > + // my_charset_bin is good enough for numbers > + String tmp(value ? (char*) "1" : (char*) "0" , 1, &my_charset_bin); > + str->append(tmp); > +} This is not needed anymore. Item_bool inherits from Item_int, and Item_int::print is void Item_int::print(String *str, enum_query_type query_type) { StringBuffer<LONGLONG_BUFFER_SIZE> buf; // my_charset_bin is good enough for numbers buf.set_int(value, unsigned_flag, &my_charset_bin); str->append(buf); } You removed str_value from Item_int::print in July. > + > + > Item *Item_bool::neg_transformer(THD *thd) > { > value= !value; > diff --git a/sql/item.h b/sql/item.h > index c4fbf8f9c0a..4cdee637415 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1969,7 +1969,7 @@ class Item: public Value_source, > virtual bool limit_index_condition_pushdown_processor(void *arg) { return 0; } > virtual bool exists2in_processor(void *arg) { return 0; } > virtual bool find_selective_predicates_list_processor(void *arg) { return 0; } > - bool cleanup_is_expensive_cache_processor(void *arg) > + virtual bool cleanup_is_expensive_cache_processor(void *arg) You've added a new virtual method? You tried so hard to remove virtual methods that it's a bit strange to see a new virtual method added specifically for these two static items. Why does it matter what cleanup_is_expensive_cache_processor will do with them? You overwrite is_expensive() anyway. > { > is_expensive_cache= (int8)(-1); > return 0; > @@ -3225,6 +3225,8 @@ class Item_literal: public Item_basic_constant > bool check_partition_func_processor(void *int_arg) { return false;} > bool const_item() const { return true; } > bool basic_const_item() const { return true; } > + bool is_expensive() { return false; } > + bool cleanup_is_expensive_cache_processor(void *arg) { return 0; } > }; > > diff --git a/sql/opt_index_cond_pushdown.cc b/sql/opt_index_cond_pushdown.cc > index 360ae028f36..d66f2f2e14e 100644 > --- a/sql/opt_index_cond_pushdown.cc > +++ b/sql/opt_index_cond_pushdown.cc > @@ -185,8 +185,8 @@ bool uses_index_fields_only(Item *item, TABLE *tbl, uint keyno, > static Item *make_cond_for_index(THD *thd, Item *cond, TABLE *table, uint keyno, > bool other_tbls_ok) > { > - if (!cond) > - return NULL; > + if (!cond || cond->basic_const_item()) > + return cond; The effect of this change is that you don't set cond->marker= ICP_COND_USES_INDEX_ONLY for basic_const_item's anymore. This looked suspicious at first. But then I noticed (and Sergey Petrunia confirmed) that ICP_COND_USES_INDEX_ONLY is not used at all. Since 5.3. So here I'd suggest to revert this your change and completely remove any ICP_COND_USES_INDEX_ONLY manipulations together with the ICP_COND_USES_INDEX_ONLY definition. > if (cond->type() == Item::COND_ITEM) > { > uint n_marked= 0; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 144b86e8fc9..1140f32c9c9 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2313,6 +2313,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd, > thd->update_server_status(); > thd->protocol->end_statement(); > query_cache_end_of_result(thd); > + /* Check that the query didn't change const items */ what does that mean? Is it a TODO comment? Or it documents what happens below? > } > if (drop_more_results) > thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 22bf2eb37b5..fdd609a96ce 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -22560,6 +22560,8 @@ make_cond_for_table_from_pred(THD *thd, Item *root_cond, Item *cond, > return new_cond; > } > } > + else if (cond->basic_const_item()) > + return cond; this is rather fragile, isn't it? You skip item->set_join_tab_idx(join_tab_idx_arg), quite understandably. And currently, as far as I can see, item->get_join_tab_idx() is always used under if (!item->const_item()), so what you did is safe. But can it change in the future? Or is join_tab_idx something that conceptually doesn't make sense for const items? > > if (is_top_and_level && used_table == rand_table_bit && > (cond->used_tables() & ~OUTER_REF_TABLE_BIT) != rand_table_bit) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] 5d9386892b5: MDEV-4633: multi_source.simple test fails sporadically
by sujatha 08 Sep '20

08 Sep '20

revision-id: 5d9386892b58477c4e28de3e40d930b44f5b42d4 (mariadb-10.1.43-274-g5d9386892b5) parent(s): 3ee2422624ffb3d7ffefff8db7ef9398816299bc author: Sujatha committer: Sujatha timestamp: 2020-09-08 13:10:36 +0530 message: MDEV-4633: multi_source.simple test fails sporadically Analysis: ======== Writes to 'rli->log_space_total' needs to be synchronized, otherwise both SQL_THREAD and IO_THREAD can try to modify the variable simultaneously resulting in incorrect rli->log_space_total. In the current test scenario SQL_THREAD is trying to decrement 'rli->log_space_total' in 'purge_first_log' and IO_THREAD is trying to increment the 'rli->log_space_total' in 'queue_event' simultaneously. Hence test occasionally fails with result mismatch. Fix: === Convert 'rli->log_space_total' variable to atomic type. --- sql/log.cc | 4 +++- sql/log.h | 4 +++- sql/rpl_rli.cc | 14 ++++++++------ sql/slave.cc | 19 +++++++++++++++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/sql/log.cc b/sql/log.cc index 8049b94bab1..90d5c46a15f 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -4400,7 +4400,9 @@ int MYSQL_BIN_LOG::purge_first_log(Relay_log_info* rli, bool included) 0, 0, &log_space_reclaimed); mysql_mutex_lock(&rli->log_space_lock); - rli->log_space_total-= log_space_reclaimed; + my_atomic_add64_explicit((volatile int64*)(&rli->log_space_total), + (-(ulonglong)log_space_reclaimed), + MY_MEMORY_ORDER_RELAXED); mysql_cond_broadcast(&rli->log_space_cond); mysql_mutex_unlock(&rli->log_space_lock); diff --git a/sql/log.h b/sql/log.h index 277e5c6f69c..9e3807e9939 100644 --- a/sql/log.h +++ b/sql/log.h @@ -712,7 +712,9 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG char buf1[22],buf2[22]; #endif DBUG_ENTER("harvest_bytes_written"); - (*counter)+=bytes_written; + + my_atomic_add64_explicit((volatile int64*)(counter), bytes_written, + MY_MEMORY_ORDER_RELAXED); DBUG_PRINT("info",("counter: %s bytes_written: %s", llstr(*counter,buf1), llstr(bytes_written,buf2))); bytes_written=0; diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc index c196a65809a..8815af4ea4a 100644 --- a/sql/rpl_rli.cc +++ b/sql/rpl_rli.cc @@ -140,8 +140,8 @@ int init_relay_log_info(Relay_log_info* rli, rli->slave_skip_counter=0; rli->abort_pos_wait=0; rli->log_space_limit= relay_log_space_limit; - rli->log_space_total= 0; - + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); char pattern[FN_REFLEN]; (void) my_realpath(pattern, slave_load_tmpdir, 0); if (fn_format(pattern, PREFIX_SQL_LOAD, pattern, "", @@ -467,7 +467,8 @@ static inline int add_relay_log(Relay_log_info* rli,LOG_INFO* linfo) linfo->log_file_name); DBUG_RETURN(1); } - rli->log_space_total += s.st_size; + my_atomic_add64_explicit((volatile int64*)(&rli->log_space_total), + s.st_size, MY_MEMORY_ORDER_RELAXED); DBUG_PRINT("info",("log_space_total: %llu", rli->log_space_total)); DBUG_RETURN(0); } @@ -477,7 +478,8 @@ static int count_relay_log_space(Relay_log_info* rli) { LOG_INFO linfo; DBUG_ENTER("count_relay_log_space"); - rli->log_space_total= 0; + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); if (rli->relay_log.find_log_pos(&linfo, NullS, 1)) { sql_print_error("Could not find first log while counting relay log space"); @@ -1202,8 +1204,8 @@ int purge_relay_logs(Relay_log_info* rli, THD *thd, bool just_reset, strmake_buf(rli->group_relay_log_name, rli->relay_log.get_log_fname()); strmake_buf(rli->event_relay_log_name, rli->relay_log.get_log_fname()); rli->group_relay_log_pos= rli->event_relay_log_pos= BIN_LOG_HEADER_SIZE; - rli->log_space_total= 0; - + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); if (count_relay_log_space(rli)) { *errmsg= "Error counting relay log space"; diff --git a/sql/slave.cc b/sql/slave.cc index 06f2b0d955a..9f4b0f53fb4 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -2371,7 +2371,10 @@ static bool wait_for_relay_log_space(Relay_log_info* rli) &rli->log_space_lock, &stage_waiting_for_relay_log_space, &old_stage); - while (rli->log_space_limit < rli->log_space_total && + while (rli->log_space_limit < + (ulonglong)my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED) && !(slave_killed=io_slave_killed(mi)) && !rli->ignore_log_space_limit) mysql_cond_wait(&rli->log_space_cond, &rli->log_space_lock); @@ -2912,7 +2915,10 @@ static bool send_show_master_info_data(THD *thd, Master_info *mi, bool full, protocol->store(mi->rli.last_error().message, &my_charset_bin); protocol->store((uint32) mi->rli.slave_skip_counter); protocol->store((ulonglong) mi->rli.group_master_log_pos); - protocol->store((ulonglong) mi->rli.log_space_total); + protocol->store((ulonglong) + my_atomic_load64_explicit((volatile int64*) + (&mi->rli.log_space_total), + MY_MEMORY_ORDER_RELAXED)); protocol->store( mi->rli.until_condition==Relay_log_info::UNTIL_NONE ? "None": @@ -4428,7 +4434,9 @@ Stopping slave I/O thread due to out-of-memory error from master"); #endif if (rli->log_space_limit && rli->log_space_limit < - rli->log_space_total && + (ulonglong) my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED) && !rli->ignore_log_space_limit) if (wait_for_relay_log_space(rli)) { @@ -7031,7 +7039,10 @@ static Log_event* next_event(rpl_group_info *rgi, ulonglong *event_size) is are able to rotate and purge sometime soon. */ if (rli->log_space_limit && - rli->log_space_limit < rli->log_space_total) + rli->log_space_limit < + (ulonglong) my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED)) { /* force rotation if not in an unfinished group */ rli->sql_force_rotate_relay= !rli->is_in_group();

1 0