January 2022 - developers - lists.mariadb.org

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] b3844107287: MDEV-16546 System versioning setting to allow history modification
by Sergei Golubchik 24 Aug '22

24 Aug '22

Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. when system_versioning_insert_history=1 > 3. Cleaned up select_insert::send_data() from setting vers_write as > this parameter is now set on TABLE initialization. I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit? > diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ x y > 9 9001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d23b190b2c5..93f8a8434e4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #endif > } > > +bool THD::vers_modify_sys_field() const don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example). > +{ > + if (lex->sql_command != SQLCOM_INSERT && > + lex->sql_command != SQLCOM_INSERT_SELECT && > + lex->sql_command != SQLCOM_LOAD) > + return false; I don't think this is enough to reject insert ... on duplicate key update row_start=xxx > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > + return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
by Sergei Golubchik 17 Apr '22

17 Apr '22

Hi, Aleksey! Could you please explain in the commit comment what the bug was and what you are fixing? I wasn't able to understad that, so I had to debug those crashes myself. So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different. I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere. But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena. Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over? We've already discussed VCOL_TABLE_ALIAS, no need to go there again. On Jan 06, Aleksey Midenkov wrote: > revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) > parent(s): a565e63c54c > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-29 00:46:31 +0300 > message: > > MDEV-24176 Server crashes after insert in the table with virtual > column generated using date_format() and if() > > When table is reopened from cache vcol_info contains stale > expression. We refresh expression via TABLE::vcol_fix_exprs() but > first we must prepare a proper context (Vcol_expr_context) and do some > preparations which meet some requirements: > > 1. fields must not be fixed, we unfix them with cleanup() via > cleanup_processor. > > 2. Item_ident must have cached_table cleared. Again, this is stale > data which was already freed. cached_table interferes with > find_field_in_tables(). > > We cannot clear cached_table directly in Item_ident::cleanup() > method. Lots of spaghetti logic depends on cached_table existence even > after cleanup() done. > > 3. If Item_ident has table_name non-NULL we trigger expr update. That > is done via Item_ident::check_vcol_func_processor() and > VCOL_TABLE_ALIAS flag. > > (4-7) The below conditions are prepared by Vcol_expr_context and used in > both fix_session_expr_for_read() and TABLE::vcol_fix_exprs(): > > 4. Any fix_expr() must be done on expr_arena as there may be new items > created. It was a bug in fix_session_expr_for_read(). It was just not > reproduced because there were no second refix. Now refix is done for > more cases so it does reproduce. Tests affected: vcol.binlog > > 5. Also name resolution context must be narrowed to the single table > with all crutches in place. Tests affected: vcol.update > > 6. sql_mode must be clean and not fail expr update. > > sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc > must not affect vcol expression update. If the table was created > successfully any further evaluation must not fail. Tests affected: > main.func_like > > 7. Warnings are disabled during expr update. It is assumed that these > warnings were printed before during initialization phase or later > during execution phase. Tests affected: vcol.wrong_arena > > Dictionary: refresh, update and refix are the synonyms for > vcol_fix_exprs() or fix_session_expr_for_read() calls. > > 8. cleanup_excluding_fields_processor() removed. It was just a quick > hack to exclude wrongly working Item_field from processing. Now it > works due to correct execution environment (Vcol_expr_context). > Related to MDEV-10355. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 12

Re: [Maria-developers] 88f8aa20bba: MDEV-23486 RBR can bypass secure_timestamp=YES
by Sergei Golubchik 26 Mar '22

26 Mar '22

Hi, Aleksey! On Jan 09, Aleksey Midenkov wrote: > revision-id: 88f8aa20bba (mariadb-10.3.31-79-g88f8aa20bba) > parent(s): d5451867af1 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-23486 RBR can bypass secure_timestamp=YES > > Pass to a slave whether master table is system-versioned via > TM_SYSTEM_VERSIONED flag. That deprecates m_vers_from_plain > detection. If secure_timestamp == YES or master did not supply row_end > data treat the row as current data and generate timestamps for it. I don't understand why does it matter whether the master is system versioned or not. secure_timestamp=YES means the table has to store actual timestamps when rows were changed. It doesn't trust the master to set timestamps. So it should not matter whether master table is versioned or not, if you don't trust the master, you cannot trust its TM_SYSTEM_VERSIONED flag either. Even if the table was system versioned on the master, the master could've had secure_timestamp=NO and a completely fake history. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support
by Sergey Petrunia 29 Jan '22

29 Jan '22

Hi Oleg, > commit 53d4d2852463714678b6e753ecffc1ecdde13891 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Thu Jan 13 19:24:22 2022 +0300 > > MDEV-10000 Add support for FORMAT=JSON > > Makes possible calls to SHOW EXPLAIN and EXPLAIN FOR CONNECTION > providing output in JSON format: > - SHOW EXPLAIN FORMAT=JSON FOR $con > - EXPLAIN FORMAT=JSON FOR CONNECTION $con > The patch is mostly ok. I have a few cosmetic comments. Please follow the coding style and make sure the lines are no longer than 80 chars. > diff --git a/sql/sql_show.h b/sql/sql_show.h > index 3d7a4d1146c..8c8ae416699 100644 > --- a/sql/sql_show.h > +++ b/sql/sql_show.h > @@ -164,6 +164,9 @@ class Show_explain_request : public Apc_target::Apc_call > THD *target_thd; /* thd that we're running SHOW EXPLAIN for */ > THD *request_thd; /* thd that run SHOW EXPLAIN command */ > > + bool is_json_format= false; /* set to TRUE if you need the result in JSON > + format, FALSE - in traditional tabular */ > + Multi-line comments must precede the variable: /* Comment here... */ bool is_json_format; > /* If true, there was some error when producing EXPLAIN output. */ > bool failed_to_produce; > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 74757970411..a7188762cbd 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -13945,12 +13945,13 @@ show_param: > Lex->spname= $3; > Lex->sql_command = SQLCOM_SHOW_CREATE_EVENT; > } > - | describe_command FOR_SYM expr > + | describe_command opt_format_json FOR_SYM expr > { > Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > - if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + if (unlikely(prepare_schema_table(thd, Lex, 0, > + Lex->explain_json ? SCH_EXPLAIN_JSON : SCH_EXPLAIN_TABULAR))) > MYSQL_YYABORT; > - add_value_to_list(thd, $3); > + add_value_to_list(thd, $4); > } > | IDENT_sys remember_tok_start wild_and_where > { > @@ -14123,12 +14124,18 @@ opt_describe_column: > ; Please fix identation of this rule to match of identation of others. Please also add a comment about the alternative syntax, both here and in the SHOW EXPLAIN rule. > explain_for_connection: > - describe_command FOR_SYM CONNECTION_SYM expr > + describe_command opt_format_json FOR_SYM CONNECTION_SYM expr > { > + Lex->wild=0; > + Lex->ident= null_clex_str; > + mysql_init_select(Lex); > + Lex->current_select->parsing_place= SELECT_LIST; > + Lex->create_info.init(); > Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > - if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + if (unlikely(prepare_schema_table(thd, Lex, 0, > + Lex->explain_json ? SCH_EXPLAIN_JSON : SCH_EXPLAIN_TABULAR))) > MYSQL_YYABORT; > - add_value_to_list(thd, $4); > + add_value_to_list(thd, $5); > } > ; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] MDEV-27021 Implement SHOW ANALYZE command
by Sergey Petrunia 28 Jan '22

28 Jan '22

Hi Oleg, > commit 92b85f455994ff9f18e30ad4b852109b3e3c5f04 > Author: Oleg Smirnov <olegs(a)teramind.co> > Date: Fri Dec 24 17:27:03 2021 +0300 > > MDEV-27021 Implement SHOW ANALYZE command > The patch looks good. A few cosmetic comments below. I've played with it and it looks usable, although I would expect that SHOW ANALYZE FORMAT=JSON would be the real killer. For SHOW ANALYZE FORMAT=JSON, I suspect we might need to add a few fields into it to make analysis easier (I'll elaborate later). Let's first get SHOW ANALYZE FORMAT=JSON to work without the extra columns. == Cosmetic comments == Please make sure there are no spaces at line ends. Some MariaDB developers feel strongly about not having them. (If your edior doesn't highlight them, "git citool" does). At risk of stating the obvious: * one should not remove the endspace if otherwise they are not changing the line of code. (such "dummy" changes make it hard to look at the revision history) * If one adds a line or changes an existing line, make sure the line doesnt have the end spaces. > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index f31f21faeb0..e18f8c61067 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7224,7 +7224,7 @@ ER_NO_SUCH_TABLE_IN_ENGINE 42S02 > hindi "टेबल '%-.192s.%-.192s' इंजन में मौजूद नहीं है" > swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern" > ER_TARGET_NOT_EXPLAINABLE > - eng "Target is not running an EXPLAINable command" > + eng "Target is not running a command that allows to produce SHOW EXPLAIN or ANALYZE" . This will soon mean "SHOW EXPLAIN or ANALYZE or EXPLAIN FOR CONNECTION". Does "Target is not executing an operation with a query plan" sound better? > ER_CONNECTION_ALREADY_EXISTS > eng "Connection '%.*s' conflicts with existing connection '%.*s'" > ER_MASTER_LOG_PREFIX BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

[Maria-developers] Igor please "review" : 768b5f70b59: Fix Item_func::is_simplifed_cond_processor
by Sergey Petrunia 27 Jan '22

27 Jan '22

Hello Igor, Could you please checkout the below patch? I've stumbled upon behavior of Item_func::is_simplifed_cond_processor which looks bad but I'm not sure if this way to fix it is correct. revision-id: 9149fcb8d504d7c1b027024df63c8120a65a7751 (mariadb-10.4.22-82-g9149fcb8d50) parent(s): b915f79e4e004fde4f6ac8f341afee980e11792b author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2022-01-27 22:58:11 +0300 message: Fix Item_func::is_simplifed_cond_processor The comment for that function says: TRUE if the function is knowingly TRUE or FALSE. I don't get why one would need to check for "!val_int()" in this case. --- mysql-test/main/having_cond_pushdown.result | 6 ------ sql/item_func.h | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/mysql-test/main/having_cond_pushdown.result b/mysql-test/main/having_cond_pushdown.result index 9b124296e3d..aade383a71f 100644 --- a/mysql-test/main/having_cond_pushdown.result +++ b/mysql-test/main/having_cond_pushdown.result @@ -3194,7 +3194,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3536,7 +3535,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1 and 1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3663,7 +3661,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3791,7 +3788,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "filesort": { "sort_key": "t1.c", "temporary_table": { @@ -3859,7 +3855,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1 and 1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3977,7 +3972,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t3", "access_type": "ALL", diff --git a/sql/item_func.h b/sql/item_func.h index b368e5872fe..2b02971d940 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -399,7 +399,7 @@ class Item_func :public Item_func_or_sum, With_sum_func_cache* get_with_sum_func_cache() { return this; } Item_func *get_item_func() { return this; } bool is_simplified_cond_processor(void *arg) - { return const_item() && !val_int(); } + { return const_item() /*&& !val_int();*/; } }; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support
by Sergey Petrunia 21 Jan '22

21 Jan '22

Hi Oleg, > commit 8e6595cff0caa22d2bb81002505e52b5c52f5ab2 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Tue Jan 11 20:25:11 2022 +0300 > > MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support ... > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 442644eddd6..74757970411 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -14121,6 +14122,15 @@ opt_describe_column: > } > ; > > +explain_for_connection: > + describe_command FOR_SYM CONNECTION_SYM expr > + { > + Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > + if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + MYSQL_YYABORT; > + add_value_to_list(thd, $4); > + } > + ; > This fails to intialize the statement properly. For SHOW EXPLAIN, the following is done at statement start: show: SHOW { LEX *lex=Lex; lex->wild=0; lex->ident= null_clex_str; if (Lex->main_select_push()) MYSQL_YYABORT; mysql_init_select(lex); lex->current_select->parsing_place= SELECT_LIST; lex->create_info.init(); } If you don't do it, EXPLAIN FOR CONNECTION may produce crashes (e.g. on my machine it was crash in JOIN::prepare due to select_lex->inner_sum_func_list=0xa5a5a5a5a5). The rest of the code looks ok. Let's have another review iteration once the aboveis fixed. I've also found one difference with MySQL, which I am not sure if we should fix: In MariaDB, if the target is not running something that has an query plan, SHOW EXPLAIN (and now EXPLAIN FOR CONNECTION) will produce this error: MariaDB [test]> show explain for 5; ERROR 1933 (HY000): Target is not running an EXPLAINable command while in MySQL it can be one of two: 1. Target is idle: mysql> explain for connection 8; Query OK, 0 rows affected (0.00 sec) 2. Target is running a command that's not supported: mysql> explain for connection 8; ERROR 3012 (HY000): EXPLAIN FOR CONNECTION command is supported only for SELECT/UPDATE/INSERT/DELETE/REPLACE I'm not sure we should try to imitate this behavior. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 673ea509e2c: MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
by Sergei Golubchik 21 Jan '22

21 Jan '22

Hi, Marko, On Jan 20, Marko Mäkelä wrote: > revision-id: 673ea509e2c (mariadb-10.2.40-230-g673ea509e2c) > parent(s): d28d3aee108 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2022-01-20 16:13:08 +0200 > message: > > MDEV-26870 --skip-symbolic-links does not disallow .isl file creation > > The InnoDB DATA DIRECTORY attribute is not implemented via > symbolic links but something similar, *.isl files that contain > the names of data files. > > InnoDB failed to reject the DATA DIRECTORY attribute even though > the server was started with --skip-symbolic-links. > > Because TRUNCATE TABLE cannot easily return an error, it will keep > rebuilding tables even if they carry the DATA DIRECTORY attribute. > > diff --git a/mysql-test/suite/encryption/t/innodb-first-page-read.test b/mysql-test/suite/encryption/t/innodb-first-page-read.test > index d661e4565d2..a0b3ca3f0ff 100644 > --- a/mysql-test/suite/encryption/t/innodb-first-page-read.test > +++ b/mysql-test/suite/encryption/t/innodb-first-page-read.test > @@ -1,6 +1,7 @@ > -- source include/have_innodb.inc > -- source include/have_file_key_management_plugin.inc > -- source include/not_embedded.inc > +-- source include/have_symlink.inc when can have_symlink be false? Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files. > --disable_warnings > SET GLOBAL innodb_file_format = `Barracuda`; > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 5d78e64a06b..a8b16ba1eb0 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -11710,9 +11710,15 @@ create_table_info_t::create_options_are_invalid() > break; > } > > - if (m_create_info->data_file_name > - && m_create_info->data_file_name[0] != '\0' > - && !create_option_data_directory_is_valid()) { > + if (!m_create_info->data_file_name > + || !m_create_info->data_file_name[0]) { > + } else if (!my_use_symdir) { > + push_warning( > + m_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_ILLEGAL_HA_CREATE_OPTION, > + "InnoDB: DATA DIRECTORY requires HAVE_SYMLINK."); > + ret = "DATA DIRECTORY"; MyISAM does push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, WARN_OPTION_IGNORED, ER_THD(thd, WARN_OPTION_IGNORED), "DATA DIRECTORY"); Archive does if (create_info->data_file_name) my_error(WARN_OPTION_IGNORED, MYF(ME_WARNING), "DATA DIRECTORY"); I suggest you do one of the above too. > + } else if (!create_option_data_directory_is_valid()) { > ret = "DATA DIRECTORY"; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
by Sergei Golubchik 20 Jan '22

20 Jan '22

Hi, Alexander! On Jan 13, Alexander Barkov wrote: > revision-id: 99e2a49acfc (mariadb-10.5.13-33-g99e2a49acfc) > parent(s): 2776635cb98 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-01-10 18:05:55 +0400 > message: > > MDEV-27018 IF and COALESCE lose "json" property > > Hybrid functions (IF, COALESCE, etc) did not preserve the JSON property > from their arguments. The same problem was repeatable for single row subselects. > > The problem happened because the method Item::is_json_type() was inconsistently > implemented across the Item hierarchy. For example, Item_hybrid_func > and Item_singlerow_subselect did not override is_json_type(). > > Solution: > > - Removing Item::is_json_type() > > - Implementing specific JSON type handlers: > Type_handler_string_json > Type_handler_varchar_json > Type_handler_tiny_blob_json > Type_handler_blob_json > Type_handler_medium_blob_json > Type_handler_long_blob_json > > - Reusing the existing data type infrastructure to pass JSON > type handlers across all item types, including classes Item_hybrid_func > and Item_singlerow_subselect. Note, these two classes themselves do not > need any changes! > > - Extending the data type infrastructure so data types can inherit > their properties (e.g. aggregation rules) from their base data types. > E.g. VARCHAR/JSON acts as VARCHAR, LONGTEXT/JSON acts as LONGTEXT > when mixed to a non-JSON data type. This is done by: > - adding virtual method Type_handler::type_handler_base() > - adding class Recursive_type_pair_iterator > - refactoring Type_handler_hybrid_field_type methods > aggregate_for_result(), aggregate_for_min_max(), > aggregate_for_num_op() to use Recursive_type_pair_iterator. > > This change also fixes: > > MDEV-27361 Hybrid functions with JSON arguments do not send format metadata > > diff --git a/sql/field.cc b/sql/field.cc > index 2c768527ced..8fa3bbd538c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7277,6 +7277,19 @@ bool Field_longstr::send(Protocol *protocol) > } > > > +const Type_handler *Field_string::type_handler() const > +{ > + if (is_var_string()) > + return &type_handler_var_string; shouldn't it be after json check? > + /* > + This is a temporary solution and will be fixed soon (in 10.9?). > + Type_handler_string_json will provide its own Field_string_json. > + */ > + if (Type_handler_json_common::has_json_valid_constraint(this)) > + return &type_handler_string_json; > + return &type_handler_string; > +} > + > /* Copy a string and fill with space */ > > int Field_string::store(const char *from, size_t length,CHARSET_INFO *cs) > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > index ddf5fc32ea4..81003be4656 100644 > --- a/sql/item_jsonfunc.cc > +++ b/sql/item_jsonfunc.cc > @@ -1441,6 +1441,32 @@ longlong Item_func_json_contains_path::val_int() > } > > > +/* > + This reproduces behavior according to the former > + Item_func_conv_charset::is_json_type() which returned args[0]->is_json_type(). > + JSON functions with multiple string input with different character sets > + wrap some arguments into Item_func_conv_charset. So the former > + Item_func_conv_charset::is_json_type() took the JSON propery from args[0], > + i.e. from the original argument before the conversion. > + This is probably not always correct because an *explicit* > + `CONVERT(arg USING charset)` is actually a general purpose string > + expression, not a JSON expression. > +*/ > +static bool is_json_type(const Item *item) > +{ > + for ( ; ; ) > + { > + if (Type_handler_json_common::is_json_type_handler(item->type_handler())) > + return true; > + const Item_func_conv_charset *func; > + if (!(func= dynamic_cast<const Item_func_conv_charset*>(item))) > + return false; > + item= func->arguments()[0]; can you have nested CONVERT()'s ? > + } > + return false; > +} > + > + > static int append_json_value(String *str, Item *item, String *tmp_val) > { > if (item->type_handler()->is_bool_type()) > diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h > index 6c4ee8cb2eb..4a394809a06 100644 > --- a/sql/sql_type_json.h > +++ b/sql/sql_type_json.h > @@ -21,18 +21,145 @@ ... > +template <class BASE, const Named_type_handler<BASE> &thbase> > +class Type_handler_general_purpose_string_to_json: > + public BASE, > + public Type_handler_json_common > { ... > + bool Item_hybrid_func_fix_attributes(THD *thd, > + const char *name, > + Type_handler_hybrid_field_type *hybrid, > + Type_all_attributes *attr, > + Item **items, uint nitems) > + const override > + { > + if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr, > + items, nitems)) > + return true; > + /* > + The above call can change the type handler on "hybrid", e.g. > + choose a proper BLOB type handler according to the calculated max_length. > + Convert general purpose string type handler to its JSON counterpart. > + This makes hybrid functions preserve JSON data types, e.g.: > + COALESCE(json_expr1, json_expr2) -> JSON > + */ > + hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler())); When this line would change hybrid->type_handler() ? > + return false; > + } > }; ..... > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index c1801c1ae3e..3b2753d80a6 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const > > /***************************************************************************/ > > +class Recursive_type_pair_iterator > +{ > + const Type_handler *m_a; > + const Type_handler *m_b; > + uint m_switched_to_base_count; > +public: > + Recursive_type_pair_iterator(const Type_handler *a, > + const Type_handler *b, > + uint switched_to_base_count= 0) > + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count) > + { } > + const Type_handler *a() const { return m_a; } > + const Type_handler *b() const { return m_b; } > + Recursive_type_pair_iterator base() const > + { > + Recursive_type_pair_iterator res(m_a->type_handler_base(), > + m_b->type_handler_base()); > + res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL); > + if (res.m_a == NULL) > + res.m_a= m_a; > + if (res.m_b == NULL) > + res.m_b= m_b; > + return res; that's an unusual semantics, not what I would expect from an iterator. It'd expect it to iterare with it++ or it.next() but not it = it.base() > + } > + bool done() const > + { > + switch (m_switched_to_base_count) > + { > + case 2: I don't think case 2 is possible anymore. ... > + } > + } > +}; > + > + > bool > Type_handler_hybrid_field_type::aggregate_for_result(const Type_handler *other) > { > - const Type_handler *hres; > - const Type_collection *c; > - if (!(c= Type_handler::type_collection_for_aggregation(m_type_handler, other)) || > - !(hres= c->aggregate_for_result(m_type_handler, other))) > - hres= type_handler_data-> > - m_type_aggregator_for_result.find_handler(m_type_handler, other); > - if (!hres) > - return true; > - m_type_handler= hres; > - return false; > + Recursive_type_pair_iterator it(m_type_handler, other); > + for ( ; ; ) do you really still need to do recursive? in what cases? > + { > + const Type_handler *hres; > + const Type_collection *c; > + if (((c= Type_handler::type_collection_for_aggregation(it.a(), it.b())) && > + (hres= c->aggregate_for_result(it.a(), it.b()))) || > + (hres= type_handler_data-> > + m_type_aggregator_for_result.find_handler(it.a(), it.b()))) > + { > + m_type_handler= hres; > + return false; > + } > + if ((it= it.base()).done()) > + break; > + } > + return true; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4