From 0d4be10a8af30cacc038c589792e772cb42683fc Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Fri, 1 Sep 2023 19:25:33 +0700 Subject: [PATCH 1/5] MDEV-14959: Control over memory allocated for SP/PS This patch adds support for controlling of memory allocation done by SP/PS that could happen on second and following executions. As soon as SP or PS has been executed the first time its memory root is marked as read only since no further memory allocation should be performed on it. In case such allocation takes place it leads to the assert hit for invariant that force no new memory allocations takes place as soon as the SP/PS has been marked as read only. The feature for control of memory allocation made on behalf SP/PS is turned on when both debug build is on and the cmake option -DWITH_PROTECT_STATEMENT_MEMROOT is set. The reason for introduction of the new cmake option -DWITH_PROTECT_STATEMENT_MEMROOT to control memory allocation of second and following executions of SP/PS is that for the current server implementation there are too many places where such memory allocation takes place. As soon as all such incorrect allocations be fixed the cmake option -DWITH_PROTECT_STATEMENT_MEMROOT can be removed and control of memory allocation made on second and following executions can be turned on only for debug build. Before every incorrect memory allocation be fixed it makes sense to guard the checking of memory allocation on read only memory by extra cmake option else we would get a lot of failing test on buildbot. Moreover, fixing of all incorrect memory allocations could take pretty long period of time, so for introducing the feature without necessary to wait until all places throughout the source code be fixed it makes sense to add the new cmake option. --- CMakeLists.txt | 9 +++++++ include/my_alloc.h | 4 +++ mysys/my_alloc.c | 7 +++++ sql/sp_head.cc | 67 ++++++++++++++++++++++++++++++++++++++++++++++ sql/sp_head.h | 37 +++++++++++++++++++++++++ sql/sql_prepare.cc | 44 ++++++++++++++++++++++++++++++ 6 files changed, 168 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7e5f3e157c3..9605d786c93 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -193,6 +193,15 @@ ENDIF() OPTION(NOT_FOR_DISTRIBUTION "Allow linking with GPLv2-incompatible system libraries. Only set it you never plan to distribute the resulting binaries" OFF) +# +# Enable protection of statement's memory root after first SP/PS execution. +# Can be switched on only for debug build. +# +OPTION(WITH_PROTECT_STATEMENT_MEMROOT "Enable protection of statement's memory root after first SP/PS execution. Turned into account only for debug build" OFF) +IF (CMAKE_BUILD_TYPE MATCHES "Debug" AND WITH_PROTECT_STATEMENT_MEMROOT) + ADD_DEFINITIONS(-DPROTECT_STATEMENT_MEMROOT) +ENDIF() + INCLUDE(check_compiler_flag) INCLUDE(check_linker_flag) diff --git a/include/my_alloc.h b/include/my_alloc.h index 9b0aad26956..6399ce67667 100644 --- a/include/my_alloc.h +++ b/include/my_alloc.h @@ -51,6 +51,10 @@ typedef struct st_mem_root */ unsigned int first_block_usage; +#ifdef PROTECT_STATEMENT_MEMROOT + int read_only; +#endif + void (*error_handler)(void); const char *name; } MEM_ROOT; diff --git a/mysys/my_alloc.c b/mysys/my_alloc.c index a6f38dcb145..4a5e48c9e80 100644 --- a/mysys/my_alloc.c +++ b/mysys/my_alloc.c @@ -74,6 +74,9 @@ void init_alloc_root(MEM_ROOT *mem_root, const char *name, size_t block_size, mem_root->first_block_usage= 0; mem_root->total_alloc= 0; mem_root->name= name; +#ifdef PROTECT_STATEMENT_MEMROOT + mem_root->read_only= 0; +#endif #if !(defined(HAVE_valgrind) && defined(EXTRA_DEBUG)) if (pre_alloc_size) @@ -218,6 +221,10 @@ void *alloc_root(MEM_ROOT *mem_root, size_t length) DBUG_PRINT("enter",("root: %p name: %s", mem_root, mem_root->name)); DBUG_ASSERT(alloc_root_inited(mem_root)); +#ifdef PROTECT_STATEMENT_MEMROOT + DBUG_ASSERT(mem_root->read_only == 0); +#endif + DBUG_EXECUTE_IF("simulate_out_of_memory", { /* Avoid reusing an already allocated block */ diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 8801d8880b1..b1a697ada2a 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -493,6 +493,9 @@ sp_head::sp_head(MEM_ROOT *mem_root_arg, sp_package *parent, :Query_arena(NULL, STMT_INITIALIZED_FOR_SP), Database_qualified_name(&null_clex_str, &null_clex_str), main_mem_root(*mem_root_arg), +#ifdef PROTECT_STATEMENT_MEMROOT + executed_counter(0), +#endif m_parent(parent), m_handler(sph), m_flags(0), @@ -738,6 +741,10 @@ sp_head::init(LEX *lex) */ lex->trg_table_fields.empty(); +#ifdef PROTECT_STATEMENT_MEMROOT + executed_counter= 0; +#endif + DBUG_VOID_RETURN; } @@ -1364,6 +1371,11 @@ sp_head::execute(THD *thd, bool merge_da_on_success) #endif /* WITH_WSREP */ err_status= i->execute(thd, &ip); +#ifdef PROTECT_STATEMENT_MEMROOT + if (!err_status) + i->mark_as_run(); +#endif + #ifdef WITH_WSREP if (WSREP(thd)) { @@ -1460,6 +1472,16 @@ sp_head::execute(THD *thd, bool merge_da_on_success) /* Reset sp_rcontext::end_partial_result_set flag. */ ctx->end_partial_result_set= FALSE; +#ifdef PROTECT_STATEMENT_MEMROOT + if (thd->is_error()) + { + // Don't count a call ended with an error as normal run + executed_counter= 0; + main_mem_root.read_only= 0; + reset_instrs_executed_counter(); + } +#endif + } while (!err_status && likely(!thd->killed) && likely(!thd->is_fatal_error) && !thd->spcont->pause_state); @@ -1571,6 +1593,20 @@ sp_head::execute(THD *thd, bool merge_da_on_success) err_status|= mysql_change_db(thd, (LEX_CSTRING*)&saved_cur_db_name, TRUE) != 0; } + +#ifdef PROTECT_STATEMENT_MEMROOT + if (!err_status) + { + if (!main_mem_root.read_only && + has_all_instrs_executed()) + { + main_mem_root.read_only= 1; + } + ++executed_counter; + DBUG_PRINT("info", ("execute counter: %lu", executed_counter)); + } +#endif + m_flags&= ~IS_INVOKED; if (m_parent) m_parent->m_invoked_subroutine_count--; @@ -3214,6 +3250,37 @@ void sp_head::add_mark_lead(uint ip, List *leads) leads->push_front(i); } +#ifdef PROTECT_STATEMENT_MEMROOT + +int sp_head::has_all_instrs_executed() +{ + sp_instr *ip; + uint count= 0; + + for (uint i= 0; i < m_instr.elements; ++i) + { + get_dynamic(&m_instr, (uchar*)&ip, i); + if (ip->has_been_run()) + ++count; + } + + return count == m_instr.elements; +} + + +void sp_head::reset_instrs_executed_counter() +{ + sp_instr *ip; + + for (uint i= 0; i < m_instr.elements; ++i) + { + get_dynamic(&m_instr, (uchar*)&ip, i); + ip->mark_as_not_run(); + } +} + +#endif + void sp_head::opt_mark() { diff --git a/sql/sp_head.h b/sql/sp_head.h index 06f5d5234ae..693a5e78703 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -132,6 +132,16 @@ class sp_head :private Query_arena, protected: MEM_ROOT main_mem_root; +#ifdef PROTECT_STATEMENT_MEMROOT + /* + The following data member is wholly for debugging purpose. + It can be used for possible crash analysis to determine how many times + the stored routine was executed before the mem_root marked read_only + was requested for a memory chunk. Additionally, a value of this data + member is output to the log with DBUG_PRINT. + */ + ulong executed_counter; +#endif public: /** Possible values of m_flags */ enum { @@ -801,6 +811,11 @@ public: return ip; } +#ifdef PROTECT_STATEMENT_MEMROOT + int has_all_instrs_executed(); + void reset_instrs_executed_counter(); +#endif + /* Add tables used by routine to the table list. */ bool add_used_tables_to_table_list(THD *thd, TABLE_LIST ***query_tables_last_ptr, @@ -1084,6 +1099,9 @@ public: /// Should give each a name or type code for debugging purposes? sp_instr(uint ip, sp_pcontext *ctx) :Query_arena(0, STMT_INITIALIZED_FOR_SP), marked(0), m_ip(ip), m_ctx(ctx) +#ifdef PROTECT_STATEMENT_MEMROOT + , m_has_been_run(false) +#endif {} virtual ~sp_instr() @@ -1173,6 +1191,25 @@ public: m_ip= dst; } +#ifdef PROTECT_STATEMENT_MEMROOT + bool has_been_run() const + { + return m_has_been_run; + } + + void mark_as_run() + { + m_has_been_run= true; + } + + void mark_as_not_run() + { + m_has_been_run= false; + } + +private: + bool m_has_been_run; +#endif }; // class sp_instr : public Sql_alloc diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 9c52aea2931..63ce4d4c26d 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -171,6 +171,16 @@ public: Server_side_cursor *cursor; uchar *packet; uchar *packet_end; +#ifdef PROTECT_STATEMENT_MEMROOT + /* + The following data member is wholly for debugging purpose. + It can be used for possible crash analysis to determine how many times + the stored routine was executed before the mem_root marked read_only + was requested for a memory chunk. Additionally, a value of this data + member is output to the log with DBUG_PRINT. + */ + ulong executed_counter; +#endif uint param_count; uint last_errno; uint flags; @@ -3995,6 +4005,9 @@ Prepared_statement::Prepared_statement(THD *thd_arg) cursor(0), packet(0), packet_end(0), +#ifdef PROTECT_STATEMENT_MEMROOT + executed_counter(0), +#endif param_count(0), last_errno(0), flags((uint) IS_IN_USE), @@ -4070,8 +4083,13 @@ void Prepared_statement::setup_set_params() Prepared_statement::~Prepared_statement() { DBUG_ENTER("Prepared_statement::~Prepared_statement"); +#ifdef PROTECT_STATEMENT_MEMROOT + DBUG_PRINT("enter",("stmt: %p cursor: %p executed_counter: %lu", + this, cursor, executed_counter)); +#else DBUG_PRINT("enter",("stmt: %p cursor: %p", this, cursor)); +#endif delete cursor; /* We have to call free on the items even if cleanup is called as some items, @@ -4237,6 +4255,10 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) } lex->set_trg_event_type_for_tables(); +#ifdef PROTECT_STATEMENT_MEMROOT + executed_counter= 0; +#endif + /* While doing context analysis of the query (in check_prepared_statement) we allocate a lot of additional memory: for open tables, JOINs, derived @@ -4506,9 +4528,31 @@ reexecute: error= reprepare(); if (likely(!error)) /* Success */ + { +#ifdef PROTECT_STATEMENT_MEMROOT + // There was reprepare so the counter of runs should be reset + executed_counter= 0; + mem_root->read_only= 0; +#endif goto reexecute; + } } reset_stmt_params(this); +#ifdef PROTECT_STATEMENT_MEMROOT + if (!error) + { + mem_root->read_only= 1; + ++executed_counter; + + DBUG_PRINT("info", ("execute counter: %lu", executed_counter)); + } + else + { + // Error on call shouldn't be counted as a normal run + executed_counter= 0; + mem_root->read_only= 0; + } +#endif return error; } From d8574dbba3c5b414c496ab9588733d5104eddf1e Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Fri, 1 Sep 2023 19:26:24 +0700 Subject: [PATCH 2/5] MDEV-14959: Moved calculation the number of items reserved for exists to in transformation It is done now before call of select_lex->setup_ref_array() in order to avoid allocation of SP/PS's memory on its second invocation. --- sql/sql_select.cc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 5955cbef8dd..805ba19f0bc 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -727,8 +727,7 @@ setup_without_group(THD *thd, Ref_ptr_array ref_pointer_array, ORDER *group, List &win_specs, List &win_funcs, - bool *hidden_group_fields, - uint *reserved) + bool *hidden_group_fields) { int res; enum_parsing_place save_place; @@ -743,13 +742,6 @@ setup_without_group(THD *thd, Ref_ptr_array ref_pointer_array, thd->lex->allow_sum_func.clear_bit(select->nest_level); res= setup_conds(thd, tables, leaves, conds); - if (thd->lex->current_select->first_cond_optimization) - { - if (!res && *conds && ! thd->lex->current_select->merged_into) - (*reserved)= (*conds)->exists2in_reserved_items(); - else - (*reserved)= 0; - } /* it's not wrong to have non-aggregated columns in a WHERE */ select->set_non_agg_field_used(saved_non_agg_field_used); @@ -1318,6 +1310,15 @@ JOIN::prepare(TABLE_LIST *tables_init, if (setup_wild(thd, tables_list, fields_list, &all_fields, wild_num, &select_lex->hidden_bit_fields)) DBUG_RETURN(-1); + + if (thd->lex->current_select->first_cond_optimization) + { + if ( conds && ! thd->lex->current_select->merged_into) + select_lex->select_n_reserved= conds->exists2in_reserved_items(); + else + select_lex->select_n_reserved= 0; + } + if (select_lex->setup_ref_array(thd, real_og_num)) DBUG_RETURN(-1); @@ -1336,8 +1337,7 @@ JOIN::prepare(TABLE_LIST *tables_init, all_fields, &conds, order, group_list, select_lex->window_specs, select_lex->window_funcs, - &hidden_group_fields, - &select_lex->select_n_reserved)) + &hidden_group_fields)) DBUG_RETURN(-1); /* From 1d502a29e56213c23ce8da9cab8296b4f764fef1 Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Fri, 1 Sep 2023 19:27:01 +0700 Subject: [PATCH 3/5] MDEV-14959: Fixed possible memory leaks that could happen on running PS/SP depending on a trigger Moved call of the function check_and_update_table_version() just before the place where the function extend_table_list() is invoked in order to avoid allocation of memory on a PS/SP memory root marked as read only. It happens by the reason that the function extend_table_list() invokes sp_add_used_routine() to add a trigger created for the table in time frame between execution the statement EXECUTE `stmt_id` . For example, the following test case create table t1 (a int); prepare stmt from "insert into t1 (a) value (1)"; execute stmt; create trigger t1_bi before insert on t1 for each row set @message= new.a; execute stmt; # (*) adds the trigger t1_bi to a list of used routines that involves allocation of a memory on PS memory root that has been already marked as read only on first run of the statement 'execute stmt'. In result, when the statement marked with (*) is executed it results in assert hit. To fix the issue call the function check_and_update_table_version() before invocation of extend_table_list() to force re-compilation of PS/SP that resets read-only flag of its memory root. --- sql/sql_base.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 47b9c4c6db9..9b1043393e7 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -3965,6 +3965,12 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags, if (tables->open_strategy && !tables->table) goto end; + /* Check and update metadata version of a base table. */ + error= check_and_update_table_version(thd, tables, tables->table->s); + + if (unlikely(error)) + goto end; + error= extend_table_list(thd, tables, prelocking_strategy, has_prelocking_list); if (unlikely(error)) goto end; @@ -3972,11 +3978,6 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags, /* Copy grant information from TABLE_LIST instance to TABLE one. */ tables->table->grant= tables->grant; - /* Check and update metadata version of a base table. */ - error= check_and_update_table_version(thd, tables, tables->table->s); - - if (unlikely(error)) - goto end; /* After opening a MERGE table add the children to the query list of tables, so that they are opened too. From be02356206cfe08a6da9ca8ed15e299741210d4b Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Fri, 1 Sep 2023 19:27:34 +0700 Subject: [PATCH 4/5] MDEV-14959: Fixed memory leak happened on re-parsing a view that substitutes a table In case a table accessed by a PS/SP is dropped after the first execution of PS/SP and a view created with the same name as a table just dropped then the second execution of PS/SP leads to allocation of a memory on SP/PS memory root already marked as read only on first execution. For example, the following test case: CREATE TABLE t1 (a INT); PREPARE stmt FROM "INSERT INTO t1 VALUES (1)"; EXECUTE stmt; DROP TABLE t1; CREATE VIEW t1 S SELECT 1; --error ER_NON_INSERTABLE_TABLE EXECUTE stmt; # (*) DROP VIEW t1; will hit assert on running the statement 'EXECUTE stmt' marked with (*) when allocation of a memory be performed on parsing the view. Memory allocation is requested inside the function mysql_make_view when a view definition being parsed. In order to avoid an assertion failure, call of the function mysql_make_view() must be moved after invocation of the function check_and_update_table_version(). It will result in re-preparing the whole PS statement or current SP instruction that will free currently allocated items and reset read_only flag for the memory root. --- sql/sql_base.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 9b1043393e7..97d214e1f17 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2018,10 +2018,6 @@ retry_share: goto err_lock; } - /* Open view */ - if (mysql_make_view(thd, share, table_list, false)) - goto err_lock; - /* This table is a view. Validate its metadata version: in particular, that it was a view when the statement was prepared. @@ -2029,6 +2025,10 @@ retry_share: if (check_and_update_table_version(thd, table_list, share)) goto err_lock; + /* Open view */ + if (mysql_make_view(thd, share, table_list, false)) + goto err_lock; + /* TODO: Don't free this */ tdc_release_share(share); From d0a872c20ebb38ed9ac40c13bcf834ecf6618080 Mon Sep 17 00:00:00 2001 From: Dmitry Shulga Date: Fri, 1 Sep 2023 19:28:12 +0700 Subject: [PATCH 5/5] MDEV-14959: Fixed memory leak relating with view and IS Fixed memory leak taken place on executing a prepared statement or a stored routine that querying a view and this view constructed on an information schema table. For example, Lets consider the following definition of the view 'v1' CREATE VIEW v1 AS SELECT table_name FROM information_schema.views ORDER BY table_name; Querying this view in PS mode result in hit of assert. PREPARE stmt FROM "SELECT * FROM v1"; EXECUTE stmt; EXECUTE stmt; (*) Running the statement marked with (*) leads to a crash in case server build with mode to control allocation of a memory from SP/PS memory root on the second and following executions of PS/SP. The reason of leaking the memory is that a memory allocated on processing of FRM file for the view requested from a PS/PS memory root meaning that this memory be released only when a stored routine be evicted from SP-cache or a prepared statement be deallocated that typically happens on termination of a user session. To fix the issue switch to a memory root specially created for allocation of short-lived objects that requested on parsing FRM. --- sql/sql_show.cc | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/sql/sql_show.cc b/sql/sql_show.cc index a621c1de29a..1917daa2788 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -4955,7 +4955,8 @@ try_acquire_high_prio_shared_mdl_lock(THD *thd, TABLE_LIST *table, open_tables function for this table */ -static int fill_schema_table_from_frm(THD *thd, TABLE *table, +static int fill_schema_table_from_frm(THD *thd, MEM_ROOT *mem_root, + TABLE *table, ST_SCHEMA_TABLE *schema_table, LEX_CSTRING *db_name, LEX_CSTRING *table_name, @@ -4967,6 +4968,9 @@ static int fill_schema_table_from_frm(THD *thd, TABLE *table, TABLE_LIST table_list; uint res= 0; char db_name_buff[NAME_LEN + 1], table_name_buff[NAME_LEN + 1]; + Query_arena i_s_arena(mem_root, Query_arena::STMT_CONVENTIONAL_EXECUTION); + Query_arena backup_arena, *old_arena; + bool i_s_arena_active= false; bzero((char*) &table_list, sizeof(TABLE_LIST)); bzero((char*) &tbl, sizeof(TABLE)); @@ -5041,6 +5045,11 @@ static int fill_schema_table_from_frm(THD *thd, TABLE *table, goto end; } + old_arena= thd->stmt_arena; + thd->stmt_arena= &i_s_arena; + thd->set_n_backup_active_arena(&i_s_arena, &backup_arena); + i_s_arena_active= true; + share= tdc_acquire_share(thd, &table_list, GTS_TABLE | GTS_VIEW); if (!share) { @@ -5122,7 +5131,16 @@ end: savepoint is safe. */ DBUG_ASSERT(thd->open_tables == NULL); + thd->mdl_context.rollback_to_savepoint(open_tables_state_backup->mdl_system_tables_svp); + + if (i_s_arena_active) + { + thd->stmt_arena= old_arena; + thd->restore_active_arena(&i_s_arena, &backup_arena); + i_s_arena.free_items(); + } + if (!thd->is_fatal_error) thd->clear_error(); return res; @@ -5351,7 +5369,8 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) if (!(table_open_method & ~OPEN_FRM_ONLY) && db_name != &INFORMATION_SCHEMA_NAME) { - if (!fill_schema_table_from_frm(thd, table, schema_table, + if (!fill_schema_table_from_frm(thd, &tmp_mem_root, + table, schema_table, db_name, table_name, &open_tables_state_backup, can_deadlock))