- developers - lists.mariadb.org

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

Re: [Maria-developers] e2f8dffc056: MDEV-29069 ER_KEY_NOT_FOUND on online autoinc addition + concurrent DELETE
by Sergei Golubchik 03 Oct '22

03 Oct '22

Hi, Nikita, Few comments below On Aug 27, Nikita Malyavin wrote: > commit e2f8dffc056 > Author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > Date: Mon Jul 25 23:52:24 2022 +0300 > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 422d496d5c3..ca17c5c07a8 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -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); > + } it'd be safer and faster to do all_values_set= bitmap_is_set_all(&table->has_value_set); and, please, add replication (not online alter) tests: 1. slave has more columns 2. binlog row format is "minimal", this might trigger it too > + > /** > Compare full record only if: > - there are no blob fields (otherwise we would also need > @@ -7969,47 +7993,68 @@ 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, > + 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. > + /* > + 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 (!(*(ptr))->is_null()) > - { > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length)) > + if (f->field_index > master_columns && !f->has_explicit_value()) > + continue; why do you check for f->field_index > master_columns? isn't !f->has_explicit_value() the one that decided? in what case will be ok to compare values when f->field_index < master_columns but no explicit value was set? > + > + if (f->is_null() != f->is_null(table->s->rec_buff_length) > + || f->cmp_binary_offset(table->s->rec_buff_length)) > { > - result= TRUE; > + result= true; > goto record_compare_exit; > } > } > - } > > record_compare_exit: > 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. > + */ > +bool Rows_log_event::key_suits_event(const KEY *key) const > +{ > + uint master_columns= m_online_alter ? 0 : m_cols.n_bits; > + for (uint p= 0; p < key->ext_key_parts; p++) > + { > + uint field_idx= key->key_part[p].fieldnr - 1; > + if (field_idx >= master_columns > + && !bitmap_is_set(&m_table->has_value_set, field_idx)) > + return false; > + } > + return true; > +} > > /** > Find the best key to use when locating the row in @c find_row(). > @@ -8030,6 +8075,18 @@ int Rows_log_event::find_key() > DBUG_ENTER("Rows_log_event::find_key"); > DBUG_ASSERT(m_table); > > + #ifdef DBUG_TRACE > + 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; it would've been simpler and shorter to do it the old style, with a goto > best_key_nr= MAX_KEY; > > /* > @@ -8041,6 +8098,8 @@ int Rows_log_event::find_key() > { > if (!m_table->s->keys_in_use.is_set(i)) > continue; > + if (!key_suits_event(key)) > + continue; 1. again, add a replication test, please. when, e.g., slave has more columns and a unique index over new columns. 2. going over all keyparts for every event is rather redundant. you can calculate the set of usable keys once and put it, for example, in m_table->keys_in_use_for_query. > /* > 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 > @@ -8221,7 +8280,8 @@ int Rows_log_event::find_row(rpl_group_info *rgi) > DBUG_DUMP("record[0]", table->record[0], table->s->reclength); > > if ((table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) && > - table->s->primary_key < MAX_KEY) > + table->s->primary_key < MAX_KEY && > + m_key_nr == table->s->primary_key) add a replication test for that: slave has an extra column which is a PK, and HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (InnoDB) > { > /* > Use a more efficient method to fetch the record given by > @@ -8494,12 +8554,18 @@ 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); > + > 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_suits_event(&m_table->key_info[m_table->s->primary_key])) > { > /* > We don't need to allocate any memory for m_key since it is not used. > */ > + m_key_nr= m_table->s->primary_key; > return 0; > } This is very strange if(). 1. it's only for delete, not for update 2. it's before invoking triggers (bug) 3. it's wrong if the master table has a different primary key (bug) I'd suggest to remove it completely. An optimization of avoiding a malloc can be done inside find_key() after the best key is found. > if (do_invoke_trigger()) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
by Sergei Golubchik 28 Sep '22

28 Sep '22

Hi, Oleksandr, Few minor comments, see below On Sep 27, Oleksandr Byelkin wrote: > revision-id: 5265f7001c6 (mariadb-10.3.36-60-g5265f7001c6) > parent(s): 86953d3df0a > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-09-27 10:23:31 +0200 > message: > > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared > > The problem is that if table definition cache (TDC) is full of real tables > which are in tables cache, view definition can not stay there so will be > removed by its own underlying tables. > In situation above old mechanism of detection matching definition in PS > and current version always require reprepare and so prevent executing > the PS. > > One work around is to increase TDC, other - improve version check for > views/triggers (which is done here). Now in suspicious cases we check: > - timestamp (microseconds) of the view to be sure that version really > have changed; > - time (microseconds) of creation of a trigger related to time > (microseconds) of statement preparation. > diff --git a/sql/table.cc b/sql/table.cc > index 506195127b2..f289c2052cb 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -8861,6 +8861,63 @@ bool TABLE_LIST::is_with_table() > return derived && derived->with_element; > } > > + > +/** > + Check if the definition are the same. > + > + If versions do not match it check definitions (with checking and setting > + trigger definition versions (times) > + > + @sa check_and_update_table_version() > +*/ > + > +bool TABLE_LIST::is_the_same_definition(THD* thd, TABLE_SHARE *s) > +{ > + enum enum_table_ref_type tp= s->get_table_ref_type(); > + if (m_table_ref_type == tp) > + { > + bool res= m_table_ref_version == s->get_table_ref_version(); > + > + /* > + If definition is different check content version > + */ if res == true, why do you need to check tabledef_version? > + if (tabledef_version.length && > + tabledef_version.length == s->tabledef_version.length && > + memcmp(tabledef_version.str, s->tabledef_version.str, > + tabledef_version.length) == 0) > + { > + if (table && table->triggers) > + { > + my_hrtime_t hr_stmt_prepare= thd->hr_prepare_time; > + if (hr_stmt_prepare.val) > + for(uint i= 0; i < TRG_EVENT_MAX; i++) > + for (uint j= 0; j < TRG_ACTION_MAX; j++) > + { > + Trigger *tr= > + table->triggers->get_trigger((trg_event_type)i, > + (trg_action_time_type)j); > + if (tr) > + if (hr_stmt_prepare.val <= tr->hr_create_time.val) > + { > + set_tabledef_version(s); > + return FALSE; > + } > + } > + } > + set_table_id(s); > + return TRUE; > + } > + else > + tabledef_version.length= 0; > + return res; > + } > + else > + set_tabledef_version(s); why not set_table_id() ? > + return FALSE; > +} > + > + > uint TABLE_SHARE::actual_n_key_parts(THD *thd) > { > return use_ext_keys && > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index ae3d1738b16..f7a2ccf2abc 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -198,7 +198,7 @@ class Table_triggers_list: public Sql_alloc > */ > List<ulonglong> definition_modes_list; > /** Create times for triggers */ > - List<ulonglong> create_times; > + List<my_hrtime_t> hr_create_times; I suspect it might be UB. You use FILE_OPTIONS_ULLLIST for hr_create_times, perhaps it's safer to use List<ulonglong> here. > > List<LEX_CSTRING> definers_list; > > @@ -328,4 +328,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create); > extern const char * const TRG_EXT; > extern const char * const TRN_EXT; > > + > +/** > + Make time compatible with MySQL 5.7 trigger time. > +*/ > + > +inline my_hrtime_t make_hr_time(my_time_t time, ulong time_sec_part) > +{ > + return my_hrtime_t({((ulonglong) time)*1000000 + time_sec_part}); > +} better put it next to hrtime_to_time/etc and, please, always 'static inline' > + > #endif /* SQL_TRIGGER_INCLUDED */ > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 17dea643144..177d01a312e 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1154,7 +1163,32 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > DBUG_RETURN(error); > } > > +/** > + Check is TABLE_LEST and SHARE match > + @param[in] view TABLE_LIST of the view > + @param[in] share Share object of view > + > + @return false on error or misspatch Eh. I don't understad this comment at all. First, I thought you made two typos, s/is/if/ and s/misspatch/mismatch/. But this function doesn't check if something matches, if gets the view version, as the name says, the name's good. But the comment seems to be from is_the_same_definition() > +*/ > + > +bool mariadb_view_version_get(TABLE_SHARE *share) > +{ > + DBUG_ASSERT(share->is_view); > + > + 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)) > + return TRUE; > + DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > + return FALSE; > +} > > /** > read VIEW .frm and create structures Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 349283c5e7a: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
by Sergei Golubchik 27 Sep '22

27 Sep '22

Hi, Oleksandr, On Jan 27, Oleksandr Byelkin wrote: > commit 349283c5e7a > Author: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Wed Apr 17 15:50:59 2019 +0200 > > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared > > The problem is that if table definition cache (TDC) is full of real tables > which are in tables cache, view definition can not stay there so will be > removed by its own underlying tables. > In situation above old mechanism of detection matching definition in PS > and current version always require reprepare and so prevent executing > the PS. > > One work around is to increase TDC, other - improve version check for > views/triggers (which is done here). Now in suspicious cases we check: > - timestamp (ms) of the view to be sure that version really have changed; > - time (ms) of creation of a trigger related to time (ms) of statement > preparation. not "ms" but "us" or "µs". microseconds, not millisecons. > diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result > index e69c6e06193..005c78a0319 100644 > --- a/mysql-test/r/ps_ddl.result > +++ b/mysql-test/r/ps_ddl.result > @@ -831,8 +831,8 @@ a b c > 20 40 100 > 30 60 150 > call p_verify_reprepare_count(1); > -SUCCESS > - > +ERROR > +Expected: 1, actual: 0 oops, forgot to update? > # Check that we properly handle ALTER VIEW statements. > execute stmt; > a b c > @@ -882,9 +882,9 @@ a b c > 10 50 60 > 20 100 120 > 30 150 180 > -call p_verify_reprepare_count(1); > -SUCCESS > - > +call p_verify_reprepare_count(0); > +ERROR > +Expected: 0, actual: 1 oops, updated too much? > execute stmt; > a b c > 10 50 60 > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 3f0fba8fc10..b9bb9c978fb 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1079,6 +1079,7 @@ class Statement: public ilink, public Query_arena > > LEX_STRING name; /* name for named prepared statements */ > LEX *lex; // parse tree descriptor > + ulonglong ms_prepare_time; // time of preparation in microseconds "ms" here too :( Better just say my_hrtime_t prepare_time; > /* > Points to the query associated with this statement. It's const, but > we need to declare it char * because all table handlers are written > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 64e4cd30561..a4a24cc1a80 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4177,6 +4177,9 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) > Query_arena *old_stmt_arena; > DBUG_ENTER("Prepared_statement::prepare"); > DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode); > + > + // The same format as for triggers to compare > + ms_prepare_time= my_hrtime().val; do it at the end instead? > /* > If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql. > However, it seems handy if com_stmt_prepare is increased always, > @@ -4519,6 +4523,22 @@ Prepared_statement::execute_loop(String *expanded_query, > DBUG_ASSERT(thd->get_stmt_da()->sql_errno() == ER_NEED_REPREPARE); > thd->clear_error(); > > + { > + /* > + Check if we too fast with reprepare: > + we can be so fast that: > + 1) make change of a trigger, > + 2) prepare, > + 3) try to exacute and reprepare > + in 1 microsecond, so we will wait till > + next microsecond before last reprepare > + */ > + while (first_prepared == my_hrtime().val) > + { > + pthread_yield(); > + } > + } I wouldn't bother. A more likely case is when a system time goes back, e.g. if there's an ntpd running and your loop doesn't help there. I suggest not to do anything special in this case. And if an ::execute() will open a trigger or a view with a timestamp >= than ms_prepare_time, it will cause reprepare every time. Which is exactly the behavior before your bugfix - opening of a trigger or a view causes reprepare - and it is not a problem unless one has a very small table definition cache. > + > error= reprepare(); > > if (! error) /* Success */ > diff --git a/sql/parse_file.cc b/sql/parse_file.cc > index 999f53bd681..92e7d82af1c 100644 > --- a/sql/parse_file.cc > +++ b/sql/parse_file.cc > @@ -174,11 +174,11 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter) > { > /* string have to be allocated already */ > LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset); > - time_t tm= my_time(0); > - > - get_date(val_s->str, GETDATE_DATE_TIME|GETDATE_GMT|GETDATE_FIXEDLENGTH, > - tm); > - val_s->length= PARSE_FILE_TIMESTAMPLENGTH; > + ulonglong tm= my_hrtime().val; add a comment here, "number of microseconds since Epoch, timezone-independent" or something like that > + // Paded to 19 characters for compatibility > + val_s->length= snprintf(val_s->str, MICROSECOND_TIMESTAMP_BUFFER_SIZE, > + "%019lld", tm); > + DBUG_ASSERT(val_s->length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > if (my_b_write(file, (const uchar *)val_s->str, > PARSE_FILE_TIMESTAMPLENGTH)) > DBUG_RETURN(TRUE); > @@ -835,15 +835,15 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root, > /* string have to be allocated already */ > LEX_STRING *val= (LEX_STRING *)(base + parameter->offset); > /* yyyy-mm-dd HH:MM:SS = 19(PARSE_FILE_TIMESTAMPLENGTH) characters */ a comment is now wrong > - if (ptr[PARSE_FILE_TIMESTAMPLENGTH] != '\n') > + if (ptr[MICROSECOND_TIMESTAMP_BUFFER_SIZE-1] != '\n') > { > my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0), > parameter->name.str, line); > DBUG_RETURN(TRUE); > } > - memcpy(val->str, ptr, PARSE_FILE_TIMESTAMPLENGTH); > - val->str[val->length= PARSE_FILE_TIMESTAMPLENGTH]= '\0'; > - ptr+= (PARSE_FILE_TIMESTAMPLENGTH+1); > + memcpy(val->str, ptr, MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > + val->str[val->length= MICROSECOND_TIMESTAMP_BUFFER_SIZE-1]= '\0'; > + ptr+= MICROSECOND_TIMESTAMP_BUFFER_SIZE; > break; > } > case FILE_OPTIONS_STRLIST: > diff --git a/sql/table.h b/sql/table.h > index 14d6b787b4e..82508388f84 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2159,7 +2175,7 @@ struct TABLE_LIST > LEX_STRING source; /* source of CREATE VIEW */ > LEX_STRING view_db; /* saved view database */ > LEX_STRING view_name; /* saved view name */ > - LEX_STRING timestamp; /* GMT time stamp of last operation */ > + LEX_STRING ms_timestamp; /* GMT time stamp of last operation */ better keep "timestamp" name. Or "hrtimestamp" if you'd like > st_lex_user definer; /* definer of view */ > ulonglong file_version; /* version of file's field set */ > ulonglong mariadb_version; /* version of server on creation */ > diff --git a/sql/table.cc b/sql/table.cc > index ca6ce02e4f2..24412966b36 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -8480,6 +8480,53 @@ bool TABLE_LIST::is_with_table() > } > > > +bool TABLE_LIST::is_table_ref_id_equal(THD* thd, TABLE_SHARE *s) > +{ > + enum enum_table_ref_type tp= s->get_table_ref_type(); > + if (m_table_ref_type == tp) > + { > + bool res= m_table_ref_version == s->get_table_ref_version(); > + > + /* > + If definition is different check content version > + */ > + if (tabledef_version.length && > + tabledef_version.length == s->tabledef_version.length && > + memcmp(tabledef_version.str, s->tabledef_version.str, > + tabledef_version.length) == 0) > + { > + if (table && table->triggers) > + { > + > + ulonglong ms_stmt_prepare= thd->ms_prepare_time; > + if (ms_stmt_prepare) > + for(uint i= 0; i < TRG_EVENT_MAX; i++) > + for (uint j= 0; j < TRG_ACTION_MAX; j++) > + { > + Trigger *tr= > + table->triggers->get_trigger((trg_event_type)i, > + (trg_action_time_type)j); > + if (tr) > + if (ms_stmt_prepare <= tr->ms_create_time) > + { > + set_tabledef_version(s); > + return FALSE; > + } > + } > + } > + set_table_id(s); > + return TRUE; > + } > + else > + tabledef_version.length= 0; > + return res; > + } > + else > + set_tabledef_version(s); > + return FALSE; > +} this is now a very confusing function. it doesn't only check is_table_ref_id_equal, it also sets some of them (tabledef or both tabledef and ref_id), and under unclear conditions. Could you please comment it? And you don't check that view's timestamp is in the past. If it's not, it can be changed still within the same microsecond. > + > + > uint TABLE_SHARE::actual_n_key_parts(THD *thd) > { > return use_ext_keys && > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index e2e6e1b7acc..70f246d4e0d 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -112,7 +112,7 @@ class Trigger :public Sql_alloc > GRANT_INFO subject_table_grants; > sql_mode_t sql_mode; > /* Store create time. Can't be mysql_time_t as this holds also sub seconds */ > - ulonglong create_time; > + ulonglong ms_create_time; // Create time timestamp in microseconds my_hrtime_t create_time; > trg_event_type event; > trg_action_time_type action_time; > uint action_order; > @@ -195,7 +195,7 @@ class Table_triggers_list: public Sql_alloc > */ > List<ulonglong> definition_modes_list; > /** Create times for triggers */ > - List<ulonglong> create_times; > + List<ulonglong> ms_create_times; ditto > > List<LEX_STRING> definers_list; > > @@ -331,4 +331,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create); > extern const char * const TRG_EXT; > extern const char * const TRN_EXT; > > + > +/** > + Make time compatible with MySQL 5.7 trigger time. > +*/ > + > +inline ulonglong make_ms_time(my_time_t time, ulong time_sec_part) make_hrtime. and better put it together with hrtime_to_time, hrtime_from_time, etc > +{ > + return ((ulonglong) time)*1000000 + time_sec_part; > +} > + > #endif /* SQL_TRIGGER_INCLUDED */ > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 020830483c5..3b51f6d2451 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > DBUG_RETURN(error); > } > > +/** > + Check is TABLE_LEST and SHARE match > + @param[in] view TABLE_LIST of the view > + @param[in] share Share object of view > + > + @return false on error or misspatch > +*/ > > +bool mariadb_view_version_get(TABLE_SHARE *share) > +{ > + DBUG_ASSERT(share->is_view); > + > + if (!(share->tabledef_version.str= > + (uchar*) alloc_root(&share->mem_root, > + MICROSECOND_TIMESTAMP_BUFFER_SIZE))) > + return TRUE; > + > + DBUG_ASSERT(share->view_def != NULL); > + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, > + view_timestamp_parameters, 1, > + &file_parser_dummy_hook)) > + return TRUE; > + DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); If you open an old view with YYYY-MM-DD... timestamp, where do you convert it? If you open a view without a timestamp at all, where do you handle it? > + return FALSE; > +} > > /** > read VIEW .frm and create structures > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 9483db9eff9..138fa4bc631 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6724,13 +6724,15 @@ static bool store_trigger(THD *thd, Trigger *trigger, > table->field[14]->store(STRING_WITH_LEN("OLD"), cs); > table->field[15]->store(STRING_WITH_LEN("NEW"), cs); > > - if (trigger->create_time) > + if (trigger->ms_create_time) > { > + /* timestamp is in microseconds */ > table->field[16]->set_notnull(); > thd->variables.time_zone->gmt_sec_to_TIME(&timestamp, > - (my_time_t)(trigger->create_time/100)); > - /* timestamp is with 6 digits */ > - timestamp.second_part= (trigger->create_time % 100) * 10000; > + (my_time_t) > + (trigger->ms_create_time/ > + 1000000)); > + timestamp.second_part= trigger->ms_create_time % 1000000; hrtime_to_time() and hrtime_sec_part() > ((Field_temporal_with_date*) table->field[16])->store_time_dec(&timestamp, > 2); > } > @@ -9810,12 +9812,14 @@ static bool show_create_trigger_impl(THD *thd, Trigger *trigger) > trigger->db_cl_name.length, > system_charset_info); > > - if (trigger->create_time) > + if (trigger->ms_create_time) > { > MYSQL_TIME timestamp; > thd->variables.time_zone->gmt_sec_to_TIME(&timestamp, > - (my_time_t)(trigger->create_time/100)); > - timestamp.second_part= (trigger->create_time % 100) * 10000; > + (my_time_t) > + (trigger->ms_create_time/ > + 1000000)); > + timestamp.second_part= trigger->ms_create_time % 1000000; ditto > p->store(&timestamp, 2); > } > else Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5