CREATEPROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt';
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
SHUTDOWN;
Roel Van de Paar
added a comment - - edited MTR Testcase:
--source plugin/spider/spider/include/init_spider.inc
CREATE TABLE t (c INT ) ENGINE=Spider;
CREATE PROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt' ;
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
SHUTDOWN;
At 10.5 bf7cfa2535618bfe9962c725555680e799fdcd18 I am able to reliably reproduce this issue with the following test
--disable_query_log
--disable_result_log
--source ../../t/test_init.inc
--enable_result_log
--enable_query_log
CREATETABLE t (c INT) ENGINE=Spider;
CREATEPROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt';
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
dropprocedure p;
droptable t;
--disable_query_log
--disable_result_log
--source ../../t/test_deinit.inc
--enable_result_log
--enable_query_log
Note that the CONTAINS SQL READS SQL DATA is not needed to reproduce.
Yuchen Pei
added a comment - - edited At 10.5 bf7cfa2535618bfe9962c725555680e799fdcd18 I am able to reliably reproduce this issue with the following test
--disable_query_log
--disable_result_log
--source ../../t/test_init.inc
--enable_result_log
--enable_query_log
CREATE TABLE t (c INT ) ENGINE=Spider;
CREATE PROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt' ;
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
drop procedure p;
drop table t;
--disable_query_log
--disable_result_log
--source ../../t/test_deinit.inc
--enable_result_log
--enable_query_log
Note that the CONTAINS SQL READS SQL DATA is not needed to reproduce.
Without creating and calling a procedure, the leak does not happen either:
# ...
CREATETABLE t (c INT) ENGINE=Spider;
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
SELECT * FROM t INTO OUTFILE 'foo.txt';
droptable t;
# ...
In this case, the cleanup happens in THD::end_statement which is only called for non-prepared statements:
// in THD::end_statement():
delete lex->result;
So the problem reduces to:
How to clean up in an sp execution after failing in optimizing stage?
Yuchen Pei
added a comment - - edited If we make it so that the CALL statement succeeds like the following, then the leak disappears. So it seems like some missing cleanup in failure mode.
--disable_query_log
--disable_result_log
--source ../../t/test_init.inc
--enable_result_log
--enable_query_log
set spider_same_server_link= 1;
evalp CREATE SERVER srv FOREIGN DATA WRAPPER mysql
OPTIONS (SOCKET "$MASTER_MYSOCK" , DATABASE 'test' , user 'root' );
create table t2 (c int );
CREATE TABLE t (c INT ) ENGINE=Spider
COMMENT= 'WRAPPER "mysql", srv "srv",TABLE "t2"' ;
CREATE PROCEDURE p() CONTAINS SQL READS SQL DATA SELECT * FROM t INTO OUTFILE 'foo.txt' ;
# --error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
drop procedure p;
drop table t, t2;
drop server srv;
--disable_query_log
--disable_result_log
--source ../../t/test_deinit.inc
--enable_result_log
--enable_query_log
More specifically, the stack of the leak suggests it is something happening in creating the outfile that has not been cleaned up:
0 in my_malloc of /home/ycp/source/mariadb-server/10.5/src/mysys/my_malloc.c:91
1 in init_io_cache_ext of /home/ycp/source/mariadb-server/10.5/src/mysys/mf_iocache.c:248
2 in init_io_cache of /home/ycp/source/mariadb-server/10.5/src/mysys/mf_iocache.c:301
3 in create_file of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3300
4 in select_export::prepare of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3322
In the passing case above, the cleanup happens in exec stage after the GBH execution:
// in Pushdown_query::execute():
if ((err= handler->end_scan()))
goto error_2;
if (!store_data_in_temp_table && join->result->send_eof())
DBUG_RETURN(1); // Don't send error to client
Compare that with our testcase, the failure happens in optimizing stage
// in JOIN::optimize_inner():
if (unlikely(make_join_statistics( this , select_lex->leaf_tables,
&keyuse)) ||
unlikely(thd->is_fatal_error))
{
DBUG_PRINT( "error" ,( "Error: make_join_statistics() failed" ));
DBUG_RETURN(1);
}
Without creating and calling a procedure, the leak does not happen either:
# ...
CREATE TABLE t (c INT ) ENGINE=Spider;
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
SELECT * FROM t INTO OUTFILE 'foo.txt' ;
drop table t;
# ...
In this case, the cleanup happens in THD::end_statement which is only called for non-prepared statements:
// in THD::end_statement():
delete lex->result;
So the problem reduces to:
How to clean up in an sp execution after failing in optimizing stage?
And the failing circumstances are rather unnatural. So perhaps spider is not meant to fail at this point, which in this specific case means spider contacting the data node for stats? Not really, as here the server layer explicitly requests status from spider, which naturally ask for them from the remote table:
// in TABLE_LIST::fetch_number_of_rows > make_join_statistics:
Yuchen Pei
added a comment - - edited However, if we do this
modified sql/sql_select.cc
@@ -2418,6 +2418,7 @@ JOIN::optimize_inner()
unlikely(thd->is_fatal_error))
{
DBUG_PRINT("error",("Error: make_join_statistics() failed"));
+ abort();
DBUG_RETURN(1);
}
And run the main suite, then only three tests fail:
Completed: Failed 3/1135 tests, 99.74% were successful.
Failing test(s): main.show_explain main.error_simulation main.unsafe_binlog_innodb
And the failing circumstances are rather unnatural. So perhaps spider is not meant to fail at this point, which in this specific case means spider contacting the data node for stats? Not really, as here the server layer explicitly requests status from spider, which naturally ask for them from the remote table:
// in TABLE_LIST::fetch_number_of_rows > make_join_statistics:
error= table->file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK);
fb45030a567 upstream/bb-10.5-mdev-35326 MDEV-35326 Clean up lex->result after a failing select
Some remaining questions:
Does lex->result being non-NULL imply it is not a select_insert/select_create?
see also commit 49bd559eb8f041de97e4ef55f280f3806d1b6c42 that introduced select_result::cleanup()
Can we come up with a prepared statement testcase?
Is this solution too general, for example, would it be better to check lex->result is a select_to_file before calling cleanup()? On the flipside, is it possible to construct a testcase that leak for a lex->result that is not a select_to_file?
Yuchen Pei
added a comment - - edited A draft patch which satisfies the CI:
fb45030a567 upstream/bb-10.5-mdev-35326 MDEV-35326 Clean up lex->result after a failing select
Some remaining questions:
Does lex->result being non-NULL imply it is not a select_insert/select_create?
see also commit 49bd559eb8f041de97e4ef55f280f3806d1b6c42 that introduced select_result::cleanup()
Can we come up with a prepared statement testcase?
Is this solution too general, for example, would it be better to check lex->result is a select_to_file before calling cleanup()? On the flipside, is it possible to construct a testcase that leak for a lex->result that is not a select_to_file?
prepare stmt1 from'SELECT * FROM t INTO OUTFILE "foo.txt"';
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
execute stmt1;
# ...
The cleanup happens in the destructor of Prepared_statement:
Prepared_statement::~Prepared_statement()
{
// [... 17 lines elided]
if (lex)
{
sp_head::destroy(lex->sphead);
delete lex->result; // cleanup happens
delete (st_lex_local *) lex;
}
free_root(&main_mem_root, MYF(0));
DBUG_VOID_RETURN;
}
0 in end_io_cache of /home/ycp/source/mariadb-server/10.5/src/mysys/mf_iocache.c:1783
1 in select_to_file::~select_to_file of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3229
2 in select_export::~select_export of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3242
3 in select_export::~select_export of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3242
4 in Prepared_statement::~Prepared_statement of /home/ycp/source/mariadb-server/10.5/src/sql/sql_prepare.cc:4125
5 in Prepared_statement::~Prepared_statement of /home/ycp/source/mariadb-server/10.5/src/sql/sql_prepare.cc:4130
6 in delete_statement_as_hash_key of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:4107
7 in my_hash_free_elements of /home/ycp/source/mariadb-server/10.5/src/mysys/hash.c:135
8 in my_hash_reset of /home/ycp/source/mariadb-server/10.5/src/mysys/hash.c:178
9 in Statement_map::reset of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:4242
10 in THD::cleanup of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:1592
11 in unlink_thd of /home/ycp/source/mariadb-server/10.5/src/sql/mysqld.cc:2627
12 in do_handle_one_connection of /home/ycp/source/mariadb-server/10.5/src/sql/sql_connect.cc:1397
Yuchen Pei
added a comment - With prepared statement e.g.
# ...
prepare stmt1 from 'SELECT * FROM t INTO OUTFILE "foo.txt"' ;
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
execute stmt1;
# ...
The cleanup happens in the destructor of Prepared_statement:
Prepared_statement::~Prepared_statement()
{
// [... 17 lines elided]
if (lex)
{
sp_head::destroy(lex->sphead);
delete lex->result; // cleanup happens
delete (st_lex_local *) lex;
}
free_root(&main_mem_root, MYF(0));
DBUG_VOID_RETURN;
}
0 in end_io_cache of /home/ycp/source/mariadb-server/10.5/src/mysys/mf_iocache.c:1783
1 in select_to_file::~select_to_file of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3229
2 in select_export::~select_export of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3242
3 in select_export::~select_export of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:3242
4 in Prepared_statement::~Prepared_statement of /home/ycp/source/mariadb-server/10.5/src/sql/sql_prepare.cc:4125
5 in Prepared_statement::~Prepared_statement of /home/ycp/source/mariadb-server/10.5/src/sql/sql_prepare.cc:4130
6 in delete_statement_as_hash_key of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:4107
7 in my_hash_free_elements of /home/ycp/source/mariadb-server/10.5/src/mysys/hash.c:135
8 in my_hash_reset of /home/ycp/source/mariadb-server/10.5/src/mysys/hash.c:178
9 in Statement_map::reset of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:4242
10 in THD::cleanup of /home/ycp/source/mariadb-server/10.5/src/sql/sql_class.cc:1592
11 in unlink_thd of /home/ycp/source/mariadb-server/10.5/src/sql/mysqld.cc:2627
12 in do_handle_one_connection of /home/ycp/source/mariadb-server/10.5/src/sql/sql_connect.cc:1397
3288b896551 upstream/bb-10.5-mdev-35326 MDEV-35326 Clean up lex->result after a failing exec of sp core function
Yuchen Pei
added a comment - Hi sanja , ptal thanks
3288b896551 upstream/bb-10.5-mdev-35326 MDEV-35326 Clean up lex->result after a failing exec of sp core function
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())
Oleksandr Byelkin
added a comment -
commit d47b1ebc06daa0716de04cd222b59f38d05ed75a (HEAD -> bb-10.5-MDEV-35326, origin/bb-10.5-MDEV-35326)
Author: Oleksandr Byelkin <sanja@mariadb.com>
Date: Mon Dec 23 22:36:01 2024 +0100
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())
MTR Testcase:
--source plugin/spider/spider/include/init_spider.inc
--error ER_CONNECT_TO_FOREIGN_DATA_SOURCE
CALL p();
SHUTDOWN;