On Fri, Jul 16, 2021 at 10:42 PM Sergei Golubchik <[email protected]> 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 <[email protected]> > > committer: Rucha Deodhar <[email protected]> > > 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 [email protected] > > Regards, Rucha Deodhar > _______________________________________________ > Mailing list: https://launchpad.net/~maria-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~maria-developers > More help : https://help.launchpad.net/ListHelp >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
