On 11/09/2017 03:03 PM, jerome brauge wrote: > > >> -----Message d'origine----- >> De : Alexander Barkov [mailto:[email protected]] >> Envoyé : jeudi 9 novembre 2017 11:05 >> À : jerome brauge >> Cc : 'MariaDB Developers ([email protected])' >> Objet : Re: MDEV-13418 Compatibility: The order of evaluation of >> SELECT..INTO assignments >> >> Hello Jerome, >> >> >> On 11/09/2017 12:03 PM, jerome brauge wrote: >>> Hello Alexander, >>> >>>> -----Message d'origine----- >>>> De : Alexander Barkov [mailto:[email protected]] Envoyé : mardi 7 >>>> novembre 2017 16:34 À : jerome brauge Cc : 'MariaDB Developers >>>> ([email protected])' >>>> Objet : Re: MDEV-13418 Compatibility: The order of evaluation of >>>> SELECT..INTO assignments >>>> >>>> Hi Jerome, >>>> >>>> >>>> On 11/06/2017 01:01 PM, jerome brauge wrote: >>>>> Hello Alexander, >>>>> We have checked all our stored procedure and the good news for us is >>>>> that >>>> only 2 statements are concerned ! >>>>> So, MDEV-13418 is not mandatory for us. >>>>> >>>>> However, can we consider to do a full parse just at compile time ? >>>>> It would allow to : >>>>> - determining variables which must have a temporary pair and >>>>> modifying stored source code to handle them >>>>> - issuing error or warning when these statements have syntax error >>>>> or call unknown functions ( like Oracle, SQLserver , UDB , .. ) >>>> >>>> I'm not sure I understood about full parser at compile time. >>>> Can you please give an example? >>>> >>> >>> Currently, the following function is compiled without any warning and run >> fine as long as b1 is not between 1 and 5. >>> >>> delimiter / >>> CREATE or replace procedure p1(b1 integer) BEGIN >>> declare res INTEGER default 0; >>> if b1 = 0 then >>> -- unknow variable >>> set res=x; >>> end if; >>> if b1 = 1 then >>> -- unknow function in set >>> set res=lenngth('v'); >>> end if; >>> if b1 = 2 then >>> -- unknow variable >>> select a into res; >>> end if; >>> if b1 = 3 then >>> -- unknow table / column / function >>> select 1 into res from unknowtable where unknowcolumn = >> unknowfunc(); >>> end if; >>> if b1 = 4 then >>> -- unknow group by column >>> select 1 into res from dual group by 13; >>> end if; >>> if b1 = 5 then >>> -- unknow function in test >>> if coalesc(res,'x') = 'y' then >>> set res:='z'; >>> end if; >>> end if; >>> >>> END >>> / >>> call p1(-1) >>> / >>> >>> My idea is to have different behavior during "create procedure" and >> load_routine. >>> Load_routine must be fast but "create procedure" can be slower and do >>> more check (at least check existence of variables, tables, columns, >> functions and procedure) It's not truly acceptable to throw this kind of >> error >> only when the code is really executed. >> >> At CREATE time, we can only add a warning when an unknown procedure or >> function is called. Issuing errors is not possible, because one would not be >> able to create two routines mutually executing each other (now it is >> possible). >> > > I agree, a warning is fine for an undefined called function. > But is there any chance to have control on used variables and columns ?
In this example: DELIMITER $$ CREATE OR REPLACE PROCEDURE p1() BEGIN DECLARE res INT DEFAULT 0; SET res=x; END; $$ DELIMITER ; I think we should generate the error at CREATE time, in all sql_mode's. Looks like a bug that we don't do it. For things like: SELECT 1 INTO var FROM unknowtable where unknowcolumn = 10; We cannot return errors by default. This behavior has been in MySQL/MariaDB since the time when stored procedures appeared: tables and columns resolution is done at execute time. But adding a new sql_mode to return errors in such cases should be possible. The solution should handle cases when CREATE PROCEDURE was done with a known column, but then the column was removed by a ALTER TABLE. So it seems both CREATE and load must do this. I'm not sure how it can affect performance. > > >> So issuing errors is possible at execution time only. >> >> When a routine is loaded, it could be checked for all called procedures and >> functions, and a warning or an error could be issued if some routine is >> missing >> (independently on conditions in control structures). >> >> >>> >>> To do this, each select list expression have to be "parse" and so we can >> determine variables which must have a temporary pair. >>> Ex: >>> CREATE PROCEDURE p1(res OUT VARCHAR) >>> AS >>> b1 INT:=10; >>> BEGIN >>> SELECT 1,CAST(b1 AS VARCHAR(10)) INTO b1, res FROM dual; END; >>> >>> Can be transform and store as >>> CREATE PROCEDURE p1(res OUT VARCHAR) >>> AS >>> b1 INT:=10; >>> BEGIN >>> DECLARE _b1 INTEGER DEFAULT b1; >>> SELECT 1,CAST(b1 AS VARCHAR(10)) INTO b1, res FROM dual; >>> SET b1=_b1; >>> END; >>> >>> So, no additional works should be done during load_routine. >> >> Now we always put the routine into mysql.proc AS IS, how the definer >> defined it. >> >> I'm not sure if we should rewrite CREATE statements... >> >> >>>>> >>>>> Without this second point we are exposed to keystrokes errors and to >>>>> find >>>> these mistakes only at runtime. >>>>> It's already very hard to have tests that cover most of the code, >>>>> but here >>>> we must have 100% of coverage ! >>>>> Fortunately for us, SQLServer and Oracle does the work for us :-) >>>>> >>>>> Other idea : maybe the behavior of "set" could be conditioned by >>>>> another >>>> sql_mode. >>>>> Sybase , SQLServer, Mariadb : multiple "set" are evaluated as >>>>> multiple >>>> single "set" >>>>> DB2 : work as select (each expression is evaluated before the >>>>> assignments are performed) >>>>> >>>>> We could have three sql_mode: >>>>> - SET_CONSISTENCY_UPDATE >>>>> - SET_CONSISTENCY_SELECT >>>>> - SET_CONSISTENCY_SET >>>> >>>> What would SET_CONSISTENCY_SELECT stand for? >>>> Can you give an example? >>> >>> I thought to: >>> - SET_CONSISTENCY_UPDATE for MDEV-13417 : UPDATE produces wrong >> values >>> if an updated column is later used as an update source >>> - SET_CONSISTENCY_SELECT for MDEV-13418 : The order of evaluation of >>> SELECT..INTO assignments >>> - SET_CONSISTENCY_SET : evaluate expression before any assignment in >>> "grouped" SET >> >> Thanks! >> >> I thought that behavior of SET is directly related to behavior of SELECT >> INTO, >> as both save data into variables. Why have two different sql_mode options >> for SET and SELECT INTO? >> >> Btw, why not have just one option SET_CONSISTENCY, which will control all >> three (UPDATE, SELECT INTO, SET) ? >> >> I didn't understand why you propose separate flags. >> Can you clarify please? >> > > For UPDATE , there is no workaround, we must implement it for Oracle > compatibility. > > For SELECT INTO : I've always thinking that it's a bad practice to use and > assign the same variable in one statement. When reading the code, we can't > know what the developer meant to do. > It's not hard to correct this kind of statement. > Introduce an overhead for most of select statement (we can optimize it when > select return only one value), is not acceptable for us and we don't want > enable this feature. > > For SET, most of RDBMS (that I know) works as Mariadb : > - Sybase and SQLServer > - and implicitly ORACLE because it can only do single assignment at one time > Others like IBM DB2 have same behavior for multi set and select into. > > So it's not a required feature for Oracle compatibility. We cannot waste sql_mode bits for small features. We'll run out of the second 32 bit slot quickly. In this case, I'm inclined to have a single SET_CONSISTENCY bit for all three purposes (UPDATE, SELECT INTO, SET). We should just make sure that SELECT INTO (and SET) works with a good performance. Multi-SET is less important than SELECT INTO, as multi-SET is neither standard, nor Oracle-compatible. > > >>> >>>> >>>> Thanks! >>>> >>>> >>>> >>>>> >>>>> Regards, >>>>> Jérôme. >>>>> >>>>> >>>>>> -----Message d'origine----- >>>>>> De : jerome brauge >>>>>> Envoyé : vendredi 3 novembre 2017 12:18 À : 'Alexander Barkov' >>>>>> Cc : MariaDB Developers ([email protected]) >>>>>> Objet : RE: MDEV-13418 Compatibility: The order of evaluation of >>>>>> SELECT..INTO assignments >>>>>> >>>>>> 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:[email protected]] Envoyé : vendredi 3 >>>>>>> novembre 2017 11:02 À : jerome brauge Cc : MariaDB Developers >>>>>>> ([email protected]) >>>>>>> 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:[email protected]] 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 >>>>>>>>> spcont->and >>>>>> LEX. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Greetings. >>>>>>>>>>> _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
