16 Mar
2017
16 Mar
'17
9:18 a.m.
Hello Jerome,
On 03/14/2017 06:07 PM, jerome brauge wrote:
> Hi Alexander,
> Can you review this patch ?
> I could add more tests when your others points will be corrected.
I have a few suggestions.
Let's discuss different topics in separate letters.
Here're my suggestions for sql_yacc_ora.yy
I found the the grammar was quite hard to read:
- Too similar names sp_decl_body_list vs sp_decls_body_list looked confusing
- It's not easy to follow when sp_push_cursors_declaration_instr() was
actually called.
I tried to add new HANDLER related rules:
opt_sp_decl_handler_list
sp_decl_handler_list
and renamed sp_decl_body to sp_decl_non_handler
(to make the name more self-descriptive).
So now variable/cursor/exception/condition declarations
are not allowed after handler declarations syntactically
and just cause a syntax error.
Advantages:
- I believe it's easier to read the rules this way.
- There is no a need for a new method sp_declarations_join_ora()
- It's now obvious that sp_push_cursors_declaration_instr() is called
between non-handler and optional handler declarations.
Disadvantages:
- The error message
"Variable or condition declaration after cursor or handler declaration"
was probably more readable than
"You have an error in your SQL syntax"
but I think this is not a big problem, because:
- Oracle users should normally use EXCEPTION rather than HANDLER.
- Later we can introduce delayed HANDLER declarations in the same way
with what we're doing for CURSORs now. So the error will just be gone.
What do you think about this proposal?
I'm sending two diffs:
- cursor-00-incremental.diff is a patch on top of your patch
- cursor-00-full.diff is the full patch
(your patch combined with cursor-00-incremental.diff)
Also, I found that in your patch a semicolon was missing at the end of
some rules. Please don't forgot semicolons, we require semicolons
in our coding style.
Thanks.
>
> Thanks.
> Jérôme.
>
>
>> -----Message d'origine-----
>> De : Alexander Barkov [mailto:bar@mariadb.org]
>> Envoyé : mardi 14 mars 2017 11:54
>> À : jerome brauge
>> Cc : MariaDB Developers (maria-developers@lists.launchpad.net)
>> Objet : Re: MDEV-10598 - bb-10.2-compatibility
>>
>> Hi Jerome,
>>
>> On 03/14/2017 02:29 PM, jerome brauge wrote:
>>> Thanks Alexander,
>>>
>>> I have two other problems to finalize my patch:
>>> - Attached script cur_err_warning.sql produce a warning 1931 (Query
>> execution was interrupted. The query examined at least 2 rows, which
>> exceeds LIMIT ROWS EXAMINED (0). The query result may be incomplete)
>> and I don't see why.
>>
>> That's my fault. Please ignore this warning for now.
>> This warning happens when the procedure opens a cursor to resolve its
>> structure. I will suppress this warning later.
>>
>>
>>>
>>> - Attached script cur_err_field_name.sql produce an error 4057 (HY000) at
>> line 23: Row variable 'rec2' does not have a field 'a'
>>
>> This is also my bug.
>> The problem is that rec2 has a name "rec1.a" instead of just "a".
>>
>>
>>
>> Please change
>>
>> CURSOR cur2 IS SELECT rec1.a ;
>>
>> to
>>
>> CURSOR cur2 IS SELECT rec1.a AS a;
>>
>>
>>>
>>> These two scripts work fine on oracle.
>>>
>>> Jérôme.
>>>
>>>
>>>> -----Message d'origine-----
>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mardi 14 mars
>>>> 2017 11:11 À : jerome brauge Objet : Re: MDEV-10598 -
>>>> bb-10.2-compatibility
>>>>
>>>> Hello Jerome,
>>>>
>>>>
>>>>
>>>> On 03/13/2017 07:46 PM, jerome brauge wrote:
>>>>> Alexander,
>>>>> I think there is a little bug with ROWTYPE: when I affect a variable
>>>>> with a
>>>> field value, the server crash.
>>>>> I attached the script which cause the issue.
>>>>
>>>> Thanks for reporting this.
>>>>
>>>>
>>>> This quick patch fixes the problem:
>>>>
>>>>
>>>> diff --git a/sql/sp_head.cc b/sql/sp_head.cc index faf9f5f..745f982
>>>> 100644
>>>> --- a/sql/sp_head.cc
>>>> +++ b/sql/sp_head.cc
>>>> @@ -347,6 +347,12 @@ Item *
>>>> sp_prepare_func_item(THD* thd, Item **it_addr, uint cols) {
>>>> DBUG_ENTER("sp_prepare_func_item");
>>>> + if (!(*it_addr)->fixed &&
>>>> + (*it_addr)->fix_fields(thd, it_addr)) {
>>>> + DBUG_PRINT("info", ("fix_fields() failed"));
>>>> + DBUG_RETURN(NULL);
>>>> + }
>>>> it_addr= (*it_addr)->this_item_addr(thd, it_addr);
>>>>
>>>> if ((!(*it_addr)->fixed &&
>>>>
>>>>
>>>>
>>>> But I'm still thinking.
>>>> Perhaps I'll end some with some different patch.
>>>> I let you know when the final fix is pushed.
>>>>
>>>>
>>>>>
>>>>> This is the call stack :
>>>>>
>>>>>> mysqld.exe!my_sigabrt_handler(int sig) Line 477 C
>>>>> mysqld.exe!raise(int signum) Line 516 C++
>>>>> mysqld.exe!abort() Line 64 C++
>>>>> mysqld.exe!common_assert_to_stderr_direct(const wchar_t * const
>>>> expression, const wchar_t * const file_name, const unsigned int
>>>> line_number) Line 124 C++
>>>>> mysqld.exe!common_assert_to_stderr<wchar_t>(const wchar_t *
>>>> const expression, const wchar_t * const file_name, const unsigned int
>>>> line_number) Line 138 C++
>>>>> mysqld.exe!common_assert<wchar_t>(const wchar_t * const
>>>> expression, const wchar_t * const file_name, const unsigned int
>>>> line_number, void * const return_address) Line 383 C++
>>>>> mysqld.exe!_wassert(const wchar_t * expression, const wchar_t *
>>>> file_name, unsigned int line_number) Line 404 C++
>>>>> mysqld.exe!Item_splocal_row_field_by_name::this_item_addr(THD
>>>> * thd, Item * * it) Line 1814 C++
>>>>> mysqld.exe!sp_prepare_func_item(THD * thd, Item * * it_addr,
>>>> unsigned int cols) Line 350 C++
>>>>> mysqld.exe!sp_eval_expr(THD * thd, Item * result_item, Field *
>>>> result_field, Item * * expr_item_ptr) Line 391 C++
>>>>> mysqld.exe!sp_rcontext::set_variable(THD * thd, unsigned int idx,
>>>> Item * * value) Line 566 C++
>>>>> mysqld.exe!sp_instr_set::exec_core(THD * thd, unsigned int *
>>>> nextp) Line 3378 C++
>>>>> mysqld.exe!sp_lex_keeper::reset_lex_and_exec_core(THD * thd,
>>>> unsigned int * nextp, bool open_tables, sp_instr * instr) Line 3097 C++
>>>>> mysqld.exe!sp_instr_set::execute(THD * thd, unsigned int * nextp)
>>>> Line 3371 C++
>>>>> mysqld.exe!sp_head::execute(THD * thd, bool
>>>> merge_da_on_success) Line 1261 C++
>>>>> mysqld.exe!sp_head::execute_procedure(THD * thd, List<Item> *
>>>> args) Line 2086 C++
>>>>> mysqld.exe!do_execute_sp(THD * thd, sp_head * sp) Line 2890
>>>> C++
>>>>> mysqld.exe!mysql_execute_command(THD * thd) Line 5919 C++
>>>>> mysqld.exe!mysql_parse(THD * thd, char * rawbuf, unsigned int
>>>> length, Parser_state * parser_state, bool is_com_multi, bool
>>>> is_next_command) Line 8006 C++
>>>>> mysqld.exe!dispatch_command(enum_server_command command,
>>>> THD * thd, char * packet, unsigned int packet_length, bool is_com_multi,
>>>> bool is_next_command) Line 1821 C++
>>>>> mysqld.exe!do_command(THD * thd) Line 1369 C++
>>>>> mysqld.exe!threadpool_process_request(THD * thd) Line 346
>>>> C++
>>>>> mysqld.exe!tp_callback(TP_connection * c) Line 192 C++
>>>>> mysqld.exe!tp_callback(_TP_CALLBACK_INSTANCE * instance, void *
>>>> context) Line 377 C++
>>>>> mysqld.exe!work_callback(_TP_CALLBACK_INSTANCE * instance,
>>>> void * context, _TP_WORK * work) Line 451 C++
>>>>>
>>>>> Can you reproduce this ?
>>>>>
>>>>> Jérôme.
>>>>>
>>>>>> -----Message d'origine-----
>>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 13
>>>>>> mars
>>>>>> 2017 14:47 À : jerome brauge Objet : Re: MDEV-10598 -
>>>>>> bb-10.2-compatibility
>>>>>>
>>>>>> Jérôme,
>>>>>>
>>>>>> On 03/13/2017 05:43 PM, jerome brauge wrote:
>>>>>>> Hello Alexander,
>>>>>>> I have to do some changes in the patch and add some tests cases
>>>>>>> (with row type) I think it will be ready this afternoon (CET).
>>>>>>
>>>>>> Excellent. You rock!
>>>>>>
>>>>>>>
>>>>>>> Jérôme.
>>>>>>>
>>>>>>>
>>>>>>>> -----Message d'origine-----
>>>>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 13
>>>>>>>> mars
>>>>>>>> 2017 14:38 À : jerome brauge Cc : maria-developers Objet : Re:
>>>>>>>> MDEV-10598 - bb-10.2-compatibility
>>>>>>>>
>>>>>>>> Hello Jerome,
>>>>>>>>
>>>>>>>> will you try to apply your patch on top of the current bb-10.2-
>>>>>> compatibility?
>>>>>>>>
>>>>>>>> Or should I do that?
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>
>>>>>>>> On 03/10/2017 02:36 PM, Alexander Barkov wrote:
>>>>>>>>> Hello Jerome,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 02/27/2017 11:39 PM, jerome brauge wrote:
>>>>>>>>>> Hello Alexander,
>>>>>>>>>> Thanks for the explanation.
>>>>>>>>>> It's something we do not use. I did not think about it.
>>>>>>>>>> I look forward to your patch.
>>>>>>>>>
>>>>>>>>> I pushed these tasks:
>>>>>>>>>
>>>>>>>>> MDEV-10581 sql_mode=ORACLE: Explicit cursor FOR LOOP
>>>>>>>>> MDEV-12098 sql_mode=ORACLE: Implicit cursor FOR loop
>>>>>>>>> MDEV-12011 sql_mode=ORACLE: cursor%ROWTYPE in variable
>>>>>> declarations
>>>>>>>>> MDEV-12133 sql_mode=ORACLE: table%ROWTYPE in variable
>>>>>> declarations
>>>>>>>>>
>>>>>>>>> Please clone the branch again.
>>>>>>>>> Git pull will not work, because I recently rebased
>>>>>>>>> bb-10.2-compatibility on top of the latest 10.2.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> When implementing cursor%ROWTYPE, I had your patch in mind
>> and
>>>>>> made
>>>>>>>>> some refactoring to help us apply MDEV-10598 easier.
>>>>>>>>> Please see a comment to MDEV-12011 in "git log".
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Now the tricky thing (when adding your patch) is to make sure
>>>>>>>>> that this work fine:
>>>>>>>>>
>>>>>>>>> CREATE PROCEDURE p1
>>>>>>>>> AS
>>>>>>>>> a INT:=10;
>>>>>>>>> CURSOR cur1 IS SELECT a;
>>>>>>>>> rec1 cur1%ROWTYPE;
>>>>>>>>> CURSOR cur2 IS SELECT rec1.a;
>>>>>>>>> rec2 cur2%ROWTYPE;
>>>>>>>>> BEGIN
>>>>>>>>> OPEN cur2;
>>>>>>>>> FETCH cur2 INTO rec2;
>>>>>>>>> CLOSE cur2;
>>>>>>>>> SELECT rec2.a;
>>>>>>>>> END;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I.e. a set of intermixed CURSOR and cursor%ROWTYPE variable
>>>>>>>>> declarations referencing each other recursively.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Jérôme.
>>>>>>>>>>
>>>>>>>>>>> -----Message d'origine-----
>>>>>>>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi
>>>>>>>>>>> 27 février 2017 11:28 À : jerome brauge Cc : maria-developers
>> Objet :
>>>>>>>>>>> Re: MDEV-10598 - bb-10.2-compatibility
>>>>>>>>>>>
>>>>>>>>>>> Hello Jerome,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 02/21/2017 07:18 PM, jerome brauge wrote:
>>>>>>>>>>>> Hello Alexander,
>>>>>>>>>>>> I've done this patch for MDEV-10598.
>>>>>>>>>>>> Can you review it ?
>>>>>>>>>>>
>>>>>>>>>>> It seems we'll have to postpone this patch.
>>>>>>>>>>>
>>>>>>>>>>> I'm currently working on:
>>>>>>>>>>>
>>>>>>>>>>> MDEV-10598 Variable declarations can go after cursor
>>>>>>>>>>> declarations
>>>>>>>>>>> MDEV-12011 sql_mode=ORACLE: cursor%ROWTYPE in variable
>>>>>>>> declarations
>>>>>>>>>>> So the trick with postponing variable declarations using a
>>>>>>>>>>> temporary list might not work properly after adding MDEV-10598
>>>>>>>>>>> abd MDEV-12011, the order of cursors and variables is important.
>>>>>>>>>>>
>>>>>>>>>>> Example:
>>>>>>>>>>>
>>>>>>>>>>> DECLARE
>>>>>>>>>>> CURSOR cur1 IS SELECT a,b FROM t1;
>>>>>>>>>>> v cur1%ROWTYPE;
>>>>>>>>>>> CURSOR cur2 IS SELECT v.a, v.b FROM DUAL; BEGIN
>>>>>>>>>>> ...
>>>>>>>>>>> END;
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So the order of cur1, v and cur2 is important.
>>>>>>>>>>>
>>>>>>>>>>> I'll let you known when I'm ready with %ROWTYPE tasks.
>>>>>>>>>>>
>>>>>>>>>>> Thanks!
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Jérôme.
>>>>>>>>>>>>