Skip to content

Commit

Permalink
MDEV-16584 SP with a cursor inside a loop wastes THD memory aggressively
Browse files Browse the repository at this point in the history
Problem:

push_handler() created sp_handler_entry instances on THD::main_mem_root,
which is freed only after the SP instructions execution.
So in case of a CONTINUE HANDLER inside a loop (e.g. WHILE) this approach
leaked thread memory on every loop iteration.

Changes:
- Removing sp_handler_entry declaration, it's not really needed.
- Fixing the data type of sp_rcontext::m_handlers from
  Dynamic_array<sp_handler_entry*> to Dynamic_array<sp_instr_hpush_jump*>
- Fixing sp_rcontext::push_handler() to push the pointer to
  an sp_instr_hpush_jump instance to the handler stack.
  This instance contains everything we need.
  There is no a need to allocate anything else.
  • Loading branch information
abarkov committed Jun 28, 2018
1 parent 445339f commit 724a510
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 48 deletions.
43 changes: 43 additions & 0 deletions mysql-test/main/sp-condition-handler.result
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#
# Start of 10.3 tests
#
#
# MDEV-16595 SP with a CONTINUE HANDLER inside a loop wastes THD memory aggressively
#
CREATE PROCEDURE p1()
BEGIN
DECLARE mem_used_old BIGINT UNSIGNED DEFAULT
(SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.SESSION_STATUS
WHERE VARIABLE_NAME='MEMORY_USED');
DECLARE i INT DEFAULT 1;
WHILE i <= 1000
DO
BEGIN
DECLARE msg TEXT;
DECLARE mem_used_cur BIGINT UNSIGNED DEFAULT
(SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.SESSION_STATUS
WHERE VARIABLE_NAME='MEMORY_USED');
DECLARE CONTINUE HANDLER FOR SQLSTATE '23000' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23001' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23002' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23003' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23004' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23005' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23006' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23007' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23008' SET @x=1;
IF (mem_used_cur >= mem_used_old * 1.1) THEN
SHOW STATUS LIKE 'Memory_used';
SET msg=CONCAT('Memory leak detected: i=', i, ' mem_used_old=',mem_used_old,' mem_used_cur=', mem_used_cur);
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT=msg;
END IF;
END;
SET i=i+1;
END WHILE;
END;
$$
CALL p1;
DROP PROCEDURE p1;
#
# End of 10.3 tests
#
50 changes: 50 additions & 0 deletions mysql-test/main/sp-condition-handler.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@

--echo #
--echo # Start of 10.3 tests
--echo #

--echo #
--echo # MDEV-16595 SP with a CONTINUE HANDLER inside a loop wastes THD memory aggressively
--echo #

DELIMITER $$;
CREATE PROCEDURE p1()
BEGIN
DECLARE mem_used_old BIGINT UNSIGNED DEFAULT
(SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.SESSION_STATUS
WHERE VARIABLE_NAME='MEMORY_USED');
DECLARE i INT DEFAULT 1;
WHILE i <= 1000
DO
BEGIN
DECLARE msg TEXT;
DECLARE mem_used_cur BIGINT UNSIGNED DEFAULT
(SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.SESSION_STATUS
WHERE VARIABLE_NAME='MEMORY_USED');
DECLARE CONTINUE HANDLER FOR SQLSTATE '23000' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23001' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23002' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23003' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23004' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23005' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23006' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23007' SET @x=1;
DECLARE CONTINUE HANDLER FOR SQLSTATE '23008' SET @x=1;
IF (mem_used_cur >= mem_used_old * 1.1) THEN
SHOW STATUS LIKE 'Memory_used';
SET msg=CONCAT('Memory leak detected: i=', i, ' mem_used_old=',mem_used_old,' mem_used_cur=', mem_used_cur);
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT=msg;
END IF;
END;
SET i=i+1;
END WHILE;
END;
$$
DELIMITER ;$$
CALL p1;
DROP PROCEDURE p1;


--echo #
--echo # End of 10.3 tests
--echo #
2 changes: 1 addition & 1 deletion sql/sp_head.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4027,7 +4027,7 @@ sp_instr_hpush_jump::execute(THD *thd, uint *nextp)
{
DBUG_ENTER("sp_instr_hpush_jump::execute");

int ret= thd->spcont->push_handler(m_handler, m_ip + 1);
int ret= thd->spcont->push_handler(this);

*nextp= m_dest;

Expand Down
2 changes: 0 additions & 2 deletions sql/sp_head.h
Original file line number Diff line number Diff line change
Expand Up @@ -1676,8 +1676,6 @@ class sp_instr_hpush_jump : public sp_instr_jump
sp_handler *get_handler()
{ return m_handler; }

private:

private:
/// Handler.
sp_handler *m_handler;
Expand Down
24 changes: 7 additions & 17 deletions sql/sp_rcontext.cc
Original file line number Diff line number Diff line change
Expand Up @@ -448,19 +448,9 @@ void sp_rcontext::pop_cursors(THD *thd, size_t count)
}


bool sp_rcontext::push_handler(sp_handler *handler, uint first_ip)
bool sp_rcontext::push_handler(sp_instr_hpush_jump *entry)
{
/*
We should create handler entries in the callers arena, as
they could be (and usually are) used in several instructions.
*/
sp_handler_entry *he=
new (callers_arena->mem_root) sp_handler_entry(handler, first_ip);

if (he == NULL)
return true;

return m_handlers.append(he);
return m_handlers.append(entry);
}


Expand Down Expand Up @@ -548,12 +538,12 @@ bool sp_rcontext::handle_sql_condition(THD *thd,

DBUG_ASSERT(found_condition);

sp_handler_entry *handler_entry= NULL;
sp_instr_hpush_jump *handler_entry= NULL;
for (size_t i= 0; i < m_handlers.elements(); ++i)
{
sp_handler_entry *h= m_handlers.at(i);
sp_instr_hpush_jump *h= m_handlers.at(i);

if (h->handler == found_handler)
if (h->get_handler() == found_handler)
{
handler_entry= h;
break;
Expand Down Expand Up @@ -582,7 +572,7 @@ bool sp_rcontext::handle_sql_condition(THD *thd,
// Mark active conditions so that they can be deleted when the handler exits.
da->mark_sql_conditions_for_removal();

uint continue_ip= handler_entry->handler->type == sp_handler::CONTINUE ?
uint continue_ip= handler_entry->get_handler()->type == sp_handler::CONTINUE ?
cur_spi->get_cont_dest() : 0;

/* End aborted result set. */
Expand All @@ -601,7 +591,7 @@ bool sp_rcontext::handle_sql_condition(THD *thd,
new (callers_arena->mem_root) Handler_call_frame(cond_info, continue_ip);
m_handler_call_stack.append(frame);

*ip= handler_entry->first_ip;
*ip= handler_entry->m_ip + 1;

DBUG_RETURN(true);
}
Expand Down
34 changes: 6 additions & 28 deletions sql/sp_rcontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
class sp_cursor;
class sp_lex_keeper;
class sp_instr_cpush;
class sp_instr_hpush_jump;
class Query_arena;
class sp_head;
class Item_cache;
Expand Down Expand Up @@ -87,27 +88,6 @@ class sp_rcontext : public Sql_alloc
sp_rcontext(const sp_rcontext &);
void operator=(sp_rcontext &);

private:
/// This is an auxillary class to store entering instruction pointer for an
/// SQL-handler.
class sp_handler_entry : public Sql_alloc
{
public:
/// Handler definition (from parsing context).
const sp_handler *handler;

/// Instruction pointer to the first instruction.
uint first_ip;

/// The constructor.
///
/// @param _handler sp_handler object.
/// @param _first_ip first instruction pointer.
sp_handler_entry(const sp_handler *_handler, uint _first_ip)
:handler(_handler), first_ip(_first_ip)
{ }
};

public:
/// This class stores basic information about SQL-condition, such as:
/// - SQL error code;
Expand Down Expand Up @@ -235,18 +215,16 @@ class sp_rcontext : public Sql_alloc
// SQL-handlers.
/////////////////////////////////////////////////////////////////////////

/// Create a new sp_handler_entry instance and push it to the handler call
/// stack.
/// Push an sp_instr_hpush_jump instance to the handler call stack.
///
/// @param handler SQL-handler object.
/// @param first_ip First instruction pointer of the handler.
/// @param entry The condition handler entry
///
/// @return error flag.
/// @retval false on success.
/// @retval true on error.
bool push_handler(sp_handler *handler, uint first_ip);
bool push_handler(sp_instr_hpush_jump *entry);

/// Pop and delete given number of sp_handler_entry instances from the handler
/// Pop and delete given number of instances from the handler
/// call stack.
///
/// @param count Number of handler entries to pop & delete.
Expand Down Expand Up @@ -411,7 +389,7 @@ class sp_rcontext : public Sql_alloc
bool m_in_sub_stmt;

/// Stack of visible handlers.
Dynamic_array<sp_handler_entry *> m_handlers;
Dynamic_array<sp_instr_hpush_jump *> m_handlers;

/// Stack of caught SQL conditions.
Dynamic_array<Handler_call_frame *> m_handler_call_stack;
Expand Down

0 comments on commit 724a510

Please sign in to comment.