Re: [Maria-developers] a11756e4fba: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
Hi, Rucha! On Jul 15, Rucha Deodhar wrote:
revision-id: a11756e4fba (mariadb-10.5.11-27-ga11756e4fba) parent(s): f0f47cbca18 author: Rucha Deodhar <rucha.deodhar@mariadb.com> committer: Rucha Deodhar <rucha.deodhar@mariadb.com> timestamp: 2021-07-05 20:14:19 +0530 message:
MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the RETURNING clause, '*' is not resolved properly because of wrong context. context->table_list is NULL or has incorrect table because context->table_list has tables from the FROM clause. Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, use first_name_resolution_table for correct resolution of item.
please, add a couple of tests with a qualified asterisk, where it's qualified _not_ by the table you're inserting into. Like INSERT INTO t1(id1,val1) VALUES(14,'m') RETURNING t2.* not very interesting case, an obvious error ^^^ INSERT INTO t2 SELECT t1.* FROM t1 WHERE id1=2 RETURNING t1.*; but what will happen here? ^^^
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1476d708d75..60d14c6dbea 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8032,12 +8032,13 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, else treat natural joins as leaves and do not iterate over their underlying tables. */ - for (TABLE_LIST *tables= (table_name ? context->table_list : - context->first_name_resolution_table); + TABLE_LIST *tables= returning_field || !table_name ? context->first_name_resolution_table : + context->table_list; + for (; tables; - tables= (table_name ? tables->next_local : - tables->next_name_resolution_table) + tables= returning_field || !table_name ? tables->next_name_resolution_table : + tables->next_local ) { Field *field; TABLE *table= tables->table;
1. please also update the comment to mention RETURNING. 2. Could you apply the following patch as a separate commit before your fix: ============================ --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8098,12 +8098,14 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, else treat natural joins as leaves and do not iterate over their underlying tables. */ - for (TABLE_LIST *tables= (table_name ? context->table_list : - context->first_name_resolution_table); - tables; - tables= (table_name ? tables->next_local : - tables->next_name_resolution_table) - ) + TABLE_LIST *first= context->first_name_resolution_table; + TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; + if (table_name) + { + first= context->table_list; + next= &TABLE_LIST::next_local; + } + for (TABLE_LIST *tables= first; tables; tables= tables->*next) { Field *field; TABLE *table= tables->table; ============================ it'll remove the condition from inside the loop, will put it in one place only. And will make your change simpler and clearer. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Fri, Jul 16, 2021 at 10:42 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Rucha!
Hi, Sergei! Thanks for the review.
On Jul 15, Rucha Deodhar wrote:
revision-id: a11756e4fba (mariadb-10.5.11-27-ga11756e4fba) parent(s): f0f47cbca18 author: Rucha Deodhar <rucha.deodhar@mariadb.com> committer: Rucha Deodhar <rucha.deodhar@mariadb.com> timestamp: 2021-07-05 20:14:19 +0530 message:
MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the RETURNING clause, '*' is not resolved properly because of wrong context. context->table_list is NULL or has incorrect table because context->table_list has tables from the FROM clause. Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, use first_name_resolution_table for correct resolution of item.
please, add a couple of tests with a qualified asterisk, where it's qualified _not_ by the table you're inserting into. Like
INSERT INTO t1(id1,val1) VALUES(14,'m') RETURNING t2.*
not very interesting case, an obvious error ^^^
INSERT INTO t2 SELECT t1.* FROM t1 WHERE id1=2 RETURNING t1.*;
but what will happen here? ^^^
In both the cases above where the asterisk is not qualified by the table we are inserting into we get Unknown table error. Will add these to the test.
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1476d708d75..60d14c6dbea 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8032,12 +8032,13 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, else treat natural joins as leaves and do not iterate over their underlying tables. */ - for (TABLE_LIST *tables= (table_name ? context->table_list : - context->first_name_resolution_table); + TABLE_LIST *tables= returning_field || !table_name ? context->first_name_resolution_table : + context->table_list; + for (; tables; - tables= (table_name ? tables->next_local : - tables->next_name_resolution_table) + tables= returning_field || !table_name ? tables->next_name_resolution_table : + tables->next_local ) { Field *field; TABLE *table= tables->table;
1. please also update the comment to mention RETURNING. 2. Could you apply the following patch as a separate commit before your fix:
Will do.
============================ --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8098,12 +8098,14 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, else treat natural joins as leaves and do not iterate over their underlying tables. */ - for (TABLE_LIST *tables= (table_name ? context->table_list : - context->first_name_resolution_table); - tables; - tables= (table_name ? tables->next_local : - tables->next_name_resolution_table) - ) + TABLE_LIST *first= context->first_name_resolution_table; + TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; + if (table_name) + { + first= context->table_list; + next= &TABLE_LIST::next_local; + } + for (TABLE_LIST *tables= first; tables; tables= tables->*next) { Field *field; TABLE *table= tables->table; ============================
it'll remove the condition from inside the loop, will put it in one place only. And will make your change simpler and clearer.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Regards, Rucha Deodhar
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (2)
-
Rucha Deodhar -
Sergei Golubchik