Hi, Oleksandr,
On Jan 09, Oleksandr Byelkin wrote:
> revision-id: d47b1ebc06d (mariadb-10.5.27-143-gd47b1ebc06d)
> parent(s): 3bbbeae7924
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2025-01-08 14:14:10 +0100
> message:
>
> MDEV-35326: Memory Leak in init_io_cache_ext upon SHUTDOWN
>
> The problems were that:
> 1) resources was freed "asimetric" normal execution in send_eof,
> in case of error in destructor.
> 2) destructor was not called in case of SP for result objects.
> (so if the last SP execution ended with error resorces was not
> freeded on reinit before execution (cleanup() called before next
> execution) and destructor also was not called due to lack of
> delete call for the object)
>
> All result method revised and freeing resources made "symetric".
>
> Destructor of result object called for SP.
>
> Added skipped invalidation in case of error in insert.
>
> Removed misleading naming of reset(thd) (could be mixed with
> with reset())
>
> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -3665,6 +3665,18 @@ int sp_lex_keeper::cursor_reset_lex_and_exec_core(THD
> *thd, uint *nextp,
> return res;
> }
>
> +sp_lex_keeper::~sp_lex_keeper()
> +{
> + if (m_lex_resp)
> + {
> + /* Prevent endless recursion. */
> + m_lex->sphead= NULL;
> + if (m_lex->result)
you don't need this if(), the standard explicitly says that delete of a
NULL pointer is valid.
> + delete m_lex->result;
> + lex_end(m_lex);
> + delete m_lex;
> + }
> +}
>
> /*
> sp_instr class functions
> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -3192,10 +3193,22 @@ bool select_send::send_eof()
> Handling writing to file
> ************************************************************************/
>
> +bool select_to_file::free_recources()
> +{
> + if (file >= 0)
> + {
> + (void) end_io_cache(&cache);
> + bool error= mysql_file_close(file, MYF(MY_WME));
> + file= -1;
> + return error;
> + }
> + return FALSE;
> +}
> +
> bool select_to_file::send_eof()
> {
> int error= MY_TEST(end_io_cache(&cache));
> - if (unlikely(mysql_file_close(file, MYF(MY_WME))) ||
> + if (unlikely(free_recources()) ||
free_recources() incluses end_io_cache(), you don't need to call it
separately above.
> unlikely(thd->is_error()))
> error= true;
>
> @@ -5934,7 +5935,15 @@ class select_to_file :public select_result_interceptor
> {
> { path[0]=0; }
> ~select_to_file();
> bool send_eof() override;
> + void abort_result_set() override;
> void cleanup() override;
> + /*
> + It is separated from cleanup() becaouse:
1. typo
2. if you rename cleanup() to reset_for_next_ps_execution()
then you won't need that comment, because, obviously, "freeing resources"
is distinct from "resetting for next ps execution"
but it's not obvious that "freeing resources" is distinct from "cleanup"
> + 1. cleanup can not be called (conventional execution)
> + 2. cleanup not only free recources but reset counters (some sends
> + counter updates in destructor)
> + */
> + bool free_recources();
> };
>
>
> diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> --- a/sql/sql_delete.cc
> +++ b/sql/sql_delete.cc
> @@ -1422,6 +1422,13 @@ void multi_delete::abort_result_set()
> {
> DBUG_ENTER("multi_delete::abort_result_set");
>
> +
> /****************************************************************************
> +
> + NOTE: if you change here be aware that almost the same code is in
> + multi_delete::send_eof().
> +
> +
> ***************************************************************************/
Let's talk about it separately.
> +
> /* the error was handled or nothing deleted and no side effects return */
> if (error_handled ||
> (!thd->transaction->stmt.modified_non_trans_table && !deleted))
> diff --git a/sql/sql_update.cc b/sql/sql_update.cc
> --- a/sql/sql_update.cc
> +++ b/sql/sql_update.cc
> @@ -2743,6 +2743,13 @@ void multi_update::abort_result_set()
> (!thd->transaction->stmt.modified_non_trans_table &&
> !updated)))
> return;
>
> +
> /****************************************************************************
> +
> + NOTE: if you change here be aware that almost the same code is in
> + multi_delete::send_eof().
multi_update::send_eof()
> +
> +
> ***************************************************************************/
> +
> /* Something already updated so we have to invalidate cache */
> if (updated)
> query_cache_invalidate3(thd, update_tables, 1);
Regards,
Sergei
Chief Architect, MariaDB Server
and secur...@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
