September 2022 - developers - lists.mariadb.org

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] 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

Re: [Maria-developers] commit 192fea6b104b249bb4764635bb3a52fb9c8a2148 MDEV-27128
by Alexey Botchkov 19 Sep '22

19 Sep '22

Hi, Rucha. I decided i dislike the overall design of the struct json_schema. Seems better to do it with C++ classes using virtual methods. So instead of structures we'd have classes as json_schema_const json_schema_enum json_scnema_string json_schema_number etc. they all inherited from the class json_schema_item { virtual parse() virtual check() } Parsing the schema we can behave like this: json_parse_schema() checks all the keys on the current level looking for 'const' 'enum' and 'typ[e' keywords only. If it gets 'const' or 'enum' it creates the json_schema_const/enum respectively For the "type" it creates the appropriate json_schema_xxx and callst ->parse() with the same JSON object. So the parse would go over it once again looking for the type-specific keywords. That way we don't need the hash_list as the destructor of the json_schema_xxx class would free it's resources. Minor comments. You compare keys like strncmp((const char*)curr_key, "const", key_len) all over the code. But it is not correct. That way 'cons' or 'con' or 'co' or even 'c' - they all will be considered equal to the 'const'. Just add the 'key_cmp()' function that would compare key lengths then do the strncmp. > double val= je->s.cs->strntod((char *) je->value, je->value_len, &end; You do this even if the value_type is not number. Not very safe. Better to avoid doing that here at all. That queriy just crashes the server. Didn't yet found why SET @schema_array= '{ "typ": "arra", "prefixItem": [ { "typ": "objec"}, { "typ": "objec", "propertie": { "number1": {"typ":"numbe"}, "string1": {"typ":"strin"}, "obj1" : {"typ":"objec", "propertie": { "key1" : {"type":"string"} } } }, "required":["obj1"] } ], "items":false }'; SELECT JSON_SCHEMA_VALID(@schema_array, '[{"key1":"value1"}, {"number1":1, "string1":"str1", "obj1":{"key1":"val1"}}]'); Feel free to discuss the design details! Best regards. HF

1 0

Re: [Maria-developers] e0e75de899f: MDEV-26715 Windows/installer - allow passwordless login for root
by Sergei Golubchik 13 Sep '22

13 Sep '22

Hi, Vladislav, Note, despite what the subject says, below is not for one commit, but for the diff over them all. Just a couple of questions and a comment: > diff --git a/libmariadb b/libmariadb > index a6665e65c05..22f7ed12b46 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit a6665e65c05c00f9df983a0f788a643ca7bc084c > +Subproject commit 22f7ed12b4645a9fc5d66974551359373090c86b are auth_gssapi plugins using only standard Windows libraries? > diff --git a/mysql-test/main/mysql_install_db_win_utf8.test b/mysql-test/main/mysql_install_db_win_utf8.test > index fc67b66d3ca..0dc75af3914 100644 > --- a/mysql-test/main/mysql_install_db_win_utf8.test > +++ b/mysql-test/main/mysql_install_db_win_utf8.test > @@ -1,6 +1,6 @@ > --source include/windows.inc > --source include/check_utf8_cli.inc > - > +use mysql; Why? > # Create database in tmp directory using mysql_install_db.exe, > # and start server from this directory. > let $ddir= $MYSQLTEST_VARDIR/tmp/датадир; > diff --git a/plugin/auth_gssapi/mysql-test/auth_gssapi/suite.pm b/plugin/auth_gssapi/mysql-test/auth_gssapi/suite.pm > index e77ba05cb5c..71a6bf33b7b 100644 > --- a/plugin/auth_gssapi/mysql-test/auth_gssapi/suite.pm > +++ b/plugin/auth_gssapi/mysql-test/auth_gssapi/suite.pm > @@ -1,8 +1,9 @@ > + > package My::Suite::AuthGSSAPI; > > @ISA = qw(My::Suite); > > -return "No AUTH_GSSAPI plugin" unless $ENV{AUTH_GSSAPI_SO}; > +return "No AUTH_GSSAPI plugin" unless ($ENV{AUTH_GSSAPI_SO} or ($^O eq 'MSWin32')); The second condition normally is something like or $::mysqld_variables{gssapi} eq "ON"; > > return "Not run for embedded server" if $::opt_embedded_server; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] f5a8327d016: MDEV-29458: Role grant commands do not propagate all grants
by Sergei Golubchik 12 Sep '22

12 Sep '22

Hi, Vicentiu, On Sep 12, Vicentiu Ciorbaru wrote: > revision-id: f5a8327d016 (mariadb-10.3.36-26-gf5a8327d016) > parent(s): fa75057b238 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2022-09-05 16:40:10 +0300 > message: > > MDEV-29458: Role grant commands do not propagate all grants > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index ba457083b75..3806fb31aa6 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2741,7 +2741,9 @@ static void acl_update_role(const char *rolename, ulong privileges) > { > ACL_ROLE *role= find_acl_role(rolename); > if (role) > + { > role->initial_role_access= role->access= privileges; > + } > } I wish you put this and the tab-to-spaces change in a separate commit. (even better - tab-to-spaces in a separate commit and this hunk - nowhere :) > @@ -9720,6 +9725,14 @@ static int handle_grant_struct(enum enum_acl_lists struct_no, bool drop, > remove_ptr_from_dynarray(&grantee->role_grants, acl_role); > } > > + /* Remove all of the role_grants from this user. */ > + while (acl_role->role_grants.elements) > + delete_dynamic_element(&acl_role->role_grants, > + acl_role->role_grants.elements - 1); this is a weird way of deleting all elements in the array. What's wrong with delete_dynamic(&acl_role->role_grants); ? > + > + /* all grants must be revoked from this role by now. propagate this */ > + propagate_role_grants(acl_role, PRIVS_TO_MERGE::ALL); > + > my_hash_delete(&acl_roles, (uchar*) acl_role); > DBUG_RETURN(1); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 3f9e6c9c111: MDEV-22133 handle_fatal_signal (sig=11) on optimized builds in handle_grant_table instead of ERROR | Buffer overflow (on optimized builds)
by Sergei Golubchik 09 Sep '22

09 Sep '22

Hi, Oleksandr, On Sep 09, Oleksandr Byelkin wrote: > revision-id: 3f9e6c9c111 (mariadb-10.3.35-36-g3f9e6c9c111) > parent(s): 5feb60ce186 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-06-24 15:37:25 +0200 > message: > > MDEV-22133 handle_fatal_signal (sig=11) on optimized builds in handle_grant_table instead of ERROR | Buffer overflow (on optimized builds) > > Return an error if we can not read a table required for ACL. > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 4c68b4505a4..38d2e9c35a1 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -7420,6 +7420,11 @@ static bool grant_load(THD *thd, > while (!p_table->file->ha_index_next(p_table->record[0])); > } > } > + else > + { > + sql_print_error("Missing system table mysql.procs_priv; " > + "please run mysql_upgrade to create it"); > + } I don't think so. The comment earlier says p_table= procs_priv.table(); // this can be NULL and if "this can be NULL" then it's not an error if it is. > > end_unlock_p: > if (p_table) > @@ -10041,6 +10046,8 @@ static int handle_grant_data(THD *thd, Grant_tables& tables, bool drop, > } > > /* Handle stored routines table. */ > + if (tables.procs_priv_table().table_exists()) > + { This is ok, but also, please, check that all accesses to other optional tables are properly protected. See FIRST_OPTIONAL_TABLE define, all tables below it may not exist. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ed477a6d30c: MDEV-29056 Replica thread reports error on ALTER ONLINE after LOCK WRITE
by Sergei Golubchik 09 Sep '22

09 Sep '22

Hi, Nikita, No, in this case (and commonly for collateral changes) I'd rather prefer to refine the original (878a92e) commit to not cause the MDEV-29056 in the first place. Ignoring LOCK= clause is logically correct and that's what the old code was doing anyway. But perhaps ALTER TABLE still needs to check that ALGORITHM= and LOCK= clauses are compatible? On Aug 27, Nikita Malyavin wrote: > revision-id: ed477a6d30c (mariadb-10.6.1-521-ged477a6d30c) > parent(s): 56206e60315 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-07-26 00:41:00 +0300 > message: > > MDEV-29056 Replica thread reports error on ALTER ONLINE after LOCK WRITE > > Commit 878a92e (MDEV-28943) changes the behavior of ALTER TABLE under > prelocking. It now ignores passed LOCK= value in that case. > > However, LOCK TABLES command is never replicated, so the replica node > remains unaware of it. > > The solution would be to guess on the replica side on the mode used by > master. To make a deduction reliable, master's locked_tables_mode state > is passed for query log events. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 68ebe0e2f5e: MDEV-29446 Change SHOW CREATE TABLE to display default collation
by Sergei Golubchik 09 Sep '22

09 Sep '22

Hi, Alexander, On Sep 09, Alexander Barkov wrote: > revision-id: 68ebe0e2f5e (mariadb-10.3.36-25-g68ebe0e2f5e) > parent(s): f501f815bc7 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-09-07 08:10:58 +0400 > message: > > MDEV-29446 Change SHOW CREATE TABLE to display default collation > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index a019c3c9dbd..cdf0bc01b6a 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -2253,18 +2247,6 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet, > { > packet->append(STRING_WITH_LEN(" CHARACTER SET ")); > packet->append(field->charset()->csname); > - } > - /* > - For string types dump collation name only if > - collation is not primary for the given charset > - > - For generated fields don't print the COLLATE clause if > - the collation matches the expression's collation. > - */ > - if (!(field->charset()->state & MY_CS_PRIMARY) && > - (!field->vcol_info || > - field->charset() != field->vcol_info->expr->collation.collation)) why, I think, this second condition could still apply, no need to remove it. > - { > packet->append(STRING_WITH_LEN(" COLLATE ")); > packet->append(field->charset()->name); > } Note that in 10.10 you need to fix json_table code inside a big if() condition: ... (!(m_explicit_cs->state & MY_CS_PRIMARY) && (str->append(STRING_WITH_LEN(" COLLATE ")) || str->append(&m_explicit_cs->coll_name))))) || ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0