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:b...@mariadb.org] > Envoyé : mercredi 27 septembre 2017 09:38 > À : jerome brauge > Cc : MariaDB Developers (maria-developers@lists.launchpad.net) > 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! _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
