Re: [Maria-developers] 6d881a271a9: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING

Hi, Rucha! On Oct 23, Rucha Deodhar wrote:

revision-id: 6d881a271a9 (mariadb-10.5.4-108-g6d881a271a9) parent(s): bbd70fcc43c author: Rucha Deodhar committer: Rucha Deodhar 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@mariadb.org