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@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
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
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
&field_def_lst, List<Item> *args) { List_iterator 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
&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!