← Back to team overview

maria-developers team mailing list archive

Re: 96b40a5e2f5: MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption

 

Hello Sergei,


On 4/21/22 6:44 PM, Alexander Barkov wrote:
   Hello Sergei,

Here's a new version:

https://github.com/MariaDB/server/commit/23a0a8becc68c867821350e6edd2e287c947088b


Now sql_mode dependent functions print themselves to FRM like this:

- substr()         - MariaDB version
- substr_oracle()  - Oracle version

for downgrade compatibility.

Yet another update:

https://github.com/MariaDB/server/commit/d67c3f88883b616a9adf3abba43938c3a07a5eee


Now the view_body_utf8 field in a view FRM file is written
with forced qualifiers.

It's used for I_S.VIEW.VIEW_DEFINITION output as is.






On 4/20/22 3:25 PM, Alexander Barkov wrote:
Hello Sergei,



On 4/19/22 7:15 PM, Alexander Barkov wrote:
Hello Sergei,
On 4/19/22 1:17 AM, Sergei Golubchik wrote:
Hi, Alexander,

Some comments/questions here. I'll need to look at the patch again after
I understand why you did it this way better.

Thanks for the review. My answers follow below.

A new version is here:

https://github.com/MariaDB/server/commit/50bb0987e10e660326763de38231d63cfc1ad17d


A new version is here:

https://github.com/MariaDB/server/commit/447f241e17e54f170e14a11bb6320fba193a6b4a


Now qualifiers are printed only if the current (according to sql_mode) does not match.

But EXPLAIN EXTENDED forces the qualifier.



On Apr 18, Alexander Barkov wrote:
revision-id: 96b40a5e2f5 (mariadb-10.3.33-61-g96b40a5e2f5)
parent(s): 7355f7b1f5c
author: Alexander Barkov
committer: Alexander Barkov
timestamp: 2022-04-07 16:22:17 +0400
message:

     MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption

A detailed comment here is a must.
What was the problem, how it was fixed.

Added.


diff --git a/sql/item_func.h b/sql/item_func.h
index 754b1cd1eb2..d844e45280a 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -35,6 +35,36 @@ extern "C"                /* Bug in BSDI include file */
  #include <cmath>
+template<class FUNC>
+FUNC* item_func_create_2_3_args(THD *thd, const LEX_CSTRING &name,
+                                List<Item> *item_list)
+{
+  int arg_count= item_list ? item_list->elements : 0;
+
+  switch (arg_count) {
+  case 2:
+  {
+    Item *param_1= item_list->pop();
+    Item *param_2= item_list->pop();
+    return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2);
+  }
+  case 3:
+  {
+    Item *param_1= item_list->pop();
+    Item *param_2= item_list->pop();
+    Item *param_3= item_list->pop();
+    return new (get_thd_memroot(thd)) FUNC(thd, param_1, param_2, param_3);

A constructor that takes a List<Item> wouldn't need a switch.
And wouldn't need a template either, your create_check_args_ge()
isn't a template.

Right. I added new constructors with List<Item>.

Now the template item_func_create_2_3_args is not needed any more.

create() methods now look like this in the new version:


Item_func_lpad_oracle*
Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name,
                               List<Item> *item_list)
{
   if (create_check_args_between(name, item_list, 2, 3))
     return NULL;
   return new (thd->mem_root) Item_func_lpad_oracle(thd, *item_list);
}




+    break;
+  }
+  default:
+    my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);
+    break;
+  }
+
+  return NULL;
+}
+
+
  class Item_func :public Item_func_or_sum
  {
    void sync_with_sum_func_and_with_field(List<Item> &list);
@@ -56,8 +86,41 @@ class Item_func :public Item_func_or_sum
    bool check_argument_types_can_return_text(uint start, uint end) const;     bool check_argument_types_can_return_date(uint start, uint end) const;     bool check_argument_types_can_return_time(uint start, uint end) const;
+
+  void print_schema_name_if_needed(String *to) const
+  {
+    const LEX_CSTRING schema_name= schema_name_cstring();
+    if (schema_name.length)
+    {
+      to->append(schema_name);
+      to->append('.');
+    }

this should check that schema_name isn't the same as the
schema in the path (implicit path, assumed by the current sql_mode).

I will try to do this.

+  }
+  void print_maybe_qualified_name(String *to) const
+  {
+    print_schema_name_if_needed(to);
+    to->append(func_name());
+  }
+
  public:
+  // Print an error message for a builtin-schema qualified function call
+  static void wrong_param_count_error(const LEX_CSTRING &schema_name,
+                                      const LEX_CSTRING &func_name);
+
+  // Check that the number of arguments is greater or equal to "expected"
+  static bool create_check_args_ge(const LEX_CSTRING &name,
+                                   const List<Item> *item_list,
+                                   uint expected)
+  {
+    if (!item_list || item_list->elements < expected)
+    {
+      my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), name.str);

what's the point of your wrong_param_count_error() if you aren't using it?

I use it for DECODE(), because DECODE() has a different argument number
in sql_mode='' and sql_mode=ORACLE.

MariaDB [test]> select decode();
ERROR 1582 (42000): Incorrect parameter count in the call to native function 'mariadb_schema.decode'

I did not use it for the other sql_mode-dependent functions yet.

Which way of printing the schema part do you prefer in error
messages for sql_mode-dependent functions?

1a. Print always
1b. Print if the number of arguments differ (like now)
1c. Print if the Item_func_xxx qualifier is not equal
    to the current sql_mode implied qualifier.

2a. Always print the explicit qualifier if it was typed by the user
2b. Handle explicit qualifiers like implicit qualifiers


<cut>

diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
index 7f1b362d91d..6b0a351f76e 100644
--- a/sql/sql_lex.cc
+++ b/sql/sql_lex.cc
@@ -1542,7 +1541,18 @@ int Lex_input_stream::lex_one_token(YYSTYPE *yylval, THD *thd)
        if (lex->parsing_options.lookup_keywords_after_qualifier)
          next_state= MY_LEX_IDENT_OR_KEYWORD;
        else
-        next_state= MY_LEX_IDENT_START;    // Next is ident (not keyword)
+      {
+        /*
+          Next is:
+          - A qualified func with a special syntax:
+            mariadb_schema.REPLACE('a','b','c')
+            mariadb_schema.SUSTRING('a',1,2)
+            mariadb_schema.TRIM('a')
+          - Or an identifier otherwise. No keyword lookup is done,
+            all keywords are treated as identifiers.
+        */
+        next_state= MY_LEX_IDENT_OR_QUALIFIED_SPECIAL_FUNC;

I don't understand why you did it in the lexer and not in the parser, like

-     | REPLACE '(' expr ',' expr ',' expr ')'
+     | opt_schema REPLACE '(' expr ',' expr ',' expr ')'

or (if the above won't work in LARL(1) parser)

-     | ident_cli '.' ident_cli '(' opt_expr_list ')'
+     | ident_cli '.' func_2nd_part

with

+    func_2nd_part: ident_cli '(' opt_expr_list ')'
+                 | REPLACE '(' expr ',' expr ',' expr ')'

etc

(not reviewing lexer changes anymore, until I understand the above)

+      }
        if (!ident_map[(uchar) yyPeek()])    // Probably ` or "
          next_state= MY_LEX_START;
        return((int) c);

My grammar effectively looks like this exactly.

But the problem is that we don't resolve keywords
after the dot character. This is a performance optimization.

So what I changed in the tokenizer is that now in this sequence:

ident1.ident2(

the "ident2" part is resolved for keywords.




Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx



References