Hi, Rucha! Looks great!
Just one question below re. using global vs session old_behavior variable. On May 05, Rucha Deodhar wrote: > revision-id: e7762b3a725 (mariadb-10.5.2-583-ge7762b3a725) > parent(s): bfedf1eb4b6 > author: Rucha Deodhar <[email protected]> > committer: Rucha Deodhar <[email protected]> > timestamp: 2021-04-20 12:50:32 +0530 > message: > > MDEV-8334: Rename utf8 to utf8mb3 > > This patch changes the main name of 3 byte character set from utf8 to > utf8mb3. New old_mode UTF8_IS_UTF8MB3 is added and set TRUE by default, > so that utf8 would mean utf8mb3. If not set, utf8 would mean utf8mb4. > diff --git a/libmariadb b/libmariadb > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit b6f8883d9687936a50a7ed79bd9e5af2340efccd > +Subproject commit 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab when rebasing and pushing into 10.6 take care not to rollback C/C changes. That is, only update C/C submodule reference if it's earlier than your 03d983b287f8a1fe855cb5ed479a3f7ab4f922ab > Binary files a/mysql-test/suite/sys_vars/r/character_set_results_basic.result > and b/mysql-test/suite/sys_vars/r/character_set_results_basic.result differ > diff --git a/sql/item.cc b/sql/item.cc > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -2359,6 +2359,9 @@ left_is_superset(const DTCollation *left, const > DTCollation *right) > > bool DTCollation::aggregate(const DTCollation &dt, uint flags) > { > + > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; if old_behavior is a session variable, then you should use session value here. > if (!my_charset_same(collation, dt.collation)) > { > /* > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -4099,6 +4098,8 @@ static int init_common_variables() > test purposes, to be able to start "mysqld" even if > the requested character set is not available (see bug#18743). > */ > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; ok, here there's no "session" yet, so global value is correct > for (;;) > { > char *next_character_set_name= strchr(default_character_set_name, ','); > diff --git a/sql/set_var.cc b/sql/set_var.cc > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -533,11 +533,12 @@ static my_old_conv old_conv[]= > CHARSET_INFO *get_old_charset_by_name(const char *name) > { > my_old_conv *conv; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; technically, you should use a session value here too. but see the old_conv array, it doesn't have "utf8" anywhere, so it doesn't matter what the flag is, you can use 0 there. > for (conv= old_conv; conv->old_name; conv++) > { > if (!my_strcasecmp(&my_charset_latin1, name, conv->old_name)) > - return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, MYF(0)); > + return get_charset_by_csname(conv->new_name, MY_CS_PRIMARY, > MYF(utf8_flag)); > } > return NULL; > } > diff --git a/sql/sp.cc b/sql/sp.cc > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -291,7 +291,8 @@ bool load_charset(MEM_ROOT *mem_root, > CHARSET_INFO **cs) > { > LEX_CSTRING cs_name; > - > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; but this needs a session value, I suppose > if (field->val_str_nopad(mem_root, &cs_name)) > { > *cs= dflt_cs; > @@ -324,9 +325,10 @@ bool load_collation(MEM_ROOT *mem_root, > *cl= dflt_cl; > return TRUE; > } > + myf utf8_flag= thd->get_utf8_flag(); Hmm, here you do use a session value. So, I suppose your using global value above was not an oversight, but you intentionally did it. What were your reasons? > > DBUG_ASSERT(cl_name.str[cl_name.length] == 0); > - *cl= get_charset_by_name(cl_name.str, MYF(0)); > + *cl= get_charset_by_name(cl_name.str, MYF(utf8_flag)); > > if (*cl == NULL) > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1051,14 +1052,16 @@ static inline void update_global_memory_status(int64 > size) > @retval NULL on error > @retval Pointter to CHARSET_INFO with the given name on success > */ > -static inline CHARSET_INFO * > -mysqld_collation_get_by_name(const char *name, > +inline CHARSET_INFO * static inline, please > +mysqld_collation_get_by_name(const char *name, bool utf8_is_utf8mb3, up to you, but wouldn't it be more convenient to pass the flag here? Then you can invoke it with thd->utf8_flag() > CHARSET_INFO *name_cs= system_charset_info) > { > CHARSET_INFO *cs; > MY_CHARSET_LOADER loader; > + myf utf8_flag= utf8_is_utf8mb3 ? MY_UTF8_IS_UTF8MB3 : 0; > my_charset_loader_init_mysys(&loader); > - if (!(cs= my_collation_get_by_name(&loader, name, MYF(0)))) > + > + if (!(cs= my_collation_get_by_name(&loader, name, MYF(utf8_flag)))) > { > ErrConvString err(name, name_cs); > my_error(ER_UNKNOWN_COLLATION, MYF(0), err.ptr()); > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -10449,7 +10449,10 @@ merge_charset_and_collation(CHARSET_INFO *cs, > CHARSET_INFO *cl) > CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs) > { > const char *csname= cs->csname; > - cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(0)); > + myf utf8_flag= global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MY_UTF8_IS_UTF8MB3 : 0; Why not a session value here? > + cs= get_charset_by_csname(csname, MY_CS_BINSORT, MYF(utf8_flag)); > if (!cs) > { > char tmp[65]; Regards, Sergei VP of MariaDB Server Engineering and [email protected] _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
