Skip to content

Commit

Permalink
A cleanup for MDEV-13919 sql_mode=ORACLE: Derive length of VARCHAR SP…
Browse files Browse the repository at this point in the history
… param...

The intent of this patch is to avoid copying arguments from
a pair "Item **args, uint arg_count" to List<Item> in
sp_head::execute_function(). If the number of a stored function parameters
is huge, such copying can affect performance.

Change:
1. Adding a new method Row_definition_list::adjust_formal_params_to_actual_params,
   which accepts a pair of "Item **, uint".
2. Modifying the code to use the new method:
   - the calls for sp_rcontext::retrieve_field_definitions() and
     Row_definition_list::adjust_formal_params_to_actual_params() have
     been moved from sp_rcontext::create() to sp_head::rcontext_create(),
     to handle different argument notations easier (Item** vs List<Item>).
   - sp_rcontext::create() now assumes that the passed Row_definition_list
     is already adjusted to the actual SP parameters, and all "TYPE OF"
     and "ROWTYPE OF" references are resolved.
3. Removing creation of List<Item> in sp_head::execute_procedure(),
   using the code with "Item**, uint" notation instead.
4. Improvement of the code for MDEV-10577:
   As a good side effect, this patch gets rid of double security context
   switch inside sp_head::execute_trigger():
   sp_rcontext is created when the context is already switched,
   so the second context switch inside sp_head::rcontext_create() was
   redundant. This is solved by adding a "bool switch_secutiry_ctx" parameter
   to rcontext_create(), so now execute_function() and execute_procedure()
   pass "true", while execute_trigger() passes "false".
  • Loading branch information
Alexander Barkov committed Sep 28, 2017
1 parent 596baeb commit 139441d
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 28 deletions.
2 changes: 2 additions & 0 deletions sql/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -4096,6 +4096,8 @@ class Row_definition_list: public List<class Spvar_definition>
return 0;
}
bool adjust_formal_params_to_actual_params(THD *thd, List<Item> *args);
bool adjust_formal_params_to_actual_params(THD *thd,
Item **args, uint arg_count);
bool resolve_type_refs(THD *);
};

Expand Down
51 changes: 37 additions & 14 deletions sql/sp_head.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1427,7 +1427,7 @@ bool sp_head::check_execute_access(THD *thd) const


/**
Create rcontext using the routine security.
Create rcontext optionally using the routine security.
This is important for sql_mode=ORACLE to make sure that the invoker has
access to the tables mentioned in the %TYPE references.
Expand All @@ -1440,25 +1440,51 @@ bool sp_head::check_execute_access(THD *thd) const
@retval !NULL - success (the invoker has rights to all %TYPE tables)
*/
sp_rcontext *sp_head::rcontext_create(THD *thd, Field *ret_value,
List<Item> *args)
Row_definition_list *defs,
bool switch_security_ctx)
{
bool has_column_type_refs= m_flags & HAS_COLUMN_TYPE_REFS;
if (!(m_flags & HAS_COLUMN_TYPE_REFS))
return sp_rcontext::create(thd, m_pcont, ret_value, *defs);
sp_rcontext *res= NULL;
#ifndef NO_EMBEDDED_ACCESS_CHECKS
Security_context *save_security_ctx;
if (has_column_type_refs &&
if (switch_security_ctx &&
set_routine_security_ctx(thd, this, &save_security_ctx))
return NULL;
#endif
sp_rcontext *res= sp_rcontext::create(thd, m_pcont, ret_value,
has_column_type_refs, args);
if (!defs->resolve_type_refs(thd))
res= sp_rcontext::create(thd, m_pcont, ret_value, *defs);
#ifndef NO_EMBEDDED_ACCESS_CHECKS
if (has_column_type_refs)
if (switch_security_ctx)
m_security_ctx.restore_security_context(thd, save_security_ctx);
#endif
return res;
}


sp_rcontext *sp_head::rcontext_create(THD *thd, Field *ret_value,
List<Item> *args)
{
DBUG_ASSERT(args);
Row_definition_list defs;
m_pcont->retrieve_field_definitions(&defs);
if (defs.adjust_formal_params_to_actual_params(thd, args))
return NULL;
return rcontext_create(thd, ret_value, &defs, true);
}


sp_rcontext *sp_head::rcontext_create(THD *thd, Field *ret_value,
Item **args, uint arg_count)
{
Row_definition_list defs;
m_pcont->retrieve_field_definitions(&defs);
if (defs.adjust_formal_params_to_actual_params(thd, args, arg_count))
return NULL;
return rcontext_create(thd, ret_value, &defs, true);
}


/**
Execute trigger stored program.
Expand Down Expand Up @@ -1556,8 +1582,9 @@ sp_head::execute_trigger(THD *thd,
init_sql_alloc(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0, MYF(0));
thd->set_n_backup_active_arena(&call_arena, &backup_arena);

if (!(nctx= sp_rcontext::create(thd, m_pcont, NULL,
m_flags & HAS_COLUMN_TYPE_REFS, NULL)))
Row_definition_list defs;
m_pcont->retrieve_field_definitions(&defs);
if (!(nctx= rcontext_create(thd, NULL, &defs, false)))
{
err_status= TRUE;
goto err_with_cleanup;
Expand Down Expand Up @@ -1638,7 +1665,6 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
MEM_ROOT call_mem_root;
Query_arena call_arena(&call_mem_root, Query_arena::STMT_INITIALIZED_FOR_SP);
Query_arena backup_arena;
List<Item> largs;
DBUG_ENTER("sp_head::execute_function");
DBUG_PRINT("info", ("function %s", m_name.str));

Expand Down Expand Up @@ -1673,10 +1699,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
init_sql_alloc(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0, MYF(0));
thd->set_n_backup_active_arena(&call_arena, &backup_arena);

for (uint i= 0 ; i < argcount ; i++)
largs.push_back(argp[i]);

if (!(nctx= rcontext_create(thd, return_value_fld, &largs)))
if (!(nctx= rcontext_create(thd, return_value_fld, argp, argcount)))
{
thd->restore_active_arena(&call_arena, &backup_arena);
err_status= TRUE;
Expand Down
6 changes: 5 additions & 1 deletion sql/sp_head.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,11 @@ class sp_head :private Query_arena,
}

sp_rcontext *rcontext_create(THD *thd, Field *retval, List<Item> *args);

sp_rcontext *rcontext_create(THD *thd, Field *retval,
Item **args, uint arg_count);
sp_rcontext *rcontext_create(THD *thd, Field *retval,
Row_definition_list *list,
bool switch_security_ctx);
private:
/**
Version of the stored routine cache at the moment when the
Expand Down
28 changes: 17 additions & 11 deletions sql/sp_rcontext.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,25 +63,15 @@ sp_rcontext::~sp_rcontext()
sp_rcontext *sp_rcontext::create(THD *thd,
const sp_pcontext *root_parsing_ctx,
Field *return_value_fld,
bool resolve_type_refs,
List<Item> *args)
Row_definition_list &field_def_lst)
{
sp_rcontext *ctx= new (thd->mem_root) sp_rcontext(root_parsing_ctx,
return_value_fld,
thd->in_sub_stmt);

if (!ctx)
return NULL;

Row_definition_list field_def_lst;
ctx->m_root_parsing_ctx->retrieve_field_definitions(&field_def_lst);

if (args &&
field_def_lst.adjust_formal_params_to_actual_params(thd, args))
return NULL;

if (ctx->alloc_arrays(thd) ||
(resolve_type_refs && field_def_lst.resolve_type_refs(thd)) ||
ctx->init_var_table(thd, field_def_lst) ||
ctx->init_var_items(thd, field_def_lst))
{
Expand Down Expand Up @@ -110,6 +100,22 @@ bool Row_definition_list::
}


bool Row_definition_list::
adjust_formal_params_to_actual_params(THD *thd,
Item **args, uint arg_count)
{
List_iterator<Spvar_definition> it(*this);
DBUG_ASSERT(elements >= arg_count );
Spvar_definition *def;
for (uint i= 0; (def= it++) && (i < arg_count) ; i++)
{
if (def->type_handler()->adjust_spparam_type(def, args[i]))
return true;
}
return false;
}


bool sp_rcontext::alloc_arrays(THD *thd)
{
{
Expand Down
3 changes: 1 addition & 2 deletions sql/sp_rcontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ class sp_rcontext : public Sql_alloc
static sp_rcontext *create(THD *thd,
const sp_pcontext *root_parsing_ctx,
Field *return_value_fld,
bool resolve_type_refs,
List<Item> *args);
Row_definition_list &defs);

~sp_rcontext();

Expand Down

0 comments on commit 139441d

Please sign in to comment.