- developers - lists.mariadb.org

[Maria-developers] Review input for MDEV-21092, 21095, 29997
by Sergei Petrunia 09 Dec '22

09 Dec '22

Hi Rex, Please find below review input for the collection of patches for these MDEVs. First, please try to have each piece of functionality in its own commit. You can use "git rebase -i" to make your last commits be one commit, as well as re-order them. Then, you can do a carefully considered "git push --force" to overwrite the contents of your branch with the new set of commits. in prune_partitions(): + String parts; + String_list parts_list; + + make_used_partitions_str(thd->mem_root, prune_param.part_info, &parts, parts_list ); + trace_partition_pruning.add("partition_prune", parts.ptr()); This creates a list of used partitions regardless whether we need it or not (and typically trace=off, so we don't'). Please put this code into if (unlikely(thd->trace_started())) { ... } Consider this testcase: create table t2 (a int, b int) partition by hash(a) partitions 10; create table t3 (a int, b int) partition by hash(a) partitions 10; set optimizer_trace=1; explain format=json select * from t2,t3 where t2.a in (2,3,4) and t3.a in (4,5); select * from information_schema.optimizer_trace\G This gives: ... { "partition_prune": "p2,p3,p4" }, { "partition_prune": "p4,p5" }, ... which makes it impossible to see which table is pruning is done for. Let's make the trace be: { "prune_partitions" : { "table": "t2", "used_partitions": "p2,p3,p4" } } { "prune_partitions" : { "table": "t3", "used_partitions": "p4,p5" } } . in item_subselect.cc: + { + OPT_TRACE_TRANSFORM(thd, trace_wrapper, trace_transform, + in_subs->get_select_lex()->select_number, + "EXISTS (SELECT)", "IN (SELECT)"); + trace_transform.add( "upper_not", ( upper_not?true:false ) ); + } coding style: everything inside { } should be indented one level (=2 spaces) right. in sql_select.cc: "attached_conditions_summary": [ { "table": "t1", "attached": "t1.c < 30", "index": "t1.a < 10 and t1.b < 20" } Please change "index" to "index_condition". and "attached" to "attached_condition". Let's not print index_condition if it is null. (Yes, this is somewhat inconsistent with printing "attached":null) in item.cc: + #include "my_json_writer.h" + const char *dbug_print_optrace( ) dbug_print_optrace() is fine. I think it needs a comment like this: /* Debug printout: print the trace trace we're currently writing if any, and return it. */ (Feel free to fix poor English if necessary) Please fix the coding style in the function body as discussed on Slack. Please move #include to the other #includes at the top of the file. Can add a comment, "// For dbug_print_optrace" Would dbug_print_opt_trace() be a better name? + const char *dbug_print_items(Item *item) + { + *big_buf= 0; + + dbug_add_print_items( item ); + return big_buf; + } This function doesn't seem to be needed? For the rest of the functions: as discussed on Slack, we can add them in a separate commit if we decide that they're useful. Could you provide a very short text describing: The entry point (which function is to be called from gdb?). The use case. Is it "to print List<Item>" ? I don't recall seeing this data structure in the server... Is there a lot of need to print Item_cond and then print its children? This looks self-repetitive... I'd like to see a log of an example gdb session...

2 2

[Maria-developers] How to get started with contribution.
by Pinak faldu 29 Nov '22

29 Nov '22

Respected sir/madam, I am Pinak Faldu, a computer science undergraduate student. Currently, I am in my first year of a master's degree at Sinhgad College. I am new to open source contributions, but I am very familiar with ReactJS, MongoDB, MySql, Javascript, and C++. I would like to contribute to your organization, but I don't know where to begin. Could you please tell me how to get started? Hoping to hear from you soon. Regards Pinak

2 1

Re: [Maria-developers] 52f489ebccb: MDEV-29069 follow-up: support partially suitable keys
by Sergei Golubchik 02 Nov '22

02 Nov '22

Hi, Nikita, This review applies to the combined diff e2f8dff^..52f489e On Oct 04, Nikita Malyavin wrote: > diff --git a/sql/log_event.h b/sql/log_event.h > index 91a9eeee10f..8452e665c98 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -5275,8 +5278,11 @@ class Rows_log_event : public Log_event > uchar *m_key; /* Buffer to keep key value during searches */ > KEY *m_key_info; /* Pointer to KEY info for m_key_nr */ > uint m_key_nr; /* Key number */ > + uint m_key_parts_suit; /* A number of key_parts suited to lookup */ that's very confusing name. Took me a while to understand what you meant. Better call it m_usable_key_parts > bool master_had_triggers; /* set after tables opening */ > > + uint key_parts_suit_event(const KEY *key) const; and this could be "get_usable_key_parts()" > + > int find_key(); // Find a best key to use in find_row() > int find_row(rpl_group_info *); > int write_row(rpl_group_info *, const bool); > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 422d496d5c3..25705d13641 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -6062,7 +6065,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > */ > sql_mode_t saved_sql_mode= thd->variables.sql_mode; > if (!is_auto_inc_in_extra_columns()) > - thd->variables.sql_mode= (rpl_data.is_online_alter() ? saved_sql_mode :0) > + thd->variables.sql_mode= (m_online_alter ? saved_sql_mode :0) wouldn't it be more correct to use saved_sql_mode also in normal replication? > | MODE_NO_AUTO_VALUE_ON_ZERO; > > // row processing loop > @@ -7949,14 +7952,35 @@ uint8 Write_rows_log_event::get_trg_event_map() > **************************************************************************/ > > #if defined(HAVE_REPLICATION) > -/* > - Compares table->record[0] and table->record[1] > +/** > + @brief Compares table->record[0] and table->record[1] > + > + @param masrter_columns a number of columns on the source replica, > + 0 if ONLINE ALTER TABLE > > - Returns TRUE if different. > + @returns true if different. > */ > -static bool record_compare(TABLE *table) > +static bool record_compare(TABLE *table, uint master_columns) > { > - bool result= FALSE; > + bool result= false; > + > + /* > + Determine if the optimized check is possible (and we can > + goto record_compare_exit). > + We should ensure that all extra columns (i.e. fieldnr > master_columns) > + have explicit values. If not, we will exclude them from comparison, > + as they can contain non-deterministic values. > + > + master_columns == 0 case is optimization for ONLINE ALTER to check > + all columns fast. > + */ > + bool all_values_set= master_columns == 0 > + && bitmap_is_set_all(&table->has_value_set); > + for (uint i = master_columns; all_values_set && i < table->s->fields; i++) > + { > + all_values_set= bitmap_is_set(&table->has_value_set, i); > + } wait, this doesn't make any sense. You only do the loop if all_values_set is true. But it is true only if bitmap_is_set_all(&table->has_value_set). There is no point in doing bitmap_is_set() for individual bits, when you already know that all bits are set :) shouldn't it simply be bool all_values_set= bitmap_is_set_all(&table->has_value_set); ? but see below, perhaps you don't need all_values_set at all > + > /** > Compare full record only if: > - there are no blob fields (otherwise we would also need > @@ -7969,40 +7993,43 @@ static bool record_compare(TABLE *table) > */ > if ((table->s->blob_fields + > table->s->varchar_fields + > - table->s->null_fields) == 0) > + table->s->null_fields) == 0 > + && all_values_set) > { > result= cmp_record(table,record[1]); > goto record_compare_exit; > } > > /* Compare null bits */ > - if (memcmp(table->null_flags, > - table->null_flags+table->s->rec_buff_length, > - table->s->null_bytes)) > + if (all_values_set && memcmp(table->null_flags, > + table->null_flags+table->s->rec_buff_length, > + table->s->null_bytes)) > { > - result= TRUE; // Diff in NULL value > + result= true; // Diff in NULL value > goto record_compare_exit; > } > > /* Compare fields */ > for (Field **ptr=table->field ; *ptr ; ptr++) > { > + Field *f= *ptr; > if (table->versioned() && (*ptr)->vers_sys_field()) > { > continue; > } > - /** > - We only compare field contents that are not null. > - NULL fields (i.e., their null bits) were compared > - earlier. > - */ > - if (!(*(ptr))->is_null()) > + /* > + We only compare fields that exist on the source (or in ONLINE > + ALTER case, that were in the original table). For reference, > + replica tables can also contain extra fields. > + */ > + if (f->field_index > master_columns && !f->has_explicit_value()) > + continue; same question as in a previous review. Do you mean that when f->field_index < master_columns, it's ok to compare values below even when !f->has_explicit_value() ? > + > + if (f->is_null() != f->is_null(table->s->rec_buff_length) > + || f->cmp_binary_offset(table->s->rec_buff_length)) may be it's better to create a fast copy of this function, like if (bitmap_is_set_all(&table->has_value_set)) { ... old record_compare() code starting from if ((table->s->blob_fields + table->s->varchar_fields + table->s->null_fields) == 0) } else { ... your code, only the loop over fields. } in this case you won't even need all_values_set or master_columns > { > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length)) > - { > - result= TRUE; > - goto record_compare_exit; > - } > + result= true; > + goto record_compare_exit; > } > } > > @@ -8010,6 +8037,32 @@ static bool record_compare(TABLE *table) > return result; > } > > +/** > + Newly added fields with non-deterministic defaults (i.e. DEFAULT(RANDOM()), > + CURRENT_TIMESTAMP, AUTO_INCREMENT) should be excluded from key search. > + Basically we exclude all the default-filled fields based on > + has_explicit_value bitmap. > + */ > +uint Rows_log_event::key_parts_suit_event(const KEY *key) const > +{ > + uint master_columns= m_online_alter ? 0 : m_cols.n_bits; get_master_cols() > + uint p= 0; > + for (;p < key->ext_key_parts; p++) > + { > + Field *f= key->key_part[p].field; > + uint non_deterministic_default= f->default_value && > + f->default_value->flags | VCOL_NOT_STRICTLY_DETERMINISTIC; > + > + int next_number_field= f->table->next_number_field ? > + f->table->next_number_field->field_index : -1; eh? 1. why it's *in the loop* ? 2. why not simply || f == f->table->next_number_field or, to be extra safe || f->table->field[f->field_index] == f->table->next_number_field > + > + if (f->field_index >= master_columns I don't think this condition ^^^ is needed. Same as elsewhere, if f->field_index < master_columns but !bitmap_is_set, does it mean the keyseg is ok? I don't think so. > + && !bitmap_is_set(&m_table->has_value_set, f->field_index) > + && (non_deterministic_default || next_number_field == f->field_index)) > + break; > + } > + return p; > +} > > /** > Find the best key to use when locating the row in @c find_row(). > @@ -8024,13 +8077,26 @@ static bool record_compare(TABLE *table) > */ > int Rows_log_event::find_key() > { > - uint i, best_key_nr, last_part; > + uint i, best_key_nr; > KEY *key, *UNINIT_VAR(best_key); > ulong UNINIT_VAR(best_rec_per_key), tmp; > DBUG_ENTER("Rows_log_event::find_key"); > DBUG_ASSERT(m_table); > > + #ifdef DBUG_TRACE #ifndef DBUG_OFF > + auto _ = make_scope_exit([this]() { > + DBUG_EXECUTE_IF("rpl_report_chosen_key", > + push_warning_printf(m_table->in_use, > + Sql_condition::WARN_LEVEL_NOTE, > + ER_UNKNOWN_ERROR, "Key chosen: %d", > + m_key_nr == MAX_KEY ? > + -1 : m_key_nr);); > + }); > + #endif > + > + m_key_nr= MAX_KEY; > best_key_nr= MAX_KEY; > + uint parts_suit= 0; > > /* > Keys are sorted so that any primary key is first, followed by unique keys, > @@ -8041,33 +8107,38 @@ int Rows_log_event::find_key() > { > if (!m_table->s->keys_in_use.is_set(i)) > continue; > + parts_suit= key_parts_suit_event(key); > + if (!parts_suit) > + continue; > /* > We cannot use a unique key with NULL-able columns to uniquely identify > a row (but we can still select it for range scan below if nothing better > is available). > */ > - if ((key->flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME) > + if ((key->flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME && > + parts_suit == key->user_defined_key_parts) this doesn't make use of ext key parts. Should be something like if (parts_suit == key->ext_key_parts || (parts_suit >= key->user_defined_key_parts && (key->flags & ....))) > { > best_key_nr= i; > best_key= key; > + m_key_parts_suit= parts_suit; > break; > } > /* > We can only use a non-unique key if it allows range scans (ie. skip > FULLTEXT indexes and such). > */ > - last_part= key->user_defined_key_parts - 1; > DBUG_PRINT("info", ("Index %s rec_per_key[%u]= %lu", > - key->name.str, last_part, key->rec_per_key[last_part])); > - if (!(m_table->file->index_flags(i, last_part, 1) & HA_READ_NEXT)) > + key->name.str, parts_suit, key->rec_per_key[parts_suit])); > + if (!(m_table->file->index_flags(i, parts_suit, 1) & HA_READ_NEXT)) > continue; > > - tmp= key->rec_per_key[last_part]; > + tmp= key->rec_per_key[parts_suit - 1]; > if (best_key_nr == MAX_KEY || (tmp > 0 && tmp < best_rec_per_key)) > { > best_key_nr= i; > best_key= key; > best_rec_per_key= tmp; > + m_key_parts_suit= parts_suit; > } > } > > @@ -8494,12 +8568,20 @@ Delete_rows_log_event::do_before_row_operations(const Slave_reporting_capability > if (get_flags(STMT_END_F)) > status_var_increment(thd->status_var.com_stat[SQLCOM_DELETE]); > > + const uchar *curr_row_end= m_curr_row_end; > + unpack_row(rgi, m_table, m_width, m_curr_row, &m_cols, > + &curr_row_end, &m_master_reclength, m_rows_end); > + > + KEY *pk_info= m_table->key_info + m_table->s->primary_key; > if ((m_table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) && > - m_table->s->primary_key < MAX_KEY) > + m_table->s->primary_key < MAX_KEY && > + key_parts_suit_event(pk_info) == pk_info->ext_key_parts) > { > /* > We don't need to allocate any memory for m_key since it is not used. > */ > + m_key_nr= m_table->s->primary_key; > + m_key_parts_suit= pk_info->ext_key_parts; > return 0; > } > if (do_invoke_trigger()) already commented in the previous review that if "We don't need to allocate any memory" than you `return 0` early wthout doing do_invoke_trigger(). Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 11

Re: [Maria-developers] b2800c06b22: MDEV-28545 MyISAM reorganize partition corrupt older table format
by Sergei Golubchik 25 Oct '22

25 Oct '22

Hi, Alexander, Ok to push. One minor comment below: On Oct 25, Alexander Barkov wrote: > revision-id: b2800c06b22 (mariadb-10.4.26-45-gb2800c06b22) > parent(s): 34163154077 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-25 11:53:39 +0400 > message: > > MDEV-28545 MyISAM reorganize partition corrupt older table format > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 6aae927800e..a621825c395 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10068,6 +10068,18 @@ do_continue:; > > set_table_default_charset(thd, create_info, alter_ctx.db); > > + > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (!fast_alter_partition) > +#endif To avoid ifdef's you can use if (IF_PARTITIONING(!fast_alter_partition, 1)) > + { > + /* > + We cannot alter partitions and change column data types at the same > + time. > + */ > + Create_field::upgrade_data_types(alter_info->create_list); > + } > + > if (create_info->check_fields(thd, alter_info, > table_list->table_name, table_list->db) || > create_info->fix_period_fields(thd, alter_info)) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 152295d1cb1: rpl: extra DEFAULTs should not be UPDATEd
by Sergei Golubchik 24 Oct '22

24 Oct '22

Hi, Nikita, As we've discussed on slack, this is wrong. The (serialized) behavior should be as if all DML happened before ALTER. Because if some DML would've happened after ALTER, it would be able to see the new table definition and refer to new columns. On Oct 24, Nikita Malyavin wrote: > revision-id: 152295d1cb1 (mariadb-10.6.1-525-g152295d1cb1) > parent(s): 90274a7428e > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-10-14 23:17:59 +0300 > message: > > rpl: extra DEFAULTs should not be UPDATEd > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 25705d13641..a72005d70ab 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -8832,11 +8832,17 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi) > message= thd->wsrep_info; > #endif /* WSREP_PROC_INFO */ > > - /* this also updates m_curr_row_end */ > thd_proc_info(thd, message); > - if (unlikely((error= unpack_current_row(rgi, &m_cols_ai)))) > + Field **default_field= m_table->default_field; > + m_table->default_field= NULL; > + /* this also updates m_curr_row_end */ > + error= unpack_current_row(rgi, &m_cols_ai); > + m_table->default_field= default_field; > + > + if (unlikely(error)) > goto err; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-29624 MDEV-29655 Fix ASAN errors on pushdown of derived table
by Sergey Petrunia 24 Oct '22

24 Oct '22

Hi Oleg, > commit 4cad1e16a85114815075e5706d6a8faf5e8fc2e5 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Wed Oct 19 13:26:19 2022 +0400 > > MDEV-29624 MDEV-29655 Fix ASAN errors on pushdown of derived table > > Deallocation of TABLE_LIST::dt_handler and TABLE_LIST::pushdown_derived > was performed in multiple places if code. This not only made the code > more difficult to maintain but also led to memory leaks and > ASAN heap-use-after-free errors. > This commit puts deallocation of TABLE_LIST::dt_handler and > TABLE_LIST::pushdown_derived to the single point - JOIN::cleanup() I think the patch is ok to push except for this part: > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index a91b4571b21..4b8956132e8 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -14029,11 +14030,11 @@ void JOIN::cleanup(bool full) > delete pushdown_query; > pushdown_query= 0; > > - if (!join_tab) > + List_iterator<TABLE_LIST> li(*join_list); > + TABLE_LIST *table_ref; > + while ((table_ref= li++)) > { > - List_iterator<TABLE_LIST> li(*join_list); > - TABLE_LIST *table_ref; > - while ((table_ref= li++)) > + if (!join_tab) > { > if (table_ref->table && > table_ref->jtbm_subselect && > @@ -14043,6 +14044,14 @@ void JOIN::cleanup(bool full) > table_ref->table= NULL; > } > } > + if (table_ref->pushdown_derived) > + { > + delete table_ref->pushdown_derived; > + table_ref->pushdown_derived= NULL; > + } > + delete table_ref->dt_handler; > + table_ref->dt_handler= NULL; > + > } > } > /* Restore ref array to original state */ What do you think of moving the cleanup to JOIN_TAB::cleanup()? I did the following and it seems to work: diff --git a/sql/sql_select.cc b/sql/sql_select.cc index a91b4571b21..11cf3e13365 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -13529,6 +13530,16 @@ void JOIN_TAB::cleanup() select= 0; delete quick; quick= 0; + if (table && table->pos_in_table_list) + { + TABLE_LIST *tl= table->pos_in_table_list; + + delete tl->pushdown_derived; + tl->pushdown_derived= NULL; + + delete tl->dt_handler; + tl->dt_handler= NULL; + } if (rowid_filter) { delete rowid_filter; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 39e20ca7e28: MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations
by Sergei Golubchik 22 Oct '22

22 Oct '22

Hi, Oleksandr, Looks good. Just one question below: On Oct 19, Oleksandr Byelkin wrote: > revision-id: 39e20ca7e28 (mariadb-10.3.36-107-g39e20ca7e28) > parent(s): d6707ab11f6 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-10-18 16:38:09 +0200 > message: > > MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations > > Read the version of the view share when we read definition to prevent > simultaniouse access to a view table SHARE (and so its MEM_ROOT) > from different threads. > > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 68a8410f559..1e4a740f871 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1175,19 +1175,24 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > bool mariadb_view_version_get(TABLE_SHARE *share) > { > DBUG_ASSERT(share->is_view); > + DBUG_ASSERT(share->tabledef_version.length == 0); > > if (!(share->tabledef_version.str= > (uchar*) alloc_root(&share->mem_root, > MICROSECOND_TIMESTAMP_BUFFER_SIZE))) > return TRUE; > - share->tabledef_version.length= 0; // safety if the drfinition file is brocken > > DBUG_ASSERT(share->view_def != NULL); > if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, > view_timestamp_parameters, 1, > &file_parser_dummy_hook)) > + { > + // safety if the definition file is brocken > + share->tabledef_version.length= 0; when can view_def->parse() fail? _Only_ if the definition is invalid? Can it succeed for one TABLE and fail for another, both of the same TABLE_SHARE? > return TRUE; > + } > DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > + > return FALSE; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 99dcf061c8c: MDEV-29481 mariadb-upgrade prints confusing statement
by Sergei Golubchik 21 Oct '22

21 Oct '22

Hi, Alexander, ok to push! On Oct 21, Alexander Barkov wrote: > revision-id: 99dcf061c8c (mariadb-10.4.26-55-g99dcf061c8c) > parent(s): 1be451ca797 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-21 14:31:02 +0400 > message: > > MDEV-29481 mariadb-upgrade prints confusing statement > > This is a new version of the patch instead of the reverted: > > MDEV-28727 ALTER TABLE ALGORITHM=NOCOPY does not work after upgrade > > Ignore the difference in key packing flags HA_BINARY_PACK_KEY and HA_PACK_KEY > during ALTER to allow ALGORITHM=INSTANT and ALGORITHM=NOCOPY in more cases. > > If for some reasons (e.g. due to a bug fix such as MDEV-20704) these > cumulative (over all segments) flags in KEY::flags are different for > the old and new table inside compare_keys_but_name(), the difference > in HA_BINARY_PACK_KEY and HA_PACK_KEY in KEY::flags is not really important: > > MyISAM and Aria can handle such cases well: per-segment flags are stored in > MYI and MAI files anyway and they are read during ha_myisam::open() > ha_maria::open() time. So indexes get opened with correct per-segment > flags that were calculated during the table CREATE time, no matter > what the old (CREATE time) and new (ALTER TIME) per-index compression > flags are, and no matter if they are equal or not. > > All other engine ignore key compression flags, so this change > is safe for other engines as well. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c83f87ece4a: MDEV-15990 REPLACE on a precise-versioned table returns duplicate key error (ER_DUP_ENTRY)
by Sergei Golubchik 15 Oct '22

15 Oct '22

Hi, Nikita, Sorry, this comes very late. But the bug is still present in 10.10, and the filter sorts by the date precisely to highlight these old bugs. So, here you are. First comment, please, port it to the latest 10.3. I wasn't able to find the repository with the fix, but the patch itself is avaliable from the pull request as https://github.com/MariaDB/server/commit/c83f87ece4a.patch so it can be salvaged. More comments - see below. On Oct 15, Nikita Malyavin wrote: > From: Nikita Malyavin <nikitamalyavin(a)gmail.com> > Date: Fri, 20 Jul 2018 00:24:40 +1000 > Subject: [PATCH] MDEV-15990: REPLACE on a precise-versioned table returns > * handle zero value in `row_start` `trx_id` > * refuse to do more than one versioned insert on the same transaction and same row > * generalize versioned cases with and without triggers > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index dda469a1426d5..cd2690128de67 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1928,32 +1928,15 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > For system versioning wa also use path through delete since we would > save nothing through this cheating. > */ > - if (last_uniq_key(table,key_nr) && > + if (last_uniq_key(table,key_nr) && !table->versioned() && ok! > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID) && table->vers_write) > - { > - bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > - table->vers_start_field()->store(0, false); > - } > - if (unlikely(error= table->file->ha_update_row(table->record[1], > - table->record[0])) && > - error != HA_ERR_RECORD_IS_THE_SAME) > + error= table->file->ha_update_row(table->record[1], table->record[0]); > + if (unlikely(error && error != HA_ERR_RECORD_IS_THE_SAME)) > goto err; > if (likely(!error)) > - { > info->deleted++; > - if (table->versioned(VERS_TIMESTAMP) && table->vers_write) > - { > - store_record(table, record[2]); > - error= vers_insert_history_row(table); > - restore_record(table, record[2]); > - if (unlikely(error)) > - goto err; > - } > - } > - else > error= 0; // error was HA_ERR_RECORD_IS_THE_SAME > thd->record_first_successful_insert_id_in_cur_stmt(table->file->insert_id_for_cur_row); > /* > @@ -1969,23 +1952,22 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > TRG_ACTION_BEFORE, TRUE)) > goto before_trg_err; > > - if (!table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) > + { > + bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); this is conceptually wrong, you cannot set table->write_set in the middle of statement execution. that is you can and it might even work for some engines, but it breaks the API. > + table->vers_start_field()->store(0, false); > + } > + > + if (!table->versioned()) > error= table->file->ha_delete_row(table->record[1]); > else > - { > - store_record(table, record[2]); > - restore_record(table, record[1]); > - table->vers_update_end(); why? is the row_end updated somewhere else? > error= table->file->ha_update_row(table->record[1], > table->record[0]); > - restore_record(table, record[2]); > - } > - if (unlikely(error)) > + if (unlikely(error && error != HA_ERR_RECORD_IS_THE_SAME)) > goto err; > - if (!table->versioned(VERS_TIMESTAMP)) > + if (likely(!error)) > info->deleted++; > - else > - info->updated++; > + error= 0; > if (!table->file->has_transactions()) > thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->triggers && > @@ -1995,6 +1977,17 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > trg_error= 1; > goto ok_or_after_trg_err; > } > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) > + { > + store_record(table, record[2]); > + error = vers_insert_history_row(table); if you do that, you shouldn't have been doing ha_update_row() above, as far as I can see. What is that update above for? > + restore_record(table, record[2]); > + if (unlikely(error)) > + goto err; > + } > + if (table->versioned()) > + goto after_trg_n_copied_inc; > + > /* Let us attempt do write_row() once more */ > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 19772f9c277: MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations
by Sergei Golubchik 13 Oct '22

13 Oct '22

Hi, Oleksandr, On Oct 13, Oleksandr Byelkin wrote: > revision-id: 19772f9c277 (mariadb-10.3.36-90-g19772f9c277) > parent(s): e05ab0cfc5f > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-10-12 15:59:46 +0200 > message: > > MDEV-29748 ASAN errors or server crash in File_parser::parse upon concurrent view operations > > Prevent simultaniouse access to a view table SHARE (and so its MEM_ROOT) from different threads. If you're writing into share, why not to do it when a share is populated, then you wouldn't need a mutex. And it'd be a logical place to initialize the share anyway. Doing it at some arbitrary point in time later looks strange. I mean, why not to call this mariadb_view_version_get after the line share->view_def= sql_parse_prepare(&pathstr, &share->mem_root, true); where it logically belongs? > + mysql_mutex_lock(&share->LOCK_share); > > if (!(share->tabledef_version.str= > (uchar*) alloc_root(&share->mem_root, > MICROSECOND_TIMESTAMP_BUFFER_SIZE))) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0