October 2022 - developers - lists.mariadb.org

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 23 Oct '22

23 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 21 Oct '22

21 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

Re: [Maria-developers] b1b38b64598: MDEV-29181 Potential corruption on FK update on a table with vcol index
by Sergei Golubchik 10 Oct '22

10 Oct '22

Hi, Nikita, On Aug 30, Nikita Malyavin wrote: > revision-id: b1b38b64598 (mariadb-10.5.17-4-gb1b38b64598) > parent(s): 3b656ac8c17 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-08-23 00:27:39 +0300 > message: > > MDEV-29181 Potential corruption on FK update on a table with vcol > index > > vc_templ->mysql_table concept is completely broken. This table pointer > persists with a global access and can be overwritten by arbitrary > thread. Like it wasn't enough, it also could become invalid after > eviction from tc cache. this is a bit misleading. I agree it's completely broken, though :) But not "this table pointer", because, of course it persists with a global access and can be overwritten and so on. This is by design, it's protected by mysql_table_query_id. The problem is that table (dict_table_t) argument of innodb_find_table_for_vc() points to a shared data structure, not thread local, as I believed. So vc_templ is shared too. I suggest to rewrite the comment like vc_templ->mysql_table concept is completely broken. table argument of innodb_find_table_for_vc() points to a shared data structure dict_table_t, and thus vc_templ is shared too and cannot be used for a thread-local cache. > This new solution simply does the following: > * Sets up a referenced table list in TABLE instance (sql_base.cc) > * Iterates through it along with dict_table_t::referenced_set > (row_upd_check_references_constraints) > * Passes corresponding prebuilt through a call chain to > row_ins_foreign_check_on_constraint > * Sets up newly created upd_node_t::prebuilt field and uses it > accordingly is cascade update. Is this upd_node_t::prebuilt used anywhere? As a prebuilt, I mean. I couldn't find it (it's a complex patch, so I couldn've missed it). As far as I can see, it's only used to store a pointer to TABLE. So it seems to me than a simpler fix for this bug could be: * remove vc_templ caching (mysql_table and mysql_table_query_id) * store TABLE* in upd_node_t. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 6

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

05 Oct '22

Hi, Alexander, See a couple of questions below: On Oct 04, Alexander Barkov wrote: > revision-id: dcfc60ea2ee (mariadb-10.4.26-28-gdcfc60ea2ee) > parent(s): 6c2c825e503 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-03 10:32:42 +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 > diff --git a/mysql-test/main/alter_table_debug_aria.test b/mysql-test/main/alter_table_debug_aria.test > new file mode 100644 > index 00000000000..22df57686e1 > --- /dev/null > +++ b/mysql-test/main/alter_table_debug_aria.test > @@ -0,0 +1,36 @@ > +--source include/have_debug.inc > +--source include/have_maria.inc > + > +--echo # > +--echo # Start of 10.4 tests > +--echo # > + > +--echo # > +--echo # MDEV-28727 ALTER TABLE ALGORITHM=NOCOPY does not work after upgrade > +--echo # > + > +CREATE TABLE t1 (a DOUBLE NOT NULL, KEY(a)) ENGINE=ARIA; > +ALTER TABLE t1 MODIFY a DOUBLE NOT NULL DEFAULT 10, ALGORITHM=NOCOPY; > +DROP TABLE t1; > + > +CREATE TABLE t1 (a DOUBLE NOT NULL, KEY(a)) ENGINE=ARIA; > +ALTER TABLE t1 MODIFY a DOUBLE NOT NULL DEFAULT 10, ALGORITHM=INSTANT; > +DROP TABLE t1; > + > +SET SESSION debug_dbug='+d,emulate_pre_mdev_20704'; > +CREATE TABLE t1 (a DOUBLE NOT NULL, KEY(a)) ENGINE=ARIA; Why did you add these debug-only tests with the emulate_pre_mdev_20704 hack, when you have perfect non-debug tests with real pre-MDEV-20704 created tables ? > +SET SESSION debug_dbug=''; > +--error ER_ALTER_OPERATION_NOT_SUPPORTED > +ALTER TABLE t1 MODIFY a DOUBLE NOT NULL DEFAULT 10, ALGORITHM=INSTANT; > +DROP TABLE t1; > + > +SET SESSION debug_dbug='+d,emulate_pre_mdev_20704'; > +CREATE TABLE t1 (a DOUBLE NOT NULL, KEY(a)) ENGINE=ARIA; > +SET SESSION debug_dbug=''; > +--error ER_ALTER_OPERATION_NOT_SUPPORTED > +ALTER TABLE t1 MODIFY a DOUBLE NOT NULL DEFAULT 10, ALGORITHM=NOCOPY; > +DROP TABLE t1; > + > +--echo # > +--echo # End of 10.4 tests > +--echo # > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2436031200a..c192a4553a9 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6748,11 +6753,13 @@ Compare_keys compare_keys_but_name(const KEY *table_key, const KEY *new_key, > const KEY *const new_pk, > const KEY *const old_pk) > { > - if (table_key->algorithm != new_key->algorithm) > + ulong key_flag_mask= HA_KEYFLAG_MASK | > + table->file->key_pack_flags_supported(); I don't understand. You said that these KEY->flags do not affect what the engine does. Why to compare them at all? > + > + if ((table_key->flags & key_flag_mask) != (new_key->flags & key_flag_mask)) > return Compare_keys::NotEqual; > > - if ((table_key->flags & HA_KEYFLAG_MASK) != > - (new_key->flags & HA_KEYFLAG_MASK)) > + if (table_key->algorithm != new_key->algorithm) > return Compare_keys::NotEqual; > > if (table_key->user_defined_key_parts != new_key->user_defined_key_parts) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0