maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Jerome,

On 09/27/2017 01:36 PM, jerome brauge wrote:
> 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  ? 

Yes please.

I know that for now only VARCHAR will do adjustment, and the loop will
be  useless if there are no any VARCHAR parameter with no length were
given.

But we'll have pluggable data types soon and new data type
plugins will want to do parameter adjustments as well.
So let's make the code ready for this right now.

The amount of SP parameters is usually small.
So the loop calling the new Type_handler virtual method
for all parameters will be cheap comparing to the entire
sp_rcontext::create().

Thanks!

> 
> 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!