revision-id: a4b84122438579e96f7aa7aabf19eca7afa9e88b (mariadb-10.5.2-589-ga4b84122438) parent(s): 36c3be336616cb95443c5164a0e082862c6270d0 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2021-04-16 23:55:17 +0300 message: MDEV-25202: JSON_TABLE: Early table reference leads to unexpected result set Address review input: switch Name_resolution_context::ignored_tables from table_map to a list of TABLE_LIST objects. The rationale is that table bits may be changed due to query rewrites, etc, which may potentially require updating ignored_tables. --- mysql-test/suite/json/r/json_table.result | 17 ++++ mysql-test/suite/json/t/json_table.test | 13 ++++ sql/item.cc | 15 ++++ sql/item.h | 7 +- sql/json_table.cc | 125 ++++++++++++++++++------------ sql/json_table.h | 7 +- sql/sql_base.cc | 19 +++-- sql/sql_base.h | 5 +- 8 files changed, 147 insertions(+), 61 deletions(-) diff --git a/mysql-test/suite/json/r/json_table.result b/mysql-test/suite/json/r/json_table.result index 6cb197890c1..a996401496f 100644 --- a/mysql-test/suite/json/r/json_table.result +++ b/mysql-test/suite/json/r/json_table.result @@ -790,6 +790,23 @@ ON t1.a = tt.b; a b c o 1 1 {} 1 2 2 [] NULL +prepare s from +"SELECT * +FROM + t1 RIGHT JOIN + v2 AS tt + LEFT JOIN + JSON_TABLE(CONCAT(tt.c,''), '$' COLUMNS(o FOR ORDINALITY)) AS jt + ON tt.b = jt.o + ON t1.a = tt.b"; +execute s; +a b c o +1 1 {} 1 +2 2 [] NULL +execute s; +a b c o +1 1 {} 1 +2 2 [] NULL DROP VIEW v2; DROP TABLE t1, t2; # diff --git a/mysql-test/suite/json/t/json_table.test b/mysql-test/suite/json/t/json_table.test index 41233c8a8ea..39c97f73496 100644 --- a/mysql-test/suite/json/t/json_table.test +++ b/mysql-test/suite/json/t/json_table.test @@ -682,6 +682,19 @@ FROM ON tt.b = jt.o ON t1.a = tt.b; +prepare s from +"SELECT * +FROM + t1 RIGHT JOIN + v2 AS tt + LEFT JOIN + JSON_TABLE(CONCAT(tt.c,''), '$' COLUMNS(o FOR ORDINALITY)) AS jt + ON tt.b = jt.o + ON t1.a = tt.b"; +execute s; +execute s; + + DROP VIEW v2; DROP TABLE t1, t2; diff --git a/sql/item.cc b/sql/item.cc index 18bb26e18eb..6b593925369 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -10704,3 +10704,18 @@ bool Item::cleanup_excluding_immutables_processor (void *arg) return false; } } + + +bool ignored_list_includes_table(ignored_tables_list_t list, TABLE_LIST *tbl) +{ + if (!list) + return false; + List_iterator<TABLE_LIST> it(*list); + TABLE_LIST *list_tbl; + while ((list_tbl = it++)) + { + if (list_tbl == tbl) + return true; + } + return false; +} diff --git a/sql/item.h b/sql/item.h index 747f366e33d..4fdac92f357 100644 --- a/sql/item.h +++ b/sql/item.h @@ -162,6 +162,9 @@ void dummy_error_processor(THD *thd, void *data); void view_error_processor(THD *thd, void *data); +typedef List<TABLE_LIST>* ignored_tables_list_t; +bool ignored_list_includes_table(ignored_tables_list_t list, TABLE_LIST *tbl); + /* Instances of Name_resolution_context store the information necessary for name resolution of Items and other context analysis of a query made in @@ -236,7 +239,7 @@ struct Name_resolution_context: Sql_alloc Bitmap of tables that should be ignored when doing name resolution. Normally it is {0}. Non-zero values are used by table functions. */ - table_map ignored_tables; + ignored_tables_list_t ignored_tables; /* Security context of this name resolution context. It's used for views @@ -247,7 +250,7 @@ struct Name_resolution_context: Sql_alloc Name_resolution_context() :outer_context(0), table_list(0), select_lex(0), error_processor_data(0), - ignored_tables(0), + ignored_tables(NULL), security_ctx(0) {} diff --git a/sql/json_table.cc b/sql/json_table.cc index a16adc25f08..c79abf942fa 100644 --- a/sql/json_table.cc +++ b/sql/json_table.cc @@ -44,9 +44,14 @@ static table_function_handlerton table_function_hton; /* @brief - Collect a bitmap of tables that a given table function cannot have + Collect a set of tables that a given table function cannot have references to. + @param + table_func The table function we are connecting info for + join_list The nested join to be processed + disallowed_tables Collect the tables here. + @detail According to the SQL standard, a table function can refer to any table that's "preceding" it in the FROM clause. @@ -80,16 +85,16 @@ static table_function_handlerton table_function_hton; we are ok with operating on the tables "in the left join order". @return - TRUE - enumeration has found the Table Function instance. The bitmap is - ready. - FALSE - Otherwise - + 0 - Continue + 1 - Finish the process, success + -1 - Finish the process, failure */ static -bool get_disallowed_table_deps_for_list(table_map table_func_bit, - List<TABLE_LIST> *join_list, - table_map *disallowed_tables) +int get_disallowed_table_deps_for_list(MEM_ROOT *mem_root, + TABLE_LIST *table_func, + List<TABLE_LIST> *join_list, + List<TABLE_LIST> *disallowed_tables) { TABLE_LIST *table; NESTED_JOIN *nested_join; @@ -99,23 +104,25 @@ bool get_disallowed_table_deps_for_list(table_map table_func_bit, { if ((nested_join= table->nested_join)) { - if (get_disallowed_table_deps_for_list(table_func_bit, - &nested_join->join_list, - disallowed_tables)) - return true; + int res; + if ((res= get_disallowed_table_deps_for_list(mem_root, table_func, + &nested_join->join_list, + disallowed_tables))) + return res; } else { - *disallowed_tables |= table->table->map; - if (table_func_bit == table->table->map) + if (disallowed_tables->push_back(table, mem_root)) + return -1; + if (table == table_func) { // This is the JSON_TABLE(...) that are we're computing dependencies // for. - return true; + return 1; // Finish the processing } } } - return false; + return 0; // Continue } @@ -129,19 +136,31 @@ bool get_disallowed_table_deps_for_list(table_map table_func_bit, See get_disallowed_table_deps_for_list @return - Bitmap of tables that table function can NOT have references to. + NULL - Out of memory + Other - A list of tables that the function cannot have references to. May + be empty. */ static -table_map get_disallowed_table_deps(JOIN *join, table_map table_func_bit) +List<TABLE_LIST>* get_disallowed_table_deps(MEM_ROOT *mem_root, + SELECT_LEX *select, + TABLE_LIST *table_func) { - table_map disallowed_tables= 0; - if (!get_disallowed_table_deps_for_list(table_func_bit, join->join_list, - &disallowed_tables)) - { - // We haven't found the table with table_func_bit in all tables? - DBUG_ASSERT(0); - } + List<TABLE_LIST> *disallowed_tables; + + if (!(disallowed_tables = new List<TABLE_LIST>)) + return NULL; + + int res= get_disallowed_table_deps_for_list(mem_root, table_func, + select->join_list, + disallowed_tables); + + // The collection process must have finished + DBUG_ASSERT(res != 0); + + if (res == -1) + return NULL; // Out of memory + return disallowed_tables; } @@ -1034,48 +1053,56 @@ bool push_table_function_arg_context(LEX *lex, MEM_ROOT *alloc) Perform name-resolution phase tasks @detail - - The only argument that needs resolution is the JSON text - - Then, we need to set dependencies: if JSON_TABLE refers to table's - column, e.g. + The only argument that needs name resolution is the first parameter which + has the JSON text: + + JSON_TABLE(json_doc, ... ) - JSON_TABLE (t1.col ... ) AS t2 + The argument may refer to other tables and uses special name resolution + rules (see get_disallowed_table_deps_for_list for details). This function + sets up Name_resolution_context object appropriately before calling + fix_fields for the argument. - then it can be computed only after table t1. - - The dependencies must not form a loop. + @return + false OK + true Fatal error */ -int Table_function_json_table::setup(THD *thd, TABLE_LIST *sql_table, +bool Table_function_json_table::setup(THD *thd, TABLE_LIST *sql_table, SELECT_LEX *s_lex) { - TABLE *t= sql_table->table; - thd->where= "JSON_TABLE argument"; - { - bool save_is_item_list_lookup; - bool res; - save_is_item_list_lookup= s_lex->is_item_list_lookup; - s_lex->is_item_list_lookup= 0; + if (!m_context_setup_done) + { + m_context_setup_done= true; // Prepare the name resolution context. First, copy the context that is // used for name resolution of the WHERE clause *m_context= s_lex->context; // Then, restrict it to only allow to refer to tables that come before the // table function reference - m_context->ignored_tables= get_disallowed_table_deps(s_lex->join, t->map); + if (!(m_context->ignored_tables= + get_disallowed_table_deps(thd->stmt_arena->mem_root, s_lex, + sql_table))) + return TRUE; // Error + } - // Do the same what setup_without_group() does: do not count the referred - // fields in non_agg_field_used: - const bool saved_non_agg_field_used= s_lex->non_agg_field_used(); + bool save_is_item_list_lookup; + save_is_item_list_lookup= s_lex->is_item_list_lookup; + s_lex->is_item_list_lookup= 0; - res= m_json->fix_fields_if_needed(thd, &m_json); + // Do the same what setup_without_group() does: do not count the referred + // fields in non_agg_field_used: + const bool saved_non_agg_field_used= s_lex->non_agg_field_used(); - s_lex->is_item_list_lookup= save_is_item_list_lookup; - s_lex->set_non_agg_field_used(saved_non_agg_field_used); + bool res= m_json->fix_fields_if_needed(thd, &m_json); - if (res) - return TRUE; - } + s_lex->is_item_list_lookup= save_is_item_list_lookup; + s_lex->set_non_agg_field_used(saved_non_agg_field_used); + + if (res) + return TRUE; // Error return FALSE; } diff --git a/sql/json_table.h b/sql/json_table.h index 09e4295d80c..beae5405d25 100644 --- a/sql/json_table.h +++ b/sql/json_table.h @@ -198,7 +198,7 @@ class Table_function_json_table : public Sql_alloc List<Json_table_column> m_columns; /*** Name resolution functions ***/ - int setup(THD *thd, TABLE_LIST *sql_table, SELECT_LEX *s_lex); + bool setup(THD *thd, TABLE_LIST *sql_table, SELECT_LEX *s_lex); int walk_items(Item_processor processor, bool walk_subquery, void *argument); @@ -226,7 +226,8 @@ class Table_function_json_table : public Sql_alloc /*** Construction interface to be used from the parser ***/ Table_function_json_table(Item *json): - m_json(json) + m_json(json), + m_context_setup_done(false) { cur_parent= &m_nested_path; last_sibling_hook= &m_nested_path.m_nested; @@ -250,6 +251,8 @@ class Table_function_json_table : public Sql_alloc /* Context to be used for resolving the first argument. */ Name_resolution_context *m_context; + bool m_context_setup_done; + /* Current NESTED PATH level being parsed */ Json_table_nested_path *cur_parent; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index a859f83c5f7..0fc5159f7b4 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -6097,7 +6097,8 @@ Field * find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, const char *name, size_t length, const char *item_name, const char *db_name, - const char *table_name, table_map ignored_tables, + const char *table_name, + ignored_tables_list_t ignored_tables, Item **ref, bool check_privileges, bool allow_rowid, uint *cached_field_index_ptr, @@ -6190,8 +6191,13 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, TABLE_LIST *table; while ((table= it++)) { - if (table->table && (table->table->map & ignored_tables)) + /* + Check if the table is in the ignore list. Only base tables can be in + the ignore list. + */ + if (table->table && ignored_list_includes_table(ignored_tables, table)) continue; + if ((fld= find_field_in_table_ref(thd, table, name, length, item_name, db_name, table_name, ignored_tables, ref, check_privileges, allow_rowid, @@ -6318,8 +6324,8 @@ Field *find_field_in_table_sef(TABLE *table, const char *name) first_table list of tables to be searched for item last_table end of the list of tables to search for item. If NULL then search to the end of the list 'first_table'. - ignored_tables Bitmap of tables that should be ignored. Do not try - to find the field in those. + ignored_tables Set of tables that should be ignored. Do not try to + find the field in those. ref if 'item' is resolved to a view field, ref is set to point to the found view field report_error Degree of error reporting: @@ -6347,7 +6353,7 @@ Field *find_field_in_table_sef(TABLE *table, const char *name) Field * find_field_in_tables(THD *thd, Item_ident *item, TABLE_LIST *first_table, TABLE_LIST *last_table, - table_map ignored_tables, + ignored_tables_list_t ignored_tables, Item **ref, find_item_error_report_type report_error, bool check_privileges, bool register_tree_change) { @@ -6471,7 +6477,8 @@ find_field_in_tables(THD *thd, Item_ident *item, for (; cur_table != last_table ; cur_table= cur_table->next_name_resolution_table) { - if (cur_table->table && (cur_table->table->map & ignored_tables)) + if (cur_table->table && + ignored_list_includes_table(ignored_tables, cur_table)) continue; Field *cur_field= find_field_in_table_ref(thd, cur_table, name, length, diff --git a/sql/sql_base.h b/sql/sql_base.h index 1836c07497a..922c61ca123 100644 --- a/sql/sql_base.h +++ b/sql/sql_base.h @@ -195,14 +195,15 @@ bool fill_record(THD *thd, TABLE *table, Field **field, List<Item> &values, Field * find_field_in_tables(THD *thd, Item_ident *item, TABLE_LIST *first_table, TABLE_LIST *last_table, - table_map ignored_tables, + ignored_tables_list_t ignored_tables, Item **ref, find_item_error_report_type report_error, bool check_privileges, bool register_tree_change); Field * find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, const char *name, size_t length, const char *item_name, const char *db_name, - const char *table_name, table_map ignored_tables, + const char *table_name, + ignored_tables_list_t ignored_tables, Item **ref, bool check_privileges, bool allow_rowid, uint *cached_field_index_ptr, bool register_tree_change, TABLE_LIST **actual_table);