maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Jerome,

Thank you very much for your contribution!

On 09/12/2017 04:42 PM, jerome brauge wrote:
> Hi Alexander,
> In sql_mode=oracle, when a stored procedure varchar parameter is defined without size, you have chosen to translate it by VARCHAR(4000) (or CHAR(2000) for CHAR parameter).
> Oracle does not work like this. Size is inherited from the size of argument at runtime.
<cut>

Your patch looks very good.
I have minor improvement suggestions, to make the code more future-proof.

Please find my comments below:


<cut>
> diff --git a/mysql-test/suite/compat/oracle/t/sp-param.test b/mysql-test/suite/compat/oracle/t/sp-param.test
> index 2320331..be929cb 100644
> --- a/mysql-test/suite/compat/oracle/t/sp-param.test
> +++ b/mysql-test/suite/compat/oracle/t/sp-param.test
<cut>
> +--error ER_DATA_TOO_LONG
> +declare str varchar(6000);
> +        pout varchar(4000);
> +begin
> +  str:=lpad('x',6000,'y');
> +  call p1(pout,str);
> +  select length(pout);
> +end;
> +/
> +drop procedure p1
> +/
> \ No newline at end of file

Please add a new line.


<cut>

> diff --git a/sql/sp_pcontext.cc b/sql/sp_pcontext.cc
> index d98f800..6531046 100644
> --- a/sql/sp_pcontext.cc
> +++ b/sql/sp_pcontext.cc
> @@ -95,7 +95,8 @@ sp_pcontext::sp_pcontext()
>    : Sql_alloc(),
>    m_max_var_index(0), m_max_cursor_index(0),
>    m_parent(NULL), m_pboundary(0),
> -  m_scope(REGULAR_SCOPE)
> +  m_scope(REGULAR_SCOPE),
> +  m_dyn_varchar_param(false)
>  {
>    init(0, 0, 0);
>  }
> diff --git a/sql/sp_pcontext.h b/sql/sp_pcontext.h
> index 215ebbe..776bd10 100644
> --- a/sql/sp_pcontext.h
> +++ b/sql/sp_pcontext.h
> @@ -692,6 +692,12 @@ class sp_pcontext : public Sql_alloc
>      return m_for_loop;
>    }
>  
> +  void set_dyn_char_param()
> +  { m_dyn_varchar_param= true; }
> +
> +  bool use_dyn_char_param() const
> +  { return m_dyn_varchar_param; }
> +
>  private:
>    /// Constructor for a tree node.
>    /// @param prev the parent parsing context
> @@ -786,6 +792,8 @@ class sp_pcontext : public Sql_alloc
>  
>    /// FOR LOOP characteristics
>    Lex_for_loop m_for_loop;
> +
> +  bool m_dyn_varchar_param;
>  }; // class sp_pcontext : public Sql_alloc
>  
>  
> diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc
> index 8c598e8..3e449cd 100644
> --- a/sql/sp_rcontext.cc
> +++ b/sql/sp_rcontext.cc
> @@ -63,7 +63,8 @@ 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)
> +                                 bool resolve_type_refs,
> +                                 List<Item> *args)
>  {
>    sp_rcontext *ctx= new (thd->mem_root) sp_rcontext(root_parsing_ctx,
>                                                      return_value_fld,
> @@ -75,6 +76,34 @@ sp_rcontext *sp_rcontext::create(THD *thd,
>    List<Spvar_definition> field_def_lst;
>    ctx->m_root_parsing_ctx->retrieve_field_definitions(&field_def_lst);
>  
> +  if (ctx->m_root_parsing_ctx->use_dyn_char_param() && args)
> +  {
> +    List_iterator<Spvar_definition> it(field_def_lst);
> +    List_iterator<Item> it_args(*args);
> +    DBUG_ASSERT(field_def_lst.elements >= args->elements );
> +    Spvar_definition *def;
> +    Item *arg;
> +    uint arg_max_length;
> +    /*
> +      If length is not specified for varchar arguments, set length to the
> +      maximum length of the real argument. Goals are:
> +      -- avoid to allocate too much inused memory for m_var_table
> +      -- allow length check inside the callee rather than during copy of
> +         returned values in output variables.
> +      -- allow varchar parameter size greater than 4000
> +      Default length has been stored in "decimal" member during parse.
> +    */
> +    while ((def= it++) && (arg= it_args++))
> +    {
> +      if (def->type_handler() == &type_handler_varchar && def->decimals)
> +      {
> +        arg_max_length= arg->max_char_length();
> +        def->length= arg_max_length > 0 ? arg_max_length : def->decimals;
> +        def->create_length_to_internal_length_string();
> +      }
> +    }
> +  }

I'd like to make the code more symmetric across data types.
It's very likely that we'll want some more
formal-param-to-actual-param-adjustment in the future.


Can you please move this code into a new virtual method in Type_handler?

Something like this:

virtual bool adjust_spparam_type(Spvar_definition *def, Item *from) const
{
  return false;
}

and override it for VARCHAR:

/*
   If length is not specified for a varchar parameter, set length to the
   maximum length of the actual argument. Goals are:
   - avoid to allocate too much unused memory for m_var_table
   - allow length check inside the callee rather than during copy of
     returned values in output variables.
   - allow varchar parameter size greater than 4000
   Default length has been stored in "decimal" member during parse.
*/
bool Type_handler_varchar::adjust_spparam_type(Spvar_definition *def,
                                               Item *param) const
{
  if (def->decimals)
  {
     arg_max_length= param->max_char_length();
     def->length= arg_max_length > 0 ? arg_max_length : def->decimals;
     def->create_length_to_internal_length_string();
  }
  return false;
}



Also, move the loop code into a new methods in sp_rcontext,
e.g. like this:


bool sp_rcontext::adjust_formal_params_to_actual_params(THD *thd,
List<Spvar_definition> &field_def_lst,
List<Item> *args)
{
  List_iterator<Spvar_definition> it(field_def_lst);
  List_iterator<Item> it_args(*args);
  DBUG_ASSERT(field_def_lst.elements >= args->elements );
  Spvar_definition *def;
  Item *arg;
  uint arg_max_length;
  while ((def= it++) && (arg= it_args++))
  {
    if (def->type_handler()->adjust_spparam_type(def, arg))
      true;
  }
  return false;
}


and call it from sp_rcontext::create() as follows:

  if (adjust_formal_params_to_actual_params(thd, args))
    return NULL.

No needs to test for ctx->m_root_parsing_ctx->use_dyn_char_param().
Just do it always unconditionally.

Please also remove:

sp_rcontext::set_dyn_char_param()
sp_rcontext::use_dyn_char_param()
sp_rcontext::m_dyn_varchar_param;

Btw, why did you add it?
Is it to safe on populating List<Item> ?

If so, then we can just pass arguments as follows;

sp_rcontext::create(THD *thd,
                    const sp_pcontext *root_parsing_ctx,
                    Field *return_value_fld,
                    bool resolve_type_refs)
                    bool resolve_type_refs,
                    Item **args, uint arg_count);

and:


bool sp_rcontext::adjust_formal_params_to_actual_params(THD *thd,
                                         List<Spvar_definition>
&field_def_lst,
                                         Item **args,
                                         uint arg_count);


<cut>

> diff --git a/sql/sql_yacc_ora.yy b/sql/sql_yacc_ora.yy
> index 97a55df..8e8a016 100644
> --- a/sql/sql_yacc_ora.yy
> +++ b/sql/sql_yacc_ora.yy
<cut>
> @@ -6565,17 +6567,20 @@ opt_field_length_default_1:
>    the maximum possible length in characters in case of mbmaxlen=4
>    (e.g. utf32, utf16, utf8mb4). However, we'll have character sets with
>    mbmaxlen=5 soon (e.g. gb18030).
> -
> -  Let's translate VARCHAR to VARCHAR(4000), which covert all possible Oracle
> -  values.
>  */
>  opt_field_length_default_sp_param_varchar:
> -          /* empty */  { $$= (char*) "4000"; }
> -        | field_length { $$= $1; }
> +          /* empty */  {
> +                         $$.set("4000", "4000");
> +                         Lex->spcont->set_dyn_char_param();
> +                       }
> +        | field_length { $$.set($1, NULL); }

Please follow our coding style for *.yy.
A multi-line code block corresponding to the "empty" rule should
go under the comment "/* empty */", like this:



opt_field_length_default_sp_param_varchar:
          /* empty */
          {
            $$.set("4000", "4000");
            Lex->spcont->set_dyn_char_param();
          }
        | field_length { $$.set($1, NULL); }


But if we don't need set_dyn_char_param(),
the above will be simplified to:

opt_field_length_default_sp_param_varchar:
          /* empty */   { $$.set("4000", "4000"); }
        | field_length  { $$.set($1, NULL); }

It's ok to put single-line code blocks to the right.


>  
>  opt_field_length_default_sp_param_char:
> -          /* empty */  { $$= (char*) "2000"; }
> -        | field_length { $$= $1; }
> +          /* empty */  {
> +                         $$.set("2000", "2000");
> +                         Lex->spcont->set_dyn_char_param();
> +                       }
> +        | field_length { $$.set($1, NULL); }

Same here.

>  
>  opt_precision:
>            /* empty */    { $$.set(0, 0); }

Thanks!