Re: 2821cf816a4: MDEV-31531 Remove my_casedn_str() and my_caseup_str()
Hi, Alexander,
commit 2821cf816a4 Author: Alexander Barkov <bar@mariadb.com> Date: Fri Jun 23 13:24:02 2023 +0400
MDEV-31531 Remove my_casedn_str() and my_caseup_str()
please, explain here why
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) ?
+ 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); + 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
extern size_t my_caseup_8bit(CHARSET_INFO *, const char *src, size_t srclen, char *dst, size_t dstlen); 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<buff_sz> & 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<buff_sz> & 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<buff_sz> & 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<buff_sz> & append_bin(const LEX_CSTRING &str)
why _bin ? and why even have a suffix, and not just append() here and above?
+ { + DBUG_ASSERT(is_sane()); + DBUG_ASSERT(!buffer_overlaps(str)); + size_t len= MY_MIN(available_size(), str.length); + memcpy(m_buff + m_length, str.str, len); + m_length+= len; + DBUG_ASSERT(is_sane()); + m_buff[m_length]= '\0'; + return *this; + } + // Append a string with casedn conversion CharBuffer<buff_sz> & append_casedn(CHARSET_INFO *cs, const LEX_CSTRING &str) { DBUG_ASSERT(is_sane()); DBUG_ASSERT(!buffer_overlaps(str)); size_t casedn_length= cs->casedn(str.str, str.length, - m_buff + m_length, buff_sz - m_length); + m_buff + m_length, available_size()); + m_length+= casedn_length; + DBUG_ASSERT(is_sane()); + m_buff[m_length]= '\0'; + return *this; + } + + CharBuffer<buff_sz> & append_opt_casedn(CHARSET_INFO *cs, + const LEX_CSTRING &str, + bool casedn) + { + return casedn ? append_casedn(cs, str) : append_bin(str); + } + + // Append a string with caseup conversion + CharBuffer<buff_sz> & append_caseup(CHARSET_INFO *cs, const LEX_CSTRING &str) + { + DBUG_ASSERT(is_sane()); + DBUG_ASSERT(!buffer_overlaps(str)); + size_t casedn_length= cs->caseup(str.str, str.length, + m_buff + m_length, available_size()); m_length+= casedn_length; DBUG_ASSERT(is_sane()); m_buff[m_length]= '\0'; return *this; }
+ CharBuffer<buff_sz> & truncate(size_t length) + { + DBUG_ASSERT(is_sane()); + if (m_length > length) + { + m_length= length; + m_buff[m_length]= '\0'; + DBUG_ASSERT(is_sane()); + } + return *this; + } + LEX_CSTRING to_lex_cstring() const { return LEX_CSTRING{m_buff, m_length}; 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<NAME_LEN> fname_upper; + fname_upper.copy_caseup(&my_charset_latin1, fname);
why bother? we don't do it in other error cases
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())
+ { + return !length || length > NAME_LEN || str[length-1] == ' '; + } };
@@ -160,4 +171,95 @@ class DBNameBuffer: public CharBuffer<SAFE_NAME_LEN + MY_CS_MBMAXLEN> };
+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?
+ @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/
+ } +}; + + #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
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)
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 ?
+ /* + 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?
+ 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_data_size + MY_CS_MBMAXLEN> key;
why ACL_KEY_LENGTH + 2 + MY_CS_MBMAXLEN ?
+ 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
+ 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_data_size + MY_CS_MBMAXLEN> 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());
+ 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_data_size + MY_CS_MBMAXLEN> 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.
+ 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 ?
+ /* + 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);
+ 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?
}
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?
}
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<FbtImpl::max_char_length()+64> 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?
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
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.
+ 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 ?
ptr[0]= 0; ptr++;
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
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 <bar@mariadb.com>
>> 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.
<cut>
>> 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<buff_sz> & 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<buff_sz> & 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<buff_sz> & 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<buff_sz> & append_bin(const LEX_CSTRING &str)
>
> why _bin ? and why even have a suffix, and not just append() here and
above?
Done, renamed.
<cut>
>> 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<NAME_LEN> 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<SAFE_NAME_LEN + MY_CS_MBMAXLEN>
>> };
>>
>>
>> +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_data_size + MY_CS_MBMAXLEN> 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_data_size + MY_CS_MBMAXLEN> 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_data_size + MY_CS_MBMAXLEN> 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<FbtImpl::max_char_length()+64> 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
Hi, Alexander, On Dec 12, Alexander Barkov wrote:
Hi, Sergei,
Thanks for your review. Please see my comments inline.
The new path version is here:
https://github.com/MariaDB/server/commit/ae0320daef76c7c00f1c7dddd2920afed43...
On 12/11/23 19:48, Sergei Golubchik wrote:
+} +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.
Oh. I thought we agreed that you add it before the function *declaration*, not before every call. But ok, I'll see the patch, may be it's fine and not too much at all.
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 + 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).
And removed thd as a function argument, I hope
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?
something like this: create database FOO; create database foo; create procedure FOO.sp () select 1; create procedure foo.sp () select 2; call FOO.sp(); call foo.sp(); select object_type, object_schema, object_name, count_star, count_statements, sum_rows_sent from performance_schema.events_statements_summary_by_program where object_type='procedure'; drop database FOO; drop database foo; Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello, Sergei, Here's a new patch version: https://github.com/MariaDB/server/commit/d2de9a89d801cbced9701479e564644340a... Please see comments below. On 12/13/23 19:16, Sergei Golubchik wrote: <cut>
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 + 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).
And removed thd as a function argument, I hope
Forgot to remove thd. Now removed. Thank.
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?
something like this:
create database FOO; create database foo; create procedure FOO.sp () select 1; create procedure foo.sp () select 2; call FOO.sp(); call foo.sp(); select object_type, object_schema, object_name, count_star, count_statements, sum_rows_sent from performance_schema.events_statements_summary_by_program where object_type='procedure'; drop database FOO; drop database foo;
Thanks. This script helped. There is a bug indeed. There are even two bugs :) I reported them as: - MDEV-33020 The database part is not case sensitive in SP names in PERFORMANCE_SCHEMA (for the problem that you noticed during the review) - MDEV-33019 The database part is not case sensitive in SP names (for another problem that I found) My last patch includes a fix for MDEV-33020, for PERFORMANCE_SCHEMA, as the patch changes the related lines anyway. It does not include a fix fir MDEV-33019 though, as the problem here is in Sp_cache, which is not touched by the current task. I suggest we fix both MDEV-33020 and MDEV-33019 should be fixed starting from 10.4. Thanks.
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi, Alexander, On Dec 14, Alexander Barkov wrote:
something like this:
create database FOO; create database foo; create procedure FOO.sp () select 1; create procedure foo.sp () select 2; call FOO.sp(); call foo.sp(); select object_type, object_schema, object_name, count_star, count_statements, sum_rows_sent from performance_schema.events_statements_summary_by_program where object_type='procedure'; drop database FOO; drop database foo;
Thanks. This script helped. There is a bug indeed. There are even two bugs :)
I reported them as:
- MDEV-33020 The database part is not case sensitive in SP names in PERFORMANCE_SCHEMA (for the problem that you noticed during the review)
- MDEV-33019 The database part is not case sensitive in SP names (for another problem that I found)
Ouch. I didn't expect that :( So the performance_schema was in fact right in my test, it actually was one sp called twice.
I suggest we fix both MDEV-33020 and MDEV-33019 should be fixed starting from 10.4.
Sure thing. Thanks. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik