- developers - lists.mariadb.org

Re: [Maria-developers] caec6b4b3d7: MDEV-17573 Assertion in federatedx on multi-update
by Sergei Golubchik 24 Oct '20

24 Oct '20

Hi, Aleksey! Looks reasonable to me. MYSQL_RES is stored in the handler. Of course, MYSQL_ROWS *current, which is a pointer into MYSQL_RES, has to be in the handler, it cannot be shared. Ok to push, thanks! On Oct 23, Aleksey Midenkov wrote: > revision-id: caec6b4b3d7 (mariadb-10.3.21-250-gcaec6b4b3d7) > parent(s): a00c668ddf2 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-10-21 13:38:37 +0300 > message: > > MDEV-17573 Assertion in federatedx on multi-update > > Cause: shared federatedx_io cannot store table-specific data. > > Fix: move current row reference `federatedx_io_mysql::current` to > ha_federatedx. > > FederatedX connection (represented by federatedx_io) is stored into > federatedx_txn::txn_list of per-server connections (see > federatedx_txn::acquire()). federatedx_txn object is stored into THD > (see ha_federatedx::external_lock()). When multiple handlers acquire > FederatedX connection they get single federatedx_io instance. Multiple > handlers do their operation via federatedx_io_mysql::mark_position() > and federatedx_io_mysql::fetch_row() in arbitrarty manner. They access > the same federatedx_io_mysql instance and same MYSQL_ROWS *current > pointer, so one handler disrupts the work of the other. > > Related to "MDEV-14551 Can't find record in table on multi-table update > with ORDER BY". Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2e0bac4688a: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! On Oct 23, Anel Husakovic wrote: > revision-id: 2e0bac4688a (mariadb-10.2.31-517-g2e0bac4688a) > parent(s): 985ede92034 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 12:38:11 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/r/grant5.result b/mysql-test/r/grant5.result > index 338bbd36e14..a69070e8399 100644 > --- a/mysql-test/r/grant5.result > +++ b/mysql-test/r/grant5.result ... > +SHOW GRANTS; > +Grants for root@localhost > +GRANT test_role TO 'root'@'localhost' WITH ADMIN OPTION > +GRANT ALL PRIVILEGES ON *.* TO 'root'@'localhost' WITH GRANT OPTION > +GRANT PROXY ON ''@'%' TO 'root'@'localhost' WITH GRANT OPTION > +GRANT USAGE ON *.* TO 'test_role' > +SET DEFAULT ROLE test_role FOR 'root'@'localhost' ... Yes, that's what I mean. I don't have a strong opinion on that, it seems that I very slightly prefer to have SET DEFAULT ROLE after GRANT PROXY, that is to have all statements for root, and then statements for roles. But having SET DEFAULT ROLE at the end makes sense too - all GRANTs first, then SET DEFAULT ROLE. Just make it the way you want. But either way it's good to have a test for it, a test that shows we've considered this issue and intentionally implemented that particular order. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6d881a271a9: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Rucha! On Oct 23, Rucha Deodhar wrote: > revision-id: 6d881a271a9 (mariadb-10.5.4-108-g6d881a271a9) > parent(s): bbd70fcc43c > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-10-23 12:57:02 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45ce4be3eb5..4ce7b034029 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -7959,7 +7959,7 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > bool any_privileges, uint *hidden_bit_fields) > { > Field_iterator_table_ref field_iterator; > - bool found; > + bool found, is_insert_or_replace_returning= false; > char name_buff[SAFE_NAME_LEN+1]; > DBUG_ENTER("insert_fields"); > DBUG_PRINT("arena", ("stmt arena: %p",thd->stmt_arena)); > @@ -7978,13 +7978,31 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > > found= FALSE; > > + if ((thd->lex->sql_command == SQLCOM_INSERT_SELECT || > + thd->lex->sql_command == SQLCOM_REPLACE_SELECT || > + thd->lex->sql_command == SQLCOM_INSERT || > + thd->lex->sql_command == SQLCOM_REPLACE) && > + thd->lex->has_returning()) > + is_insert_or_replace_returning= true; > + > /* > If table names are qualified, then loop over all tables used in the query, > else treat natural joins as leaves and do not iterate over their underlying > tables. > + Also, When we have INSERT/REPLACE...RETURNING and have qualified asterisk, > + table_name is not NULL and context->table_list is either NULL or has > + incorrect reference because context->table_list has tables from the > + FROM clause. > + context->table_list has incorrect reference (has table from the FROM clause > + instead of table we are inserting into) for > + INSERT/REPLACE...SELECT...RETURNING because we have a FROM clause from the > + SELECT statement. For INSERT/REPLACE...RETURNING it is NULL because > + there is no FROM clause. > */ > - for (TABLE_LIST *tables= (table_name ? context->table_list : > - context->first_name_resolution_table); > + for (TABLE_LIST *tables= (table_name ? (is_insert_or_replace_returning ? > + context->first_name_resolution_table : > + context->table_list) : > + (context->first_name_resolution_table)); ok, I've compiled it to run few queries, now I see that this is wrong. you need to understand the old code and the old comment before modifying them. one way to do it is to change the condition from table_name ? context->table_list : context->first_name_resolution_table to simply context->first_name_resolution_table and see what breaks. You'll see that main.type_varchar will break with At line 319: query 'SELECT t1.* FROM t1 LEFT JOIN t2 USING (c1)' failed: 1051: Unknown table 'test.t1' indeed, t1 is only present in context->table_list, but not in context->first_name_resolution_table. So after your fix the following statement will no longer work: INSERT INTO t2 SELECT t1.* FROM t1 JOIN t1 AS t USING (id1) WHERE id1=1 RETURNING *; it's because you set is_insert_or_replace_returning both for the SELECT and for the RETURNING clauses. > tables; > tables= (table_name ? tables->next_local : > tables->next_name_resolution_table) also this looks suspicious. Normally one starts from table_list and follows the next_local pointer or one starts from first_name_resolution_table and follows the next_name_resolution_table pointer. Your change creates a strange mix of both, it looks like it would be wrong. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 581857bd999: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! Looks good, thanks! A couple of small comments below. On Oct 23, Anel Husakovic wrote: > revision-id: 581857bd999 (mariadb-10.1.43-314-g581857bd999) let's fix it in 10.2 and up, we're doing the very last 10.1 release, so better be extra-super-conservative about it. > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 00:20:53 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 74a69952124..bf97a930097 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -52,6 +52,23 @@ disconnect u1; > drop user u1@localhost; > drop database mysqltest1; > > +# > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE test_role; oops, sorry, my mistake. Please, add `SET ROLE test_role` before SHOW GRANTS. The goal is to see that SET DEFAULT ROLE is shown *after* all grants to root@localhost, but before any grants to test_role. > +SHOW GRANTS; > +SET DEFAULT ROLE NONE; > +SHOW GRANTS; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > # End of 10.1 tests > # > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..6c852976061 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,15 +8007,44 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *hostname, > + ACL_USER *acl_entry, char *buff, size_t buffsize) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING def_rolename= acl_entry->default_rolename; > + > + if (def_rolename.length) > + { > + String def_str(buff, buffsize, system_charset_info); > + def_str.length(0); > + def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(acl_entry->host.hostname, acl_entry->hostname_length, > + system_charset_info); > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; looks good. The hostname argument seems to be unused now, doesn't it? > +} > + > -static bool show_role_grants(THD *thd, const char *username, > - const char *hostname, ACL_USER_BASE *acl_entry, > +static bool show_role_grants(THD *thd, const char *hostname, > + ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > { > uint counter; > Protocol *protocol= thd->protocol; > LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > > - String grant(buff,sizeof(buff),system_charset_info); > + String grant(buff, buffsize, system_charset_info); ouch, indeed. thanks! > for (counter= 0; counter < acl_entry->role_grants.elements; counter++) > { > grant.length(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 01612f594b3: MDEV-23769: MTR can abort before it prints the test result summary
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thanks for the review. > > Having warning/error is a necessary condition. > Sufficient condition is to call at least 2 tests. > Incorrect behaviour can be seen if: > - Both tests are correct but there is error generated (incorrect option > passed): > So this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st` will > generate and write statistic at the end, > but this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st 1st > --force` will not generate the statistic. > Note: warning is generated (--mysqld=--lock-wait-timeout=-1) statistic > will be generated Okay, thanks. This would be useful to show in the commit comment, that one can use ./mtr --mysqld=--lock-wait-timeout=-xx 1st 1st --force --parallel 1 to repeat the error. But then I don't understand why this happens. Why one failure is not enough? Why --parallel 2 destroys the effect? Apparently, it's check_warnings_post_shutdown() that causes that, not exit(1). One can change to exit(0) or remove exit(1) completely - the bug will still happen. Why check_warnings_post_shutdown() writing to the master server causes this bug? It looks rather fishy that in your fix it's the worker process that prints the statistics, not the master. Btw, note that check_warnings_post_shutdown() doesn't have any return statement, it very implicitly return "number of bytes sent" - it would be easier to read if it'd return something explicitly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 99c14476251: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > revision-id: 99c14476251 (mariadb-10.1.43-314-g99c14476251) > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-22 16:49:28 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 74a69952124..8756648c3e1 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -53,5 +53,19 @@ drop user u1@localhost; > drop database mysqltest1; > > # > -# End of 10.1 tests > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; > +SHOW GRANTS; The point is to run `SHOW GRANTS` for a user with a default role. E.g. SET DEFAULT ROLE test_role; SHOW GRANTS; SET DEFAULT ROLE NONE; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > +# End of 10.1 tests > +# > \ No newline at end of file new line at the end of file? > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..f42f7bdcfaf 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,6 +8007,36 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *hostname, > + ACL_USER *acl_entry, char *buff) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > + > + LEX_STRING def_rolename= acl_entry->default_rolename; > + if (def_rolename.length) > + { > + String def_str(buff,sizeof(buff),system_charset_info); eh? sizeof(buff) is sizeof(char*) that is usually 8 on 64-bit, 4 on 32-bit architecture. You want the size of the buffer here, not the size of the pointer to the buffer. > + def_str.length(0); > + def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(acl_entry->user.str, acl_entry->user.length, > + system_charset_info); again, just as above, you can use def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(&host); why not to use def_str.append(acl_entry->host.hostname, acl_entry->hostname_length, system_charset_info); > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6729bf5c031: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thank you for your review. > How is easier for you (or to ask a general question: how does the review > process go?) regarding the commits? > Is it better to add a new commit as a review commit and if the patch gets > accepted to squash after, or to rebase commits and send only a single > commit again for the review? One single commit is better, thanks. At least for small patches. > Addressed in the new patch: > https://github.com/MariaDB/server/commit/99c144762514d4dea3b6ec395de96aaf10… Thanks! I'll reply to the commit directly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 83614913d9a: MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Aleksey! On Oct 22, Aleksey Midenkov wrote: > revision-id: 83614913d9a (mariadb-10.4.11-291-g83614913d9a) > parent(s): efb1023b6f4 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 16:44:13 +0200 > message: > > MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 8989a918c0c..f287cfddcd2 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -879,9 +879,12 @@ void partition_info::vers_set_hist_part(THD *thd) > } > return; > warn: > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name); > + if (vers_info->hist_part->warn_vers_part_full) > + { > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name); > + } would it make sense to put warn_vers_part_full into vers_info, instead of storing it per-partition? > } > > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2830b9c290e: MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Dmitry! On Oct 22, Dmitry Shulga wrote: > revision-id: 2830b9c290e (mariadb-10.2.31-489-g2830b9c290e) > parent(s): 8984d779103 > author: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > committer: Dmitry Shulga <dmitry.shulga(a)mariadb.com> > timestamp: 2020-10-22 00:42:10 +0700 > message: > > MDEV-23926: Fix warnings generated during compilation of plugin/auth_pam/mapper/pam_user_map.c on MacOS > > Compiler warnings like one listed below are generated during server build on MacOS: > > > --- > plugin/auth_pam/CMakeLists.txt | 26 ++++++++++++++++++++++++++ > plugin/auth_pam/config.h.cmake | 1 + > plugin/auth_pam/mapper/pam_user_map.c | 30 +++++++++++++++++++----------- > 3 files changed, 46 insertions(+), 11 deletions(-) Nice! Could you also - in a second follow-up commit, not to confuse with the pure bug fix - do a small cleanup and move all other checks from plugin/auth_pam/CMakeLists.txt into your new config.h.cmake ? Currently they're added to the command line, like IF(HAVE_PAM_EXT_H) ADD_DEFINITIONS(-DHAVE_PAM_EXT_H) ENDIF() but as you've introduced config.h.cmake, there's no need to put them all on the command line anymore. You'll need to include config_auth_pam.h into more source files. And, I suppose. add config_auth_pam.h to .gitignore too. ok to push these two commits when done. Thanks! Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 65af1d07771: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Rucha! On Oct 22, Rucha Deodhar wrote: > revision-id: 65af1d07771 (mariadb-10.5.4-108-g65af1d07771) > parent(s): bbd70fcc43c > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-08-06 17:23:49 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Anaylsis: When we have INSERT/REPLACE...RETURNING and have qualified asterisk, > table_name is not NULL and context->table_list is either NULL or has > incorrect reference because context->table_list has tables from the FROM clause. > context->table_list has incorrect reference (has table from the FROM clause > instead of table we are inserting into) for INSERT/REPLACE...SELECT...RETURNING > because we have a FROM clause from the SELECT statement. > For INSERT/REPLACE...RETURNING it is NULL because there is no FROM clause. > Fix: If table_name is not null, check if we have INSERT/REPLACE...RETURNING. > If so, the reference should be the table we are inserting into and not > the table in the FROM clause. > > diff --git a/mysql-test/main/insert_returning.result b/mysql-test/main/insert_returning.result > index e664e02bedc..e1bab6f0329 100644 > --- a/mysql-test/main/insert_returning.result > +++ b/mysql-test/main/insert_returning.result > @@ -89,6 +89,9 @@ total val1 id1 && id1 id1 UPPER(val1) f(id1) good tests! > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45ce4be3eb5..4ce7b034029 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -7959,7 +7959,7 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > bool any_privileges, uint *hidden_bit_fields) > { > Field_iterator_table_ref field_iterator; > - bool found; > + bool found, is_insert_or_replace_returning= false; > char name_buff[SAFE_NAME_LEN+1]; > DBUG_ENTER("insert_fields"); > DBUG_PRINT("arena", ("stmt arena: %p",thd->stmt_arena)); > @@ -7978,13 +7978,31 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > > found= FALSE; > > + if ((thd->lex->sql_command == SQLCOM_INSERT_SELECT || > + thd->lex->sql_command == SQLCOM_REPLACE_SELECT || > + thd->lex->sql_command == SQLCOM_INSERT || > + thd->lex->sql_command == SQLCOM_REPLACE) && > + thd->lex->has_returning()) > + is_insert_or_replace_returning= true; What about INSERT t1 SELECT t2.* FROM t2 RETURNING t1.*; can your is_insert_or_replace_returning distinguish between the first and the second asterisk? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0