- 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

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

31 Oct '21

Hi Serg, Ok the patch moving in the right direction but it's not there, yet. Please find the input below. 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). It is not clear how the checker interacts with Single_line_formatting_helper. The code like one the one I'm quoting below shows some non-trivial overlap: > @@ -28,7 +36,16 @@ void Json_writer::append_indent() > > void Json_writer::start_object() > { > +#ifndef NDEBUG > + DBUG_ASSERT(got_name == named_item_expected()); > + named_items_expectation.push_back(true); > + size_t depth= named_items_expectation.size(); > +#endif > fmt_helper.on_start_object(); > +#ifndef NDEBUG > + if (depth != named_items_expectation.size()) > + named_items_expectation.pop_back(); > +#endif > > if (!element_started) > start_element(); This looks very cryptic. Can you elaborate about this? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 1

Re: [Maria-developers] 85aa3f8cf66: MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+
by Sergei Golubchik 30 Oct '21

30 Oct '21

Hi, Anel! Looks ok. Except that you have that pointless $gcc_version=~ line that I wrote about in a previous review. Did you verify that this script actually delivers meaningful results? On Oct 30, Anel Husakovic wrote: > revision-id: 85aa3f8cf66 (mariadb-10.2.39-96-g85aa3f8cf66) > parent(s): afe00bb7cce > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-07-29 12:42:44 +0200 > message: > > MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+ > > Reviewed by: serg(a)mariadb.com > > --- > mysql-test/dgcov.pl | 55 +++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index 2c00c64d1ff..543099c1b4c 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -62,12 +62,15 @@ my $res; > + > +my $gcc_version= `gcc -dumpversion`; > +$gcc_version=~ s/(\d).*$/$1/; > > find(\&gcov_one_file, $root); > find(\&write_coverage, $root) if $opt_generate; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2c8f52ea53d: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 29 Oct '21

29 Oct '21

Hi, Seppo, Jan! this is pretty much the same as in 10.4, so same comments apply, otherwise ok. On Oct 29, sjaakola wrote: > revision-id: 2c8f52ea53d (mariadb-10.5.12-108-g2c8f52ea53d) > parent(s): 910fc72a7f1 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-28 12:00:41 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 778d88ea2d9: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 29 Oct '21

29 Oct '21

Hi, Seppo, Jan! Looks pretty much ok too. A few comments, see below On Oct 29, sjaakola wrote: > revision-id: 778d88ea2d9 (mariadb-10.4.21-76-g778d88ea2d9) > parent(s): e2e4e06f081 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-28 11:06:22 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index c1c753d69fd..a1562016aa4 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -8987,6 +8987,18 @@ bool Xid_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info) > > > #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT) > +static bool wsrep_must_replay(THD *thd) > +{ > +#ifdef WITH_WSREP > + mysql_mutex_lock(&thd->LOCK_thd_data); > + bool res= WSREP(thd) && thd->wsrep_trx().state() == wsrep::transaction::s_must_replay; check WSREP(thd) outside of the mutex > + mysql_mutex_unlock(&thd->LOCK_thd_data); > + return res; > +#else > + return false; > +#endif > +} > + > int Xid_log_event::do_apply_event(rpl_group_info *rgi) > { > bool res; > diff --git a/sql/service_wsrep.cc b/sql/service_wsrep.cc > index 6566bb372d8..18067a4d0ec 100644 > --- a/sql/service_wsrep.cc > +++ b/sql/service_wsrep.cc > @@ -29,12 +29,14 @@ extern "C" my_bool wsrep_on(const THD *thd) > > extern "C" void wsrep_thd_LOCK(const THD *thd) > { > + mysql_mutex_lock(&thd->LOCK_thd_kill); > mysql_mutex_lock(&thd->LOCK_thd_data); I presume, you can remove wsrep_thd_kill_LOCK() and wsrep_thd_kill_UNLOCK() that I've added > } > > extern "C" void wsrep_thd_UNLOCK(const THD *thd) > { > mysql_mutex_unlock(&thd->LOCK_thd_data); > + mysql_mutex_unlock(&thd->LOCK_thd_kill); > } > > extern "C" void wsrep_thd_kill_LOCK(const THD *thd) > @@ -295,8 +301,6 @@ extern "C" my_bool wsrep_thd_is_aborting(const MYSQL_THD thd) > default: > return false; > } > - > - return false; this looks like it begs for the compiler warning. please, keep the return where it was. > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 002192b92cf: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 27 Oct '21

27 Oct '21

Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: 002192b92cf (mariadb-10.3.31-60-g002192b92cf) > parent(s): 736da112b65 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:15:48 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This 10.3 version is also ok to push when you have 10.4 and 10.5 versions ready. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d0e18d694f0: MDEV-23328 Server hang due to Galera lock conflict resolution
by Sergei Golubchik 27 Oct '21

27 Oct '21

Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: d0e18d694f0 (mariadb-10.2.40-128-gd0e18d694f0) > parent(s): 408277e8012 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:12:53 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This commit is ok to push, thanks! Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0