maria-developers team mailing list archive

[jerome brauge <j.brauge@xxxxxxxxxxx>]

Hi Alexander,
I just need a  confirmation on remove test on use_dyn_char_param().

This test avoid unnecessary  loop on parameters at runtime in sql_mode=default or when all varchar parameter size are specified in sql_mode=oracle (I agree, it's not really Oracle compliant, but it's a useful feature).
Do you really want I remove it  ? 

Thank you very much.


> -----Message d'origine-----
> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx]
> Envoyé : mercredi 27 septembre 2017 09:38
> À : jerome brauge
> Cc : MariaDB Developers (maria-developers@xxxxxxxxxxxxxxxxxxx)
> Objet : Re: MDEV-10596
> 
> 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!