Hi, Sergei,

Thanks for your review. Please see my comments inline.

The new path version is here:

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


On 12/11/23 19:48, Sergei Golubchik wrote:
 > Hi, Alexander,
 >
 >> commit 2821cf816a4
 >> Author: Alexander Barkov 
 >> Date:   Fri Jun 23 13:24:02 2023 +0400
 >>
 >>      MDEV-31531 Remove my_casedn_str() and my_caseup_str()
 >
 > please, explain here why
 >

Done

 >>
 >> diff --git a/include/m_ctype.h b/include/m_ctype.h
 >> index 9d13989d1fe..d2629121cfe 100644
 >> --- a/include/m_ctype.h
 >> +++ b/include/m_ctype.h
 >> @@ -848,6 +844,26 @@ struct charset_info_st
 >>       return (cset->casedn)(this, src, srclen, dst, dstlen);
 >>     }
 >>
 >> +  /* Convert to a lower-cased 0-terminated string */
 >> +  size_t casedn_z(const char *src, size_t srclen,
 >> +                  char *dst, size_t dstlen) const
 >> +  {
 >> +    DBUG_ASSERT(dstlen);
 >
 > DBUG_ASSERT(src != dst)  ?

Done, added the assert.

 >
 >> +    size_t len= casedn(src, srclen, dst, dstlen - 1);
 >> +    dst[len]= '\0';
 >> +    return len;
 >> +  }
 >> +
 >> +  /* Convert to a upper-cased 0-terminated string */
 >> +  size_t caseup_z(const char *src, size_t srclen,
 >> +                  char *dst, size_t dstlen) const
 >> +  {
 >> +    DBUG_ASSERT(dstlen);

And added the assert here, too.

 >> +    size_t len= caseup(src, srclen, dst, dstlen - 1);
 >> +    dst[len]= '\0';
 >> +    return len;
 >> +  }
 >> +
 >>     uint caseup_multiply() const
 >>     {
 >>       return (cset->caseup_multiply)(this);
 >> @@ -1512,6 +1528,14 @@ size_t my_copy_fix_mb(CHARSET_INFO *cs,
 >>   /* Functions for 8bit */
 >>   extern size_t my_caseup_str_8bit(CHARSET_INFO *, char *);
 >>   extern size_t my_casedn_str_8bit(CHARSET_INFO *, char *);
 >> +static inline size_t my_caseup_str_latin1(char *str)
 >> +{
 >> +  return my_caseup_str_8bit(&my_charset_latin1, str);
 >> +}
 >> +static inline size_t my_casedn_str_latin1(char *str)
 >> +{
 >> +  return my_casedn_str_8bit(&my_charset_latin1, str);
 >> +}
 >
 > may be my_charset_ascii? with an assert that it's indeed ascii?
 > and a comment that it's used for generated temp table names and such

"ascii" is now not a fully built-in character set.
It's generated from conf_to_src. We should make it fully
built-in like my_charset_latin1 eventually.

For now, as we agreed on slack, I added a comment before every
my_caseXX_str_latin1() call explaining that the argument is
in fact a pure ASCII string, so using latin1 is fine.



 >> diff --git a/sql/char_buffer.h b/sql/char_buffer.h
 >> index 7b855f5401c..1091775259d 100644
 >> --- a/sql/char_buffer.h
 >> +++ b/sql/char_buffer.h
 >> @@ -71,25 +76,92 @@ class CharBuffer
 >>       m_buff[m_length]= '\0';
 >>       return *this;
 >>     }
 >> +  CharBuffer & copy_caseup(CHARSET_INFO *cs, const 
LEX_CSTRING &str)
 >> +  {
 >> +    DBUG_ASSERT(!buffer_overlaps(str));
 >> +    m_length= cs->cset->caseup(cs, str.str, str.length, m_buff, 
buff_sz);
 >> +    DBUG_ASSERT(is_sane());
 >> +    m_buff[m_length]= '\0'; // See comments in copy_casedn()
 >> +    return *this;
 >> +  }
 >>     CharBuffer & copy_casedn(CHARSET_INFO *cs, const 
LEX_CSTRING &str,
 >>                                       bool casedn)
 >>     {
 >>       casedn ? copy_casedn(cs, str) : copy_bin(str);
 >>       return *this;
 >>     }
 >> +
 >> +  // Append one character
 >> +  CharBuffer & append_char(char ch)
 >> +  {
 >> +    DBUG_ASSERT(is_sane());
 >> +    if (available_size())
 >> +    {
 >> +      m_buff[m_length++]= ch;
 >> +      m_buff[m_length]= '\0';
 >> +    }
 >> +    DBUG_ASSERT(is_sane());
 >> +    return *this;
 >> +  }
 >> +
 >> +  // Append a string
 >> +  CharBuffer & append_bin(const LEX_CSTRING &str)
 >
 > why _bin ? and why even have a suffix, and not just append() here and 
above?

Done, renamed.



 >> diff --git a/sql/item_sum.cc b/sql/item_sum.cc
 >> index bbd09a59267..824ed42ec91 100644
 >> --- a/sql/item_sum.cc
 >> +++ b/sql/item_sum.cc
 >> @@ -3769,20 +3769,15 @@ int 
group_concat_key_cmp_with_order_with_nulls(void *arg, const void *key1_arg,
 >>   }
 >>
 >>
 >> -static void report_cut_value_error(THD *thd, uint row_count, const 
char *fname)
 >> +static void report_cut_value_error(THD *thd, uint row_count,
 >> +                                   const LEX_CSTRING &fname)
 >>   {
 >> -  size_t fn_len= strlen(fname);
 >> -  char *fname_upper= (char *) my_alloca(fn_len + 1);
 >> -  if (!fname_upper)
 >> -    fname_upper= (char*) fname;                 // Out of memory
 >> -  else
 >> -    memcpy(fname_upper, fname, fn_len+1);
 >> -  my_caseup_str(&my_charset_latin1, fname_upper);
 >> +  CharBuffer fname_upper;
 >> +  fname_upper.copy_caseup(&my_charset_latin1, fname);
 >
 > why bother? we don't do it in other error cases

Done. Removed the casefolding.

 >
 >>     push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
 >>                         ER_CUT_VALUE_GROUP_CONCAT,
 >>                         ER_THD(thd, ER_CUT_VALUE_GROUP_CONCAT),
 >> -                      row_count, fname_upper);
 >> +                      row_count, fname_upper.ptr());
 >> -  my_afree(fname_upper);
 >>   }
 >>
 >>
 >> diff --git a/sql/lex_ident.h b/sql/lex_ident.h
 >> index f39273b7da5..db96e40cab8 100644
 >> --- a/sql/lex_ident.h
 >> +++ b/sql/lex_ident.h
 >> @@ -45,6 +46,10 @@ class Lex_ident_fs: public LEX_CSTRING
 >>     bool is_in_lower_case() const;
 >>     bool ok_for_lower_case_names() const;
 >>   #endif
 >> +  bool check_db_name_quick() const
 >
 > please, use a name that makes it clear what the return value is
 > Not
 >
 >    if (str.check_db_name_quick())
 >
 > but
 >
 >    if (str.is_invalid_db_name())

Historically we have a function check_db_name() returning the value
according to our coding notation (true on failure, false on success).
This function performs a similar thing. So I decided to call it
in a similar way. I propose to keep it as it. All developers are used
to check_db_name().


 >
 >> +  {
 >> +    return !length || length > NAME_LEN || str[length-1] == ' ';
 >> +  }
 >>   };
 >>
 >>
 >> @@ -160,4 +171,95 @@ class DBNameBuffer: public 
CharBuffer
 >>   };
 >>
 >>
 >> +class Identifier_chain2
 >> +{
 >> +  LEX_CSTRING m_name[2];
 >> +public:
 >> +  Identifier_chain2()
 >> +   :m_name{Lex_cstring(), Lex_cstring()}
 >> +  { }
 >> +  Identifier_chain2(const LEX_CSTRING &a, const LEX_CSTRING &b)
 >> +   :m_name{a, b}
 >> +  { }
 >> +
 >> +  const LEX_CSTRING& operator [] (size_t i) const
 >> +  {
 >> +    return m_name[i];
 >> +  }
 >> +
 >> +  static Identifier_chain2 split(const LEX_CSTRING &txt)
 >> +  {
 >> +    DBUG_ASSERT(txt.str[txt.length] == '\0'); // Expect 
0-terminated input
 >> +    const char *dot= strchr(txt.str, '.');
 >> +    if (!dot)
 >> +      return Identifier_chain2(Lex_cstring(), txt);
 >> +    size_t length0= dot - txt.str;
 >> +    Lex_cstring name0(txt.str, length0);
 >> +    Lex_cstring name1(txt.str + length0 + 1, txt.length - length0 - 1);
 >> +    return Identifier_chain2(name0, name1);
 >> +  }
 >> +
 >> +  // Export as a qualified name string: 'db.name'
 >> +  size_t make_qname(char *dst, size_t dstlen) const
 >> +  {
 >> +    return my_snprintf(dst, dstlen, "%.*s.%.*s",
 >> +                       (int) m_name[0].length, m_name[0].str,
 >> +                       (int) m_name[1].length, m_name[1].str);
 >> +  }
 >> +
 >> +  // Export as a qualified name string, allocate on mem_root.
 >> +  bool make_qname(MEM_ROOT *mem_root, LEX_CSTRING *dst) const
 >> +  {
 >> +    const uint dot= !!m_name[0].length;
 >> +    char *tmp;
 >> +    /* format: [pkg + dot] + name + '\0' */
 >> +    dst->length= m_name[0].length + dot + m_name[1].length;
 >> +    if (unlikely(!(dst->str= tmp= (char*) alloc_root(mem_root,
 >> +                                                     dst->length + 
1))))
 >> +      return true;
 >> +    snprintf(tmp, dst->length + 1, "%.*s%.*s%.*s",
 >> +            (int) m_name[0].length, (m_name[0].length ? 
m_name[0].str : ""),
 >> +            dot, ".",
 >> +            (int) m_name[1].length, m_name[1].str);
 >> +    return false;
 >> +  }
 >> +
 >> +  /*
 >> +    Build a separated two step name, e.g. "ident1/ident2", 
0-terminated.
 >> +    with an optional lower-case conversion.
 >
 > may be should be inside InnoDB?

I thought it's a very general purpose functionality and
can be reused on other places. So I internionally moved it
out of InnoDB.


 >> +    @param [OUT] dst      - the destination
 >> +    @param       dst_size - number of bytes available in dst
 >> +    @param       sep      - the separator character
 >> +    @param       casedn   - whether to convert components to lower case
 >> +    @return               - number of bytes written to "dst", not 
counting
 >> +                            the trailing '\0' byte.
 >> +  */
 >> +  size_t make_sep_name_opt_casedn(char *dst, size_t dst_size,
 >> +                                  int sep, bool casedn) const
 >> +  {
 >> +    /*
 >> +      Minimal possible buffer is 4 bytes: 'd/t\0'
 >> +      where 'd' is the database name, 't' is the table name.
 >> +      Callers should never pass a smaller buffer.
 >> +    */
 >> +    DBUG_ASSERT(dst_size > 4);
 >> +    DBUG_ASSERT(m_name[0].length + m_name[1].length + 2 < FN_REFLEN 
- 1);
 >> +    if (casedn)
 >> +    {
 >> +      CHARSET_INFO *cs= &my_charset_utf8mb3_general_ci;
 >> +      char *ptr= dst, *end= dst + dst_size;
 >> +      ptr+= cs->casedn(m_name[0].str, m_name[0].length, ptr, end - 
ptr - 2);
 >> +      *ptr++= (char) sep;
 >> +      ptr+= cs->casedn_z(m_name[1].str, m_name[1].length, ptr, end 
- ptr);
 >> +      return ptr - dst;
 >> +    }
 >> +    memcpy(dst, m_name[0].str, m_name[0].length);
 >> +    dst[m_name[0].length] = (char) sep;
 >> +    /* Copy the name and null-byte. */
 >> +    memcpy(dst + m_name[0].length + 1, m_name[1].str, 
m_name[1].length + 1);
 >> +    return m_name[0].length + 1/*slash*/ + m_name[1].length;
 >
 > s/slash/sep/

Done

 >
 >> +  }
 >> +};
 >> +
 >> +
 >>   #endif // LEX_IDENT_INCLUDED
 >> diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc
 >> index 6030202adc7..0fc1d4ce962 100644
 >> --- a/sql/log_event_server.cc
 >> +++ b/sql/log_event_server.cc
 >> @@ -5777,19 +5778,29 @@ int 
Table_map_log_event::do_apply_event(rpl_group_info *rgi)
 >>     /* Step the query id to mark what columns that are actually used. */
 >>     thd->set_query_id(next_query_id());
 >>
 >> +  db_mem_alloced= tname_mem_alloced= NAME_LEN *
 >> +                                     (lower_case_table_names ? 1 :
 >> + 
files_charset_info->casedn_multiply())
 >> +                                      + 1 /* for '\0' */;
 >
 > 1. may be db_mem_length/tname_mem_length instead of NAME_LEN?
 > 2. is the ?: condition right? Looks like it's inverted

1. Done.
2. Fixed. Thanks for noticing the wrong condition.

 >
 >>     if (!(memory= my_multi_malloc(PSI_INSTRUMENT_ME, MYF(MY_WME),
 >>                                   &table_list, (uint) 
sizeof(RPL_TABLE_LIST),
 >> -                                &db_mem, (uint) NAME_LEN + 1,
 >> -                                &tname_mem, (uint) NAME_LEN + 1,
 >> +                                &db_mem, (uint) db_mem_alloced,
 >> +                                &tname_mem, (uint) tname_mem_alloced,
 >>                                   NullS)))
 >>       DBUG_RETURN(HA_ERR_OUT_OF_MEM);
 >>
 >> -  db_mem_length= strmov(db_mem, m_dbnam) - db_mem;
 >> -  tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem;
 >>     if (lower_case_table_names)
 >>     {
 >> -    my_casedn_str(files_charset_info, (char*)tname_mem);
 >> -    my_casedn_str(files_charset_info, (char*)db_mem);
 >> +    db_mem_length= files_charset_info->casedn_z(m_dbnam, m_dblen,
 >> +                                                db_mem, 
db_mem_alloced);
 >> +    tname_mem_length= files_charset_info->casedn_z(m_tblnam, m_tbllen,
 >> +                                                   tname_mem,
 >> +                                                   tname_mem_alloced);
 >> +  }
 >> +  else
 >> +  {
 >> +    db_mem_length= strmov(db_mem, m_dbnam) - db_mem;
 >> +    tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem;
 >>     }
 >>
 >>     /* call from mysql_client_binlog_statement() will not set rli->mi */
 >> diff --git a/sql/sp.cc b/sql/sp.cc
 >> index 0a8aa24789b..537d29fb67e 100644
 >> --- a/sql/sp.cc
 >> +++ b/sql/sp.cc
 >> @@ -2281,12 +2281,20 @@ Sp_handler::sp_exist_routines(THD *thd, 
TABLE_LIST *routines) const
 >>     for (routine= routines; routine; routine= routine->next_global)
 >>     {
 >>       sp_name *name;
 >> -    LEX_CSTRING lex_db;
 >> +    LEX_CSTRING lex_db= thd->make_ident_opt_casedn(routine->db,
 >> + 
lower_case_table_names);
 >> +    if (!lex_db.str)
 >> +      DBUG_RETURN(TRUE); // EOM
 >
 > // EOM, error was already sent
 > right?
 > (also, in other places)

Done, here and in other places.

 >
 >>       LEX_CSTRING lex_name;
 >> -    thd->make_lex_string(&lex_db, routine->db.str, routine->db.length);
 >>       thd->make_lex_string(&lex_name, routine->table_name.str,
 >>                            routine->table_name.length);
 >
 > Rather inconsistent :(
 > May be, makes sense to create
 >
 >   LEX_CSTRING make_lex_string(const char* str, size_t length) const
 >

Thanks. We already had this method in THD, I reused it. Looks better now.


 > ?
 >
 >> +    /*
 >> +      routine->db was earlier tested with check_db_name().
 >> +      Now it's lower-cased according to lower_case_table_names.
 >> +      It's safe to make a Lex_ident_db_normalized.
 >> +    */
 >> -    name= new sp_name(&lex_db, &lex_name, true);
 >> +    name= new (thd->mem_root) sp_name(Lex_ident_db_normalized(lex_db),
 >> +                                      lex_name, true);
 >>       sp_object_found= sp_find_routine(thd, name, false) != NULL;
 >>       thd->get_stmt_da()->clear_warning_info(thd->query_id);
 >>       if (! sp_object_found)
 >> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
 >> index 09af5523c76..70dde7dc0ed 100644
 >> --- a/sql/sql_acl.cc
 >> +++ b/sql/sql_acl.cc
 >> @@ -2543,6 +2543,37 @@ static void push_new_user(const ACL_USER &user)
 >>   }
 >>
 >>
 >> +/**
 >> +  Make a database name on mem_root from a String,
 >> +  apply lower-case conversion if lower_case_table_names says so.
 >> +  Perform database name length limit validation.
 >> +
 >> +  @param thd      - the THD, to get the warning text from
 >> +  @param mem_root - allocate the result on this memory root
 >> +  @param dbstr    - the String, e.g. with Field::val_str() result
 >> +
 >> +  @return         - {NULL,0} in case of EOM or a bad database name,
 >> +                    or a good database name otherwise.
 >> +*/
 >> +static LEX_STRING make_and_check_db_name(THD *thd, MEM_ROOT *mem_root,
 >> +                                         const String &dbstr)
 >> +{
 >> +  LEX_STRING dbls= lower_case_table_names ?
 >> +                   lex_string_casedn_root(mem_root, files_charset_info,
 >> +                                          dbstr.ptr(), 
dbstr.length()) :
 >> +                   lex_string_strmake_root(mem_root,
 >> +                                           dbstr.ptr(), 
dbstr.length());
 >> +  if (!dbls.str)
 >> +    return LEX_STRING{NULL, 0}; // EOM
 >> +  if (dbls.length > SAFE_NAME_LEN)
 >> +  {
 >> +    sql_print_warning(ER_THD(thd, ER_WRONG_DB_NAME), dbls.str);
 >
 > It's an error log, should every message be localized according to
 > connection-specific locale?

Done. Fixed to ER_DEFAULT(ER_WRONG_DB_NAME).

 >
 >> +    return LEX_STRING{NULL, 0}; // Bad name
 >> +  }
 >> +  return dbls; // Good name
 >> +}
 >> +
 >> +
 >>   /*
 >>     Initialize structures responsible for user/db-level privilege 
checking
 >>     and load information about grants from open privilege tables.
 >> @@ -3680,26 +3702,27 @@ privilege_t acl_get(const char *host, const 
char *ip,
 >>   {
 >>     privilege_t host_access(ALL_KNOWN_ACL), db_access(NO_ACL);
 >>     uint i;
 >> -  size_t key_length;
 >> -  char key[ACL_KEY_LENGTH],*tmp_db,*end;
 >> +  const char *tmp_db;
 >>     acl_entry *entry;
 >>     DBUG_ENTER("acl_get");
 >>
 >> -  tmp_db= strmov(strmov(key, safe_str(ip)) + 1, user) + 1;
 >> -  end= strnmov(tmp_db, db, key + sizeof(key) - tmp_db);
 >> -
 >> -  if (end >= key + sizeof(key)) // db name was truncated
 >> +  constexpr size_t key_data_size= ACL_KEY_LENGTH + 2;
 >> +  CharBuffer key;
 >
 > why ACL_KEY_LENGTH + 2 + MY_CS_MBMAXLEN ?

The extra MY_CS_MBMAXLEN is needed catch truncation.
I added a comment explaining how it works.

 >
 >> +  key.append_bin(Lex_cstring_strlen(safe_str(ip)));
 >> +  key.append_char('\0');
 >> +  key.append_bin(Lex_cstring_strlen(user));
 >> +  key.append_char('\0');
 >
 > there was strmov(strmov(...)) pattern here before.
 > as far as I can see, you can chain too:
 >
 >    key.append_bin().append_char()
 >       .append_bin().append_char();
 >
 > let's use that

Done.

 >
 >> +  tmp_db= key.end();
 >> +  key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db),
 >> +                        lower_case_table_names);
 >> +  db= tmp_db;
 >> +
 >> +  if (key.length() >= key_data_size) // db name was truncated
 >>       DBUG_RETURN(NO_ACL);        // no privileges for an invalid db 
name
 >>
 >> -  if (lower_case_table_names)
 >> -  {
 >> -    my_casedn_str(files_charset_info, tmp_db);
 >> -    db=tmp_db;
 >> -  }
 >> -  key_length= (size_t) (end-key);
 >> -
 >>     mysql_mutex_lock(&acl_cache->lock);
 >> -  if (!db_is_pattern && (entry=acl_cache->search((uchar*) key, 
key_length)))
 >> +  if (!db_is_pattern &&
 >> +      (entry= acl_cache->search((uchar*) key.ptr(), key.length())))
 >>     {
 >>       db_access=entry->access;
 >>       mysql_mutex_unlock(&acl_cache->lock);
 >> @@ -5630,28 +5662,26 @@ static GRANT_NAME *name_hash_search(HASH 
*name_hash,
 >>                                       const char *user, const char 
*tname,
 >>                                       bool exact, bool name_tolower)
 >>   {
 >> -  char helping[SAFE_NAME_LEN*2+USERNAME_LENGTH+3];
 >> -  char *hend = helping + sizeof(helping);
 >> -  uint len;
 >> +  constexpr size_t key_data_size= SAFE_NAME_LEN * 2 + 
USERNAME_LENGTH + 3;
 >> +  CharBuffer key;
 >>     GRANT_NAME *grant_name,*found=0;
 >>     HASH_SEARCH_STATE state;
 >>
 >> -  char *db_ptr= strmov(helping, user) + 1;
 >> -  char *tname_ptr= strnmov(db_ptr, db, hend - db_ptr) + 1;
 >> -  if (tname_ptr > hend)
 >> -    return 0; // invalid name = not found
 >> -  char *end= strnmov(tname_ptr, tname, hend - tname_ptr) + 1;
 >> -  if (end > hend)
 >> +  key.append_bin(Lex_cstring_strlen(user));
 >> +  key.append_char('\0');
 >> +  key.append_bin(Lex_cstring_strlen(db));
 >
 > is db already normalized? If yes, then, please, document it
 > with DBUG_ASSERT(ok_for_lower_case_names());

No tests failed with this DBUG_ASSERT added.
But I'm not sure if db is always normalized.
It can also be a poor test coverage.

 >
 >> +  key.append_char('\0');
 >> +  key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(tname),
 >> +                        name_tolower);
 >> +  key.append_char('\0');
 >> +  if (key.length() >= key_data_size)
 >>       return 0; // invalid name = not found
 >>
 >> -  len  = (uint) (end - helping);
 >> -  if (name_tolower)
 >> -    my_casedn_str(files_charset_info, tname_ptr);
 >> -  for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) 
helping,
 >> -                                               len, &state);
 >> +  for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) 
key.ptr(),
 >> +                                               key.length(), &state);
 >>          grant_name ;
 >> -       grant_name= (GRANT_NAME*) my_hash_next(name_hash,(uchar*) 
helping,
 >> -                                              len, &state))
 >> +       grant_name= (GRANT_NAME*) my_hash_next(name_hash, (uchar*) 
key.ptr(),
 >> +                                              key.length(), &state))
 >>     {
 >>       if (exact)
 >>       {
 >> @@ -8826,33 +8856,28 @@ static bool check_grant_db_routine(THD *thd, 
const char *db, HASH *hash)
 >>   bool check_grant_db(THD *thd, const char *db)
 >>   {
 >>     Security_context *sctx= thd->security_ctx;
 >> -  char helping [SAFE_NAME_LEN + USERNAME_LENGTH+2], *end;
 >> -  char helping2 [SAFE_NAME_LEN + USERNAME_LENGTH+2], *tmp_db;
 >> -  uint len, UNINIT_VAR(len2);
 >> +  constexpr size_t key_data_size= SAFE_NAME_LEN + USERNAME_LENGTH + 2;
 >> +  CharBuffer key, key2;
 >>     bool error= TRUE;
 >>
 >> -  tmp_db= strmov(helping, sctx->priv_user) + 1;
 >> -  end= strnmov(tmp_db, db, helping + sizeof(helping) - tmp_db);
 >> -
 >> -  if (end >= helping + sizeof(helping)) // db name was truncated
 >> -    return 1;                           // no privileges for an 
invalid db name
 >> -
 >> -  if (lower_case_table_names)
 >> -  {
 >> -    end = tmp_db + my_casedn_str(files_charset_info, tmp_db);
 >> -    db=tmp_db;
 >> -  }
 >> +  key.append_bin(Lex_cstring_strlen(sctx->priv_user));
 >> +  key.append_char('\0');
 >> +  key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db),
 >> +                        lower_case_table_names);
 >> +  key.append_char('\0');
 >>
 >> -  len= (uint) (end - helping) + 1;
 >> +  if (key.length() >= key_data_size) // db name was truncated
 >> +    return 1;                        // no privileges for an 
invalid db name
 >>
 >>     /*
 >>        If a role is set, we need to check for privileges here as well.
 >>     */
 >>     if (sctx->priv_role[0])
 >>     {
 >> -    end= strmov(helping2, sctx->priv_role) + 1;
 >> -    end= strnmov(end, db, helping2 + sizeof(helping2) - end);
 >> -    len2= (uint) (end - helping2) + 1;
 >> +    key2.append_bin(Lex_cstring_strlen(sctx->priv_role));
 >> +    key2.append_char('\0');
 >> +    key2.append_bin(Lex_cstring_strlen(db));
 >
 > No append_opt_casedn() here? Looks wrong.

Fixed. Thanks for noticing this.

 >
 >> +    key2.append_char('\0');
 >>     }
 >>
 >>
 >> diff --git a/sql/sql_class.h b/sql/sql_class.h
 >> index 214118d8431..decb9a9064e 100644
 >> --- a/sql/sql_class.h
 >> +++ b/sql/sql_class.h
 >> @@ -4918,11 +4948,46 @@ class THD: public THD_count, /* this must be 
first */
 >>           my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
 >>         return TRUE;
 >>       }
 >> +    return false;
 >> +  }
 >> +
 >> +  /*
 >> +    Copy the current database to the argument. Use the current arena to
 >> +    allocate memory for a deep copy: current database may be freed 
after
 >> +    a statement is parsed but before it's executed.
 >> +
 >> +    Can only be called by owner of thd (no mutex protection)
 >> +  */
 >> +  bool copy_db_to(LEX_CSTRING *to)
 >> +  {
 >> +    if (check_if_current_db_is_set_with_error())
 >> +      return true;
 >>
 >>       to->str= strmake(db.str, db.length);
 >>       to->length= db.length;
 >>       return to->str == NULL;                     /* True on error */
 >>     }
 >> +
 >> +  /*
 >> +    Make a normalized copy of the current database.
 >> +    Raise an error if no current database is set.
 >> +    Note, in case of lower_case_table_names==2, thd->db can contain the
 >> +    name in arbitrary case typed by the user, so it must be 
lower-cased.
 >> +    For other lower_case_table_names values the name is already in
 >> +    its normalized case, so it's copied as is.
 >> +  */
 >> +  Lex_ident_db_normalized copy_db_normalized()
 >> +  {
 >> +    if (check_if_current_db_is_set_with_error())
 >> +      return Lex_ident_db_normalized();
 >> +    LEX_STRING ident= make_ident_opt_casedn(db, 
lower_case_table_names == 2);
 >
 > LEX_STRING or LEX_CSTRING ?

It does not really matter. Both are fine:
a. The variable is assigned from a function returning LEX_STRING.
b. The contructor below gets a LEX_CSTRING.

It does not matter when the conversion from LEX_STRING to LEX_CSTRING 
happens,
in (a) or in (b). Anyway, I changed to LEX_CSTRING.

 >
 >> +    /*
 >> +      A non-empty thd->db is always known to satisfy check_db_name().
 >> +      So after optional lower-casing above it's safe to
 >> +      make Lex_ident_db_normalized.
 >> +    */
 >> +    return Lex_ident_db_normalized(ident);
 >> +  }
 >>     /* Get db name or "". Use for printing current db */
 >>     const char *get_db()
 >>     { return safe_str(db.str); }
 >> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
 >> index a84fd29affc..8cafad65538 100644
 >> --- a/sql/sql_lex.cc
 >> +++ b/sql/sql_lex.cc
 >> @@ -4186,6 +4186,18 @@ bool LEX::copy_db_to(LEX_CSTRING *to)
 >>     return thd->copy_db_to(to);
 >>   }
 >>
 >> +
 >> +Lex_ident_db_normalized LEX::copy_db_normalized()
 >> +{
 >> +  if (sphead && sphead->m_name.str)
 >> +  {
 >> +    DBUG_ASSERT(sphead->m_db.str && sphead->m_db.length);
 >
 > Please,
 >
 >    DBUG_ASSERT(sphead->m_db.str);
 >    DBUG_ASSERT(sphead->m_db.length);

Fixed.

 >
 >> +    return thd->to_ident_db_normalized_with_error(sphead->m_db);
 >> +  }
 >> +  return thd->copy_db_normalized();
 >> +}
 >> +
 >> +
 >>   /**
 >>     Initialize offset and limit counters.
 >>
 >> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
 >> index 1fcafd6ec08..0f39a13632a 100644
 >> --- a/sql/sql_show.cc
 >> +++ b/sql/sql_show.cc
 >> @@ -3856,30 +3859,31 @@ static bool show_status_array(THD *thd, 
const char *wild,
 >>         names until lp:1306875 has been fixed.
 >>         TODO: remove once lp:1306875 has been addressed.
 >>        */
 >> -    if (!(*prefix) && !strncasecmp(name_buffer, "wsrep", 
strlen("wsrep")))
 >> +    if (!prefix.length &&
 >> +        !strncasecmp(name_buffer.ptr(), "wsrep", strlen("wsrep")))
 >>       {
 >>         is_wsrep_var= TRUE;
 >>       }
 >>   #endif /* WITH_WSREP */
 >>
 >>       if (ucase_names)
 >> -      my_caseup_str(system_charset_info, name_buffer);
 >> +      name_buffer.append_caseup(system_charset_info, var_name);
 >>       else
 >>       {
 >> -      my_casedn_str(system_charset_info, name_buffer);
 >> -      DBUG_ASSERT(name_buffer[0] >= 'a');
 >> -      DBUG_ASSERT(name_buffer[0] <= 'z');
 >> -
 >> +      name_buffer.append_casedn(system_charset_info, var_name);
 >>         // WSREP_TODO: remove once lp:1306875 has been addressed.
 >>         if (IF_WSREP(is_wsrep_var == FALSE, 1) &&
 >>             status_var)
 >> -        name_buffer[0]-= 'a' - 'A';
 >> +      {
 >> +        char *ptr= (char*) name_buffer.ptr();
 >> +        if (ptr[0] >= 'a' && ptr[0] <= 'z')
 >> +          ptr[0]-= 'a' - 'A';
 >> +      }
 >
 > is it still needed?

Wsrep status variables are historically printed with title case:
- with a capital 'W' on the first position
- with lower case letters on other positions

We can change this if needed to print everything in lower case,
like other status variables are printed. But I propose to do it in
a separate MDEV. For now I preserved the old behavior.

 >
 >>       }
 >>
 >>
 >>       restore_record(table, s->default_values);
 >> -    table->field[0]->store(name_buffer, strlen(name_buffer),
 >> -                           system_charset_info);
 >> +    table->field[0]->store(name_buffer.to_lex_cstring(), 
system_charset_info);
 >>
 >>       /*
 >>         Compare name for types that can't return arrays. We do this 
to not
 >> @@ -4355,18 +4361,15 @@ bool get_lookup_field_values(THD *thd, COND 
*cond, bool fix_table_name_case,
 >>
 >>     if (lower_case_table_names && !rc)
 >>     {
 >> -    /*
 >> -      We can safely do in-place upgrades here since all of the 
above cases
 >> -      are allocating a new memory buffer for these strings.
 >> -    */
 >>       if (lookup_field_values->db_value.str && 
lookup_field_values->db_value.str[0])
 >> -      my_casedn_str(system_charset_info,
 >> -                    (char*) lookup_field_values->db_value.str);
 >> +      lookup_field_values->db_value= thd->make_ident_casedn(
 >> +                                       lookup_field_values->db_value);
 >> +
 >>       if (fix_table_name_case &&
 >>           lookup_field_values->table_value.str &&
 >>           lookup_field_values->table_value.str[0])
 >> -      my_casedn_str(system_charset_info,
 >> -                    (char*) lookup_field_values->table_value.str);
 >> +      lookup_field_values->table_value= thd->make_ident_casedn(
 >> + 
lookup_field_values->table_value);
 >
 > Why is it even needed? If db/table names are compared case insensitively,
 > it shouldn't matter if they're lowercased or not, should it?

This function get_lookup_field_values() has a special parameter
"fix_table_name_case".

When this function is used e.g. for I_S.PLUGINS and I_S.ROUTINES,
this parameter is "true".

The result if this function not only is used for comparison purposes,
but also is used in the SQL result set.

 >
 >>     }
 >>
 >>     return rc;
 >> diff --git a/sql/sql_type_fixedbin.h b/sql/sql_type_fixedbin.h
 >> index 8aaa7582cd5..e9728b78a3d 100644
 >> --- a/sql/sql_type_fixedbin.h
 >> +++ b/sql/sql_type_fixedbin.h
 >> @@ -267,8 +267,7 @@ class Type_handler_fbt: public Type_handler
 >>       void print(String *str, enum_query_type query_type) override
 >>       {
 >>         StringBuffer tmp;
 >> -      tmp.append(singleton()->name().lex_cstring());
 >> -      my_caseup_str(&my_charset_latin1, tmp.c_ptr());
 >> +      tmp.copy_caseup(&my_charset_latin1, 
singleton()->name().lex_cstring());
 >
 > why bother?

Fixed. Removed the casefolding.

 >
 >>         str->append(tmp);
 >>         str->append('\'');
 >>         m_value.to_string(&tmp);
 >> diff --git a/storage/innobase/dict/dict0load.cc 
b/storage/innobase/dict/dict0load.cc
 >> index bf59fed9ee4..104a8dc8780 100644
 >> --- a/storage/innobase/dict/dict0load.cc
 >> +++ b/storage/innobase/dict/dict0load.cc
 >> @@ -2955,7 +2955,7 @@ dict_load_foreign(
 >>
 >>   	foreign->foreign_table_name = mem_heap_strdupl(
 >>   		foreign->heap, (char*) field, len);
 >> -	dict_mem_foreign_table_name_lookup_set(foreign, TRUE);
 >> +	foreign->foreign_table_name_lookup_set();
 >
 > please, ask Marko to review InnoDB changes

Ok, I will, when we're done with the non-InnoDB part.

 >
 >>
 >>   	const size_t foreign_table_name_len = len;
 >>   	const size_t table_name_len = strlen(table_name);
 >> diff --git a/storage/perfschema/pfs_instr_class.h 
b/storage/perfschema/pfs_instr_class.h
 >> index f353c410d4c..d083db63968 100644
 >> --- a/storage/perfschema/pfs_instr_class.h
 >> +++ b/storage/perfschema/pfs_instr_class.h
 >> @@ -265,6 +265,40 @@ struct PFS_table_share_key
 >>     char m_hash_key[PFS_TABLESHARE_HASHKEY_SIZE];
 >>     /** Length in bytes of @c m_hash_key. */
 >>     uint m_key_length;
 >> +
 >> +  size_t available_length() const
 >> +  {
 >> +    return sizeof(m_hash_key) - m_key_length;
 >> +  }
 >> +  char *end()
 >> +  {
 >> +    return m_hash_key + m_key_length;
 >> +  }
 >> +  // Append and 0-terminate a string with an optional lower-case 
conversion
 >> +  void append_opt_casedn_z(CHARSET_INFO *cs,
 >> +                           const char *str, size_t length,
 >> +                           bool casedn)
 >> +  {
 >> +    DBUG_ASSERT(length <= sizeof(m_hash_key)); // Expect valid 
db/tbl names
 >> +    MY_STRCOPY_STATUS st;
 >> +    size_t dst_length= available_length();
 >> +    if (dst_length > 0)
 >> +    {
 >> +      dst_length--;
 >> +      /*
 >> +        The code below uses copy_fix() instead of memcpy() to make
 >> +        sure we don't break a multi-byte character in the middle.
 >> +      */
 >> +      m_key_length+= (uint) (casedn ?
 >> +                             cs->casedn(str, length, end(), 
dst_length) :
 >> +                             cs->copy_fix(end(), dst_length,
 >> +                                          str, length, length, &st));
 >
 > may be let's not do copy_fix here? there are table names and
 > in a perfschema, for PFS_table_share_key. memcpy would be enough.

Fixed. memcpy() is enough here indeed.

 >
 >> +      m_hash_key[m_key_length++]= '\0';
 >> +    }
 >> +  }
 >> +  void set(bool temporary,
 >> +           const char *schema_name, size_t schema_name_length,
 >> +           const char *table_name, size_t table_name_length);
 >>   };
 >>
 >>   /** Table index or 'key' */
 >> diff --git a/storage/perfschema/pfs_program.cc 
b/storage/perfschema/pfs_program.cc
 >> index de456610519..6c35a745d30 100644
 >> --- a/storage/perfschema/pfs_program.cc
 >> +++ b/storage/perfschema/pfs_program.cc
 >> @@ -118,31 +118,20 @@ static void set_program_key(PFS_program_key *key,
 >>      */
 >>
 >>     char *ptr= &key->m_hash_key[0];
 >> +  const char *end= ptr + sizeof(key->m_hash_key) - 1;
 >>
 >>     ptr[0]= object_type;
 >>     ptr++;
 >>
 >>     if (object_name_length > 0)
 >> -  {
 >> -    char tmp_object_name[COL_OBJECT_NAME_SIZE + 1];
 >> -    memcpy(tmp_object_name, object_name, object_name_length);
 >> -    tmp_object_name[object_name_length]= '\0';
 >> -    my_casedn_str(system_charset_info, tmp_object_name);
 >> -    memcpy(ptr, tmp_object_name, object_name_length);
 >> -    ptr+= object_name_length;
 >> -  }
 >> +    ptr+= system_charset_info->casedn(object_name, object_name_length,
 >> +                                      ptr, end - ptr);
 >>     ptr[0]= 0;
 >>     ptr++;
 >>
 >>     if (schema_name_length > 0)
 >> -  {
 >> -    char tmp_schema_name[COL_OBJECT_SCHEMA_SIZE + 1];
 >> -    memcpy(tmp_schema_name, schema_name, schema_name_length);
 >> -    tmp_schema_name[schema_name_length]='\0';
 >> -    my_casedn_str(system_charset_info, tmp_schema_name);
 >> -    memcpy(ptr, tmp_schema_name, schema_name_length);
 >> -    ptr+= schema_name_length;
 >> -  }
 >> +    ptr+= system_charset_info->casedn(schema_name, schema_name_length,
 >> +                                      ptr, end - ptr);
 >
 > That's strange. casedn of schema when lower_case_table_names==0 ?

I'm not strong with this part of the code.
So here I just tried to preserve the behavior before the change.
Can you suggest an SQL script which would likely to return a wrong
result because of of this code?


 >
 >>     ptr[0]= 0;
 >>     ptr++;
 >>
 >
 > Regards,
 > Sergei
 > Chief Architect, MariaDB Server
 > and security@mariadb.org