- developers - lists.mariadb.org

Re: [Maria-developers] InnoDB blob for primary key
by Sachin Setia 18 Apr '16

18 Apr '16

Hi Sergei! Actually I was going through the mysql source code for unique long constraints in file sql_tmp_table.cc in function create_tmp_table they make a new field and a new key(hash_key) and pass this table obejct to storage engine.They actually refer this field as a hash field On the time of insert they call bool check_unique_constraint(TABLE *table) function which first calculate the hash and store it in field then they see for duplicate hash and retrive ha_index_next_same if records are not same then record We can do the same thing in mariadb by adding one more field and key in mysql_prepare_create_table in this we check for blob with unlimited length or varchar for length greater then internal storage engine by doing this in mysql_prepare_create_table there will be no issues of frm file inconsistance. In case of insert first we will fill the hash field in fill_record function of sql_base.cc by first calculating the hash. Then we will retrive the index map using ha_index_read_map if returened value is zero then we will comapare two records and if they match then we will through error I am not sure where to place this code either in fill_record or later Or i can simple just fill hash in field in fill_record and then check for duplicates later on. Current I am not sure how to hide columns from user.Sir, can you suggest me where to look But there is one problem we can make unique key by this approch but not primary key because primary key is clustered and hashes can collide so i think we can't use hash field as primary key. To overcome this problem I have one idea instead of storing just hash we can make hash field length 10 bytes and in last two bytes we can store short int which tells how much time hash is repeated this can make hash unique in case of collusion. And also we are not doing more computation because we already retrive all records with same hashes. What do you think of this idea?. And there is one more problem how to make it foreign key. Will send you a prototype code tomorrow. Regards sachin On Fri, Apr 15, 2016 at 12:23 AM, Sergei Golubchik <serg(a)mariadb.org> wrote: > Hi, Sachin! > > On Apr 13, Sachin Setia wrote: >> Hello Sergei >> Sorry I did not see your mail. Actually i was thinking something like >> this before implementing the prototype but if i am more closer to >> innodb the more performance i will i get. I will definitively think >> about it. > > Great! > > Could you please tell me (mailing list, that is) what you think before > next Monday (before April 18h, that is)? > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org

2 5

[Maria-developers] Deadlock with STOP SLAVE and LOCK_active_mi
by Kristian Nielsen 18 Apr '16

18 Apr '16

So while looking at MDEV-9573, I found this code in mysql_execute_command() for STOP SLAVE: mysql_mutex_lock(&LOCK_active_mi); if ((mi= (master_info_index-> get_master_info(&lex_mi->connection_name, Sql_condition::WARN_LEVEL_ERROR)))) if (!stop_slave(thd, mi, 1/* net report*/)) my_ok(thd); mysql_mutex_unlock(&LOCK_active_mi); So basically, this code is holding LOCK_active_mi for the entire duration of the STOP SLAVE operation. This seems completely broken. Anything in a slave thread that tries to take LOCK_active_mi will then deadlock with the STOP SLAVE operation. It is simple enough to trigger, testcase (with sleep-injecting patch) at the end of the email. This uses INFORMATION_SCHEMA.SESSSION_STATUS; I could imagine accessing a number of system variables could trigger it as well. >From a quick check, it looks like this has been like this forever, eg. 5.1 has similar code. Any suggestions for how this is supposed to work? Or is it just broken by design, but saved because normally slave threads do not need to access SHOW STATUS or system variables? - Kristian. ----------------------------------------------------------------------- diff --git a/sql/mysqld.cc b/sql/mysqld.cc index ced9225..2358d6b 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -7765,6 +7765,7 @@ static int show_heartbeat_period(THD *thd, SHOW_VAR *var, char *buff, var->type= SHOW_CHAR; var->value= buff; +if (thd->slave_thread) my_sleep(5000000); mysql_mutex_lock(&LOCK_active_mi); if (master_info_index) { ----------------------------------------------------------------------- --source include/have_innodb.inc --source include/master-slave.inc --connection master CREATE TABLE t1 (a INT PRIMARY KEY) ENGINE=InnoDB; --save_master_pos --connection slave --sync_with_master --source include/stop_slave.inc --connection master INSERT INTO t1 SELECT VARIABLE_VALUE FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='COM_COMMIT'; --save_master_pos --connection slave --source include/start_slave.inc # Wait a bit to hit a sleep() in code after taking LOCK_active_mi. SELECT SLEEP(2); STOP SLAVE; SELECT * FROM t1 ORDER BY a; START SLAVE; --sync_with_master SELECT * FROM t1 ORDER BY a; # Clean up. --connection master DROP TABLE t1; --source include/rpl_end.inc -----------------------------------------------------------------------

2 2

[Maria-developers] MDEV-9521 Least function returns 0000-00-00 for null date columns instead of null
by Alexander Barkov 18 Apr '16

18 Apr '16

Hi Sergei, Please review a patch for MDEV-9521. Thanks.

2 3

[Maria-developers] Enabling DBUG_* macros
by Игорь Пашев 18 Apr '16

18 Apr '16

Hi all :-) How can I enable all those nice DBUG_* macros and/or see their output? I'm configuring MariaDB with cmake -DCMAKE_BUILD_TYPE=Debug, but it seems I don't have those macros.

2 1

[Maria-developers] nariadb 10.1.13 fails with openssl on gentoo
by Benny Pedersen 17 Apr '16

17 Apr '16

if openssl works for othres i like to know a working my.cnf to make it work, i have added my ssql same way as used in dovecot / postfix, no ssl error in mysql, but openssl s_client -showcerts -connect 127.0.0.1:3306 says ssl23 fails, at best i see ssl3 tlsv1 fails, output is CONNECTED(00000003) --- no peer certificate available --- No client certificate CA names sent --- SSL handshake has read 7 bytes and written 308 bytes --- New, (NONE), Cipher is (NONE) Secure Renegotiation IS NOT supported Compression: NONE Expansion: NONE No ALPN negotiated SSL-Session: Protocol : TLSv1.2 Cipher : 0000 Session-ID: Session-ID-ctx: Master-Key: Key-Arg : None PSK identity: None PSK identity hint: None SRP username: None Start Time: 1460845475 Timeout : 300 (sec) Verify return code: 0 (ok) --- how to debug it more from here ?

3 5

Re: [Maria-developers] 56ef213: MDEV-9836 Connection lost when using SSL
by Sergei Golubchik 16 Apr '16

16 Apr '16

Hi, Vlad! On Mar 30, wlad(a)mariadb.com wrote: > revision-id: 56ef213b8be7c91331168df002c4f69d54a26081 (mariadb-5.5.48-15-g56ef213) > parent(s): 11b77e9b18a8d97063b4c4a96e40bf9c75bd0e8b > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2016-03-30 23:09:57 +0200 > message: > > MDEV-9836 Connection lost when using SSL > > Don't read from socket in yassl in SSL_pending(). > Just return size of the buffered unprocessed data. As far as I understand, you make it to return the size of buffered *processed* data. Which is correct, according to OpenSSL documentation: SSL_pending() returns the number of bytes which have been processed, buffered and are available inside ssl for immediate read. If you agree with my understanding, please fix the commit comment to say "processed" and add a reference to the manual (a quote as above or "as specified in the OpenSSL manual" or whatever you prefer). Then ok push. > diff --git a/extra/yassl/src/ssl.cpp b/extra/yassl/src/ssl.cpp > index 9516e8b..2346533 100644 > --- a/extra/yassl/src/ssl.cpp > +++ b/extra/yassl/src/ssl.cpp > @@ -1471,10 +1471,6 @@ int SSL_peek(SSL* ssl, void* buffer, int sz) > > int SSL_pending(SSL* ssl) > { > - // Just in case there's pending data that hasn't been processed yet... > - char c; > - SSL_peek(ssl, &c, 1); > - > return ssl->bufferedData(); > } > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Error compiling MariaDB 10.1.13
by Alexey Strokach 14 Apr '16

14 Apr '16

I am having trouble compiling MariaDB as a conda package on Ubuntu 14.04. I pushed the conda recipe that I am using as well as the the stdout and stderr output to a github repo <https://github.com/ostrokach/conda-recipes-extra/tree/master/mariadb-10>. In brief, I am compiling MariaDB using CFLAGS="-I$PREFIX/include -I$PREFIX/include/ncurses" \ CPPFLAGS="-I$PREFIX/include -I$PREFIX/include/ncurses" \ CXXFLAGS="-I$PREFIX/include -I$PREFIX/include/ncurses" \ LDFLAGS="-L$PREFIX/lib" \ cmake . \ -DBUILD_CONFIG=mysql_release \ -DCMAKE_PREFIX_PATH:PATH="$PREFIX" \ -DCMAKE_INSTALL_PREFIX:PATH="$PREFIX" \ -DMYSQL_DATADIR:PATH="$PREFIX/data" make make install where $PREFIX is the location of the conda build environment containing the following packages: requirements: build: - cmake - bison - ncurses - zlib - lzo - jemalloc ==3.6.0 - aio - readline - openssl - zeromq I am using the system gcc compiler: gcc (Ubuntu 4.8.5-2ubuntu1~14.04.1) 4.8.5 Copyright (C) 2015 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. I get the following error at 91% during make: + CFLAGS='-I/home/user/anaconda3/envs/_build/include -I/home/user/anaconda3/envs/_build/include/ncurses' + CPPFLAGS='-I/home/user/anaconda3/envs/_build/include -I/home/user/anaconda3/envs/_build/include/ncurses' + CXXFLAGS='-I/home/user/anaconda3/envs/_build/include -I/home/user/anaconda3/envs/_build/include/ncurses' + LDFLAGS=-L/home/user/anaconda3/envs/_build/lib + cmake . -DBUILD_CONFIG=mysql_release -DCMAKE_PREFIX_PATH:PATH=/home/user/anaconda3/envs/_build -DCMAKE_INSTALL_PREFIX:PATH=/home/user/anaconda3/envs/_build -DMYSQL_DATADIR:PATH=/home/user/anaconda3/envs/_build/data + make /usr/bin/ar: creating /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/libmysql/libmysqlclient.a /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/sql/sql_yacc.yy:1022.1-12: warning: deprecated directive, use ‘%pure-parser’ [-Wdeprecated] %pure_parser /* We have threads */ ^^^^^^^^^^^^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/mroonga/vendor/groonga/lib/expr.c: In function ‘grn_expr_exec’: /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/mroonga/vendor/groonga/lib/expr.c:3543:1: warning: const/copy propagation disabled: 21688 basic blocks and 63352 registers [-Wdisabled-optimization] } ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/mroonga/vendor/groonga/lib/expr.c:3543:1: warning: PRE disabled: 21688 basic blocks and 63352 registers [-Wdisabled-optimization] /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/mroonga/vendor/groonga/lib/expr.c:3543:1: warning: const/copy propagation disabled: 21688 basic blocks and 63352 registers [-Wdisabled-optimization] /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/mroonga/vendor/groonga/lib/expr.c:3543:1: warning: const/copy propagation disabled: 19113 basic blocks and 64360 registers [-Wdisabled-optimization] CMake Warning (dev) in CMakeLists.txt: No cmake_minimum_required command is present. A line of code such as cmake_minimum_required(VERSION 3.5) should be added at the top of the file. The version specified may be lower if you wish to support older CMake versions for this project. For more information run "cmake --help-policy CMP0000". This warning is for project developers. Use -Wno-dev to suppress it. /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/tokudb/PerconaFT/xz/src/build_lzma/src/liblzma/lz/lz_encoder.c: In function ‘fill_window’: /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/storage/tokudb/PerconaFT/xz/src/build_lzma/src/liblzma/lz/lz_encoder.c:89:9: warning: variable ‘in_used’ set but not used [-Wunused-but-set-variable] size_t in_used; ^ In file included from /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/plugin/auth_gssapi/client_plugin.cc:37:0: /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/include/mysql/client_plugin.h:64:9: warning: ‘_mysql_client_plugin_declaration_’ initialized and declared ‘extern’ [enabled by default] _mysql_client_plugin_declaration_ = { \ ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/plugin/auth_gssapi/client_plugin.cc:101:1: note: in expansion of macro ‘mysql_declare_client_plugin’ mysql_declare_client_plugin(AUTHENTICATION) ^ troff: fatal error: can't find macro file m troff: fatal error: can't find macro file m /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc: In function ‘int com_status(String*, char*)’: /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4738:13: error: ‘A_BOLD’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4738:19: error: ‘vidattr’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4740:13: error: ‘A_NORMAL’ was not declared in this scope vidattr(A_NORMAL); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4808:13: error: ‘A_BOLD’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4808:19: error: ‘vidattr’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4810:13: error: ‘A_NORMAL’ was not declared in this scope vidattr(A_NORMAL); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc: In function ‘int put_info(const char*, INFO_TYPE, uint, const char*)’: /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4905:47: error: ‘setupterm’ was not declared in this scope (void) setupterm((char *)0, 1, (int *) 0); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4918:15: error: ‘A_STANDOUT’ was not declared in this scope vidattr(A_STANDOUT); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4918:25: error: ‘vidattr’ was not declared in this scope vidattr(A_STANDOUT); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4937:15: error: ‘A_BOLD’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4937:21: error: ‘vidattr’ was not declared in this scope vidattr(A_BOLD); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4939:13: error: ‘A_NORMAL’ was not declared in this scope vidattr(A_NORMAL); ^ /home/user/anaconda3/conda-bld/work/server-mariadb-10.1.13/client/mysql.cc:4939:21: error: ‘vidattr’ was not declared in this scope vidattr(A_NORMAL); ^ make[2]: *** [client/CMakeFiles/mysql.dir/mysql.cc.o] Error 1 make[1]: *** [client/CMakeFiles/mysql.dir/all] Error 2 make: *** [all] Error 2 Using Anaconda Cloud api site https://api.anaconda.org Command failed: /bin/bash -x -e /home/user/anaconda/conda-recipes-extra/mariadb-10/build.sh Any help would be much appreciated, Alexey

2 1

[Maria-developers] InnoDB blob for primary key
by Sachin Setia 13 Apr '16

13 Apr '16

Hello Developers, Hi this is sachin.Actually i was currently experimenting with with blob uniques in innodb there is three main problems 1.Unique blob 2.Blob Forigen key 3.Blob primary key 1. For blob unique we can simply store hash in unclustered index 2. Blob Forigen key i am currently working on it 3. Blob primary key :- for this i thought we create a 4 byte column which stores the hash of blob primary key.Internally this column will work as primary key and key for clustered index. I already successufully tested this here is my output MariaDB [sachin]> create table t4 (abc blob primary key); Query OK, 0 rows affected (0.10 sec) MariaDB [sachin]> insert into t4 values('sachin'); Query OK, 1 row affected (0.01 sec) MariaDB [sachin]> insert into t4 values('sachin'); ERROR 1062 (23000): Duplicate entry 'sachin' for key 'PRIMARY' MariaDB [sachin]> insert into t4 values('sachin setiya'); Query OK, 1 row affected (0.00 sec) MariaDB [sachin]> insert into t4 values('sachin setiya'); ERROR 1062 (23000): Duplicate entry 'sachin setiya' for key 'PRIMARY' MariaDB [sachin]> desc t4; +-------+------+------+-----+---------+-------+ | Field | Type | Null | Key | Default | Extra | +-------+------+------+-----+---------+-------+ | abc | blob | NO | PRI | NULL | | +-------+------+------+-----+---------+-------+ 1 row in set (0.01 sec) MariaDB [sachin]> select * from t4; +---------------+ | abc | +---------------+ | sachin | | sachin setiya | +---------------+ 2 rows in set (0.01 sec) @Sergei hi! Actually i invested arround 2 months in mariadb So for me now it does not matter either i got selected in gsoc i want to do work in innodb blob unique from today.Please sir allow me to do this I am including the patch file and t4.ibd and t4.frm file Regards sachin

2 3

Re: [Maria-developers] 4c51152: MDEV-8931: (server part of) session state tracking
by Sergei Golubchik 08 Apr '16

08 Apr '16

Hi, Sanja! Summary: Lots of comments below, many changes needed. When you copy a big feature from MySQL next time - please, take a more critical look at what you're merging. On Mar 28, OleksandrByelkin wrote: > revision-id: 4c51152d9f43e271e17a2bc266f5887ce092c00f (mariadb-10.1.8-187-g4c51152) > parent(s): 69b5c4a4220c8fa98bbca8b3f935b2a3735e19ac > committer: Oleksandr Byelkin > timestamp: 2016-03-28 19:19:54 +0200 > message: > > MDEV-8931: (server part of) session state tracking > > diff --git a/include/mysql_com.h b/include/mysql_com.h > index c13999a..c6608b7 100644 > --- a/include/mysql_com.h > +++ b/include/mysql_com.h > @@ -520,6 +544,30 @@ enum enum_mysql_set_option > MYSQL_OPTION_MULTI_STATEMENTS_OFF > }; > > +/* > + Type of state change information that the server can include in the Ok > + packet. > + Note : 1) session_state_type shouldn't go past 255 (i.e. 1-byte boundary). > + 2) Modify the definition of SESSION_TRACK_END when a new member is > + added. > +*/ > +enum enum_session_state_type > +{ > + SESSION_TRACK_SYSTEM_VARIABLES, /* Session system variables */ support comes in the next patch, I know > + SESSION_TRACK_SCHEMA, /* Current schema */ suported, I presume > + SESSION_TRACK_STATE_CHANGE, /* track session state changes */ suported, I presume > + SESSION_TRACK_GTIDS, not suported, I presume > + SESSION_TRACK_TRANSACTION_CHARACTERISTICS, /* Transaction chistics */ > + SESSION_TRACK_TRANSACTION_STATE /* Transaction state */ to be imlemented later, I suppose > +}; > + > +#define SESSION_TRACK_BEGIN SESSION_TRACK_SYSTEM_VARIABLES > + > +#define SESSION_TRACK_END SESSION_TRACK_TRANSACTION_STATE > + > +#define IS_SESSION_STATE_TYPE(T) \ > + (((int)(T) >= SESSION_TRACK_BEGIN) && ((T) <= SESSION_TRACK_END)) > + > #define net_new_transaction(net) ((net)->pkt_nr=0) > > #ifdef __cplusplus > diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result > index a35693e..f69b615 100644 > --- a/mysql-test/r/mysqld--help.result > +++ b/mysql-test/r/mysqld--help.result > @@ -903,6 +903,11 @@ The following options may be given as the first argument: > files within specified directory > --server-id=# Uniquely identifies the server instance in the community > of replication partners > + --session-track-schema > + Track changes to the 'default schema'. > + (Defaults to on; use --skip-session-track-schema to disable.) > + --session-track-state-change > + Track changes to the 'session state'. > --show-slave-auth-info > Show user and password in SHOW SLAVE HOSTS on this > master. > diff --git a/mysql-test/suite/sys_vars/r/session_track_schema_basic.result b/mysql-test/suite/sys_vars/r/session_track_schema_basic.result > new file mode 100644 > index 0000000..be4b892 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/r/session_track_schema_basic.result > @@ -0,0 +1,116 @@ we don't need these "basic" sysvar tests anymore. The test that checks for them is disabled. sysvar_* tests are now used to show basic sysvar characteristics > +# > +# Variable name : session_track_schema > +# Scope : Global & Session > +# > +# Global - default > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +1 > +# Session - default > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > + > +# via INFORMATION_SCHEMA.GLOBAL_VARIABLES > +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; > +VARIABLE_NAME VARIABLE_VALUE > +# via INFORMATION_SCHEMA.SESSION_VARIABLES > +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; > +VARIABLE_NAME VARIABLE_VALUE > +SET @global_saved_tmp = @@global.session_track_schema; > + > +# Altering global variable's value > +SET @@global.session_track_schema = 0; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > +SET @@global.session_track_schema = TrUe; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +1 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > +SET @@global.session_track_schema = FaLsE; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > + > +# Altering session variable's value > +SET @@session.session_track_schema = 0; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +0 > + > +# Variables' values in a new session. > +# Global - expect 0 > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > + > +# Session - expect 0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +0 > + > +# Switching to the default connection. > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +0 > + > +# Test if DEFAULT is working as expected. > +SET @@global.session_track_schema = DEFAULT; > +SET @@session.session_track_schema = DEFAULT; > + > +# Global - expect 1 > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +1 > +# Session - expect 1 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > + > +# Variables' values in a new session (con2). > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +1 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > + > +# Altering session should not affect global. > +SET @@session.session_track_schema = FALSE; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +1 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +0 > + > +# Variables' values in a new session (con3). > +# Altering global should not affect session. > +SET @@global.session_track_schema = OFF; > +SELECT @@global.session_track_schema; > +@@global.session_track_schema > +0 > +SELECT @@session.session_track_schema; > +@@session.session_track_schema > +1 > + > +# Switching to the default connection. > +# Restoring the original values. > +SET @@global.session_track_schema = @global_saved_tmp; > +# End of tests. > diff --git a/mysql-test/suite/sys_vars/t/session_track_schema_basic.test b/mysql-test/suite/sys_vars/t/session_track_schema_basic.test > new file mode 100644 > index 0000000..220ae35 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/t/session_track_schema_basic.test > @@ -0,0 +1,105 @@ another meaningless basic test > +--source include/not_embedded.inc > + > +--echo # > +--echo # Variable name : session_track_schema > +--echo # Scope : Global & Session > +--echo # > + > +--echo # Global - default > +SELECT @@global.session_track_schema; > +--echo # Session - default > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # via INFORMATION_SCHEMA.GLOBAL_VARIABLES > +--disable_warnings > +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; > +--enable_warnings > + > +--echo # via INFORMATION_SCHEMA.SESSION_VARIABLES > +--disable_warnings > +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; > +--enable_warnings > + > +# Save the global value to be used to restore the original value. > +SET @global_saved_tmp = @@global.session_track_schema; > +--echo > + > +--echo # Altering global variable's value > +SET @@global.session_track_schema = 0; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > + > +SET @@global.session_track_schema = TrUe; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > + > +SET @@global.session_track_schema = FaLsE; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Altering session variable's value > +SET @@session.session_track_schema = 0; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Variables' values in a new session. > +connect (con1,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +--echo # Global - expect 0 > +SELECT @@global.session_track_schema; > +--echo > +--echo # Session - expect 0 > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Switching to the default connection. > +connection default; > + > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Test if DEFAULT is working as expected. > +SET @@global.session_track_schema = DEFAULT; > +SET @@session.session_track_schema = DEFAULT; > +--echo > + > +--echo # Global - expect 1 > +SELECT @@global.session_track_schema; > +--echo # Session - expect 1 > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Variables' values in a new session (con2). > +connect (con2,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Altering session should not affect global. > +SET @@session.session_track_schema = FALSE; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Variables' values in a new session (con3). > +connect (con3,"127.0.0.1",root,,test,$MASTER_MYPORT,); > + > +--echo # Altering global should not affect session. > +SET @@global.session_track_schema = OFF; > +SELECT @@global.session_track_schema; > +SELECT @@session.session_track_schema; > +--echo > + > +--echo # Switching to the default connection. > +connection default; > + > +--echo # Restoring the original values. > +SET @@global.session_track_schema = @global_saved_tmp; > + > +--echo # End of tests. > + > diff --git a/sql/protocol.cc b/sql/protocol.cc > index 9e52870..c1e66d7 100644 > --- a/sql/protocol.cc > +++ b/sql/protocol.cc > @@ -197,6 +197,8 @@ bool net_send_error(THD *thd, uint sql_errno, const char *err, > @param affected_rows Number of rows changed by statement > @param id Auto_increment id for first row (if used) > @param message Message to send to the client (Used by mysql_status) > + @param eof_indentifier when true [FE] will be set in OK header > + else [00] will be used eh, I understand that's what MySQL has, and we want to be protocol-compatible, but can you put a meaningful comment here, please? Not like i++; // increment i by 1 and a better name for the parameter would be nice too. > > @return > @retval FALSE The message was successfully sent > @@ -221,9 +233,32 @@ net_send_ok(THD *thd, > DBUG_RETURN(FALSE); > } > > - buff[0]=0; // No fields > - pos=net_store_length(buff+1,affected_rows); > - pos=net_store_length(pos, id); > + start= buff; > + > + /* > + Use 0xFE packet header if eof_identifier is true > + unless we are talking to old client > + */ fix this comment, please > + if (eof_identifier && > + (thd->client_capabilities & CLIENT_DEPRECATE_EOF)) > + buff[0]= 254; > + else > + buff[0]= 0; I don't understand this, because the comment doesn't explain anything > + > + /* affected rows */ > + pos= net_store_length(buff + 1, affected_rows); > + > + /* last insert id */ > + pos= net_store_length(pos, id); > + > + if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && > + thd->session_tracker.enabled_any() && > + thd->session_tracker.changed_any()) > + { > + server_status |= SERVER_SESSION_STATE_CHANGED; > + state_changed= true; I'd rather have trackers to set server_status when they're changed, instead of iterating the list of trackers here twice for every statement just because we apparently have free CPU cycles to burn > + } > + > if (thd->client_capabilities & CLIENT_PROTOCOL_41) > { > DBUG_PRINT("info", > @@ -247,9 +282,45 @@ net_send_ok(THD *thd, > } > thd->get_stmt_da()->set_overwrite_status(true); > > + if ((thd->client_capabilities & CLIENT_SESSION_TRACK)) > + { > + /* the info field */ > + if (state_changed || (message && message[0])) > + pos= net_store_data(pos, (uchar*) message, message ? strlen(message) : 0); > + /* session state change information */ > + if (unlikely(state_changed)) > + { > + store.set_charset(thd->variables.collation_database); > + > + /* > + First append the fields collected so far. In case of malloc, memory > + for message is also allocated here. > + */ > + store.append((const char *)start, (pos - start), MYSQL_ERRMSG_SIZE); no, don't use "buf or store, depending on whether session tracking is used" that's ridiculous (and an extra memcpy) always use store, remove buf. declare store as StringBuffer<MYSQL_ERRMSG_SIZE+10> store(thd->variables.collation_database); > + > + /* .. and then the state change information. */ > + thd->session_tracker.store(thd, store); > + > + start= (uchar *) store.ptr(); > + pos= start+store.length(); > + } > + } > - if (message && message[0]) > + else if (message && message[0]) > + { > + /* the info field, if there is a message to store */ > pos= net_store_data(pos, (uchar*) message, strlen(message)); > - error= my_net_write(net, buff, (size_t) (pos-buff)); > + } > + > + /* OK packet length will be restricted to 16777215 bytes */ > + if (((size_t) (pos - start)) > MAX_PACKET_LENGTH) > + { > + net->error= 1; I'm surprised, this doesn't have a comment "setting net->error to 1" would be quite in style with what I've seen so far :) > + net->last_errno= ER_NET_OK_PACKET_TOO_LARGE; > + my_error(ER_NET_OK_PACKET_TOO_LARGE, MYF(0)); > + DBUG_PRINT("info", ("OK packet too large")); > + DBUG_RETURN(1); > + } > + error= my_net_write(net, start, (size_t) (pos - start)); > if (!error) > error= net_flush(net); > > @@ -559,7 +630,20 @@ bool Protocol::send_ok(uint server_status, uint statement_warn_count, > bool Protocol::send_eof(uint server_status, uint statement_warn_count) > { > DBUG_ENTER("Protocol::send_eof"); > - const bool retval= net_send_eof(thd, server_status, statement_warn_count); > + bool retval; > + /* > + Normally end of statement reply is signaled by OK packet, but in case > + of binlog dump request an EOF packet is sent instead. Also, old clients > + expect EOF packet instead of OK > + */ > +#ifndef EMBEDDED_LIBRARY > + if ((thd->client_capabilities & CLIENT_DEPRECATE_EOF) && > + (thd->get_command() != COM_BINLOG_DUMP )) > + retval= net_send_ok(thd, server_status, statement_warn_count, 0, 0, NULL, > + true); > + else > +#endif > + retval= net_send_eof(thd, server_status, statement_warn_count); better keep the old code here, just net_send_eof(). and in net_send_eof() you do, like if (thd->client_capabilities & CLIENT_DEPRECATE_EOF) return net_send_ok(..., 0, 0, NULL, true); > DBUG_RETURN(retval); > } > > @@ -1551,8 +1641,14 @@ bool Protocol_binary::send_out_parameters(List<Item_param> *sp_params) > /* Restore THD::server_status. */ > thd->server_status&= ~SERVER_PS_OUT_PARAMS; > > - /* Send EOF-packet. */ > - net_send_eof(thd, thd->server_status, 0); > + if (thd->client_capabilities & CLIENT_DEPRECATE_EOF) > + ret= net_send_ok(thd, > + (thd->server_status | SERVER_PS_OUT_PARAMS | > + SERVER_MORE_RESULTS_EXISTS), > + 0, 0, 0, NULL, true); > + else > + /* In case of old clients send EOF packet. */ > + ret= net_send_eof(thd, thd->server_status, 0); > > /* > Reset SERVER_MORE_RESULTS_EXISTS bit, because this is the last packet note that unlike 5.7 you we have this "Reset SERVER_MORE_RESULTS_EXISTS" *after* net_send_eof() (bugfix for MDEV-4604) 1. you don't need "| SERVER_MORE_RESULTS_EXISTS" for net_send_ok() 2. you might try to use only net_send_eof() here too (with both flags set) > diff --git a/sql/session_tracker.h b/sql/session_tracker.h > new file mode 100644 > index 0000000..7477f96 > --- /dev/null > +++ b/sql/session_tracker.h > @@ -0,0 +1,255 @@ > +#ifndef SESSION_TRACKER_INCLUDED > +#define SESSION_TRACKER_INCLUDED > + > +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > + > +#include "m_string.h" > +#include "thr_lock.h" > + > +/* forward declarations */ > +class THD; > +class set_var; > +class String; > + > + > +enum enum_session_tracker > +{ > + SESSION_SYSVARS_TRACKER, /* Session system variables */ > + CURRENT_SCHEMA_TRACKER, /* Current schema */ > + SESSION_STATE_CHANGE_TRACKER, > + SESSION_GTIDS_TRACKER, /* Tracks GTIDs */ > + TRANSACTION_INFO_TRACKER /* Transaction state */ why not to use the same enum from mysql_com.h ? > +}; > + > +#define SESSION_TRACKER_END TRANSACTION_INFO_TRACKER > + > + > +/** > + State_tracker > + ------------- > + An abstract class that defines the interface for any of the server's > + 'session state change tracker'. A tracker, however, is a sub- class of > + this class which takes care of tracking the change in value of a part- > + icular session state type and thus defines various methods listed in this > + interface. The change information is later serialized and transmitted to > + the client through protocol's OK packet. > + > + Tracker system variables :- > + A tracker is normally mapped to a system variable. So in order to enable, > + disable or modify the sub-entities of a tracker, the user needs to modify > + the respective system variable either through SET command or via command > + line option. As required in system variable handling, this interface also > + includes two functions to help in the verification of the supplied value > + (ON_CHECK) and the updation (ON_UPDATE) of the tracker system variable, > + namely - check() and update(). this is illogical. check() and update() do not belong in the base State_tracker > +*/ > + > +class State_tracker > +{ > +protected: > + /** Is tracking enabled for a particular session state type ? */ > + bool m_enabled; > + > + /** Has the session state type changed ? */ > + bool m_changed; > + > +public: > + /** Constructor */ > + State_tracker() : m_enabled(false), m_changed(false) > + {} > + > + /** Destructor */ > + virtual ~State_tracker() > + {} > + > + /** Getters */ > + bool is_enabled() const > + { return m_enabled; } > + > + bool is_changed() const > + { return m_changed; } > + > + /** Called in the constructor of THD*/ > + virtual bool enable(THD *thd)= 0; > + > + /** To be invoked when the tracker's system variable is checked (ON_CHECK). */ > + virtual bool check(THD *thd, set_var *var)= 0; > + > + /** To be invoked when the tracker's system variable is updated (ON_UPDATE).*/ > + virtual bool update(THD *thd)= 0; > + > + /** Store changed data into the given buffer. */ > + virtual bool store(THD *thd, String &buf)= 0; > + > + /** Mark the entity as changed. */ > + virtual void mark_as_changed(THD *thd, LEX_CSTRING *name)= 0; > +}; > + > + > +/** > + Session_tracker > + --------------- > + This class holds an object each for all tracker classes and provides > + methods necessary for systematic detection and generation of session > + state change information. > +*/ > + > +class Session_tracker > +{ > +private: > + State_tracker *m_trackers[SESSION_TRACKER_END + 1]; > + > + /* The following two functions are private to disable copying. */ > + /** Copy constructor */ > + Session_tracker(Session_tracker const &other) > + { > + DBUG_ASSERT(FALSE); > + } > + > + /** Copy assignment operator */ > + Session_tracker& operator= (Session_tracker const &rhs) > + { > + DBUG_ASSERT(FALSE); > + return *this; > + } > + > +public: judging by other comments in this file (above and below), I'm surprised this keyword does not have a comment, like /* declare the rest of the object public. that makes the methods below accessible from the other code, not only from this object itself */ Serisouly, why do these comments explain basic C++ features? or repeat whatever the function is obviously doing (like, get_tracker() or enabled_any()) And non-trivial functions are not commented, for example server_boot_verify() > + > + /** Constructor */ > + Session_tracker() > + {} > + > + /** Destructor */ > + ~Session_tracker() > + { > + } > + /** > + Initialize Session_tracker objects and enable them based on the > + tracker_xxx variables' value that the session inherit from global > + variables at the time of session initialization (see plugin_thdvar_init). > + */ > + void init(const CHARSET_INFO *char_set); > + void enable(THD *thd); > + bool server_boot_verify(const CHARSET_INFO *char_set, LEX_STRING var_list); > + > + /** Returns the pointer to the tracker object for the specified tracker. */ > + State_tracker *get_tracker(enum_session_tracker tracker) const; > + > + /** Checks if m_enabled flag is set for any of the tracker objects. */ > + bool enabled_any(); > + > + /** Checks if m_changed flag is set for any of the tracker objects. */ > + bool changed_any(); > + > + /** > + Stores the session state change information of all changes session state > + type entities into the specified buffer. > + */ > + void store(THD *thd, String &main_buf); > + void deinit() > + { > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + delete m_trackers[i]; > + } > +}; > + > +/* > + Session_state_change_tracker > + ---------------------------- > + This is a boolean tracker class that will monitor any change that contributes > + to a session state change. > + Attributes that contribute to session state change include: > + - Successful change to System variables > + - User defined variables assignments > + - temporary tables created, altered or deleted > + - prepared statements added or removed > + - change in current database > +*/ > + > +class Session_state_change_tracker : public State_tracker Why is this one in the header, not in session_tracker.cc? > +{ > +private: > + > + void reset(); > + > +public: > + Session_state_change_tracker(); > + bool enable(THD *thd); > + bool check(THD *thd, set_var *var) > + { return false; } > + bool update(THD *thd); > + bool store(THD *thd, String &buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > + bool is_state_changed(THD*); > + void ensure_enabled(THD *thd) > + {} > +}; > + > + > +/** > + Transaction_state_tracker > + ---------------------- > + This is a tracker class that enables & manages the tracking of > + current transaction info for a particular connection. > +*/ > + > +/** > + Transaction state (no transaction, transaction active, work attached, etc.) > +*/ > +enum enum_tx_state { > + TX_EMPTY = 0, ///< "none of the below" > + TX_EXPLICIT = 1, ///< an explicit transaction is active > + TX_IMPLICIT = 2, ///< an implicit transaction is active > + TX_READ_TRX = 4, ///< transactional reads were done > + TX_READ_UNSAFE = 8, ///< non-transaction reads were done > + TX_WRITE_TRX = 16, ///< transactional writes were done > + TX_WRITE_UNSAFE = 32, ///< non-transactional writes were done > + TX_STMT_UNSAFE = 64, ///< "unsafe" (non-deterministic like UUID()) stmts > + TX_RESULT_SET = 128, ///< result-set was sent > + TX_WITH_SNAPSHOT= 256, ///< WITH CONSISTENT SNAPSHOT was used > + TX_LOCKED_TABLES= 512 ///< LOCK TABLES is active > +}; > + > +/** > + Transaction access mode > +*/ > +enum enum_tx_read_flags { > + TX_READ_INHERIT = 0, ///< not explicitly set, inherit session.tx_read_only > + TX_READ_ONLY = 1, ///< START TRANSACTION READ ONLY, or tx_read_only=1 > + TX_READ_WRITE = 2, ///< START TRANSACTION READ WRITE, or tx_read_only=0 > +}; > + > +/** > + Transaction isolation level > +*/ > +enum enum_tx_isol_level { > + TX_ISOL_INHERIT = 0, ///< not explicitly set, inherit session.tx_isolation > + TX_ISOL_UNCOMMITTED = 1, > + TX_ISOL_COMMITTED = 2, > + TX_ISOL_REPEATABLE = 3, > + TX_ISOL_SERIALIZABLE= 4 > +}; > + > +/** > + Transaction tracking level > +*/ > +enum enum_session_track_transaction_info { > + TX_TRACK_NONE = 0, ///< do not send tracker items on transaction info > + TX_TRACK_STATE = 1, ///< track transaction status > + TX_TRACK_CHISTICS = 2 ///< track status and characteristics > +}; Better remove all declarations related to transaction tracking and add them later in a separate patch (with the complete transaction tracking feature). > + > +#endif /* SESSION_TRACKER_INCLUDED */ > diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc > new file mode 100644 > index 0000000..6abff2a > --- /dev/null > +++ b/sql/session_tracker.cc > @@ -0,0 +1,454 @@ > +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. and "Copyright(c) 2016, MariaDB" because it's not 1:1 copy > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > + > + > +#include "session_tracker.h" > + > +#include "hash.h" > +#include "table.h" > +#include "rpl_gtid.h" > +#include "sql_class.h" > +#include "sql_show.h" > +#include "sql_plugin.h" > +//#include "xa.h" this is for SESSION_TRACK_TRANSACTION_STATE too I suppose > + > +static void store_lenenc_string(String &to, const char *from, > + size_t length); > + > +class Dummy_tracker : public State_tracker > +{ > +bool enable(THD *thd __attribute__((unused))) why __attribute__((unused)) if it is used? and why is it used, if the tracker is dummy? btw, seeing how this Dummy_tracker is used, I'd rather rename it to Not_implemented_tracker > + { return update(thd); } > + bool check(THD *thd __attribute__((unused)), > + set_var *var __attribute__((unused))) > + { return false; } it's C++, you don't need __attribute__((unused)), write bool check(THD *, set_var *) { return false; } > + bool update(THD *thd __attribute__((unused))) > + { return false; } > + bool store(THD *thd __attribute__((unused)), > + String &buf __attribute__((unused))) > + > + { return false; } > + void mark_as_changed(THD *thd __attribute__((unused)), > + LEX_CSTRING *tracked_item_name __attribute__((unused))) > + {} > + > +}; > + > + > +/** > + Current_schema_tracker > + ---------------------- > + This is a tracker class that enables & manages the tracking of current > + schema for a particular connection. > +*/ > + > +class Current_schema_tracker : public State_tracker > +{ > +private: > + bool schema_track_inited; > + void reset(); > + > +public: > + > + /** Constructor */ > + Current_schema_tracker() > + { > + schema_track_inited= false; > + } > + > + bool enable(THD *thd) > + { return update(thd); } > + bool check(THD *thd, set_var *var) > + { return false; } > + bool update(THD *thd); > + bool store(THD *thd, String &buf); > + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); > +}; > + > +/* To be used in expanding the buffer. */ > +static const unsigned int EXTRA_ALLOC= 1024; > + > +/////////////////////////////////////////////////////////////////////////////// > + > +/** > + @brief Enable/disable the tracker based on @@session_track_schema's value. > + > + @param thd [IN] The thd handle. > + > + @return > + false (always) > +*/ > + > +bool Current_schema_tracker::update(THD *thd) > +{ > + m_enabled= (thd->variables.session_track_schema)? true: false; > + return false; > +} > + > + > +/** > + @brief Store the schema name as length-encoded string in the specified > + buffer. Once the data is stored, we reset the flags related to > + state-change (see reset()). > + > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @return > + false Success > + true Error > +*/ > + > +bool Current_schema_tracker::store(THD *thd, String &buf) > +{ > + ulonglong db_length, length; > + > + length= db_length= thd->db_length; > + length += net_length_size(length); > + > + uchar *to= (uchar *) buf.prep_append(net_length_size(length) + 1, > + EXTRA_ALLOC); > + > + /* Session state type (SESSION_TRACK_SCHEMA) */ > + to= net_store_length(to, (ulonglong)SESSION_TRACK_SCHEMA); > + > + /* Length of the overall entity. */ > + to= net_store_length(to, length); > + > + /* Length of the changed current schema name. */ > + net_store_length(to, db_length); > + > + /* Current schema name (length-encoded string). */ > + store_lenenc_string(buf, thd->db, thd->db_length); > + > + reset(); > + > + return false; > +} > + > + > +/** > + @brief Mark the tracker as changed. > + > + @param name [IN] Always null. > + > + @return void > +*/ > + > +void Current_schema_tracker::mark_as_changed(THD *thd, > + LEX_CSTRING *tracked_item_name > + __attribute__((unused))) you don't need __attribute__((unused)) here, it's C++ besides, as I've just found, we compile with -Wno-unused-parameter (this was merged from MySQL, perhaps we should remove that flag) > +{ > + m_changed= true; > + thd->lex->safe_to_cache_query= 0; > +} > + > + > +/** > + @brief Reset the m_changed flag for next statement. > + > + @return void > +*/ > + > +void Current_schema_tracker::reset() > +{ > + m_changed= false; > +} > + > + > +/////////////////////////////////////////////////////////////////////////////// > + > +/** Constructor */ > +Session_state_change_tracker::Session_state_change_tracker() > +{ > + m_changed= false; > +} > + > +/** > + @brief Initiate the value of m_enabled based on > + @@session_track_state_change value. > + > + @param thd [IN] The thd handle. > + @return false (always) > + > +**/ > + > +bool Session_state_change_tracker::enable(THD *thd) > +{ > + m_enabled= (thd->variables.session_track_state_change)? true: false; > + return false; > +} > + > +/** > + @Enable/disable the tracker based on @@session_track_state_change value. > + > + @param thd [IN] The thd handle. > + @return false (always) > + > +**/ > + > +bool Session_state_change_tracker::update(THD *thd) > +{ > + return enable(thd); > +} you're kidding, right? For Dummy_tracker and Current_schema_tracker enable() calls upate(), for Session_state_change_tracker update() calls enable(). and no matter what method calls what, it's always one_method(THD *thd) { return other_method(thd); } other_method(THD *thd) { m_enabled= (thd->variables.session_track_XXX)? true: false; } why two methods enable/update and not one? why "? true : false", and not m_enabled=thd->variables.session_track_XXX (hint: m_enabled is bool, ?true:false is the same as cast to bool anyway) and why to copy the value in the first place??? Just use the value from thd->variables.session_track_XXX > + > +/** > + @brief Store the 1byte boolean flag in the specified buffer. Once the > + data is stored, we reset the flags related to state-change. If > + 1byte flag valie is 1 then there is a session state change else > + there is no state change information. > + > + @param thd [IN] The thd handle. > + @paran buf [INOUT] Buffer to store the information to. > + > + @return > + false Success > + true Error > +**/ > + > +bool Session_state_change_tracker::store(THD *thd, String &buf) > +{ > + /* since its a boolean tracker length is always 1 */ > + const ulonglong length= 1; > + > + uchar *to= (uchar *) buf.prep_append(3,EXTRA_ALLOC); > + > + /* format of the payload is as follows: > + [ tracker type] [length] [1 byte flag] */ Noooo! Do not copy the payload format logic to every tracker. Keep it in one place (preferrably, in the protocol) > + /* Session state type (SESSION_TRACK_STATE_CHANGE) */ > + to= net_store_length(to, (ulonglong)SESSION_TRACK_STATE_CHANGE); > + > + /* Length of the overall entity it is always 1 byte */ > + to= net_store_length(to, length); > + > + /* boolean tracker will go here */ > + *to= (is_state_changed(thd) ? '1' : '0'); > + > + reset(); > + > + return false; > +} > + > +/** > + @brief Mark the tracker as changed and associated session > + attributes accordingly. > + > + @param name [IN] Always null. > + @return void > +*/ > + > +void Session_state_change_tracker::mark_as_changed(THD *thd, > + LEX_CSTRING *tracked_item_name) > +{ > + /* do not send the boolean flag for the tracker itself > + in the OK packet */ > + if(tracked_item_name && > + (strncmp(tracked_item_name->str, "session_track_state_change", 26) == 0)) > + m_changed= false; Grrr. 1. Why not? 2. what if it is SET @foobar=5, @@session_track_state_change=ON; > + else > + { > + m_changed= true; > + thd->lex->safe_to_cache_query= 0; why? > + } > +} > + > +/** > + @brief Reset the m_changed flag for next statement. > + > + @return void > +*/ > + > +void Session_state_change_tracker::reset() > +{ > + m_changed= false; > +} > + > +/** > + @brief find if there is a session state change > + > + @return > + true - if there is a session state change > + false - if there is no session state change > +**/ > + > +bool Session_state_change_tracker::is_state_changed(THD* thd) > +{ > + return m_changed; > +} > + > +/////////////////////////////////////////////////////////////////////////////// > + > +/** > + @brief Initialize session tracker objects. > + > + @param char_set [IN] The character set info. > + > + @return void > +*/ > + > +void Session_tracker::init(const CHARSET_INFO *char_set) > +{ > + m_trackers[SESSION_SYSVARS_TRACKER]= > + new (std::nothrow) Dummy_tracker; > + m_trackers[CURRENT_SCHEMA_TRACKER]= > + new (std::nothrow) Current_schema_tracker; > + m_trackers[SESSION_STATE_CHANGE_TRACKER]= > + new (std::nothrow) Session_state_change_tracker; > + m_trackers[SESSION_GTIDS_TRACKER]= > + new (std::nothrow) Dummy_tracker; > + m_trackers[TRANSACTION_INFO_TRACKER]= > + new (std::nothrow) Dummy_tracker; > +} > + > +/** > + @brief Enables the tracker objects. > + > + @param thd [IN] The thread handle. > + > + @return void > +*/ > +void Session_tracker::enable(THD *thd) > +{ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + m_trackers[i]->enable(thd); > +} > + > +/** > + @brief Method called during the server startup to verify the contents > + of @@session_track_system_variables. I see. Please remove it from this patch. It should've been added in the next (sysvar) patch, but as it's a wrong approach in general, I hope we'll do it correctly. > + > + @param char_set The character set info. > + @param var_list Value of @@session_track_system_variables. > + > + @return false Success > + true failure > +*/ > +bool Session_tracker::server_boot_verify(const CHARSET_INFO *char_set, > + LEX_STRING var_list) > +{ > + return false; > +} > + > + > + > +/** > + @brief Returns the pointer to the tracker object for the specified tracker. > + > + @param tracker [IN] Tracker type. > + > + @return Pointer to the tracker object. > +*/ > + > +State_tracker * > +Session_tracker::get_tracker(enum_session_tracker tracker) const > +{ > + return m_trackers[tracker]; > +} make that inline in session_tracker.h > + > + > +/** > + @brief Checks if m_enabled flag is set for any of the tracker objects. > + > + @return > + true - At least one of the trackers is enabled. > + false - None of the trackers is enabled. > + > +*/ > + > +bool Session_tracker::enabled_any() > +{ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + { > + if (m_trackers[i]->is_enabled()) > + return true; > + } > + return false; > +} remove enabled_any() and changed_any(). See my comments where these methods are used. > + > +/** > + @brief Checks if m_changed flag is set for any of the tracker objects. > + > + @return > + true At least one of the entities being tracker has > + changed. > + false None of the entities being tracked has changed. > +*/ > + > +bool Session_tracker::changed_any() > +{ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + { > + if (m_trackers[i]->is_changed()) > + return true; > + } > + return false; > +} > + > + > +/** > + @brief Store all change information in the specified buffer. > + > + @param thd [IN] The thd handle. > + @param buf [OUT] Reference to the string buffer to which the state > + change data needs to be written. > + > + @return void > +*/ > + > +void Session_tracker::store(THD *thd, String &buf) > +{ > + /* Temporary buffer to store all the changes. */ > + String temp; > + size_t length; > + > + /* Get total length. */ > + for (int i= 0; i <= SESSION_TRACKER_END; i ++) > + { > + if (m_trackers[i]->is_changed()) > + m_trackers[i]->store(thd, temp); > + } > + > + length= temp.length(); > + /* Store length first.. */ > + char *to= buf.prep_append(net_length_size(length), EXTRA_ALLOC); > + net_store_length((uchar *) to, length); > + > + /* .. and then the actual info. */ > + buf.append(temp); Nice. Copy all data into a temporary malloced buffer, them memcopy it again into the malloced real buffer. At least use StringBuffer here. it would be good to avoid memcpy too, but I only see two possible approaches, both not perfect: 1. assume it's, say, less than 251 byte in the most common case. put the data directly into buf, starting from buf.ptr() + 1. when done, put the length into the reserved byte. If the length is more than 251 - memmove everything to free more bytes. 2. iterate all session trackers twice. first - to get the length, second - to put the actual data ah, yes, and don't use references (String &buf) use pointers (String *buf) > +} > + > + > +/** > + @brief Stores the given string in length-encoded format into the specified > + buffer. > + > + @param to [IN] Buffer to store the given string in. > + @param from [IN] The give string to be stored. > + @param length [IN] Length of the above string. > + > + @return void. > +*/ > + > +static > +void store_lenenc_string(String &to, const char *from, size_t length) why not make it a method of String ? Like, append_lenenc() ? > +{ > + char *ptr; > + ptr= to.prep_append(net_length_size(length), EXTRA_ALLOC); > + net_store_length((uchar *) ptr, length); > + to.append(from, length); > +} > + > diff --git a/sql/set_var.cc b/sql/set_var.cc > index b5430c5..a55fc54 100644 > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -199,8 +199,24 @@ bool sys_var::update(THD *thd, set_var *var) > (on_update && on_update(this, thd, OPT_GLOBAL)); > } > else > - return session_update(thd, var) || > + { > + bool ret= session_update(thd, var) || > (on_update && on_update(this, thd, OPT_SESSION)); > + > + /* > + Make sure we don't session-track variables that are not actually > + part of the session. tx_isolation and and tx_read_only for example > + exist as GLOBAL, SESSION, and one-shot ("for next transaction only"). > + */ > + if (var->type == OPT_SESSION) > + { > + if ((!ret) && > + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) > + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, &var->var->name); don't check for is_enabled here. Alternatives: 1. just do mark_as_changed unconditionally. this works if mark_as_changed is cheap and has no side-effects. neither is true now, but I don't understand why. 2. create a wrapper method, like Session_tracker::mark_as_changed(tracker) at least you won't need to copy this long line in every place where a state changes. doing 1 is preferrable, but I don't know if it's easily possible (because I don't understand why it's done that way now) > + } > + > + return ret; > + } > } > > uchar *sys_var::session_value_ptr(THD *thd, const LEX_STRING *base) > @@ -965,6 +983,8 @@ int set_var_collation_client::update(THD *thd) > thd->variables.character_set_results= character_set_results; > thd->variables.collation_connection= collation_connection; > thd->update_charset(); > + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) > + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); > thd->protocol_text.init(thd); > thd->protocol_binary.init(thd); > return 0; what about set_var_password::update and set_var_role::update ? Wouldn't it be better to mark_as_changed in sql_set_variables instead of doing that in individual sys_var descendands? > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 0d168e9..933f060 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7154,3 +7154,8 @@ ER_WRONG_ORDER_IN_WITH_CLAUSE > eng "The definition of the table '%s' refers to the table '%s' defined later in a non-recursive WITH clause" > ER_RECURSIVE_QUERY_IN_WITH_CLAUSE > eng "Recursive queries in WITH clause are not supported yet" > +ER_DUP_LIST_ENTRY > + eng "Duplicate entry '%-.192s'." really, a bug? remove this. having the same value twice is not a bug: MariaDB (test)> set sql_mode='ansi,ansi'; Query OK, 0 rows affected (0.00 sec) Let's be consistent > +ER_NET_OK_PACKET_TOO_LARGE 08S01 > + eng "OK packet too large" > + ukr "Пакет OK надто великий" > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index d58b51a..6205dd6 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -2975,6 +2975,13 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp, > > reinit_stmt_before_use(thd, m_lex); > > +#ifndef EMBEDDED_LIBRARY why? > + if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && > + thd->session_tracker.enabled_any() && > + thd->session_tracker.changed_any()) > + thd->lex->safe_to_cache_query= 0; why? > +#endif > + > if (open_tables) > res= instr->exec_open_and_lock_tables(thd, m_lex->query_tables); > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index e3b7056..bf70594 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1461,6 +1461,11 @@ void THD::init(void) > /* Initialize the Debug Sync Facility. See debug_sync.cc. */ > debug_sync_init_thread(this); > #endif /* defined(ENABLED_DEBUG_SYNC) */ > + > + /* Initialize session_tracker and create all tracker objects */ > + session_tracker.init(this->charset()); > + session_tracker.enable(this); 1. you don't need charset here, it's unused 2. thus you don't need to invoke init() explicitly at all, do it from the Session_tracker constructor. In fact, move init functionality into the constructor. > + > apc_target.init(&LOCK_thd_data); > DBUG_VOID_RETURN; > } > @@ -1620,6 +1625,12 @@ void THD::cleanup(void) > /* All metadata locks must have been released by now. */ > DBUG_ASSERT(!mdl_context.has_locks()); > > + /* > + Destroy trackers only after finishing manipulations with transaction > + state to avoid issues with Transaction_state_tracker. > + */ > + session_tracker.deinit(); may be you can do that from the destructor too > + > apc_target.destroy(); > cleanup_done=1; > DBUG_VOID_RETURN; > diff --git a/sql/sql_class.h b/sql/sql_class.h > index be652e6..063198e 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -681,6 +682,11 @@ typedef struct system_variables > > my_bool pseudo_slave_mode; > > + char *track_sysvars_ptr; no track_sysvars_ptr in this patch please > + my_bool session_track_schema; > + my_bool session_track_state_change; > + ulong session_track_transaction_info; and no session_track_transaction_info either > + > } SV; > > /** > diff --git a/sql/sql_db.cc b/sql/sql_db.cc > index 2ba67cb..53719e5 100644 > --- a/sql/sql_db.cc > +++ b/sql/sql_db.cc > @@ -1034,7 +1034,18 @@ mysql_rm_db_internal(THD *thd,char *db, bool if_exists, bool silent) > it to 0. > */ > if (thd->db && cmp_db_names(thd->db, db) && !error) > - mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server); > + { > + mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);\ eh? trailing backslash? > + /* > + Check if current database tracker is enabled. If so, set the 'changed' flag. > + */ > + if (thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->is_enabled()) > + { > + LEX_CSTRING dummy= { C_STRING_WITH_LEN("") }; > + dummy.length= dummy.length*1; WAT??? > + thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->mark_as_changed(thd, &dummy); alternatively, make mark_as_changed to take const char* and size_t, then you won't need to create LEX_CSTRING alternatively, create LEX_CSTRING dummy statically and use it always alternatively, use empty_lex_string btw, what's the point of using an *empty string* as an argument? other trackers accept NULL. why empty string is better? and anyway, why to construct and pass down an argument that is ignored anyway??? > + } > + } > my_dirend(dirp); > DBUG_RETURN(error); > } > @@ -1588,6 +1597,18 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch) > > mysql_change_db_impl(thd, &new_db_file_name, db_access, db_default_cl); > > +done: > + /* > + Check if current database tracker is enabled. If so, set the 'changed' flag. > + */ > + if (thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->is_enabled()) > + { > + LEX_CSTRING dummy= { C_STRING_WITH_LEN("") }; > + dummy.length= dummy.length*1; > + thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->mark_as_changed(thd, &dummy); same > + } > + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) > + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); > DBUG_RETURN(FALSE); > } > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index dbe1967..244ff5b 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -322,6 +322,8 @@ static void unlock_variables(THD *thd, struct system_variables *vars); > static void cleanup_variables(struct system_variables *vars); > static void plugin_vars_free_values(sys_var *vars); > static void restore_ptr_backup(uint n, st_ptr_backup *backup); > +#define my_intern_plugin_lock(A,B) intern_plugin_lock(A,B) > +#define my_intern_plugin_lock_ci(A,B) intern_plugin_lock(A,B) remove that > static plugin_ref intern_plugin_lock(LEX *lex, plugin_ref plugin); > static void intern_plugin_unlock(LEX *lex, plugin_ref plugin); > static void reap_plugins(void); > @@ -2776,22 +2778,23 @@ static void update_func_double(THD *thd, struct st_mysql_sys_var *var, > System Variables support > ****************************************************************************/ > > - > -sys_var *find_sys_var(THD *thd, const char *str, uint length) > +sys_var *find_sys_var_ex(THD *thd, const char *str, size_t length, > + bool throw_error, bool locked) revert all sql_plugin.cc and sql_plugin.h changes. if you need them for sysvars - do them in the second patch > { > sys_var *var; > sys_var_pluginvar *pi= NULL; > plugin_ref plugin; > - DBUG_ENTER("find_sys_var"); > + DBUG_ENTER("find_sys_var_ex"); > > - mysql_mutex_lock(&LOCK_plugin); > + if (!locked) > + mysql_mutex_lock(&LOCK_plugin); > mysql_rwlock_rdlock(&LOCK_system_variables_hash); > if ((var= intern_find_sys_var(str, length)) && > (pi= var->cast_pluginvar())) > { > mysql_rwlock_unlock(&LOCK_system_variables_hash); > LEX *lex= thd ? thd->lex : 0; > - if (!(plugin= intern_plugin_lock(lex, plugin_int_to_ref(pi->plugin)))) > + if (!(plugin= my_intern_plugin_lock(lex, plugin_int_to_ref(pi->plugin)))) > var= NULL; /* failed to lock it, it must be uninstalling */ > else > if (!(plugin_state(plugin) & PLUGIN_IS_READY)) > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 8e5ab71..03f3a93 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -2755,7 +2755,13 @@ void mysql_sql_stmt_prepare(THD *thd) > thd->stmt_map.erase(stmt); > } > else > + { > + /* send the boolean tracker in the OK packet when rewrite the comment into something that makes sense, please > + @@session_track_state_change is set to ON */ > + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) > + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); > my_ok(thd, 0L, 0L, "Statement prepared"); > + } > > DBUG_VOID_RETURN; > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] ca24c9d: MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1
by Kristian Nielsen 07 Apr '16

07 Apr '16

Hi Nirbhay, Do you want to review this patch for MDEV-9383? It concerns your code added for do_domain_ids=(...). - Kristian. Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > revision-id: ca24c9d1671e90e43daa483af32fc19891d1a646 (mariadb-10.1.13-8-gca24c9d) > parent(s): 4b6a3518e4dc9088d1f42cd9bc487d06137d2760 > committer: Kristian Nielsen > timestamp: 2016-04-07 14:44:29 +0200 > message: > > MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1 > > In some cases, MariaDB 10.0 could write a master.info file that was read > incorrectly by 10.1 and could cause server to fail to start after an upgrade. > > (If writing a new master.info file that is shorter than the old, extra > junk may remain at the end of the file. This is handled properly in > 10.1 with an END_MARKER line, but this line is not written by > 10.0. The fix here is to make 10.1 robust at reading the master.info > files written by 10.0). > > Fix several things around reading master.info and read_mi_key_from_file(): > > - read_mi_key_from_file() did not distinguish between a line with and > without an eqals '=' sign. > > - If a line was empty, read_mi_key_from_file() would incorrectly return > the key from the previous call. > > - An extra using_gtid=X line left-over by MariaDB 10.0 might incorrectly > be read and overwrite the correct value. > > - Fix incorrect usage of strncmp() which should be strcmp(). > > - Add test cases. > > --- > mysql-test/std_data/bad2_master.info | 35 +++++ > mysql-test/std_data/bad3_master.info | 37 +++++ > mysql-test/std_data/bad4_master.info | 35 +++++ > mysql-test/std_data/bad5_master.info | 35 +++++ > mysql-test/std_data/bad6_master.info | 36 +++++ > mysql-test/std_data/bad_master.info | 35 +++++ > .../suite/rpl/r/rpl_upgrade_master_info.result | 88 +++++++++++ > .../suite/rpl/t/rpl_upgrade_master_info.test | 163 +++++++++++++++++++++ > sql/rpl_mi.cc | 81 +++++----- > 9 files changed, 512 insertions(+), 33 deletions(-) > > diff --git a/mysql-test/std_data/bad2_master.info b/mysql-test/std_data/bad2_master.info > new file mode 100644 > index 0000000..6172256 > --- /dev/null > +++ b/mysql-test/std_data/bad2_master.info > @@ -0,0 +1,35 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > +=0 > diff --git a/mysql-test/std_data/bad3_master.info b/mysql-test/std_data/bad3_master.info > new file mode 100644 > index 0000000..6e632cd > --- /dev/null > +++ b/mysql-test/std_data/bad3_master.info > @@ -0,0 +1,37 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > + > + > +0 > diff --git a/mysql-test/std_data/bad4_master.info b/mysql-test/std_data/bad4_master.info > new file mode 100644 > index 0000000..87572ef > --- /dev/null > +++ b/mysql-test/std_data/bad4_master.info > @@ -0,0 +1,35 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > +d=1 > diff --git a/mysql-test/std_data/bad5_master.info b/mysql-test/std_data/bad5_master.info > new file mode 100644 > index 0000000..4ea8113 > --- /dev/null > +++ b/mysql-test/std_data/bad5_master.info > @@ -0,0 +1,35 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > +using_gtid > diff --git a/mysql-test/std_data/bad6_master.info b/mysql-test/std_data/bad6_master.info > new file mode 100644 > index 0000000..0f48f48 > --- /dev/null > +++ b/mysql-test/std_data/bad6_master.info > @@ -0,0 +1,36 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > +END_MARKER > +do_domain_ids=20 Hulubulu!!?! > diff --git a/mysql-test/std_data/bad_master.info b/mysql-test/std_data/bad_master.info > new file mode 100644 > index 0000000..1541fdf > --- /dev/null > +++ b/mysql-test/std_data/bad_master.info > @@ -0,0 +1,35 @@ > +33 > +mysql-bin.000001 > +4 > +127.0.0.1 > +root > + > +3310 > +60 > +0 > + > + > + > + > + > +0 > +1800.000 > + > +0 > + > +0 > + > + > + > + > + > + > + > + > + > + > + > + > + > +using_gtid=1 > + > diff --git a/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result b/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result > new file mode 100644 > index 0000000..f966f18 > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result > @@ -0,0 +1,88 @@ > +include/master-slave.inc > +[connection master] > +*** MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1 *** > +include/stop_slave.inc > +CHANGE MASTER TO master_use_gtid=CURRENT_POS; > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +CREATE TABLE t1 (a INT PRIMARY KEY); > +INSERT INTO t1 VALUES (1); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1; > +a > +1 > +include/stop_slave.inc > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +INSERT INTO t1 VALUES (2); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a > +1 > +2 > +include/stop_slave.inc > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +INSERT INTO t1 VALUES (3); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a > +1 > +2 > +3 > +include/stop_slave.inc > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +INSERT INTO t1 VALUES (4); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a > +1 > +2 > +3 > +4 > +include/stop_slave.inc > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +INSERT INTO t1 VALUES (5); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a > +1 > +2 > +3 > +4 > +5 > +include/stop_slave.inc > +include/rpl_stop_server.inc [server_number=2] > +include/rpl_start_server.inc [server_number=2] > +INSERT INTO t1 VALUES (6); > +include/save_master_gtid.inc > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a > +1 > +2 > +3 > +4 > +5 > +6 > +DROP TABLE t1; > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test b/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test > new file mode 100644 > index 0000000..e81e7c0 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test > @@ -0,0 +1,163 @@ > +--source include/master-slave.inc > + > +--echo *** MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1 *** > + > +--connection slave > +--source include/stop_slave.inc > +CHANGE MASTER TO master_use_gtid=CURRENT_POS; > +--let $datadir= `SELECT @@datadir` > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +CREATE TABLE t1 (a INT PRIMARY KEY); > +INSERT INTO t1 VALUES (1); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1; > + > +--source include/stop_slave.inc > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad2_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +INSERT INTO t1 VALUES (2); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > + > +--source include/stop_slave.inc > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad3_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +INSERT INTO t1 VALUES (3); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > + > +--source include/stop_slave.inc > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad4_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +INSERT INTO t1 VALUES (4); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > + > +--source include/stop_slave.inc > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad5_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +INSERT INTO t1 VALUES (5); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > + > +--source include/stop_slave.inc > + > +--let $rpl_server_number= 2 > +--source include/rpl_stop_server.inc > + > +--remove_file $datadir/master.info > +--copy_file $MYSQL_TEST_DIR/std_data/bad6_master.info $datadir/master.info > + > +--let $rpl_server_number= 2 > +--source include/rpl_start_server.inc > + > +--source include/wait_until_connected_again.inc > + > +--connection master > +INSERT INTO t1 VALUES (6); > +--source include/save_master_gtid.inc > + > +--connection slave > +# Fix the port after we replaced master.info. > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1 > +eval CHANGE MASTER TO master_host='127.0.0.1', master_port=$SERVER_MYPORT_1; > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > + > + > +# Cleanup > +--connection master > +DROP TABLE t1; > +--source include/rpl_end.inc > diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc > index df72134..706824c 100644 > --- a/sql/rpl_mi.cc > +++ b/sql/rpl_mi.cc > @@ -205,43 +205,56 @@ void init_master_log_pos(Master_info* mi) > > /** > Parses the IO_CACHE for "key=" and returns the "key". > + If no '=' found, returns the whole line (for END_MARKER). > > @param key [OUT] Key buffer > @param max_size [IN] Maximum buffer size > @param f [IN] IO_CACHE file > + @param found_equal [OUT] Set true if a '=' was found. > > @retval 0 Either "key=" or '\n' found > @retval 1 EOF > */ > -static int read_mi_key_from_file(char *key, int max_size, IO_CACHE *f) > +static int read_mi_key_from_file(char *key, int max_size, IO_CACHE *f, > + bool *found_equal) > { > int i= 0, c; > - char *last_p; > > DBUG_ENTER("read_key_from_file"); > > - while (((c= my_b_get(f)) != '\n') && (c != my_b_EOF)) > + *found_equal= false; > + if (max_size <= 0) > + DBUG_RETURN(1); > + for (;;) > { > - last_p= key + i; > - > - if (i < max_size) > + if (i >= max_size-1) > { > - if (c == '=') > - { > - /* We found '=', replace it by 0 and return. */ > - *last_p= 0; > - DBUG_RETURN(0); > - } > - else > - *last_p= c; > + key[i] = '\0'; > + DBUG_RETURN(0); > + } > + c= my_b_get(f); > + if (c == my_b_EOF) > + { > + DBUG_RETURN(1); > + } > + else if (c == '\n') > + { > + key[i]= '\0'; > + DBUG_RETURN(0); > + } > + else if (c == '=') > + { > + key[i]= '\0'; > + *found_equal= true; > + DBUG_RETURN(0); > + } > + else > + { > + key[i]= c; > + ++i; > } > - ++i; > } > - > - if (c == my_b_EOF) > - DBUG_RETURN(1); > - > - DBUG_RETURN(0); > + /* NotReached */ > } > > enum { > @@ -539,6 +552,10 @@ file '%s')", fname); > if (lines >= LINE_FOR_LAST_MYSQL_FUTURE) > { > uint i; > + bool got_eq; > + bool seen_using_gtid= false; > + bool seen_do_domain_ids=false, seen_ignore_domain_ids=false; > + > /* Skip lines used by / reserved for MySQL >= 5.6. */ > for (i= LINE_FOR_FIRST_MYSQL_5_6; i <= LINE_FOR_LAST_MYSQL_FUTURE; ++i) > { > @@ -551,11 +568,12 @@ file '%s')", fname); > for "key=" and returns the "key" if found. The "value" can then the > parsed on case by case basis. The "unknown" lines would be ignored to > facilitate downgrades. > + 10.0 does not have the END_MARKER before any left-overs at the end > + of the file. So ignore any but the first occurrence of a key. > */ > - while (!read_mi_key_from_file(buf, sizeof(buf), &mi->file)) > + while (!read_mi_key_from_file(buf, sizeof(buf), &mi->file, &got_eq)) > { > - /* using_gtid */ > - if (!strncmp(buf, STRING_WITH_LEN("using_gtid"))) > + if (got_eq && !seen_using_gtid && !strcmp(buf, "using_gtid")) > { > int val; > if (!init_intvar_from_file(&val, &mi->file, 0)) > @@ -566,15 +584,13 @@ file '%s')", fname); > mi->using_gtid= Master_info::USE_GTID_SLAVE_POS; > else > mi->using_gtid= Master_info::USE_GTID_NO; > - continue; > + seen_using_gtid= true; > } else { > sql_print_error("Failed to initialize master info using_gtid"); > goto errwithmsg; > } > } > - > - /* DO_DOMAIN_IDS */ > - if (!strncmp(buf, STRING_WITH_LEN("do_domain_ids"))) > + else if (got_eq && !seen_do_domain_ids && !strcmp(buf, "do_domain_ids")) > { > if (mi->domain_id_filter.init_ids(&mi->file, > Domain_id_filter::DO_DOMAIN_IDS)) > @@ -582,11 +598,10 @@ file '%s')", fname); > sql_print_error("Failed to initialize master info do_domain_ids"); > goto errwithmsg; > } > - continue; > + seen_do_domain_ids= true; > } > - > - /* IGNORE_DOMAIN_IDS */ > - if (!strncmp(buf, STRING_WITH_LEN("ignore_domain_ids"))) > + else if (got_eq && !seen_ignore_domain_ids && > + !strcmp(buf, "ignore_domain_ids")) > { > if (mi->domain_id_filter.init_ids(&mi->file, > Domain_id_filter::IGNORE_DOMAIN_IDS)) > @@ -595,9 +610,9 @@ file '%s')", fname); > "ignore_domain_ids"); > goto errwithmsg; > } > - continue; > + seen_ignore_domain_ids= true; > } > - else if (!strncmp(buf, STRING_WITH_LEN("END_MARKER"))) > + else if (!got_eq && !strcmp(buf, "END_MARKER")) > { > /* > Guard agaist extra left-overs at the end of file, in case a later > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 2