- developers - lists.mariadb.org

Re: [Maria-developers] 2858f9fb7e0: MDEV-22189: Change error messages inside code to have mariadb instead of
by Sergei Golubchik 21 Mar '21

21 Mar '21

Hi, Rucha! First, please check the buildbot and make sure all tests pass. You've changed MySQL->MariaDB in result files, but without corresponding changes in the code your result files now differ from the actual test output. One more comment below On Mar 21, Rucha Deodhar wrote: > revision-id: 2858f9fb7e0 (mariadb-10.5.2-274-g2858f9fb7e0) > parent(s): 41485551c58 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-03-18 13:58:34 +0530 > message: > > MDEV-22189: Change error messages inside code to have mariadb instead of > mysql > > Fix: Changed error messages, rerecorded results and changed other relevant > files. > > diff --git a/debian/additions/innotop/innotop b/debian/additions/innotop/innotop > index 19229a57a82..a045a479be9 100644 > --- a/debian/additions/innotop/innotop > +++ b/debian/additions/innotop/innotop > @@ -987,12 +987,12 @@ sub parse_tx_text { > # Parsing the line that begins 'MySQL thread id' is complicated. The only > # thing always in the line is the thread and query id. See function > # innobase_mysql_print_thd in InnoDB source file sql/ha_innodb.cc. > - my ( $thread_line ) = $txn =~ m/^(MySQL thread id .*)$/m; > + my ( $thread_line ) = $txn =~ m/^(MariaDB thread id .*)$/m; > my ( $mysql_thread_id, $query_id, $hostname, $ip, $user, $query_status ); > > if ( $thread_line ) { > # These parts can always be gotten. > - ( $mysql_thread_id, $query_id ) = $thread_line =~ m/^MySQL thread id $d, .*?query id $d/m; > + ( $mysql_thread_id, $query_id ) = $thread_line =~ m/^MariaDB thread id $d, .*?query id $d/m; > > # If it's a master/slave thread, "Has (read|sent) all" may be the thread's > # proc_info. In these cases, there won't be any host/ip/user info > @@ -4144,15 +4144,15 @@ my $innodb_parser = InnoDBParser->new; > > my $nonfatal_errs = join('|', > 'Access denied for user', > - 'Unknown MySQL server host', > + 'Unknown MariaDB server host', > 'Unknown database', > - 'Can\'t connect to local MySQL server through socket', > - 'Can\'t connect to MySQL server on', > + 'Can\'t connect to local MariaDB server through socket', > + 'Can\'t connect to MariaDB server on', > 'MySQL server has gone away', > 'Cannot call SHOW INNODB STATUS', > 'Access denied', > 'AutoCommit', > - 'Lost connection to MySQL server', > + 'Lost connection to MariaDB server', > 'Too many connections', > ); innotop is generally a separate tool that technically could work both with MariaDB and MySQL. So, better use (?:MariaDB|MySQL) in all regexp patterns above. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-13467 review.
by Alexey Botchkov 20 Mar '21

20 Mar '21

One thing i'd still like you to do differently. + if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint) + { + const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1 for the type + char point_temp[point_size]; + memset(point_temp+4, Geometry::wkb_point, 1); + memcpy(point_temp+5, static_cast<const Gis_multi_point *>(g)->get_data_ptr()+5, 4); + memcpy(point_temp+4+WKB_HEADER_SIZE, g->get_data_ptr()+4+WKB_ HEADER_SIZE, + POINT_DATA_SIZE); + point_temp[point_size-1]= '\0'; + Geometry_buffer gbuff; + Geometry *gg= Geometry::construct(&gbuff, point_temp, point_size-1); > Gis_multi_point::geometry_n(uint32 num, String *result) stores the point in String *, > but I need Geometry* to use further, so still would need some recalculation. You could call the ::construct with the string->prr() instead of point_temp. Like str->q_append(0); //SRID g->geometry_n(str, 1); gg= Geometry::construct(&gbuff, str->ptr(), str->length()); But I propose an even simpler method. { Gis_pont p; p.set_data_ptr(g->get_data_ptr()+4+WKB_HEADER_SIZE, POINT_DATA_SIZE); calculate_haversine(&p, radius); } > Regarding radius, Daniel has suggestion: when r=NULL, result= NULL, but r=0, > how should that be treated? > Here is MySQL example (no error, but warning) There i see that MySQL returns the error when the radius is 0 or below. Right? I think it's ok for us to do same. > In a single function we are calculating radian for x,y, I think it is faster than to call function 2 times for x and y. I doubt so. Really would like to compare the speed here. Still i'm ok with that single function if you like it better. Best regards. HF On Tue, Mar 2, 2021 at 1:16 PM Anel Husakovic <anel(a)mariadb.org> wrote: > Hi Alexey, > thanks for your review, hope everything is ok now. > Here is the new patch 1f618e36577a2a322807 > <https://github.com/MariaDB/server/commit/1f618e36577a2a32280705c640d6f9a748…> according > to your review and comments below. > > On Sat, Jan 9, 2021 at 2:09 PM Alexey Botchkov <holyfoot(a)mariadb.com> > wrote: > >> Hi, Anel! >> >> Sorry for that long delay with this review. Had real difficult weeks of >> my life :( >> Now it seems to be over, so getting back to normal. >> Below is my opinions of the code i see in the branch >> bb-10.1-anel-MDEV-13467-gis-feature-st_sphere-v2 >> Hope it's the recent version. >> >> + Adapter for functions that takes two or three arguments. >> +*/ >> + >> +class Create_func_arg2_or_3 : public Create_func >> +{ >> Do you think we need the separate Create_fund_arg2_or_3 class? >> If you don't see any other class to inherit from it, >> i'd recommend to leave the Create_func_distance_sphere alone. >> No need to have two constructors in the Item_func_sphere_distance >> too. Just use the list argument as it's done for the >> Item_func_json_contains (in 10.2). >> > > Ack ^ droped the Create_func_arg2_or_3 and inherited from > Create_native_function which can use a variable number of parameters. > Thanks > > >> Anyway i belive we should get rid of duplicating code >> about the 'param1' and 'param2'. They can be popped/checked >> with no if (arg_count == 2) condition. >> >> + double distance= 0.0; >> + null_value= (args[0]->null_value || args[1]->null_value); >> + if (null_value) >> + { >> + // Returned item must be set to Null >> + DBUG_ASSERT(maybe_null); >> don't think it's needed. >> + return 0; >> Should be return 0.0 i belive. Or goto handle_errors. >> Thet goes to all but one 'return' statements in this function. >> >> + } >> >> > ... >> > if (!arg1 || !arg2) >> >> This last condition should never happen since we have not-null_value. >> So i'd remove that 'if' section completely. >> And anyway it should do the 'return' in that branch. >> >> + null_value= args[2]->null_value; >> + if (null_value) >> + { >> + return 0; >> + } >> >> seems nicer to me this way >> if (args[2]->null_value) >> { >> null_value= true; >> goto handle_errors; >> } >> >> > Ack. > Regarding radius, Daniel has suggestion > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…>: > when r=NULL, result= NULL, but r=0, how should that be treated? > Here is MySQL > <https://dbfiddle.uk/?rdbms=mysql_8.0&fiddle=d467903dce23464df0ebf396220da886>example > (no error, but warning) > > > >> + if (!(g1= Geometry::construct(&buffer1, arg1->ptr(), arg1->length())) >> || >> + !(g2= Geometry::construct(&buffer2, arg2->ptr(), arg2->length()))) >> + { >> + goto handle_errors; >> >> we should launch the ER_GIS_INVALID_DATA here. >> >> > ER_GIS_INVALID_DATA is introduced in 10.2, I will change this (and test > case) when pushing to 10.2, but I would also push to 10.1 since there may > be users using it in 10.1. > Also when inspecting this error in 10.2 noted that lot tests have > commented this error, so will remove that comment in the same commit. > > mysql-test/suite/innodb_gis/t/gis.test > > mysql-test/suite/innodb_gis/t/1.test > > mysql-test/suite/innodb_gis/t/bug16236208.test > > mysql-test/suite/innodb_gis/t/precise.test > > mysql-test/suite/innodb_gis/t/rtree.test > > > + // Generate error message in case different geometry is used? >> Yes, i think the explainig message here is good to have. >> >> +double Item_func_sphere_distance::spherical_distance(Geometry *g1, >> + Geometry *g2, >> + const double >> sphere_radius) >> +double Item_func_sphere_distance::spherical_distance_points(Geometry *g1, >> + Geometry *g2, >> + const double >> r) >> Why these two are member static functions, not simply static? >> If you plan any 'external' use for them, please add comments about >> what exacly they do. >> >> > Changed function to be member function only. > > > >> +double Item_func_sphere_distance::spherical_distance(Geometry *g1, >> + Geometry *g2, >> + const double >> sphere_radius) >> +{ >> + double res= 0.0; >> + switch (g1->get_class_info()->m_type_id) >> + { >> + case Geometry::wkb_point: >> + res= spherical_distance_points(g1, g2, sphere_radius); >> + break; >> + >> + case Geometry::wkb_multipoint: >> + res= spherical_distance_points(g1, g2, sphere_radius); >> + break; >> >> Why do we need this function? >> We confirmed already that the type is either wkb_point or >> wkb_multipolygon, >> so just can call spherical_distance_points. No? >> >> > Indeed, left from earlier implementation (v1). > > >> >>>>> >> >> >> +class Item_func_sphere_distance: public Item_real_func >> +{ >> + double sphere_radius= 6370986.0; // Default radius equals Earth radius >> >> Some compilers don't like this syntax, and we didn't use it yet, so >> i'd recomment to move this to the constructor or even better to the >> ::val_real(). >> >> +double Gis_point::calculate_haversine(const Geometry *g, >> + const double sphere_radius) >> +{ >> + DBUG_ASSERT(sphere_radius > 0); >> + double x1r= 0.0; >> + double x2r= 0.0; >> + double y1r= 0.0; >> + double y2r= 0.0; >> That lot of unnecessary initializations seems excessive. >> >> Ack > > >> + if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint) >> + { >> + const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1 >> for the type >> + char point_temp[point_size]; >> + memset(point_temp+4, Geometry::wkb_point, 1); >> + memcpy(point_temp+5, static_cast<const Gis_multi_point >> *>(g)->get_data_ptr()+5, 4); >> + memcpy(point_temp+4+WKB_HEADER_SIZE, >> g->get_data_ptr()+4+WKB_HEADER_SIZE, >> + POINT_DATA_SIZE); >> + point_temp[point_size-1]= '\0'; >> + Geometry_buffer gbuff; >> + Geometry *gg= Geometry::construct(&gbuff, point_temp, point_size-1); >> >> There is the Gis_multi_point::geometry_n() function, should be used here >> to get the point. >> >> Gis_multi_point::geometry_n(uint32 num, String *result) stores the point > in String *, but I need Geometry* to use further, so still would need some > recalculation. > > I didn’t find a multipoint API to convert String * to Geometry* so I did > my own calculation after debugging from row data. > > > I don't like that lot of static_cast-s. >> Isn't it nicer to have just the function of >> double calculate_harvesine_points(Geometry *p1, Geometry *p2, double >> radius) >> double calculate_harvesine_multipoint_point(Geometry *mp, Geometry *p, >> double radius) >> double calculate_harvesine_multipoint_multipoint(Geometry *mp1, Geometry >> *mp2, double radius) >> which would call each other? >> >> > I would prefer to have 2 static member functions and use static cast where > needed, if you don't mind. > The same could be done with 3 functions but with the code repeated. > > >> The ::get_xy_radian() method looks unneeded to me. >> It's enough to have the to_radian(double d) function. >> >> > In a single function we are calculating radian for x,y, I think it is > faster than to call function 2 times for x and y. > > >> BTW it's possible to do the multipoint/multipoint calculations faster >> than n^2. >> Not necessary to do it right now, just not to remember. >> >> > Regarding this there are some implementations that don't use Multipoints > <https://postgis.net/docs/manual-1.4/ST_Distance_Sphere.html> as both > arguments, MySQL > <https://dev.mysql.com/doc/refman/8.0/en/spatial-convenience-functions.html>also > use only Multipoints,Points combination, not MultiPoints,MultiPoints, so we > need to decide what to support. > However, there are algorithms to calculate max > <http://citeseerx.ist.psu.edu/viewdoc/download;jsessionid=CA2AE9B932194B2970…> > ,min <https://core.ac.uk/download/pdf/82507217.pdf> distance and > O(nlong) using convex hull <https://en.wikipedia.org/wiki/Convex_hull> > (which we have implemented <https://mariadb.com/kb/en/st_convexhull/>) > and using rotating calipers > <https://en.wikipedia.org/wiki/Rotating_calipers> from N points from > convex hull it could be O(n). > > Also there is suggested usage of iteration > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…>from > Daniel. > > > <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c11…> > > > >> Best regards. >> HF >> >> > > -- > *Anel Husaković, * > *Software developer* > *MariaDB Foundation* >

1 0

Re: [Maria-developers] 0fc221a012d: MDEV-8334: Rename utf8 to utf8mb3
by Sergei Golubchik 19 Mar '21

19 Mar '21

Hi, Rucha! Thanks for combining all commits in one and a cleanup. See comments below. On Mar 18, Rucha Deodhar wrote: > revision-id: 0fc221a012d (mariadb-10.5.2-402-g0fc221a012d) > parent(s): a1542f8a573 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-03-16 23:31:12 +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/client/mysqlcheck.c b/client/mysqlcheck.c > index fb3103a318d..a8990d8cb6b 100644 > --- a/client/mysqlcheck.c > +++ b/client/mysqlcheck.c > @@ -437,7 +437,7 @@ static int get_options(int *argc, char ***argv) > if (!default_charset) > { > if (opt_fix_db_names || opt_fix_table_names) > - default_charset= (char*) "utf8"; > + default_charset= (char*) "utf8mb3"; why not to keep it utf8? > else > default_charset= (char*) MYSQL_AUTODETECT_CHARSET_NAME; > } > diff --git a/client/mysqldump.c b/client/mysqldump.c > index 7c363973da2..900456b31b2 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -3235,7 +3235,7 @@ static uint get_table_structure(const char *table, const char *db, char *table_t > { > fprintf(sql_file, > "/*!40101 SET @saved_cs_client = @@character_set_client */;\n" > - "/*!40101 SET character_set_client = utf8 */;\n" > + "/*!40101 SET character_set_client = utf8mb3 */;\n" why not to keep it utf8? > "%s%s;\n" > "/*!40101 SET character_set_client = @saved_cs_client */;\n", > is_log_table ? "CREATE TABLE IF NOT EXISTS " : "", > diff --git a/extra/mariabackup/backup_mysql.cc b/extra/mariabackup/backup_mysql.cc > index 3083326a7e0..c62252257b9 100644 > --- a/extra/mariabackup/backup_mysql.cc > +++ b/extra/mariabackup/backup_mysql.cc > @@ -117,7 +117,7 @@ xb_mysql_connect() > mysql_options(connection, MYSQL_PLUGIN_DIR, xb_plugin_dir); > } > mysql_options(connection, MYSQL_OPT_PROTOCOL, &opt_protocol); > - mysql_options(connection,MYSQL_SET_CHARSET_NAME, "utf8"); > + mysql_options(connection,MYSQL_SET_CHARSET_NAME, "utf8mb3"); why not to keep it utf8? > > msg("Connecting to MySQL server host: %s, user: %s, password: %s, " > "port: %s, socket: %s", opt_host ? opt_host : "localhost", > @@ -1506,7 +1506,7 @@ write_xtrabackup_info(MYSQL *connection, const char * filename, bool history, > "incremental ENUM('Y', 'N') DEFAULT NULL," > "format ENUM('file', 'tar', 'xbstream') DEFAULT NULL," > "compressed ENUM('Y', 'N') DEFAULT NULL" > - ") CHARACTER SET utf8 ENGINE=innodb", false); > + ") CHARACTER SET utf8mb3 ENGINE=innodb", false); why not to keep it utf8? > > > #define ESCAPE_BOOL(expr) ((expr)?"'Y'":"'N'") > diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc > index 62fdace654d..0f500c35ff6 100644 > --- a/extra/mariabackup/xtrabackup.cc > +++ b/extra/mariabackup/xtrabackup.cc > @@ -1716,7 +1716,7 @@ static int create_bootstrap_file() > if(!f) > return -1; > > - fputs("SET NAMES UTF8;\n",f); > + fputs("SET NAMES UTF8MB3;\n",f); why not to keep it utf8? > enumerate_ibd_files(append_export_table); > for (std::set<std::string>::iterator it = tables_for_export.begin(); > it != tables_for_export.end(); it++) > diff --git a/mysql-test/main/create-uca.test b/mysql-test/main/create-uca.test > index 0acb51f7286..f73f6114962 100644 > --- a/mysql-test/main/create-uca.test > +++ b/mysql-test/main/create-uca.test > @@ -1,5 +1,5 @@ > # Prerequisites > -let collation=utf8_unicode_ci; > +let collation=utf8mb3_unicode_ci; that's fine, as we generally don't want tests to depend on the old_mode. > --source include/have_collation.inc > > # Initial cleanup > diff --git a/mysql-test/main/ctype_ldml.result b/mysql-test/main/ctype_ldml.result > index 22b7a316111..7c284520733 100644 > --- a/mysql-test/main/ctype_ldml.result > +++ b/mysql-test/main/ctype_ldml.result > @@ -8,7 +8,6 @@ Variable_name Value > character_sets_dir MYSQL_TEST_DIR/std_data/ldml/ > show collation like 'utf8_phone_ci'; > Collation Charset Id Default Compiled Sortlen > -utf8_phone_ci utf8 352 8 I suppose the test should show there is a _phone_ci collation. As you've renamed it, you need to adjust the test to do show collation like 'utf8mb3_phone_ci'; > CREATE TABLE t1 ( > name VARCHAR(64), > phone VARCHAR(64) CHARACTER SET utf8 COLLATE utf8_phone_ci > @@ -37,7 +36,6 @@ Bar +7-912-800-80-01 > DROP TABLE t1; > show collation like 'utf8_test_ci'; > Collation Charset Id Default Compiled Sortlen > -utf8_test_ci utf8 353 8 same > create table t1 (c1 char(1) character set utf8 collate utf8_test_ci); > insert into t1 values ('a'); > select * from t1 where c1='b'; > @@ -526,7 +524,6 @@ DROP TABLE t1; > SET NAMES utf8 COLLATE utf8_phone_ci; > SHOW COLLATION LIKE 'utf8_phone_ci'; > Collation Charset Id Default Compiled Sortlen > -utf8_phone_ci utf8 352 8 and here > SET NAMES utf8; > SELECT hex(weight_string(_utf8mb4'a' collate utf8mb4_test_400_ci)); > hex(weight_string(_utf8mb4'a' collate utf8mb4_test_400_ci)) > diff --git a/mysql-test/main/show_check.result b/mysql-test/main/show_check.result > index d031c792922..1c85ef596c9 100644 > --- a/mysql-test/main/show_check.result > +++ b/mysql-test/main/show_check.result > @@ -874,12 +874,11 @@ set names utf8; > ---------------------------------------------------------------- > SHOW CHARACTER SET LIKE 'utf8'; > Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr > -def information_schema CHARACTER_SETS CHARACTER_SETS CHARACTER_SET_NAME Charset 253 96 4 N 1 0 33 > -def information_schema CHARACTER_SETS CHARACTER_SETS DESCRIPTION Description 253 180 13 N 1 0 33 > -def information_schema CHARACTER_SETS CHARACTER_SETS DEFAULT_COLLATE_NAME Default collation 253 96 15 N 1 0 33 > -def information_schema CHARACTER_SETS CHARACTER_SETS MAXLEN Maxlen 8 3 1 N 32769 0 63 > +def information_schema CHARACTER_SETS CHARACTER_SETS CHARACTER_SET_NAME Charset 253 96 0 N 1 0 33 > +def information_schema CHARACTER_SETS CHARACTER_SETS DESCRIPTION Description 253 180 0 N 1 0 33 > +def information_schema CHARACTER_SETS CHARACTER_SETS DEFAULT_COLLATE_NAME Default collation 253 96 0 N 1 0 33 > +def information_schema CHARACTER_SETS CHARACTER_SETS MAXLEN Maxlen 8 3 0 N 32769 0 63 > Charset Description Default collation Maxlen > -utf8 UTF-8 Unicode utf8_general_ci 3 again, I suspect this test should now do `SHOW CHARACTER SET LIKE 'utf8mb3'; > ---------------------------------------------------------------- > SHOW COLLATION LIKE 'latin1_bin'; > Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr > diff --git a/mysql-test/suite/funcs_1/r/charset_collation.result b/mysql-test/suite/funcs_1/r/charset_collation.result > index 31bd30c5acf..6b52e80d6ba 100644 > --- a/mysql-test/suite/funcs_1/r/charset_collation.result > +++ b/mysql-test/suite/funcs_1/r/charset_collation.result > @@ -9,7 +9,6 @@ ORDER BY character_set_name; > CHARACTER_SET_NAME DEFAULT_COLLATE_NAME DESCRIPTION MAXLEN > binary binary Binary pseudo charset 1 > latin1 latin1_swedish_ci cp1252 West European 1 > -utf8 utf8_general_ci UTF-8 Unicode 3 and here too, changing the test is in order > > SELECT * > FROM information_schema.collations > diff --git a/mysql-test/suite/funcs_1/r/is_column_privileges.result b/mysql-test/suite/funcs_1/r/is_column_privileges.result > index b6be9118048..46b2d515041 100644 > --- a/mysql-test/suite/funcs_1/r/is_column_privileges.result > +++ b/mysql-test/suite/funcs_1/r/is_column_privileges.result > @@ -45,7 +45,7 @@ COLUMN_PRIVILEGES CREATE TEMPORARY TABLE `COLUMN_PRIVILEGES` ( > `COLUMN_NAME` varchar(64) NOT NULL DEFAULT '', > `PRIVILEGE_TYPE` varchar(64) NOT NULL DEFAULT '', > `IS_GRANTABLE` varchar(3) NOT NULL DEFAULT '' > -) ENGINE=MEMORY DEFAULT CHARSET=utf8 > +) ENGINE=MEMORY DEFAULT CHARSET=utf8mb3 Just a thought. Did you also fix all --embedded, all --ps, and all --big --big tests to pass? > SHOW COLUMNS FROM information_schema.COLUMN_PRIVILEGES; > Field Type Null Key Default Extra > GRANTEE varchar(190) NO > diff --git a/mysql-test/suite/innodb/r/innodb_ctype_ldml.result b/mysql-test/suite/innodb/r/innodb_ctype_ldml.result > index 502f57156c3..8a96c023134 100644 > --- a/mysql-test/suite/innodb/r/innodb_ctype_ldml.result > +++ b/mysql-test/suite/innodb/r/innodb_ctype_ldml.result > @@ -8,7 +8,6 @@ Variable_name Value > character_sets_dir MYSQL_TEST_DIR/std_data/ldml/ > show collation like 'utf8_phone_ci'; > Collation Charset Id Default Compiled Sortlen > -utf8_phone_ci utf8 352 8 deja vu. I rememeber I've commented on this very thing already :) please, update this test too. > CREATE TABLE t1 ( > name VARCHAR(64), > phone VARCHAR(64) CHARACTER SET utf8 COLLATE utf8_phone_ci > 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/mysql-test/suite/sys_vars/r/old_mode_basic.result b/mysql-test/suite/sys_vars/r/old_mode_basic.result > index 39c8e554be2..a6b95f1c60c 100644 > --- a/mysql-test/suite/sys_vars/r/old_mode_basic.result > +++ b/mysql-test/suite/sys_vars/r/old_mode_basic.result > @@ -167,8 +167,100 @@ NO_PROGRESS_INFO > SET @@global.old_mode = @global_start_value; > SELECT @@global.old_mode; > @@global.old_mode > - > +UTF8_IS_UTF8MB3 > SET @@session.old_mode = @session_start_value; > SELECT @@session.old_mode; > @@session.old_mode > - > +UTF8_IS_UTF8MB3 > +# > +# Beginning of 10.6 test > +# > +# MDEV-8334: Rename utf8 to utf8mb3 > +# Ah! Great. > +# Save and display old values > +SET @save_old_mode = @@OLD_MODE; > +SET @save_character_set_server = @@character_set_server; > +SET @save_character_set_client = @@character_set_client; > +SET @save_character_set_results = @@character_set_results; > +SET @save_character_set_connection = @@character_set_connection; > +SET @save_character_set_filesystem = @@character_set_filesystem; > +SET @save_character_set_database = @@character_set_database; > +SET @save_collation_connection = @@collation_connection; > +SET @save_collation_server = @@collation_server; > +SET @save_collation_database = @@collation_database; > +SELECT @@OLD_MODE; > +@@OLD_MODE > +UTF8_IS_UTF8MB3 > +SELECT @@character_set_server,@@character_set_client,@@character_set_results, > +@@character_set_connection, @@character_set_filesystem, @@character_set_database, > +@@collation_connection, @@collation_server, @@collation_database; > +@@character_set_server @@character_set_client @@character_set_results @@character_set_connection @@character_set_filesystem @@character_set_database @@collation_connection @@collation_server @@collation_database > +latin1 latin1 latin1 latin1 binary latin1 latin1_swedish_ci latin1_swedish_ci latin1_swedish_ci > +# > +# UTF8MB3 alias for UTF8 > +# > +SET @@character_set_server = utf8; > +SET @@character_set_client = utf8; > +SET @@character_set_results = utf8; > +SET @@character_set_connection = utf8; > +SET @@character_set_filesystem = utf8; > +SET @@character_set_database = utf8; > +SET @@collation_connection = utf8_general_ci; > +SET @@collation_server = utf8_unicode_ci; > +SET @@collation_database = utf8_bin; > +SELECT @@character_set_server, @@character_set_client, @@character_set_results, > +@@character_set_connection, @@character_set_filesystem, @@character_set_database, > +@@collation_connection, @@collation_server, @@collation_database; > +@@character_set_server @@character_set_client @@character_set_results @@character_set_connection @@character_set_filesystem @@character_set_database @@collation_connection @@collation_server @@collation_database > +utf8mb3 utf8mb3 utf8mb3 utf8mb3 utf8mb3 utf8mb3 utf8mb3_general_ci utf8mb3_unicode_ci utf8mb3_bin > +CREATE DATABASE db1 CHARACTER SET = 'utf8' COLLATE = 'utf8_general_ci'; > +ALTER DATABASE db1 CHARACTER SET = 'utf8' COLLATE = 'utf8_unicode_ci'; > +CREATE TABLE tb1 (id1 INT) CHARACTER SET 'utf8' COLLATE 'utf8_bin'; > +SHOW CREATE TABLE tb1; > +Table Create Table > +tb1 CREATE TABLE `tb1` ( > + `id1` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_bin > +DROP TABLE tb1; > +DROP DATABASE db1; > +# > +# UTF8MB4 is alias for UTF8 > +# > +SET @@OLD_MODE=0; > +SET @@character_set_server = utf8; > +SET @@character_set_client = utf8; > +SET @@character_set_results = utf8; > +SET @@character_set_connection = utf8; > +SET @@character_set_filesystem = utf8; > +SET @@character_set_database = utf8; > +SET @@collation_connection = utf8_general_ci; > +SET @@collation_server = utf8_unicode_ci; > +SET @@collation_database = utf8_bin; > +SELECT @@character_set_server, @@character_set_client, @@character_set_results, > +@@character_set_connection, @@character_set_filesystem, @@character_set_database, > +@@collation_connection, @@collation_server, @@collation_database; > +@@character_set_server @@character_set_client @@character_set_results @@character_set_connection @@character_set_filesystem @@character_set_database @@collation_connection @@collation_server @@collation_database > +utf8mb4 utf8mb4 utf8mb4 utf8mb4 utf8mb4 utf8mb4 utf8mb4_general_ci utf8mb4_unicode_ci utf8mb4_bin > +CREATE DATABASE db1 CHARACTER SET = 'utf8' COLLATE = 'utf8_general_ci'; > +ALTER DATABASE db1 CHARACTER SET = 'utf8' COLLATE = 'utf8_unicode_ci'; > +CREATE TABLE tb1 (id1 INT) CHARACTER SET 'utf8' COLLATE 'utf8_bin'; > +SHOW CREATE TABLE tb1; > +Table Create Table > +tb1 CREATE TABLE `tb1` ( > + `id1` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin > +DROP TABLE tb1; > +DROP DATABASE db1; > +SET @@OLD_MODE = @save_old_mode; > +SET @@character_set_server = @save_character_set_server; > +SET @@character_set_client = @save_character_set_client; > +SET @@character_set_results = @save_character_set_results; > +SET @@character_set_connection = @save_character_set_connection; > +SET @@character_set_filesystem = @save_character_set_filesystem; > +SET @@character_set_database = @save_character_set_database; > +SET @@collation_connection = @save_collation_connection; > +SET @@collation_server = @save_collation_server; > +SET @@collation_database = @save_collation_database; > +# > +# End of 10.6 test > +# > diff --git a/mysys/charset.c b/mysys/charset.c > index 32cfeb56e2d..dbb2749d217 100644 > --- a/mysys/charset.c > +++ b/mysys/charset.c > @@ -763,22 +764,13 @@ get_charset_number_internal(const char *charset_name, uint cs_flags) > } > > > -static const char* > -get_charset_name_alias(const char *name) > -{ > - if (!my_strcasecmp(&my_charset_latin1, name, "utf8mb3")) > - return "utf8"; > - return NULL; > -} > - > - > uint get_charset_number(const char *charset_name, uint cs_flags) > { > uint id; > my_pthread_once(&charsets_initialized, init_available_charsets); > if ((id= get_charset_number_internal(charset_name, cs_flags))) > return id; > - if ((charset_name= get_charset_name_alias(charset_name))) > + if ((charset_name= !my_strcasecmp(&my_charset_latin1, charset_name, "utf8") ? "utf8mb3" : NULL)) Huh? Why do you not check MY_UTF8_IS_UTF8MB3 here? > return get_charset_number_internal(charset_name, cs_flags); > return 0; > } > @@ -820,7 +812,7 @@ static CHARSET_INFO *find_collation_data_inheritance_source(CHARSET_INFO *cs) > char name[MY_CS_NAME_SIZE + 1]; > memcpy(name, beg, end - beg); > name[end - beg]= '\0'; > - return inheritance_source_by_id(cs, get_collation_number(name)); > + return inheritance_source_by_id(cs, get_collation_number(name,MYF(0))); and not here? > } > return NULL; > } > @@ -961,7 +953,28 @@ my_collation_get_by_name(MY_CHARSET_LOADER *loader, > CHARSET_INFO *get_charset_by_name(const char *cs_name, myf flags) > { > MY_CHARSET_LOADER loader; > + my_bool utf8_is_utf8mb3= flags & MY_UTF8_IS_UTF8MB3 ? 1 : 0; > + char *copy_of_name= (char*)cs_name; > + char start[6], result[64]; > + char *temp_cs_name; > + > my_charset_loader_init_mysys(&loader); > + > + if (!strcasecmp("utf8",copy_of_name)) > + cs_name = (const char*)(utf8_is_utf8mb3 ? "utf8mb3" : "utf8mb4"); > + > + strncpy(start, cs_name, 5); > + temp_cs_name= (char *)(utf8_is_utf8mb3 ? "utf8mb3_":"utf8mb4_"); > + > + if (!strncasecmp("utf8_", start,5)) > + { > + copy_of_name+= 5; > + result[63]='\0'; > + strcpy(result, temp_cs_name); > + strcat(result, copy_of_name); > + result[strlen(copy_of_name)+strlen(temp_cs_name)]='\0'; > + cs_name= (const char *) result; > + } And why do you do all that ^^^ ? Old code didn't try to change utf8mb3 to utf8 here, because, I suppose, my_collation_get_by_name() below did all that. Why did you add alias resolution where none was? > return my_collation_get_by_name(&loader, cs_name, flags); > } > > @@ -1005,12 +1018,16 @@ get_charset_by_csname(const char *cs_name, uint cs_flags, myf flags) > { > MY_CHARSET_LOADER loader; > my_charset_loader_init_mysys(&loader); > + > + if (!strcasecmp("utf8",cs_name)) > + cs_name= (const char*)(flags & MY_UTF8_IS_UTF8MB3 ? "utf8mb3" : "utf8mb4"); same here > + > return my_charset_get_by_name(&loader, cs_name, cs_flags, flags); > } > > > /** > - Resolve character set by the character set name (utf8, latin1, ...). > + Resolve character set by the character set name (utf8mb3, latin1, ...). > > The function tries to resolve character set by the specified name. If > there is character set with the given name, it is assigned to the "cs" > @@ -1453,8 +1472,8 @@ static const MY_CSET_OS_NAME charsets[] = > > {"US-ASCII", "latin1", my_cs_approx}, > > - {"utf8", "utf8", my_cs_exact}, > - {"utf-8", "utf8", my_cs_exact}, > + {"utf8mb3", "utf8mb3", my_cs_exact}, eh, no. Try to understand what this array is for. > + {"utf-8", "utf8mb3", my_cs_exact}, > #endif > {NULL, NULL, 0} > }; > diff --git a/plugin/handler_socket/client/hslongrun.cpp b/plugin/handler_socket/client/hslongrun.cpp > index b7c02951340..7f88d48fff2 100644 > --- a/plugin/handler_socket/client/hslongrun.cpp > +++ b/plugin/handler_socket/client/hslongrun.cpp > @@ -897,7 +897,7 @@ hs_longrun_init_table(const config& conf, int num_prepare, > "v1 varchar(32) not null," > "v2 varchar(32) not null," > "v3 varchar(32) not null" > - ") character set utf8 collate utf8_bin engine = innodb"); > + ") character set utf8mb3 collate utf8_bin engine = innodb"); just keep it utf8 > for (int i = 0; i < num_prepare; ++i) { > const std::string i_str = to_stdstring(i); > const std::string v1 = "pv1_" + i_str; > diff --git a/plugin/handler_socket/client/hstest.pl b/plugin/handler_socket/client/hstest.pl > index 1363e153c44..5924d8a0ce5 100755 > --- a/plugin/handler_socket/client/hstest.pl > +++ b/plugin/handler_socket/client/hstest.pl > @@ -52,7 +52,7 @@ for my $action (@actions) { > "k $keytype primary key" . > ",v varchar(32) not null" . > $moreflds . > - ") character set utf8 collate utf8_bin " . > + ") character set utf8mb3 collate utf8_bin " . and here. forget about handlersocket > "engine = $engine"); > } elsif ($action eq "insert") { > print("INSERT $db.$table tablesize=$tablesize\n"); > diff --git a/plugin/win_auth_client/common.cc b/plugin/win_auth_client/common.cc > index 8b7319252ac..ddd34aec7da 100644 > --- a/plugin/win_auth_client/common.cc > +++ b/plugin/win_auth_client/common.cc > @@ -384,7 +384,7 @@ char* wchar_to_utf8(const wchar_t *string, size_t *len) > buf= (char*)malloc(buf_len + 1); > if (!buf) > { > - DBUG_PRINT("error",("Out of memory when converting string '%S' to utf8", > + DBUG_PRINT("error",("Out of memory when converting string '%S' to utf8mb3", Nope, see what this function is doing. > string)); > return NULL; > } > @@ -408,7 +408,7 @@ char* wchar_to_utf8(const wchar_t *string, size_t *len) > > #ifndef DBUG_OFF > Error_message_buf error_buf; > - DBUG_PRINT("error", ("Could not convert string '%S' to utf8" > + DBUG_PRINT("error", ("Could not convert string '%S' to utf8mb3" same > ", WideCharToMultiByte() failed with error %X (%s)", > string, GetLastError(), > get_last_error_message(error_buf))); > @@ -451,7 +451,7 @@ wchar_t* utf8_to_wchar(const char *string, size_t *len) > > if (!buf) > { > - DBUG_PRINT("error",("Out of memory when converting utf8 string '%s'" > + DBUG_PRINT("error",("Out of memory when converting utf8mb3 string '%s'" same > " to wide-char representation", string)); > return NULL; > } > diff --git a/scripts/fill_help_tables.sql b/scripts/fill_help_tables.sql > index d0efb750330..ad7c4fce9a4 100644 > --- a/scripts/fill_help_tables.sql > +++ b/scripts/fill_help_tables.sql > @@ -22,7 +22,7 @@ don't change help tables, please. they'll be regenerated from the documentation > > -- mysql -u root -p mysql < file_name > > -set names 'utf8'; > +set names 'utf8mb3'; > > set sql_log_bin = 0; > > diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql > index e390f36a98b..7c8532577a1 100644 > --- a/scripts/mysql_system_tables.sql > +++ b/scripts/mysql_system_tables.sql > @@ -209,7 +209,7 @@ SET @create_transaction_registry="CREATE TABLE IF NOT EXISTS transaction_registr > UNIQUE KEY (commit_id), > INDEX (begin_timestamp), > INDEX (commit_timestamp, transaction_id) > -) ENGINE=INNODB DEFAULT CHARSET=utf8 COLLATE=utf8_bin STATS_PERSISTENT=0"; > +) ENGINE=INNODB DEFAULT CHARSET=utf8mb3 COLLATE=utf8_bin STATS_PERSISTENT=0"; here and in all other places in this file and other .sql files: you need to change the collation too, not just the charset. > > SET @str=IF(@have_innodb <> 0, @create_innodb_table_stats, "SET @dummy = 0"); > PREPARE stmt FROM @str; > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 0bf21e02002..cc7568990b4 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -4039,7 +4039,10 @@ static int init_common_variables() > *next_character_set_name++= '\0'; > if (!(default_charset_info= > get_charset_by_csname(default_character_set_name, > - MY_CS_PRIMARY, MYF(MY_WME)))) > + MY_CS_PRIMARY, > + global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3 | MY_WME) : MYF(MY_WME)))) may be you'd better get this multi-line assignment out of if() ? e.g. myf utf8_flag= global_system_variables.old_behavior & OLD_MODE_UTF8_IS_UTF8MB3 : 0; default_charset_info= get_charset_by_csname(default_character_set_name, MY_CS_PRIMARY, MYF(utf8_flag | MY_WME)); if (!default_charset_info) ... or add a helper thd->utf8_alias() as I suggested below > { > if (next_character_set_name) > { > @@ -4056,7 +4059,10 @@ static int init_common_variables() > if (default_collation_name) > { > CHARSET_INFO *default_collation; > - default_collation= get_charset_by_name(default_collation_name, MYF(0)); > + default_collation= get_charset_by_name(default_collation_name, > + global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3) : MYF(0)); and here you'll be able to use utf8_flag without the conditional operator > if (!default_collation) > { > #ifdef WITH_PERFSCHEMA_STORAGE_ENGINE > @@ -4097,7 +4103,10 @@ static int init_common_variables() > > if (!(character_set_filesystem= > get_charset_by_csname(character_set_filesystem_name, > - MY_CS_PRIMARY, MYF(MY_WME)))) > + MY_CS_PRIMARY, > + global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3 | MY_WME) : MYF(MY_WME)))) and here > return 1; > global_system_variables.character_set_filesystem= character_set_filesystem; > > @@ -7415,7 +7424,9 @@ static void usage(void) > DBUG_ENTER("usage"); > if (!(default_charset_info= get_charset_by_csname(default_character_set_name, > MY_CS_PRIMARY, > - MYF(MY_WME)))) > + global_system_variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3 | MY_WME) : MYF(MY_WME)))) split this one too? > exit(1); > if (!default_collation_name) > default_collation_name= (char*) default_charset_info->name; > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 50b746fe514..15aa9ca8199 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1007,6 +1008,36 @@ inline void update_global_memory_status(int64 size) > my_atomic_add64_explicit(ptr, size, MY_MEMORY_ORDER_RELAXED); > } > > +inline const char* get_alias_collation_or_charset_name(const char* name, > + bool utf8_is_utf8mb3) > +{ > + char *copy_of_name= (char*)name; > + char start[6], result[64]; > + char *temp_cs_name; > + > + if (!strchr(name,'_')) > + { > + if (!strcasecmp("utf8",name)) > + name = utf8_is_utf8mb3 ? "utf8mb3" : "utf8mb4"; > + return name; > + } > + else > + { > + strncpy(start, name, 5); > + temp_cs_name= (char *)(utf8_is_utf8mb3 ? "utf8mb3_":"utf8mb4_"); > + if (!strncasecmp("utf8_", start,5)) > + { > + copy_of_name+= 5; > + result[63]='\0'; > + strcpy(result, temp_cs_name); > + strcat(result, copy_of_name); > + result[strlen(copy_of_name)+strlen(temp_cs_name)]='\0'; > + strcpy((char*)name,result); > + } > + } > + return name; > +} Please, no. First, you cannot just copy `result` into `name`, because `result` is longer, you'll overwrite whatever was in memory after name's value. Second, you don't need to resolve aliases here, charset code already does it, don't duplicate that. Just pass MY_UTF8_IS_UTF8MB3 down to my_collation_get_by_name() below. > + > /** > Get collation by name, send error to client on failure. > @param name Collation name > diff --git a/sql/sql_db.cc b/sql/sql_db.cc > index 9bf16220535..f471d8edc66 100644 > --- a/sql/sql_db.cc > +++ b/sql/sql_db.cc > @@ -583,9 +583,14 @@ bool load_db_opt(THD *thd, const char *path, Schema_specification_st *create) > default-collation commands. > */ > if (!(create->default_table_charset= > - get_charset_by_csname(pos+1, MY_CS_PRIMARY, MYF(0))) && > + get_charset_by_csname(pos+1, MY_CS_PRIMARY, > + thd->variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3) : MYF(0))) && > !(create->default_table_charset= > - get_charset_by_name(pos+1, MYF(0)))) > + get_charset_by_name(pos+1, thd->variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3) : MYF(0)))) again, please move myf flags manipulations out of if() > { > sql_print_error("Error while loading database options: '%s':",path); > sql_print_error(ER_THD(thd, ER_UNKNOWN_CHARACTER_SET),pos+1); > @@ -595,7 +600,9 @@ bool load_db_opt(THD *thd, const char *path, Schema_specification_st *create) > else if (!strncmp(buf,"default-collation", (pos-buf))) > { > if (!(create->default_table_charset= get_charset_by_name(pos+1, > - MYF(0)))) > + thd->variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3) : MYF(0)))) and here > { > sql_print_error("Error while loading database options: '%s':",path); > sql_print_error(ER_THD(thd, ER_UNKNOWN_COLLATION),pos+1); > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 6871699dc5b..ad322eda097 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -2789,7 +2789,10 @@ int Lex_input_stream::scan_ident_middle(THD *thd, Lex_ident_cli_st *str, > body_utf8_append(m_cpp_text_start, m_cpp_tok_start + length); > ErrConvString csname(str->str + 1, str->length - 1, &my_charset_bin); > CHARSET_INFO *cs= get_charset_by_csname(csname.ptr(), > - MY_CS_PRIMARY, MYF(0)); > + MY_CS_PRIMARY, > + thd->variables.old_behavior & > + OLD_MODE_UTF8_IS_UTF8MB3 ? > + MYF(MY_UTF8_IS_UTF8MB3) : MYF(0)); may be it could be a helper in thd, like THD::utf8_alias() { return variables.old_behavior & OLD_MODE_UTF8_IS_UTF8MB3 ? MY_UTF8_IS_UTF8MB3 : 0; } > if (cs) > { > *introducer= cs; > diff --git a/storage/connect/mysql-test/connect/my.cnf b/storage/connect/mysql-test/connect/my.cnf > index 6310772d01f..83f0aa8ab30 100644 > --- a/storage/connect/mysql-test/connect/my.cnf > +++ b/storage/connect/mysql-test/connect/my.cnf > @@ -14,4 +14,4 @@ MASTER_MYSOCK= @mysqld.1.socket > SLAVE_MYPORT= @mysqld.2.port > SLAVE_MYSOCK= @mysqld.2.socket > > -PGCLIENTENCODING= UTF8 > +PGCLIENTENCODING= UTF8MB3 eh... really? Are you sure you've tested it and it worked? by the name of it I suspect it's a postgresql client encoding. > diff --git a/storage/connect/mysql-test/connect/t/odbc_postgresql.sql b/storage/connect/mysql-test/connect/t/odbc_postgresql.sql > index 1c302294393..3c78120a7a2 100644 > --- a/storage/connect/mysql-test/connect/t/odbc_postgresql.sql > +++ b/storage/connect/mysql-test/connect/t/odbc_postgresql.sql > @@ -4,13 +4,13 @@ > -- Run this script as a admin user: > -- psql -U postgres < odbc_postgresql.sql > > -SET NAMES 'UTF8'; > +SET NAMES 'UTF8MB3'; same, postgresql > > DROP DATABASE IF EXISTS mtr; > DROP USER IF EXISTS mtr; > > CREATE USER mtr WITH PASSWORD 'mtr'; > -CREATE DATABASE mtr OWNER=mtr ENCODING='UTF8'; > +CREATE DATABASE mtr OWNER=mtr ENCODING='UTF8MB3'; same > GRANT ALL ON DATABASE mtr TO mtr; > \c mtr > SET role mtr; > diff --git a/storage/innobase/fts/fts0opt.cc b/storage/innobase/fts/fts0opt.cc > index e5164fcc4fa..a26a05c2cf9 100644 > --- a/storage/innobase/fts/fts0opt.cc > +++ b/storage/innobase/fts/fts0opt.cc > @@ -330,7 +330,7 @@ fts_word_t* > fts_word_init( > /*==========*/ > fts_word_t* word, /*!< in: word to initialize */ > - byte* utf8, /*!< in: UTF-8 string */ > + byte* utf8mb3, /*!< in: UTF-8 string */ don't rename variables, please > ulint len) /*!< in: length of string in bytes */ > { > mem_heap_t* heap = mem_heap_create(sizeof(fts_node_t)); > diff --git a/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_french.result b/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_french.result > index 3f24de87035..3659aa5aee8 100644 > --- a/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_french.result > +++ b/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_french.result > @@ -7,5 +7,4 @@ FULLTEXT INDEX (content) > INSERT INTO diaries VALUES ("Je suis un garçon."); > SELECT * FROM diaries WHERE MATCH (content) AGAINST ("garcon"); > content > -Je suis un garçon. looks like a bug > DROP TABLE diaries; > diff --git a/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_japanese.result b/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_japanese.result > index 94ef2608b81..79dac1e63a7 100644 > --- a/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_japanese.result > +++ b/storage/mroonga/mysql-test/mroonga/storage/r/collation_utf8_unicode_ci_japanese.result > @@ -7,5 +7,4 @@ FULLTEXT INDEX (content) > INSERT INTO diaries VALUES ("ひらがなとカタカナを覚えました。"); > SELECT * FROM diaries WHERE MATCH (content) AGAINST ("かたかな"); > content > -ひらがなとカタカナを覚えました。 that too > DROP TABLE diaries; > diff --git a/storage/mroonga/vendor/groonga/CMakeLists.txt b/storage/mroonga/vendor/groonga/CMakeLists.txt > index d271d4c4eb9..fc134b81cde 100644 > --- a/storage/mroonga/vendor/groonga/CMakeLists.txt > +++ b/storage/mroonga/vendor/groonga/CMakeLists.txt > @@ -268,7 +268,7 @@ if(UNIX) > ac_check_funcs(pthread_condattr_setpshared) > endif() > > -option(GRN_WITH_NFKC "use NFKC based UTF8 normalization." ON) > +option(GRN_WITH_NFKC "use NFKC based UTF8MB3 normalization." ON) not here, please > > if(WIN32) > ac_check_headers(winsock2.h) > diff --git a/storage/mroonga/vendor/groonga/benchmark/bench-nfkc.c b/storage/mroonga/vendor/groonga/benchmark/bench-nfkc.c > index ebae95b273b..3997b933e87 100644 > --- a/storage/mroonga/vendor/groonga/benchmark/bench-nfkc.c > +++ b/storage/mroonga/vendor/groonga/benchmark/bench-nfkc.c > @@ -83,11 +83,11 @@ static void > bench_char_type(gpointer user_data) > { > uint64_t code_point; > - char utf8[7]; > + char utf8mb3[7]; don't rename variables > > for (code_point = 1; code_point < MAX_UNICODE; code_point++) { > - ucs2utf8(code_point, (unsigned char *)utf8); > - grn_nfkc50_char_type(utf8); > + ucs2utf8(code_point, (unsigned char *)utf8mb3); > + grn_nfkc50_char_type(utf8mb3); > } > } > > diff --git a/storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/tool/dump_difference_utf8.rb b/storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/tool/dump_difference_utf8.rb > index 4b6fde8c7b0..ce20a2c5b40 100644 > --- a/storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/tool/dump_difference_utf8.rb > +++ b/storage/mroonga/vendor/groonga/vendor/plugins/groonga-normalizer-mysql/tool/dump_difference_utf8.rb > @@ -36,8 +36,8 @@ parser.sorted_pages.each do |page, characters| > next if base == sort > n_differences += 1 > utf8s = [base, upper, lower, sort] > - formatted_code_points = utf8s.collect do |utf8| > - "%#07x" % Unicode.from_utf8(utf8) > + formatted_code_points = utf8s.collect do |utf8mb3| > + "%#07x" % Unicode.from_utf8(utf8mb3) just revert all changes under storage/mroonga/vendor/* > end > if sort.bytesize > base.bytesize > n_expanded_sort_characters += 1 > diff --git a/storage/sphinx/mysql-test/sphinx/my.cnf b/storage/sphinx/mysql-test/sphinx/my.cnf > index f60380b7171..22cc06914f4 100644 > --- a/storage/sphinx/mysql-test/sphinx/my.cnf > +++ b/storage/sphinx/mysql-test/sphinx/my.cnf > @@ -7,7 +7,7 @@ xmlpipe_command = cat @ENV.MTR_SUITE_DIR/testdata.xml > [index test1] > source = src1 > docinfo = extern > -charset_type = utf-8 > +charset_type = utf-8mb3 revert > path = @ENV.MYSQLTEST_VARDIR/searchd/test1 > > [indexer] > diff --git a/storage/spider/mysql-test/spider/bg/my.cnf b/storage/spider/mysql-test/spider/bg/my.cnf > index 246099c623e..39f5bd01c67 100644 > --- a/storage/spider/mysql-test/spider/bg/my.cnf > +++ b/storage/spider/mysql-test/spider/bg/my.cnf > @@ -75,15 +75,15 @@ MASTER_1_MYSOCK= @mysqld.1.1.socket > MASTER_1_ENGINE_TYPE= Spider > #MASTER_1_ENGINE_TYPE= MyISAM > MASTER_1_ENGINE= ENGINE=Spider > -MASTER_1_CHARSET= DEFAULT CHARSET=utf8 > +MASTER_1_CHARSET= DEFAULT CHARSET=utf8mb3 Make sure to update both charset and collation here and in all other .cnf files in the spider suite > MASTER_1_ENGINE2= ENGINE=MyISAM > -MASTER_1_CHARSET2= DEFAULT CHARSET=utf8 > -MASTER_1_CHARSET3= DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci > +MASTER_1_CHARSET2= DEFAULT CHARSET=utf8mb3 > +MASTER_1_CHARSET3= DEFAULT CHARSET=utf8mb3 COLLATE=utf8_unicode_ci > SLAVE1_1_MYPORT= @mysqld.4.1.port > SLAVE1_1_MYSOCK= @mysqld.4.1.socket > SLAVE1_1_ENGINE_TYPE= MyISAM > SLAVE1_1_ENGINE= ENGINE=MyISAM > -SLAVE1_1_CHARSET= DEFAULT CHARSET=utf8 > +SLAVE1_1_CHARSET= DEFAULT CHARSET=utf8mb3 > USE_CHILD_GROUP2= 1 > OUTPUT_CHILD_GROUP2= 0 > CHILD2_1_MYPORT= @mysqld.2.1.port > diff --git a/storage/spider/spd_init_query.h b/storage/spider/spd_init_query.h > index 19b04d50b82..f12cef377e2 100644 > --- a/storage/spider/spd_init_query.h > +++ b/storage/spider/spd_init_query.h > @@ -559,7 +559,7 @@ static LEX_STRING spider_init_queries[] = { > " table_name char(64) not null default ''," > " primary key (table_id)," > " unique uk1(db_name, table_name)" > - " ) engine=Aria transactional=1 default charset=utf8 collate=utf8_bin;" > + " ) engine=Aria transactional=1 default charset=utf8mb3 collate=utf8_bin;" always change both charset and collation (everywhere in this file) > " create table if not exists mysql.spider_rewrite_table_tables(" > " table_id bigint unsigned not null," > " partition_id bigint unsigned not null auto_increment," > diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c > index 0043786d477..5d9213591fb 100644 > --- a/tests/mysql_client_test.c > +++ b/tests/mysql_client_test.c > @@ -19236,7 +19236,7 @@ static void test_bug12337762() > rc= mysql_query(mysql, "create table charset_tab("\ > "txt1 varchar(32) character set Latin1,"\ > "txt2 varchar(32) character set Latin1 collate latin1_bin,"\ > - "txt3 varchar(32) character set utf8 collate utf8_bin"\ > + "txt3 varchar(32) character set utf8mb3 collate utf8_bin"\ both charset and collation > ")"); > > DIE_UNLESS(rc == 0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Old buildbot 10.5 branch failure review
by Otto Kekäläinen 19 Mar '21

19 Mar '21

Hello! I noticed there are quite a lot of upgrade test failures on the 10.5 branch, which is a stable release and should definitely be all green all the time. Here is quick review of those issues with suggested fixes. Maybe Daniel or Elena can do them? They are all easy ones and would make 7 currently red CI steps turn into green. ## Overview: http://buildbot.askmonty.org/buildbot/grid?branch=10.5&category=main&catego… ## kvm-deb-buster-amd6 minor-upgrade-all (http://buildbot.askmonty.org/buildbot/builders/kvm-deb-buster-amd64/builds/…) ++ diff -U1000 /home/buildbot/mariadb.org-tools-master/buildbot/baselines/ldd.10.5.buster.amd64 /home/buildbot/ldd.new --- /home/buildbot/mariadb.org-tools-master/buildbot/baselines/ldd.10.5.buster.amd64 2021-03-16 21:03:04.000000000 -0400 +++ /home/buildbot/ldd.new 2021-03-17 17:23:42.066634818 -0400 - liblz4.so.1 => /usr/lib/x86_64-linux-gnu/liblz4.so.1 Seems some dependency stopped including liblz4. This didn't affect MariaDB. This is a valid change and the fix is to "reset" the ldd.10.5.buster.amd64 file on buildbot.askmonty.org so that there would be no diff no more. ## kvm-deb-buster-amd64 minor-upgrade-all - liblz4.so.1 => /usr/lib/x86_64-linux-gnu/liblz4.so.1 Same as above. ## kvm-deb-sid-x86 - minor-upgrade-all and ## kvm-deb-sid-amd64 - minor-upgrade-all: - libhogweed.so.5 => /lib/x86_64-linux-gnu/libhogweed.so.5 + libhogweed.so.6 => /lib/x86_64-linux-gnu/libhogweed.so.6 - libnettle.so.7 => /lib/x86_64-linux-gnu/libnettle.so.7 + libnettle.so.8 => /lib/x86_64-linux-gnu/libnettle.so.8 Similar issue to buster minor-upgrade-all failure. This is a valid dependency change and the fix is to "reset" the ldd.10.5.sid.amd64 and ldd.10.5.sid.x86 filse on buildbot.askmonty.org so that there would be no diff no more. ## kvm-zyp-opensuse150-amd64 minor-upgrade-all: --- /tmp/ldd.baseline 2021-03-17 19:42:17.468328791 +0000 +++ /home/buildbot/ldd.new 2021-03-17 19:42:16.928328910 +0000 === /usr/sbin/mysqld === /usr/bin/mysql + libedit.so.0 => /usr/lib64/libedit.so.0 === /usr/bin/mysqlaccess === /usr/bin/mysqladmin === /usr/bin/mysqlbinlog === /usr/bin/mysqlcheck === /usr/bin/mysql_client_test === /usr/bin/mysql_client_test_embedded === /usr/bin/mysql_config === /usr/bin/mysql_convert_table_format === /usr/bin/mysqld_multi === /usr/bin/mysqld_safe === /usr/bin/mysqld_safe_helper === /usr/bin/mysqldump === /usr/bin/mysqldumpslow === /usr/bin/mysql_embedded + libedit.so.0 => /usr/lib64/libedit.so.0 Seems legit, library has correct path now. Fix by "resetting" the ldd.baseline. # kvm-zyp-sles150-amd64 minor-upgrade-all: --- /tmp/ldd.baseline 2021-03-17 20:59:56.412361298 +0000 +++ /home/buildbot/ldd.new 2021-03-17 20:59:56.024361282 +0000 === /usr/sbin/mysqld - libpcre2-8.so.0 => /usr/lib64/libpcre2-8.so.0 === /usr/bin/mysql === /usr/bin/mysqlaccess === /usr/bin/mysqladmin === /usr/bin/mysqlbinlog === /usr/bin/mysqlcheck === /usr/bin/mysql_client_test === /usr/bin/mysql_client_test_embedded - libpcre2-8.so.0 => /usr/lib64/libpcre2-8.so.0 === /usr/bin/mysql_config === /usr/bin/mysql_convert_table_format === /usr/bin/mysqld_multi === /usr/bin/mysqld_safe === /usr/bin/mysqld_safe_helper === /usr/bin/mysqldump === /usr/bin/mysqldumpslow === /usr/bin/mysql_embedded - libpcre2-8.so.0 => /usr/lib64/libpcre2-8.so.0 === /usr/bin/mysql_find_rows === /usr/bin/mysql_fix_extensions === /usr/bin/mysqlhotcopy === /usr/bin/mysqlimport === /usr/bin/mysql_install_db === /usr/bin/mysql_ldb === /usr/bin/mysql_plugin === /usr/bin/mysql_secure_installation === /usr/bin/mysql_setpermission === /usr/bin/mysqlshow === /usr/bin/mysqlslap === /usr/bin/mysqltest - libpcre2-8.so.0 => /usr/lib64/libpcre2-8.so.0 - libpcre2-posix.so.2 => /usr/lib64/libpcre2-posix.so.2 === /usr/bin/mysqltest_embedded - libpcre2-8.so.0 => /usr/lib64/libpcre2-8.so.0 - libpcre2-posix.so.2 => /usr/lib64/libpcre2-posix.so.2 Seems legit, library has correct path now. Fix by "resetting" the ldd.baseline. ## kvm-deb-sid-amd64 minor-upgrade-columnstore: The following packages have unmet dependencies: mariadb-plugin-columnstore : Depends: mariadb-server-10.5 (= 1:10.5.8+maria~sid) but 1:10.5.9+maria~sid is to be installed E: Unable to correct problems, you have held broken packages. The mariadb-plugin-columnstore binary packages don't exist for kvm-deb-sid-amd64, as they were skipped when making the 10.5.9 release. Either build them or disable the test until 10.5.10 is out and they are again built. By disabling the test there would be less "red" and less time wasted by developers checking the red ones on why their commits (potentially) fail on CI. Thanks!

4 3

[Maria-developers] 0038c24f2d3: MDEV-16146: MariaDB slave stops with following errors.
by sujatha 18 Mar '21

18 Mar '21

revision-id: 0038c24f2d388b9b0f3bff9219a53c8900153d02 (mariadb-10.5.4-539-g0038c24f2d3) parent(s): 190a8312f598fc4892331225104297f6288f23ac author: Sujatha committer: Sujatha timestamp: 2021-03-18 18:41:30 +0530 message: MDEV-16146: MariaDB slave stops with following errors. Problem: ======== 180511 11:07:58 [ERROR] Slave I/O: Unexpected master's heartbeat data: heartbeat is not compatible with local info;the event's data: log_file_name mysql-bin.000009 log_pos 1054262041, Error_code: 1623 Analysis: ========= In replication setup when master server doesn't have any events to send to slave server it sends an 'Heartbeat_log_event'. This event carries the current binary log filename and offset details. The offset values is stored within 4 bytes of event header. When the size of binary log is higher than UINT32_MAX the log_pos values will not fit in 4 bytes memory. It overflows and hence slave stops with an error. Fix: === Since we cannot extend the fixed header of Log_event class an additional header named 'extra_header' is introduced. 'extra_header' contains HB_FLAGS - 2 bytes HB_LONG_LOG_POS_OFFSET - 8 bytes This 'extra_header' is added only in a case where log_pos > UINT32_MAX and 'HB_LONG_LOG_POS_OFFSET_F' will be enabled. On slave while reading the 'Heartbeat_log_event' if 'HB_LONG_LOG_POS_OFFSET_F' is found to be set then value of 'log_pos' is extracted from 'extra_header'. --- .../suite/rpl/r/rpl_incompatible_heartbeat.result | 20 +++++++++ .../suite/rpl/t/rpl_incompatible_heartbeat.test | 47 ++++++++++++++++++++++ sql/log_event.h | 19 ++++++++- sql/log_event_server.cc | 8 +++- sql/slave.cc | 14 +++++++ sql/sql_repl.cc | 26 ++++++++++-- 6 files changed, 128 insertions(+), 6 deletions(-) diff --git a/mysql-test/suite/rpl/r/rpl_incompatible_heartbeat.result b/mysql-test/suite/rpl/r/rpl_incompatible_heartbeat.result new file mode 100644 index 00000000000..fe2b0436f2d --- /dev/null +++ b/mysql-test/suite/rpl/r/rpl_incompatible_heartbeat.result @@ -0,0 +1,20 @@ +include/master-slave.inc +[connection master] +connection master; +SET @saved_dbug = @@GLOBAL.debug_dbug; +SET @@global.debug_dbug= '+d,simulate_pos_4G'; +connection slave; +include/stop_slave.inc +SET @saved_dbug = @@GLOBAL.debug_dbug; +SET @@global.debug_dbug= '+d,simulate_pos_4G'; +CHANGE MASTER TO MASTER_HEARTBEAT_PERIOD=0.001; +include/start_slave.inc +connection master; +SET @@GLOBAL.debug_dbug = @saved_dbug; +connection slave; +SET @@GLOBAL.debug_dbug = @saved_dbug; +connection master; +CREATE TABLE t (f INT) ENGINE=INNODB; +INSERT INTO t VALUES (10); +DROP TABLE t; +include/rpl_end.inc diff --git a/mysql-test/suite/rpl/t/rpl_incompatible_heartbeat.test b/mysql-test/suite/rpl/t/rpl_incompatible_heartbeat.test new file mode 100644 index 00000000000..b1aa5435ff4 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_incompatible_heartbeat.test @@ -0,0 +1,47 @@ +# ==== Purpose ==== +# +# Test verifies that slave IO thread can process heartbeat events with log_pos +# values higher than UINT32_MAX. +# +# ==== Implementation ==== +# +# Steps: +# 0 - Stop slave threads. Configure a small master_heartbeat_period. +# 1 - Using debug points, simulate a huge binlog offset higher than +# UINT32_MAX on master. +# 2 - Start the slave and observe that slave IO thread is able to process +# the offset received through heartbeat event. +# +# ==== References ==== +# +# MDEV-16146: MariaDB slave stops with incompatible heartbeat +# +--source include/have_debug.inc +--source include/have_innodb.inc +--source include/have_binlog_format_mixed.inc +# Test simulates binarylog offsets higher than UINT32_MAX +--source include/have_64bit.inc +--source include/master-slave.inc + +--connection master +SET @saved_dbug = @@GLOBAL.debug_dbug; +SET @@global.debug_dbug= '+d,simulate_pos_4G'; + +--connection slave +--source include/stop_slave.inc +SET @saved_dbug = @@GLOBAL.debug_dbug; +SET @@global.debug_dbug= '+d,simulate_pos_4G'; +CHANGE MASTER TO MASTER_HEARTBEAT_PERIOD=0.001; +--source include/start_slave.inc + +--connection master +sleep 1; +SET @@GLOBAL.debug_dbug = @saved_dbug; +--sync_slave_with_master +SET @@GLOBAL.debug_dbug = @saved_dbug; + +--connection master +CREATE TABLE t (f INT) ENGINE=INNODB; +INSERT INTO t VALUES (10); +DROP TABLE t; +--source include/rpl_end.inc diff --git a/sql/log_event.h b/sql/log_event.h index 4e193232f4b..72d8766cea5 100644 --- a/sql/log_event.h +++ b/sql/log_event.h @@ -576,6 +576,22 @@ class String; #define MARIA_SLAVE_CAPABILITY_MINE MARIA_SLAVE_CAPABILITY_GTID +#define HB_EXTRA_HEADER_LEN 10 +/* + Extra header contains flags specific to Heartbeat_log_event and + respective data. The length needs to be updated when new memebers are + added to this header +*/ +#define HB_FLAGS_OFFSET 0 +/* + When the size of 'log_pos' within Heartbeat_log_event exceeds UINT_MAX32 it + cannot be stored in standard header as 'log_pos' is of 4 bytes size. Hence + extra_header is introduced. First 2 bytes represent flags. In case of long + 'log_pos' value 'HB_LONG_LOG_POS_OFFSET_F' bit within the flag will be set. + The log_pos is stored witin 'long_log_pos' variables. +*/ +#define HB_LONG_LOG_POS_OFFSET_F 0x1 +#define HB_LONG_LOG_POS_OFFSET 2 /** @enum Log_event_type @@ -5718,7 +5734,8 @@ bool copy_cache_to_file_wrapped(IO_CACHE *body, class Heartbeat_log_event: public Log_event { public: - Heartbeat_log_event(const char* buf, uint event_len, + uint16 hb_flags; + Heartbeat_log_event(const char* buf, ulong event_len, const Format_description_log_event* description_event); Log_event_type get_type_code() { return HEARTBEAT_LOG_EVENT; } bool is_valid() const diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 6dee9b9adf6..2701f4f2f9a 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -8493,14 +8493,18 @@ void Ignorable_log_event::pack_info(Protocol *protocol) #if defined(HAVE_REPLICATION) -Heartbeat_log_event::Heartbeat_log_event(const char* buf, uint event_len, +Heartbeat_log_event::Heartbeat_log_event(const char* buf, ulong event_len, const Format_description_log_event* description_event) :Log_event(buf, description_event) { + uint16 hb_flags; uint8 header_size= description_event->common_header_len; - ident_len = event_len - header_size; + ident_len = event_len - (header_size + HB_EXTRA_HEADER_LEN); set_if_smaller(ident_len,FN_REFLEN-1); log_ident= buf + header_size; + hb_flags= uint2korr(buf + header_size + ident_len); + if (hb_flags & HB_LONG_LOG_POS_OFFSET_F) + log_pos= uint8korr(buf + header_size + ident_len + HB_LONG_LOG_POS_OFFSET); } #endif diff --git a/sql/slave.cc b/sql/slave.cc index 1da030084ef..f365a1e7b5c 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -6493,6 +6493,12 @@ static int queue_event(Master_info* mi,const char* buf, ulong event_len) TODO: handling `when' for SHOW SLAVE STATUS' snds behind */ + DBUG_EXECUTE_IF("simulate_pos_4G", + { + inc_pos= mi->master_log_pos; // temp_save + mi->master_log_pos= ((ulong)2394967295); + };); + if (memcmp(mi->master_log_name, hb.get_log_ident(), hb.get_ident_len()) || mi->master_log_pos > hb.log_pos) { /* missed events of heartbeat from the past */ @@ -6504,6 +6510,14 @@ static int queue_event(Master_info* mi,const char* buf, ulong event_len) error_msg.append_ulonglong(hb.log_pos); goto err; } + DBUG_EXECUTE_IF("simulate_pos_4G", + { + if (hb.log_pos > UINT32_MAX) + { + mi->master_log_pos= inc_pos; // restore + DBUG_SET("-d, simulate_pos_4G"); + } + };); /* Heartbeat events doesn't count in the binlog size, so we don't have to diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc index 010ea794254..d68ae05b60e 100644 --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -824,12 +824,14 @@ get_slave_until_gtid(THD *thd, String *out_str) */ static int send_heartbeat_event(binlog_send_info *info, NET* net, String* packet, - const struct event_coordinates *coord, + struct event_coordinates *coord, enum enum_binlog_checksum_alg checksum_alg_arg) { DBUG_ENTER("send_heartbeat_event"); ulong ev_offset; + char extra_buf[HB_EXTRA_HEADER_LEN]; + uint16 hb_flags= 0; if (reset_transmit_packet(info, info->flags, &ev_offset, &info->errmsg)) DBUG_RETURN(1); @@ -850,19 +852,30 @@ static int send_heartbeat_event(binlog_send_info *info, size_t event_len = ident_len + LOG_EVENT_HEADER_LEN + (do_checksum ? BINLOG_CHECKSUM_LEN : 0); int4store(header + SERVER_ID_OFFSET, global_system_variables.server_id); + if (coord->pos <= UINT32_MAX) + { + int4store(header + LOG_POS_OFFSET, coord->pos); // log_pos + } + else + { + hb_flags|= HB_LONG_LOG_POS_OFFSET_F; + int2store(extra_buf + HB_FLAGS_OFFSET, hb_flags); + int8store(extra_buf + HB_LONG_LOG_POS_OFFSET, coord->pos); + event_len+= HB_EXTRA_HEADER_LEN; + } int4store(header + EVENT_LEN_OFFSET, event_len); int2store(header + FLAGS_OFFSET, 0); - int4store(header + LOG_POS_OFFSET, coord->pos); // log_pos - packet->append(header, sizeof(header)); packet->append(p, ident_len); // log_file_name + packet->append(extra_buf, sizeof(extra_buf)); if (do_checksum) { char b[BINLOG_CHECKSUM_LEN]; ha_checksum crc= my_checksum(0, (uchar*) header, sizeof(header)); crc= my_checksum(crc, (uchar*) p, ident_len); + crc= my_checksum(crc, (uchar*) extra_buf, sizeof(extra_buf)); int4store(b, crc); packet->append(b, sizeof(b)); } @@ -2525,6 +2538,13 @@ static int wait_new_events(binlog_send_info *info, /* in */ } #endif mysql_bin_log.unlock_binlog_end_pos(); + + DBUG_EXECUTE_IF("simulate_pos_4G", + { + coord.pos= ((ulong)5394967295); + DBUG_SET("-d, simulate_pos_4G"); + };); + ret= send_heartbeat_event(info, info->net, info->packet, &coord, info->current_checksum_alg);

1 0

Re: [Maria-developers] 9acabefd276: MDEV-22010: use executables MariaDB named in scripts
by Sergei Golubchik 17 Mar '21

17 Mar '21

Hi, Rucha! On Mar 17, Rucha Deodhar wrote: > > mysql-test-run and mysql-stress-test was also supposed to be renamed as > part of this task. > I need to check again if debs and rpms package symlink. > > > > > > diff --git a/mysql-test/.mtr b/mysql-test/.mtr > > > new file mode 120000 > > > index 00000000000..68986c8cd7e > > > --- /dev/null > > > +++ b/mysql-test/.mtr > > > @@ -0,0 +1 @@ > > > +./mariadb-test-run > > > > What is that? What is .mtr file? > > I think after i built it got committed along with other files. please remove it from the commit > > > diff --git a/mysql-test/mysql-stress-test.pl b/mysql-test/ > > mysql-stress-test.pl > > > new file mode 120000 > > > index 00000000000..e704a5d70bf > > > --- /dev/null > > > +++ b/mysql-test/mysql-stress-test.pl > > > @@ -0,0 +1 @@ > > > +./mariadb-stress-test.pl > > > > looks like you've checked in files you shouldn't have. Like symlinks. > > As mysql-test-run and mysql-stress-test was supposed to be renamed too. So, > I created a symlink > so that ./mysql-test-run and ./mysql-stress-test would still work if > anybody uses it. symlinks - yes. But they are created by cmake during the build, they should not be added to a commit. please, remove. You might want to add them to the .gitignore file though. > > > \ No newline at end of file > > > diff --git a/scripts/msql2mysql.sh b/scripts/msql2mysql.sh > > > index 72a609fa6e7..11923fa15c7 100644 > > > --- a/scripts/msql2mysql.sh > > > +++ b/scripts/msql2mysql.sh > > > @@ -16,4 +16,4 @@ > > > # Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston > > > # MA 02110-1335 USA. > > > > > > -@bindir@/replace msqlConnect mysql_connect msqlListDBs mysql_list_dbs > > msqlNumRows mysql_num_rows msqlFetchRow mysql_fetch_row msqlFetchField > > mysql_fetch_field msqlFreeResult mysql_free_result msqlListFields > > mysql_list_fields msqlListTables mysql_list_tables msqlErrMsg > > 'mysql_error(mysql)' msqlStoreResult mysql_store_result msqlQuery > > mysql_query msqlField mysql_field msqlSelect mysql_select msqlSelectDB > > mysql_select_db msqlNumFields mysql_num_fields msqlClose mysql_close > > msqlDataSeek mysql_data_seek m_field MYSQL_FIELD m_result MYSQL_RES m_row > > MYSQL_ROW msql mysql mSQL mySQL MSQL MYSQL msqlCreateDB mysql_create_db > > msqlDropDB mysql_drop_db msqlFieldSeek mysql_field_seek -- $* > > > +@bindir@/replace msqlConnect mysql_connect msqlListDBs mysql_list_dbs > > msqlNumRows mysql_num_rows msqlFetchRow mysql_fetch_row msqlFetchField > > mysql_fetch_field msqlFreeResult mysql_free_result msqlListFields > > mysql_list_fields msqlListTables mysql_list_tables msqlErrMsg > > 'mysql_error(mysql)' msqlStoreResult mysql_store_result msqlQuery > > mysql_query msqlField mysql_field msqlSelect mysql_select msqlSelectDB > > mysql_select_db msqlNumFields mysql_num_fields msqlClose mysql_close > > msqlDataSeek mysql_data_seek m_field MYSQL_FIELD m_result MYSQL_RES m_row > > MYSQL_ROW msql mysql mysql mariadb mSQL mySQL MSQL MYSQL msqlCreateDB > > mysql_create_db msqlDropDB mysql_drop_db msqlFieldSeek mysql_field_seek -- > > $* > > > > Why? > > So that mysql in bin directory would get replaced with mariadb no, that's not it. See other renames, msqlListTables -> mysql_list_tables, msqlFetchRow -> mysql_fetch_row etc this (prehistoric) script was supposed to help migrating programs that work with mSQL (https://en.wikipedia.org/wiki/MSQL) to MySQL by renaming C API calls in the source code. In short, revert this change, it doesn't belong to this script. > > > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh > > > index 5f183afe8fc..084af42d01c 100644 > > > --- a/scripts/mysql_install_db.sh > > > +++ b/scripts/mysql_install_db.sh > > > @@ -338,11 +338,15 @@ then > > > cannot_find_file resolveip @resolveip_locations@ > > > exit 1 > > > fi > > > - mysqld=`find_in_dirs mysqld @mysqld_locations@` > > > + mysqld=`find_in_dirs mariadbd @mysqld_locations@` > > > if test -z "$mysqld" > > > then > > > - cannot_find_file mysqld @mysqld_locations@ > > > - exit 1 > > > + mysqld=`find_in_dirs mysqld @mysqld_locations@` > > > + if test -z "$mysqld" > > > + then > > > + cannot_find_file mysqld @mysqld_locations@ > > > > Hmm. At least, it should be mariadbd in the cannot_find_file line. > > > > Why do you search for mysqld after mariadbd wasn't found? > > > I added it for additional check, just in case there is mysqld and not > mariadbd or if it could affect compatibility. I don't think it could. These scripts you've edited are installed together with MariaDB, there is no supported configuration where these scripts exist and mariadb* binaries don't. That is, I believe this extra check is not needed. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9acabefd276: MDEV-22010: use executables MariaDB named in scripts
by Sergei Golubchik 17 Mar '21

17 Mar '21

Hi, Rucha! On Mar 17, Rucha Deodhar wrote: > revision-id: 9acabefd276 (mariadb-10.5.2-386-g9acabefd276) > parent(s): de407e7cb4d > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-02-11 15:19:29 +0530 > message: > > MDEV-22010: use executables MariaDB named in scripts > > As a part of this MDEV following changes were made: > 1) Mariadb named executables used instead of mysql named executables in scripts > 2) renamed mysql-test-run and mysql-stress-test to mariadb-test-run and > mariadb-stress-test and created a symlink. > > diff --git a/debian/mariadb-test.links b/debian/mariadb-test.links > index 718a5355474..b3577667d01 100644 > --- a/debian/mariadb-test.links > +++ b/debian/mariadb-test.links > @@ -2,5 +2,7 @@ usr/bin/mariadb-client-test usr/bin/mysql_client_test > usr/bin/mariadb-client-test-embedded usr/bin/mysql_client_test_embedded > usr/bin/mariadb-test usr/bin/mysqltest > usr/bin/mariadb-test-embedded usr/bin/mysqltest_embedded > -usr/share/mysql/mysql-test/mysql-test-run.pl usr/share/mysql/mysql-test/mtr > -usr/share/mysql/mysql-test/mysql-test-run.pl usr/share/mysql/mysql-test/mysql-test-run > +usr/share/mysql/mysql-test/mariadb-test-run.pl usr/share/mysql/mysql-test/mysql-test-run.pl > +usr/share/mysql/mysql-test/mariadb-stress-test.pl usr/share/mysql/mysql-test/mysql-stress-test.pl > +usr/share/mysql/mysql-test/mariadb-test-run.pl usr/share/mysql/mysql-test/mysql-test-run > +usr/share/mysql/mysql-test/mariadb-test-run.pl usr/share/mysql/mysql-test/mtr Not sure that was part of the task, but ok. Did you check that both debs and rpms package all new symlinks correctly? > diff --git a/mysql-test/.mtr b/mysql-test/.mtr > new file mode 120000 > index 00000000000..68986c8cd7e > --- /dev/null > +++ b/mysql-test/.mtr > @@ -0,0 +1 @@ > +./mariadb-test-run What is that? What is .mtr file? > \ No newline at end of file > diff --git a/mysql-test/mysql-stress-test.pl b/mysql-test/mysql-stress-test.pl > new file mode 120000 > index 00000000000..e704a5d70bf > --- /dev/null > +++ b/mysql-test/mysql-stress-test.pl > @@ -0,0 +1 @@ > +./mariadb-stress-test.pl looks like you've checked in files you shouldn't have. Like symlinks. > \ No newline at end of file > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > new file mode 120000 > index 00000000000..56150a0ecad > --- /dev/null > +++ b/mysql-test/mysql-test-run.pl > @@ -0,0 +1 @@ > +./mariadb-test-run.pl another one > \ No newline at end of file > diff --git a/scripts/msql2mysql.sh b/scripts/msql2mysql.sh > index 72a609fa6e7..11923fa15c7 100644 > --- a/scripts/msql2mysql.sh > +++ b/scripts/msql2mysql.sh > @@ -16,4 +16,4 @@ > # Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston > # MA 02110-1335 USA. > > -@bindir@/replace msqlConnect mysql_connect msqlListDBs mysql_list_dbs msqlNumRows mysql_num_rows msqlFetchRow mysql_fetch_row msqlFetchField mysql_fetch_field msqlFreeResult mysql_free_result msqlListFields mysql_list_fields msqlListTables mysql_list_tables msqlErrMsg 'mysql_error(mysql)' msqlStoreResult mysql_store_result msqlQuery mysql_query msqlField mysql_field msqlSelect mysql_select msqlSelectDB mysql_select_db msqlNumFields mysql_num_fields msqlClose mysql_close msqlDataSeek mysql_data_seek m_field MYSQL_FIELD m_result MYSQL_RES m_row MYSQL_ROW msql mysql mSQL mySQL MSQL MYSQL msqlCreateDB mysql_create_db msqlDropDB mysql_drop_db msqlFieldSeek mysql_field_seek -- $* > +@bindir@/replace msqlConnect mysql_connect msqlListDBs mysql_list_dbs msqlNumRows mysql_num_rows msqlFetchRow mysql_fetch_row msqlFetchField mysql_fetch_field msqlFreeResult mysql_free_result msqlListFields mysql_list_fields msqlListTables mysql_list_tables msqlErrMsg 'mysql_error(mysql)' msqlStoreResult mysql_store_result msqlQuery mysql_query msqlField mysql_field msqlSelect mysql_select msqlSelectDB mysql_select_db msqlNumFields mysql_num_fields msqlClose mysql_close msqlDataSeek mysql_data_seek m_field MYSQL_FIELD m_result MYSQL_RES m_row MYSQL_ROW msql mysql mysql mariadb mSQL mySQL MSQL MYSQL msqlCreateDB mysql_create_db msqlDropDB mysql_drop_db msqlFieldSeek mysql_field_seek -- $* Why? > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh > index 5f183afe8fc..084af42d01c 100644 > --- a/scripts/mysql_install_db.sh > +++ b/scripts/mysql_install_db.sh > @@ -338,11 +338,15 @@ then > cannot_find_file resolveip @resolveip_locations@ > exit 1 > fi > - mysqld=`find_in_dirs mysqld @mysqld_locations@` > + mysqld=`find_in_dirs mariadbd @mysqld_locations@` > if test -z "$mysqld" > then > - cannot_find_file mysqld @mysqld_locations@ > - exit 1 > + mysqld=`find_in_dirs mysqld @mysqld_locations@` > + if test -z "$mysqld" > + then > + cannot_find_file mysqld @mysqld_locations@ Hmm. At least, it should be mariadbd in the cannot_find_file line. Why do you search for mysqld after mariadbd wasn't found? > + exit 1 > + fi > fi > langdir=`find_in_dirs --dir errmsg.sys @errmsg_locations@` > if test -z "$langdir" > @@ -579,7 +583,7 @@ else > echo > echo " shell> $mysqld --skip-grant-tables --general-log &" > echo > - echo "and use the command line tool $bindir/mysql" > + echo "and use the command line tool $bindir/mariadb or $bindir/mysql" again, why both? > echo "to connect to the mysql database and look at the grant tables:" > echo > echo " shell> $bindir/mysql -u root mysql" > @@ -613,7 +617,9 @@ then > echo "PLEASE REMEMBER TO SET A PASSWORD FOR THE MariaDB root USER !" > echo "To do so, start the server, then issue the following commands:" > echo > + echo "'$bindir/mariadb-admin' -u root password 'new-password' or" > echo "'$bindir/mysqladmin' -u root password 'new-password'" > + echo "'$bindir/mariadb-admin' -u root -h $hostname password 'new-password' or" > echo "'$bindir/mysqladmin' -u root -h $hostname password 'new-password'" and here > echo > echo "Alternatively you can run:" > diff --git a/scripts/mysql_secure_installation.sh b/scripts/mysql_secure_installation.sh > index b2a9edf4953..12364f1be43 100644 > --- a/scripts/mysql_secure_installation.sh > +++ b/scripts/mysql_secure_installation.sh > @@ -159,15 +159,19 @@ then > cannot_find_file my_print_defaults $basedir/bin $basedir/extra > exit 1 > fi > - mysql_command=`find_in_basedir mysql bin` > + mysql_command=`find_in_basedir mariadb bin` > if test -z "$mysql_command" > then > - cannot_find_file mysql $basedir/bin > - exit 1 > + mysql_command=`find_in_basedir mysql bin` > + if test -z "$mysql_command" > + then > + cannot_find_file mysql $basedir/bin > + exit 1 > + fi same as above > fi > else > print_defaults="@bindir@/my_print_defaults" > - mysql_command="@bindir@/mysql" > + mysql_command="@bindir@/mariadb" > fi > > if test ! -x "$print_defaults" > diff --git a/scripts/mysqld_safe.sh b/scripts/mysqld_safe.sh > index 126eb924814..7763fb9db91 100644 > --- a/scripts/mysqld_safe.sh > +++ b/scripts/mysqld_safe.sh > @@ -531,7 +531,11 @@ else > ledir='@libexecdir@' > fi > > -helper=`find_in_bin mysqld_safe_helper` > +helper=`find_in_bin mariadbd-safe-helper` > +if test -x helper > +then > + helper=`find_in_bin mysqld_safe_helper` > +fi and here > print_defaults=`find_in_bin my_print_defaults` > # Check if helper exists > command -v $helper --help >/dev/null 2>&1 > diff --git a/scripts/wsrep_sst_common.sh b/scripts/wsrep_sst_common.sh > index 5e134570881..cf38bda8f70 100644 > --- a/scripts/wsrep_sst_common.sh > +++ b/scripts/wsrep_sst_common.sh > @@ -273,13 +273,21 @@ SCRIPTS_DIR="$(cd $(dirname "$0"); pwd -P)" > EXTRA_DIR="$SCRIPTS_DIR/../extra" > CLIENT_DIR="$SCRIPTS_DIR/../client" > > -if [ -x "$CLIENT_DIR/mysql" ]; then > +if [-x "$CLIENT_DIR/mariadb"]; then > + MYSQL_CLIENT="$CLIENT_DIR/mariadb" > +elif [-x mariadb] then; > + MYSQL_CLIENT=mariadb > +elif [ -x "$CLIENT_DIR/mysql" ]; then > MYSQL_CLIENT="$CLIENT_DIR/mysql" > else > MYSQL_CLIENT=mysql > fi > > -if [ -x "$CLIENT_DIR/mysqldump" ]; then > +if [ -x "$CLIENT_DIR/mariadb-dump" ]; then > + MYSQLDUMP="$CLIENT_DIR/mariadb-dump" > +elif [-x mariadb-dump]; then > + MYSQLDUMP=mariadb-dump > +elif [ -x "$CLIENT_DIR/mysqldump" ]; then > MYSQLDUMP="$CLIENT_DIR/mysqldump" > else > MYSQLDUMP=mysqldump and here Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ee538938345: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 16 Mar '21

16 Mar '21

Hi, Andrei! Few comments/questions below. I still have troubles understanding the logic of your solution, so I did not ask any questions about those your replies - as I'd be just repeating the same thing over and over. After I'll start getting it finally, I'll go over your reply again. Meanwhile - minor comments/questions, not related to the main algorithm. On Mar 04, Andrei Elkin wrote: > >> To facilitate the failover on the slave side conditions to accept > >> own events (having been discarded by the above recovery) are > >> relaxed to let so for the semisync slave that connects to master in > >> gtid mode. gtid_strict_mode is further recommended to secure from > >> inadvertent re-applying out of order gtids in general. Non-gtid > >> mode connected semisync slave would require > >> --replicate-same-server-id (mind --log-slave-updates must be OFF > >> then). > > > > Sorry, I failed to understand this paragraph :( > > 'gtid_strict_mode is further recommended to secure > from inadvertent re-applying out of order gtids in general.' > > attempted to promote GTID connection with master, > which of course should not be of this patch's > business, but in a way it is 'cos the non-gtid's mode > (CHANGE MASTER TO ... master_use_gtid = NO) is covered by the patch > with a limitation of '--log-slave-updates must be OFF'. > > The GTID-ON connection is covered, and without any `--replicate-same-server-id'. This doesn't seem to clarify anything. I mean the whole paragraph, starting from "To facilitate" and to "then)." Could you try to rephrase it to make it clearer, please? > >> +--let $query1 = INSERT INTO t VALUES (20) > >> +--let $query2 = DELETE FROM t2 WHERE f = 666 /* no such record */ > >> +--source binlog_truncate_active_log.inc > >> + > >> +--echo # Case B. > >> +# The inverted sequence ends up to truncate only $query2 > >> +--let $this_search_pattern = Successfully truncated.*to remove transactions starting from GTID 0-1-10 > >> +--let $query1 = DELETE FROM t2 WHERE f = 0 > > > > why not `= 666 /* no such record */` ? > > is `= 0` important here? > > Yes, there's a fine detail here. > > 'The inverted sequence ends up' to leave in binlog > > $query1 = DELETE FROM t2 WHERE f = 0 > > which is as ineffective as 666. The patch recognizes the ineffectiveness > through a special value of engines involved counter in Gtid event. > In principle the truncation could > start from it rather than the following $query2. > But that would mean more complicated coding. > So the semantics of the zero is to be good, as we're so used to :-)! Sorry, I didn't understand anything. What does the value in the WHERE clause has to do with engines and where the binlog is truncated? > > > >> +--let $query2 = INSERT INTO t VALUES (20) > >> +--source binlog_truncate_active_log.inc ... > >> +--connection master2 > >> +SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > >> +if ($pre_q2) > > > > is $pre_q2 a flag or a query string? > > it's used as a flag, it's set as a query string. > > It's actually both with the idea execute it when defined. > Legit, right? legit, but it's not both. Were it both, you'd have if ($pre_q2) { eval $pre_q2; } but you have, if ($pre_q2) { CALL sp_blank_xa; } > >> +{ > >> + CALL sp_blank_xa; > >> +} ... > >> diff --git a/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test > >> new file mode 100644 > >> index 00000000000..fe153e5703c > >> --- /dev/null > >> +++ b/mysql-test/suite/binlog/t/binlog_truncate_multi_engine.test > >> +# > >> +# ==== Implementation ==== > >> +# > >> +# Steps: > >> +# 0 - Create two tables in innodb and rocksdb engines, > >> +# > >> +# In loop for A,B,C cases (below) do 1-5: > > > > there's no loop now (which is good, don't add it please :) [x] ? > >> +# The 1st trx binlogs, rotate binlog and hold on before committing at engine > >> +SET DEBUG_SYNC= "commit_after_release_LOCK_after_binlog_sync SIGNAL master1_ready WAIT_FOR master1_go"; > > > > you don't wait for master1_ready anywhere. intentional? > > No, actually > > SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > > must be run by below master2 (sim to how master3 waits for master2). meaning [x] ? > >> +--source include/have_binlog_format_row.inc > >> +--let $rpl_topology=1->2 > >> +--source include/rpl_init.inc > > > > isn't this a normal master-slave.inc topology? > > Initially yes. > > I am guessing you mean why not to > --source it. > The test heavily uses 'server_`index`' format to automate > replication direction. as far as I remember, master-slave.inc also defines server_1 and server_2 connections. > >> diff --git a/sql/sql_class.h b/sql/sql_class.h > >> index 140394fefc1..87fa88d4c89 100644 > >> --- a/sql/sql_class.h > >> +++ b/sql/sql_class.h > >> @@ -4632,7 +4632,7 @@ class THD :public Statement, > >> LF_PINS *tdc_hash_pins; > >> LF_PINS *xid_hash_pins; > >> bool fix_xid_hash_pins(); > >> - > >> + bool is_1pc_ro_trans; > > > > I don't like how it looks, it's in THD (for every single connection > > and more) and it's only used in some exotic cases. It's set in two > > places around ha_commit_one_phase(), when it would be better to set > > it inside ha_commit_one_phase(), in one place only. > > > > But I cannot suggest how to get rid of it yet as I don't understand > > what it's for, I've asked a question about it below, in > > Gtid_log_event::Gtid_log_event > > The flag is to help sorting out ineffective (no engine data change) > STATEMENT binlog-format logged transaction. I thought it's practical > enough to face and important to handle. > Through an example, > let the table `t' be initially empty, and > let binlog contain > > trx1: INSERT INTO t SET a = 1; /* slave acked, trx got committed in engine */ > trx2: INSERT INTO t SET a = 2; /* not acked, trx remains prepared in engine */ > trx3: DELETE FROM t WHERE a = 0; /* ineffective, does not have Xid event */ > > prior a crash with their states as commented. > The truncate trx should be trx2 as the trx3's status > suggests it's harmless for involving into > truncation. > However we have yet to distinguish it from unsafe (to truncate) > event group ("transaction") that also ends with COMMIT query not Xid event. Hmm. I see. But thd isn't a universal dumpster for passing arguments to the function, don't use it like that. Please, pass this "read-only transaction with no xid" flag to the Gtid_log_event constructor as an argument. Or recalculate it inside the constructor, it's not such an expensive thing to do. > One rather drastic solution could be to not binlog DELETE altogether. > I did not look into this with much of interest earlier, but how about it? > Perhaps it'd more like a bug fixes than breaking legacy. No, I suspect it's too drastic > >> /* Members related to temporary tables. */ > >> public: > >> /* Opened table states. */ > >> diff --git a/sql/slave.cc b/sql/slave.cc > >> index 28e08e32346..9b3f73e5341 100644 > >> --- a/sql/slave.cc > >> +++ b/sql/slave.cc > >> @@ -6944,7 +6945,9 @@ static int queue_event(Master_info* mi,const char* buf, ulong event_len) > >> } > >> else > >> if ((s_id == global_system_variables.server_id && > >> - !mi->rli.replicate_same_server_id) || > >> + (!mi->rli.replicate_same_server_id && > >> + !(semisync_recovery= (rpl_semi_sync_slave_enabled && > >> + mi->using_gtid != Master_info::USE_GTID_NO)))) || > > > > How can "semisync recovery" code path reach queue_event()? > > This is a failover now, after recovery. > The slave which is ex-master tries to adopt "own" (generated by it) events. 1. don't call it "semisync recovery" then 2. add a comment explaining this case (it's a rare and not obvious one) 3. why does it have to be specially recognized anyway? > > > >> event_that_should_be_ignored(buf) || > >> /* > >> the following conjunction deals with IGNORE_SERVER_IDS, if set > >> diff --git a/sql/log_event.h b/sql/log_event.h > >> index 8a342cb5cd3..1588ab85104 100644 > >> --- a/sql/log_event.h > >> +++ b/sql/log_event.h > >> @@ -482,6 +482,16 @@ class String; > >> */ > >> #define LOG_EVENT_IGNORABLE_F 0x80 > >> > >> +/** > >> + @def LOG_EVENT_ACCEPT_OWN_F > >> + > >> + Flag sets by the gtid-mode connected semisync slave for > >> + the same server_id ("own") events which the slave must not have > >> + in its state. Typically such events were never committed by > >> + their originator (this server) and discared at its crash recovery > > > > sorry, I failed to understand that > > This should've been explained above. > The slave which is ex-master truncated at recovery some of its ("own") transactions. > The flag serves to help recognize that at execution of the replication events. 1. yes, this is clearer, thanks. may be you should rewrite your comment putting this explanation in it? 2. why does it have to be specially recognized anyway? ... > > 3. also it's somewhat an unfounded assumption that only r/w engines > > will have the transaction prepared. But we can make it a fact, if we > > won't prepare r/o engines at all (in ha_prepare). > > [?] > > Sorry, I don't get it. > Do you mean that an r/o engine (branch) might need to be counted by > `ha_count_rw()`? But then it would be contributing with binlogged > events of its branch... I can't imagine that. I mean the following. Say, you have two engines participating in a transaction, like in INSERT innodb_table SELECT * FROM rocksdb_table; One is r/w, the other is r/o. It's a 2pc transaction, the server does prepare for everyone, then commit for everyone. I don't think there is any guarantee that prepare for an r/o engine will be a no-op. It is completely up to the engine what to do in this case, it's possible that the engine will optimize it to a no-op, but it is not a certainty. What I think we should do is not to call prepare() for r/o engines at all. Just skip straight to commit/rollback on the second phase. > >> diff --git a/sql/handler.cc b/sql/handler.cc > >> index 6792e80b8fe..247ab7267bf 100644 > >> --- a/sql/handler.cc > >> +++ b/sql/handler.cc > >> @@ -1245,6 +1245,24 @@ int ha_prepare(THD *thd) > >> DBUG_RETURN(error); > >> } > >> > >> +/* > >> + Returns counted number of > >> + read-write recoverable transaction participants. > >> +*/ > >> +uint ha_count_rw(THD *thd, bool all) > > > > the name doesn't match the implementation, please, rename > > [?] > > Please help here. > I could not find what specifically you mean. > It > returns rw_ha_count > > which I think as 'read-write recoverable transaction participants.' I read the name is "a number of rw engines in the ha_info", it does not mention "recoverable", and I from the name I woulnd't have guessed. May be ha_count_rw_2pc ? > >> +{ > >> + unsigned rw_ha_count= 0; > >> + THD_TRANS *trans=all ? &thd->transaction.all : &thd->transaction.stmt; > >> + > >> + for (Ha_trx_info * ha_info= trans->ha_list; ha_info; > >> + ha_info= ha_info->next()) > >> + { > >> + if (ha_info->is_trx_read_write() && ha_info->ht()->recover) > >> + ++rw_ha_count; > >> + } > >> + return rw_ha_count; > >> +} > >> + > >> /** > >> Check if we can skip the two-phase commit. > >> > >> @@ -1960,8 +1982,122 @@ struct xarecover_st > >> XID *list; > >> HASH *commit_list; > >> bool dry_run; > >> + MEM_ROOT *mem_root; > >> + bool error; > >> }; > >> > >> +/** > >> + Inserts a new hash member. > >> + > >> + returns a successfully created and inserted @c xid_recovery_member > >> + into hash @c hash_arg, > >> + or NULL. > >> +*/ > >> +static xid_recovery_member* > >> +xid_member_insert(HASH *hash_arg, my_xid xid_arg, MEM_ROOT *ptr_mem_root) > >> +{ > >> + xid_recovery_member *member= (xid_recovery_member*) > >> + alloc_root(ptr_mem_root, sizeof(xid_recovery_member)); > >> + if (!member) > >> + return NULL; > >> + > >> + member->xid= xid_arg; > >> + member->in_engine_prepare= 1; > >> + member->decided_to_commit= false; > >> + > >> + return my_hash_insert(hash_arg, (uchar*) member) ? NULL : member; > >> +} > >> + > >> +/* > >> + Inserts a new or updates an existing hash member to increment > >> + the member's prepare counter. > >> + > >> + returns false on success, > >> + true otherwise. > >> +*/ > >> +static bool xid_member_replace(HASH *hash_arg, my_xid xid_arg, > >> + MEM_ROOT *ptr_mem_root) > >> +{ > >> + xid_recovery_member* member; > >> + if ((member= (xid_recovery_member *) > >> + my_hash_search(hash_arg, (uchar *)& xid_arg, sizeof(xid_arg)))) > >> + member->in_engine_prepare++; > >> + else > >> + member= xid_member_insert(hash_arg, xid_arg, ptr_mem_root); > >> + > >> + return member == NULL; > >> +} > >> + > >> +/* > >> + Hash iterate function to complete with commit or rollback as decided > >> + (typically at binlog recovery processing) in member->in_engine_prepare. > >> +*/ > >> +static my_bool xarecover_do_commit_or_rollback(void *member_arg, > >> + void *hton_arg) > >> +{ > >> + xid_recovery_member *member= (xid_recovery_member*) member_arg; > >> + handlerton *hton= (handlerton*) hton_arg; > >> + xid_t x; > >> + my_bool rc; > >> + > >> + x.set(member->xid); > >> + rc= member->decided_to_commit ? hton->commit_by_xid(hton, &x) : > >> + hton->rollback_by_xid(hton, &x); > >> + > >> + DBUG_ASSERT(rc || member->in_engine_prepare > 0); > >> + > >> + if (!rc) > >> + { > > > > I don't think you can trust rc on that. > > if it's non-zero, it's an error all right. > > but it's a bit of a stretch to presume that > > nonexisting xid is always an error. > > > > also commit_by_xid returns int, not my_bool. > > [?] I mean, if an engine returns success when you ask it to commit or rollback an unknown xid - this is not a bug, right? Commit means "make all changes by this xid persistent", if there are no changes, there is nothing to do, so doing nothing and reporting success isn't exactly wrong. Same for rollback. > I'd rather to stick with the patch and require from any engine to > return \cite{xa spec} > > [XAER_NOTA] The specified XID is not known by the resource manager > > What'd you say? Okay. I'd say I was wrong, if the standard requires a resource manager to return an error for an unknown XID, then you surely can rely on that :) Sorry for confusion. > >> + > >> + return false; > >> +} > >> + > >> +static my_bool xarecover_do_count_in_prepare(void *member_arg, > >> + void *ptr_count) > >> +{ > >> + xid_recovery_member *member= (xid_recovery_member*) member_arg; > >> + if (member->in_engine_prepare) > >> + { > >> + *(uint*) ptr_count += member->in_engine_prepare; > > > > This is a rather weird semantics. > > it's kind of a number of transactions times number of engines. > > if one transaction wasn't committed in two engines and another > > transaction wasn't rolled back in one engine, the counter will be 3. > > how are you going to explain this to users? > > So `ptr_count` actually is for transaction branches, not transactions. > But the ultimate warning speaks erronously the latter > > + sql_print_warning("Could not complete %u number of transactions in " > "engines.", count_in_prepare); > > So I'd convert the counter to count the transactions: > > (*(uint*) ptr_count) ++ > > [?] yes, that'd be easier to understand, thanks > >> diff --git a/sql/log.cc b/sql/log.cc > >> index 8073f09ab88..a9808ed8823 100644 > >> --- a/sql/log.cc > >> +++ b/sql/log.cc ... > >> + char buf[21]; > >> + > >> + longlong10_to_str(ptr_gtid->seq_no, buf, 10); > >> + sql_print_information("Successfully truncated binlog file:%s " > >> + "to pos:%llu to remove transactions starting from " > >> + "GTID %u-%u-%s", file_name, pos, > >> + ptr_gtid->domain_id, ptr_gtid->server_id, buf); > >> + } > >> + if (!(error= init_io_cache(&cache, file, IO_SIZE, WRITE_CACHE, > >> + (my_off_t) pos, 0, MYF(MY_WME|MY_NABP)))) > >> + { > >> + /* > >> + Write Stop_log_event to ensure clean end point for the binary log being > >> + truncated. > >> + */ > > > > I'm not sure about it. The less you touch the corrupted binlog the better. > > simply truncating it would be enough, wouldn't it? > > Yes, it would. > But I somewhat preferred a tidy-up style. > > [?] > > If you feel strong to remove it will be done. I feel it'd be safer not to write anything into the corrupted binlog. > >> + Stop_log_event se; > >> + se.checksum_alg= cs_alg; > >> + if ((error= write_event(&se, NULL, &cache))) > >> + { > >> + sql_print_error("Failed to write stop event to " > >> + "binary log. Errno:%d", > >> + (cache.error == -1) ? my_errno : error); > >> + goto end; > >> + } > >> + clear_inuse_flag_when_closing(cache.file); > >> + if ((error= flush_io_cache(&cache)) || > >> + (error= mysql_file_sync(file, MYF(MY_WME|MY_SYNC_FILESIZE)))) > >> + { > >> + sql_print_error("Faild to write stop event to " > >> + "binary log. Errno:%d", > >> + (cache.error == -1) ? my_errno : error); > >> + } > >> + } > >> + else > >> + sql_print_error("Failed to initialize binary log " > >> + "cache for writing stop event. Errno:%d", > >> + (cache.error == -1) ? my_errno : error); > > > > 1. I don't see why you need to sync after every operation. > > Yep, there's one fsync before Stop event writing, redundant if Stop will > stay in the final patch. As far as I understand, if Stop is removed, you shouldn't need any syncs at all, just truncate and close. > >> + int next_binlog_or_round(int& round, > >> + const char *last_log_name, > >> + const char *binlog_checkpoint_name, > >> + LOG_INFO *linfo, MYSQL_BIN_LOG *log); > >> + bool is_safe() > > > > what do you mean "safe" ? > > Safe to truncate condition is explained elsewhere to mean there are no > non-transactional group of events within the truncated tail. > Earlier mentioned ineffective COMMIT-query ended "transactions" are safe. may be is_safe_to_truncate() then? ... > >> + if (!truncate_validated) > >> + { > >> + DBUG_ASSERT(round <= 2); > >> + /* > >> + Either truncate was not set or was reset, else > >> + it gets incremented, otherwise it may only set to an earlier > >> + offset only at the turn out of the 1st round. > > > > sorry, I cannot parse that :( > > The comments are for the assert in a branch that acts when the truncate position > (gtid) is not yet finalized. > Let me annotate under each assert's line. > > > > >> + */ > >> + DBUG_ASSERT(truncate_gtid.seq_no == 0 || > > 'truncate was not set or was reset' ^, that is truncate_gtid. > The 'was reset' is actually not present in the assert - that'd be > reflected with > truncate_reset_done == true > > > >> + last_gtid_coord >= binlog_truncate_coord || > > 'it gets incremented' ^ why does that mean "it gets incremented" ? > > >> + (round == 2 && truncate_set_in_1st)); > > And if it's neither of the above there must've been truncate candidate > set in the 1st round. ... > >> switch (typ) > >> { > >> + case FORMAT_DESCRIPTION_EVENT: > >> + if (round > 1) > >> + { > >> + if (fdle != fdle_arg) > >> + delete fdle; > >> + fdle= (Format_description_log_event*) ev; > > > > why is it needed? all binlog files should have the same > > Format_description_log_event anyway. > > Actually contrary to what was stated (the patch removed that claim) in > the parent revision of this commit, the FD:s of the > recovery binlog files may have different checksums. I.e > > set @@global.binlog_checksum = value > > can be issued to rotate binlog to a new hot file having different checksum value > while the binlog *CheckPoint* file is behind. So this is possible to > have at recovery: > > B_1(CP, checksum = NONE), B_2(hot, checksum = CRC32) > > which also manifests a normal use case bug I have not reported any bug (though wanted to). okay, checksum can differ. But how does it affect Format_description_log_event ? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ee538938345: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 15 Mar '21

15 Mar '21

Hi, Andrei! On Mar 01, Andrei Elkin wrote: > > I've reviewed almost everything, see comments below. But not the > > Recovery_context methods. Please explain how it works and how all > > these truncate_validated, truncate_reset_done, truncate_set_in_1st, > > etc all work together. > > ...specifically to this point. Just in case I hope you did not miss to > read recovery_design.txt from the MDEV, which does not go into coding > details that you're effectively about in above. I've read it now. Still I don't understand why you need an extra round of binlog scanning (two rounds if only one file, three rounds if many). Also, this recovery_design.txt is not part of the code, so whoever will look at it later will be just as puzzled as I was. ... > [ G1, G2, ..., G_k, g_{k+1}, ... g_n ] > > here the uppercase `G' stands for committed trx, the smallcase `g' for > prepared,`_k' - sub-scripts in the recovery sequence. As the capital > letter first rules in the single-engine case the first occurrence of a > pattern `G_k,g_k+1' identifies the truncate index. The patch reflects > such fact with raising `truncate_validated' flag. > > But it's more complicated in the multiple binlog files / engines case. > The first `Gg' letter-case drop is not guaranteed to be the only drop > so `truncate_reset_done' and `truncate_set_in_1st' are introduced to > help with truncate index identification. Why is it not guaranteed? > When `truncate_validated' is set that indicates the truncate index is > determined and may not change in the current (1st of 2nd) nor future > rounds. `truncate_reset_done' says that an "inverse" `g_k,G_k+1' pair > is found so that any earlier truncation candidate gets reset (to > "zero"). If there will be later any candidate found in *this* (1st or > 2nd) round in the sequence its index will be obviously greater. > > `truncate_set_in_1st' function is to remember that the truncate > candidate was found in the 1st round (in the "hot" binlog file), but > if the candidate has not been validated `!truncate_validated' it may > be exacted in the 2nd round and then to an earlier transaction. So the > flag helps to handle exception from truncate candidate monotony rule: > e.g the hot binlog B2 contains `[g5,g6,...g_n]' and a ref to binlog > checkpoint file B1 that contains `[G1,g2,G3,g4]'. The first round > truncate candidate of g5 would be first exacted to `g2' before finally > ascertained to `g4' in the 2nd round. (Notice `g2 -> g4' preserves the > truncate index monotony). > > Notice that due to exacting like `g2 -> g4' in the 2nd round of the > above example `g2' got "to be up-cased" into `G2' for committing > (feasible with two trx:s on two different engine scenario - g2 with > Innodb only got prepared, G4 - on Rocksdb, and got committed prior the > crash). That's what the 3rd round is for. > > I hope this will be helpful. Unfortunately, not very much. It describes how you juggle with variables and scanning rounds. But not *what* you're trying to find. And it's kind of difficult to reverse engineer your "how" back into "what" :( Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MDEV-17399: JSON_TABLE: Incorrect code with table elimination
by Sergey Petrunia 15 Mar '21

15 Mar '21

Hi Alexey, > diff --git a/sql/opt_table_elimination.cc b/sql/opt_table_elimination.cc > index 3958797ec44..f2497d524ec 100644 > --- a/sql/opt_table_elimination.cc > +++ b/sql/opt_table_elimination.cc > @@ -637,6 +637,16 @@ void eliminate_tables(JOIN *join) > List_iterator<Item> it(join->fields_list); > while ((item= it++)) > used_tables |= item->used_tables(); > + > + { > + List_iterator<TABLE_LIST> it(*join->join_list); > + TABLE_LIST *tbl; > + while ((tbl= it++)) > + { > + if (tbl->table_function) > + used_tables|= tbl->table_function->used_tables(); > + } > + } > This only walks the tables that are "at the top level" of the join. if a JSON_TABLE(...) is inside a nested outer join, it will not be found. Please walk select_lex->leaf_tables instead. Please add the testcase (I provide one below). Please also add a comment, clarifying what is being done, something like: Table function JSON_TABLE() can have references to other tables. Do not eliminate the tables that JSON_TABLE() refers to. Note: the JSON_TABLE itself cannot be eliminated as it doesn't have unique keys. == Testcase == create table t20 (a int not null); create table t21 (a int not null primary key, js varchar(100)); insert into t20 select seq from seq_1_to_100; insert into t21 select a, '{"a":100}' from t20; create table t31(a int); create table t32(b int); insert into t31 values (1); insert into t32 values (1); explain select t20.a, jt1.ab from t20 left join t21 on t20.a=t21.a join (t31 left join (t32 join JSON_TABLE(t21.js,'$' COLUMNS (ab INT PATH '$.a')) AS jt1) on t31.a<3); BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 3