November 2021 - developers - lists.mariadb.org

Re: [Maria-developers] MDEV-23766: Optimizer tracing code is prone to producing invalid JSON
by Sergey Petrunia 05 Nov '21

05 Nov '21

Hi Serg, On Tue, Nov 02, 2021 at 02:19:05AM +0200, Sergei Krivonos wrote: > Applied fixes by your tests patch 638930f85f… <https://github.com/MariaDB/server/commit/638930f85ff40bd39354da81e53e7278a4…> > #1: Please check out the commit adding unit tests: http://lists.askmonty.org/pipermail/commits/2021-November/014773.html It turns out it is not that hard to add them. Any comments/ways to improve this? #2: In the above commit, note that two last unit tests show a scenario that is not currently checked. #3: What is the point of functions Json_writer::on_add_str(), Json_writer::on_start_array, Json_writer::on_start_object() ? I would understand if all the checking code was isolated in these functions. But it isn't. Please either remove the functions or explain the reasoning. #4 See the input below about the commit comment. Please change (feel free to force-push into the branch after that). I think this is all the input, and the patch will be ready for pushing once all of this is addressed. > > Item #2: please improve the commit comment as was requested in my previous > > e-mail: > > > > On Thu, Oct 28, 2021 at 04:58:35PM +0300, Sergey Petrunia wrote: > >> First, input on the commit comment. Please consider it as a request applying > >> to ALL further commits to the MariaDB codebase. > >> > >> > >>> MDEV-23766: implemented requested debug checks > >> > >> Imagine somebody looking at this in a few years. Will they know what checks > >> were requested? They might get a clue by looking at the Jira text, but the > >> idea to make the commit comments concise and self-contained descriptions. > >> > >> Something like this: > >> > >> Line #1: one-line description of the patch: > >> > >>>> MDEV-23766: Make attempts to write invalid JSON cause assertion failures > >> > >> Subsequent lines: > >> > >>>> Make JSON writing API (class Json_writer) check the produced JSON document > >>>> is valid. The following checks are made: > >>>> - JSON objects must contain named members > >>>> - JSON arrays must contain unnamed members. > >>>> - (TODO: add other restrictions we're enforcing). BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 80749146b7d: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
by Sergei Golubchik 04 Nov '21

04 Nov '21

Hi, Rucha! On Nov 04, Rucha Deodhar wrote: > revision-id: 80749146b7d (mariadb-10.6.1-77-g80749146b7d) > parent(s): 76149650764 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-09-12 02:11:19 +0530 > message: > > MDEV-26238: Remove inconsistent behaviour of --default-* options > in my_print_defaults > > Analysis: --defaults* option is recognized anywhere in the commandline > instead of only at the beginning because handle_options() recognizes > options in any order. > Fix: use get_defaults_options() which recognizes --defaults* options only at > the beginning. After this is done, we only want to recognize other options > given in any order which can be done using handle_options(). So only skip > --defaults* options and pass rest of them to handle_options(). > Also, removed -e, -g and -c because only my_print_defaults supports them. > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c > index b7f52382721..607509a309e 100644 > --- a/extra/my_print_defaults.c > +++ b/extra/my_print_defaults.c > @@ -141,50 +124,39 @@ static int get_options(int *argc,char ***argv) ... > int main(int argc, char **argv) > { > - int count= 0, error, no_defaults= 0; > + int count, error, args_used; > char **load_default_groups= 0, *tmp_arguments[6]; > char **argument, **arguments, **org_argv; > int nargs, i= 0; > MY_INIT(argv[0]); > > org_argv= argv; > - if (*argv && !strcmp(*argv, "--no-defaults")) > - { > - argv++; > - ++count; > - no_defaults= 1; > - } > - /* Copy program name and --no-defaults if present*/ > + args_used= get_defaults_options(argv); > + > + /* Copy defaults-xxx arguments & program name */ > + count= args_used; > arguments= tmp_arguments; > - memcpy((char*) arguments, (char*) org_argv, (++count)*sizeof(*org_argv)); > + memcpy((char*) arguments, (char*) org_argv, count*sizeof(*org_argv)); > arguments[count]= 0; I still cannot understand why you're doing it. Why do you need a copy of argv? > > + /* > + We already process --defaults* options at the beginning in > + get_defaults_options(). So skip --defaults* options and > + pass remaining options to handle_options(). > + */ > + if (is_prefix(org_argv[1], "--no-defaults")) > + args_used--; I'm sorry, I don't understand. Why do you need that? all *defaults* options were handled in get_defaults_options(). > + why do you need org_argv? First you do org_argv= argv; then after some code that doesn't change either argv or org_argv, you copy the vallue back: > + argv=org_argv; > + argv+=args_used-1; > + argc-=args_used-1; > + > /* Check out the args */ > if (get_options(&argc,&argv)) > cleanup_and_exit(1); > > - if (!no_defaults) > - { > - if (opt_defaults_file_used) > - arguments[count++]= make_args("--defaults-file=", config_file); > - if (my_defaults_extra_file) > - arguments[count++]= make_args("--defaults-extra-file=", > - my_defaults_extra_file); > - if (my_defaults_group_suffix) > - arguments[count++]= make_args("--defaults-group-suffix=", > - my_defaults_group_suffix); > - arguments[count]= 0; > - } > - > nargs= argc + 1; > if (opt_mysqld) > nargs+= array_elements(mysqld_groups); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] [MDEV-10654] Review request
by Manohar K B 04 Nov '21

04 Nov '21

Hi All, Good day. I have fixed ticket MDEV-10654 and raised PR. Can someone kindly review it? This is the first time I am contributing to MariaDB, so please guide me if I missed anything. Best regards, Manohar.

2 1

Re: [Maria-developers] bb1737b0482: MDEV-12459: The information_schema tables for getting temporary tables info is missing, at least for innodb there is no INNODB_TEMP_TABLE_INFO
by Sergei Golubchik 01 Nov '21

01 Nov '21

Hi, Anel! On Nov 01, Anel Husakovic wrote: > revision-id: bb1737b0482 (mariadb-10.6.1-73-gbb1737b0482) > parent(s): 1bc82aaf0a7 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-09-06 08:58:46 +0200 > message: > > MDEV-12459: The information_schema tables for getting temporary tables info is missing, at least for innodb there is no INNODB_TEMP_TABLE_INFO > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 6b351726320..5444de8c1ca 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -69,6 +69,8 @@ > > #include "lex_symbol.h" > #define KEYWORD_SIZE 64 > +#define IS_USER_TEMP_TABLE(A) ((A->tmp_table == TRANSACTIONAL_TMP_TABLE) || \ > + (A->tmp_table == NON_TRANSACTIONAL_TMP_TABLE)) > > extern SYMBOL symbols[]; > extern size_t symbols_length; > @@ -151,6 +153,8 @@ static int show_create_sequence(THD *thd, TABLE_LIST *table_list, > static const LEX_CSTRING *view_algorithm(TABLE_LIST *table); > > bool get_lookup_field_values(THD *, COND *, TABLE_LIST *, LOOKUP_FIELD_VALUES *); > +void process_i_s_table_temporary_tables(THD *thd, TABLE * table, LEX_CSTRING *db_name, > + LEX_CSTRING *table_name, TABLE *tmp_tbl); > > /** > Try to lock a mutex, but give up after a short while to not cause deadlocks > @@ -5161,6 +5166,10 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > uint table_open_method= tables->table_open_method; > bool can_deadlock; > MEM_ROOT tmp_mem_root; > + Dynamic_array<LEX_CSTRING> system_tables(PSI_INSTRUMENT_MEM); > + All_tmp_tables_list *temp_tables= NULL; > + // Scan for temporary tables > + TMP_TABLE_SHARE *share_temp; > DBUG_ENTER("get_all_tables"); > > bzero(&tmp_mem_root, sizeof(tmp_mem_root)); > @@ -5223,6 +5232,12 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > init_alloc_root(PSI_INSTRUMENT_ME, &tmp_mem_root, SHOW_ALLOC_BLOCK_SIZE, > SHOW_ALLOC_BLOCK_SIZE, MY_THREAD_SPECIFIC); > > + system_tables.push(INFORMATION_SCHEMA_NAME); > + system_tables.push(PERFORMANCE_SCHEMA_DB_NAME); > + system_tables.push(MYSQL_SCHEMA_NAME); > + system_tables.push(SYS_SCHEMA_NAME); > + system_tables.push(MTR_SCHEMA_NAME); > + > for (size_t i=0; i < db_names.elements(); i++) > { > LEX_CSTRING *db_name= db_names.at(i); > @@ -5234,6 +5249,36 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > sctx->master_access & (DB_ACLS | SHOW_DB_ACL) || > acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name->str, 0)) > #endif > + > + // Only if there is IS.tables allow temporary tables to be shown > + if (schema_table_idx == SCH_TABLES && !temp_tables) > + { > + for (size_t k=0; k < system_tables.elements(); k++) > + { > + if (db_name != &system_tables.at(k)) > + { > + temp_tables= open_tables_state_backup.temporary_tables; > + if (temp_tables) > + { > + All_tmp_tables_list::Iterator it(*open_tables_state_backup.temporary_tables); > + while ((share_temp= it++)) > + { > + All_share_tables_list::Iterator it2(share_temp->all_tmp_tables); > + while (TABLE *tbl= it2++) > + { > + if (IS_USER_TEMP_TABLE(share_temp)) > + { > + // Now we have the data and we should process_table() manually > + process_i_s_table_temporary_tables(thd, table, db_name, > + &share_temp->table_name, tbl); > + } > + } > + } > + } > + break; > + } > + } > + } hmmmm. What do you do here? I understand, basically, nothing here. Could you explain, line by line? > { > Dynamic_array<LEX_CSTRING*> table_names(PSI_INSTRUMENT_MEM); > int res= make_table_name_list(thd, &table_names, lex, > @@ -5495,6 +5540,8 @@ static int get_schema_tables_record(THD *thd, TABLE_LIST *tables, > table->field[3]->store(STRING_WITH_LEN("SYSTEM VIEW"), cs); > else if (share->table_type == TABLE_TYPE_SEQUENCE) > table->field[3]->store(STRING_WITH_LEN("SEQUENCE"), cs); > + else if (IS_USER_TEMP_TABLE(share)) > + table->field[3]->store(STRING_WITH_LEN("TEMPORARY"), cs); what about INTERNAL_TMP_TABLE and SYSTEM_TMP_TABLE? > else > { > DBUG_ASSERT(share->tmp_table == NO_TMP_TABLE); > @@ -9032,7 +9096,6 @@ ST_FIELD_INFO tables_fields_info[]= > NOT_NULL, "Comment", OPEN_FRM_ONLY), > Column("MAX_INDEX_LENGTH",ULonglong(), NULLABLE, "Max_index_length", > OPEN_FULL_TABLE), > - Column("TEMPORARY", Varchar(1), NULLABLE, "Temporary", OPEN_FRM_ONLY), > CEnd() > }; Let's remove it in a separate commit. After you implement "TEMPORARY" for table type. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0