maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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.

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.

> 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.

> +    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).

> +  }
> +  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?

> +      return true;
> +    }
> +    return false;
> +  }
> +
>    table_map not_null_tables_cache;
>  
>    enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC,
> diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
> index 75abd9906cd..e45fe6ef6d7 100644
> --- a/sql/item_strfunc.cc
> +++ b/sql/item_strfunc.cc
> @@ -3204,6 +3239,39 @@ static String *default_pad_str(String *pad_str, CHARSET_INFO *collation)
>    return pad_str;
>  }
>  
> +
> +Item_func_lpad*
> +Item_func_lpad::create(THD *thd, const LEX_CSTRING &name,
> +                       List<Item> *item_list)
> +{
> +  return item_func_create_2_3_args<Item_func_lpad>(thd, name, item_list);

may be item_func_create_2_3_args should be in Item_func?
Like create_check_args_ge() is?
and may be create_check_args_ge() should be a template too?

> +}
> +
> +
> +Item_func_lpad_oracle*
> +Item_func_lpad_oracle::create(THD *thd, const LEX_CSTRING &name,
> +                              List<Item> *item_list)
> +{
> +  return item_func_create_2_3_args<Item_func_lpad_oracle>(thd, name, item_list);
> +}
> +
> +
> +Item_func_rpad*
> +Item_func_rpad::create(THD *thd, const LEX_CSTRING &name,
> +                       List<Item> *item_list)
> +{
> +  return item_func_create_2_3_args<Item_func_rpad>(thd, name, item_list);
> +}
> +
> +
> +Item_func_rpad_oracle*
> +Item_func_rpad_oracle::create(THD *thd, const LEX_CSTRING &name,
> +                              List<Item> *item_list)
> +{
> +  return item_func_create_2_3_args<Item_func_rpad_oracle>(thd, name, item_list);
> +}
> +
> +
>  bool Item_func_pad::fix_length_and_dec()
>  {
>    if (arg_count == 3)
> 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);

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