maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> committer: Rucha Deodhar <rucha.deodhar@xxxxxxxxxxx>
> 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@xxxxxxxxxxx