maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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