maria-developers team mailing list archive

[jerome brauge <j.brauge@xxxxxxxxxxx>]

Hi Alexander,
Thanks for this idea, but for us, cpu is a critical hotspot.
I will evaluate how many statements in our application are affected. If this number is not to high, we modify them.
In this case, we will need two sql_mode (one for MDEV-13417 and one for MDEV-13418).

Best regards.
Jérôme.


> -----Message d'origine-----
> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx]
> Envoyé : vendredi 3 novembre 2017 11:02
> À : jerome brauge
> Cc : MariaDB Developers (maria-developers@xxxxxxxxxxxxxxxxxxx)
> Objet : Re: MDEV-13418 Compatibility: The order of evaluation of
> SELECT..INTO assignments
> 
> Hello Jerome,
> 
> 
> On 11/03/2017 12:22 PM, jerome brauge wrote:
> > Hello Alexander.
> > I've begun to implement your proposal but now I'm not sure that it's a
> better solution than mine.
> > Let me explain .
> > - first : number of temporary variables can be significant because we don't
> know when they are really needed and their scope are local to the
> statement.
> >
> >   declare b1 INTEGER;
> >   declare res INTEGER;
> > ...
> >   if b1 = 0 then
> >     select 1,b1+1 into b1, res from dual;
> >   end if;
> >   if b1 = 1 then
> >     select 2,b1+2 into b1, res from dual;
> >   end if;
> >
> > will be transform in :
> >
> >   declare b1 INTEGER;
> >   declare res INTEGER;
> > ...
> >   if b1 = 0 then
> >     declare _b1 INTEGER default res;
> >     declare _res INTEGER default res;
> >     select 1,b1+1 into _b1, _res from dual;
> >     set b1=_b1;
> >     set res=_res;
> >     -- _res is not needed, but we don't know because the select statement is
> not parsed
> >   end if;
> >   if b1 = 1 then
> >     declare _b1 INTEGER default res;
> >     declare _res INTEGER default res;
> >     select 2,b1+2 into b1, res from dual;
> >     set b1=_b1;
> >     set res=_res;
> >     -- same thing here, and we have declare two variables for each target
> variables
> >   end if;
> >
> > Perhaps we could
> >  - declare these temporary variables only one time in the first frame
> > of the stored procedure (may be tricky)
> >  - parse columns of each select to know what variables are really
> > assigned and reused  (heavy cost in cpu and time)
> >
> > - second : if we can't determine variables which must have a temporary
> variable, number of sp_instr_set and sp_instr_set_var will be too high and
> their cpu cost is not acceptable.
> >
> > My first solution has a fixed memory impact (and memory is not an issue
> nowadays), and especially a very light cpu cost.
> >
> > What do you think about ?
> 
> I agree that determining variables which must have a temporary pair by full
> scanning the SELECT list expressions is probably not a good idea.
> 
> Declaring these variables only one time is easy.
> I earlier made about the same trick with cursor parameters.
> See sp_pcontext::retrieve_field_definitions().
> 
> The idea is that we can put such backup variables into a separate child
> sp_pcontext frame, to make sure that only one backup variable exists if the
> real variable appears multiple time as a SELECT INTO target. Having a
> dedicated frame allows:
> - to add variables in the middle of a BEGIN..END block, without having to re-
> enumerate local variables of the same block.
> - handle unique names
> 
> See the comment in  sp_pcontext::retrieve_field_definitions() about frame
> positions and run-time positions, and the CURSOR declaration grammar in
> sql_yacc_ora.yy.
> 
> 
> Every sp_pcontext frame should have one sp_pcontext frame for backup
> variables, which should be put into m_children.
> 
> sp_pcontext should probably have a new flag:
>   bool m_is_backup;
> So we can iterate through m_children and find the backup frame.
> 
> Another option is to add:
> 
>   int m_backup_child_context_index;
> 
> which will store -1 if the current frame does not have a backup child yet, or a
> non-negative value meaning the index in m_children.
> So a new method could looks like this:
> 
> sp_pcontext *get_backup_context()
> {
>   if (m_backup_child_contex_index < 0)
>     return 0;
>   return m_children(m_backup_child_context_index);
> }
> 
> 
> >
> > Regard,
> > Jérôme.
> >
> >
> >> -----Message d'origine-----
> >> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx] Envoyé : lundi 30
> >> octobre 2017 10:02 À : jerome brauge Objet : Re: MDEV-14139 Anchored
> >> data types for variables
> >>
> >>
> >>
> >> On 10/28/2017 07:29 PM, Alexander Barkov wrote:
> >>>
> >>> On 10/27/2017 10:27 PM, Alexander Barkov wrote:
> >>>>   Hello Jerome,
> >>>>
> >>>> I have implemented "MDEV-14139 Anchored data types for variables".
> >>>> and pushed it to bb-10.2-ext:
> >>>>
> >>>>
> >>
> https://github.com/MariaDB/server/commit/5dd5253f7e50c21fa758e2eb58f
> >> 3
> >>>> aa9c9754e733
> >>>>
> >>>> So now it should be easier to implement consistent SET by creating
> >>>> backup variables.
> >>>>
> >>>>
> >>>> LEX::sp_variable_declarations_vartype_finalize() implements the
> >>>> logic which copies data type from another variable.
> >>>>
> >>>>
> >>>> The idea is that for all variables, which are assignment targets in
> >>>> a SET or a SELECT INTO statement, we create a backup variable.
> >>>>
> >>>> It will involve these calls for every such variable:
> >>>>
> >>>> LEX::sp_variable_declarations_init(thd, 1);
> >>>> sp_pcontext::add_variable(thd, backup_variable_name);
> >>>> LEX::sp_variable_declarations_vartype_finalize(thd, 1,
> >>>> orig_variable_name, def);
> >>>>
> >>>> where "def" is Item_splocal created for the original variable.
> >>>
> >>> Just an idea: instead of creating sp_instr_set, it's easier to
> >>> introduce a new class sp_instr_set_var, to copy the value from one
> >>> variable to another variable.
> >>>
> >>> This operation will not need neither Item, nor sp_lex_keeper. It
> >>> will only need two offsets:
> >>> - the source variable offset and
> >>> - the target variable offset.
> >>>
> >>> Using these offsets, we can access to
> >>> spcont->m_var_table->field[source] and
> >>> spcont->spcont->m_var_table->field[target]
> >>> and copy the value between them using Field::store_field().
> >>>
> >>> This won't however for the ROW variables at the moment, because ROW
> >>> fields are stored in the Item_spvar_args::m_table member of
> >>> Item_field_row.
> >>>
> >>> It seems we need a new class Field_row and move Virtual_tmp_table
> >>> from Item_field_row to Field_row.
> >>>
> >>> I will try to do it.
> >>
> >>
> >> I have implemented "MDEV-14212 Add Field_row for SP ROW variables"
> >> and pushed to bb-10.2-ext.
> >>
> >> Also, added a comment:
> >>
> >> MDEV-13418 Compatibility: The order of evaluation of SELECT..INTO
> >> assignments
> >>
> >>
> >> Now, when MDEV-14212 is done, these declarations:
> >>
> >>     DECLARE a_tmp TYPE OF a DEFAULT a;
> >>     DECLARE b_tmp TYPE OF b DEFAULT b;
> >>
> >> and these assignments:
> >>
> >>     SET a=a_tmp;
> >>     SET b=b_tmp;
> >>
> >> can use a new sp_instr_setvar instead of sp_inst_set.
> >>
> >> The new command sp_instr_setvar can do direct copying between two
> >> spcont->m_var_table->field[XXX], without a need to create Item and LEX.
> >>
> >>>
> >>>>
> >>>> Greetings.
> >>>>