From c893e354a96b421e346808a7975a26f5198c7479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 08:37:51 +0200 Subject: [PATCH 01/17] Add missing variable The numlocks variable is used when older OpenSSL versions are used. --- server/core/gateway.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index d7d726711..169595442 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -1362,6 +1362,7 @@ int main(int argc, char **argv) sigset_t saved_mask; bool to_stdout = false; void (*exitfunp[4])(void) = { mxs_log_finish, cleanup_process_datadir, write_footer, NULL }; + int numlocks = 0; bool pid_file_created = false; // NOTE: These are set here since global_defaults() is called inside config_load(). From c85f83fa2b1e9d5b51c062dd3427d0a1d58dbffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 09:26:22 +0200 Subject: [PATCH 02/17] Fix strcpy overlap in binlogrouter The source and destination buffers could overlap which is why an intermediate buffer is required. --- server/modules/routing/binlogrouter/blr_file.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/binlogrouter/blr_file.c b/server/modules/routing/binlogrouter/blr_file.c index 3950faf83..746cb98a6 100644 --- a/server/modules/routing/binlogrouter/blr_file.c +++ b/server/modules/routing/binlogrouter/blr_file.c @@ -421,7 +421,12 @@ blr_file_create(ROUTER_INSTANCE *router, char *file) { close(router->binlog_fd); spinlock_acquire(&router->binlog_lock); - strcpy(router->binlog_name, file); + + /// Use an intermediate buffer in case the source and destination overlap + char new_binlog[strlen(file) + 1]; + strcpy(new_binlog, file); + strcpy(router->binlog_name, new_binlog); + router->binlog_fd = fd; router->current_pos = BINLOG_MAGIC_SIZE; /* Initial position after the magic number */ router->binlog_position = BINLOG_MAGIC_SIZE; From 653b8429d492e812023b70edc983110373a401a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 10:46:26 +0200 Subject: [PATCH 03/17] Extend cdc_datatypes test The test now also checks DATE, DATETIME and TIMESTAMP types. --- .../cdc_datatypes/cdc_datatypes.cpp | 41 +++++++++++++++++-- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp index f9a29f379..5695d6d7a 100644 --- a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp +++ b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp @@ -36,6 +36,7 @@ static const char* decimal_types[] = "FLOAT", "DOUBLE", "DECIMAL(10, 2)", + "DECIMAL(32, 2)", NULL }; @@ -89,16 +90,48 @@ static const char* binary_values[] = NULL }; +static const char* datetime_types[] = +{ + "DATETIME(1)", + "DATETIME(2)", + "DATETIME(3)", + "DATETIME(4)", + "DATETIME(5)", + "DATETIME(6)", + "TIMESTAMP", + NULL +}; + +static const char* datetime_values[] = +{ + "'2018-01-01 11:11:11'", + NULL +}; + +static const char* date_types[] = +{ + "DATE", + NULL +}; + +static const char* date_values[] = +{ + "'2018-01-01'", + NULL +}; + struct { const char** types; const char** values; } test_set[] { - { integer_types, integer_values }, - { decimal_types, decimal_values }, - { string_types, string_values }, - { binary_types, binary_values }, + { integer_types, integer_values }, + { decimal_types, decimal_values }, + { string_types, string_values }, + { binary_types, binary_values }, + { datetime_types, datetime_values }, + { date_types, date_values }, { 0 } }; From 4e9a5af9267fff0cdac4aaf4bdfb56678cf2ecbc Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Fri, 26 Jan 2018 09:23:32 +0200 Subject: [PATCH 04/17] add execution of add_core_conf.sh to 'check_backend' to set up core dump saving --- maxscale-system-test/check_backend.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index fb045c532..fcdb3926a 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -8,8 +8,21 @@ int main(int argc, char *argv[]) { + char src[1024]; + char dst[1024]; + TestConnections * Test = new TestConnections(argc, argv); + for (int i = 0; i < Test->maxscales->N; i++) + { + sprintf(src, "%s/mdbci/add_core_cnf.sh", test_dir); + Test->maxscales->ssh_node_f(i, false, "mkdir %s/ccore", Test->maxscales->access_homedir[i]); + sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); + Test->maxscales->copy_to_node(i, src, dst); + sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); + Test->maxscales->ssh_node_f(i, true, "%s/ccore/add_core_cnf.sh", Test->maxscales->access_homedir[i]); + } + /*Test->restart_maxscale(); sleep(5);*/ Test->set_timeout(10); From 3da4aa166515a84ab42aa01911781dae3baa6412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 10:23:05 +0200 Subject: [PATCH 05/17] Remove trailing whitespace in check_backend Removed trailing whitespace in check_backend --- maxscale-system-test/check_backend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index fcdb3926a..83579ae48 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -10,7 +10,7 @@ int main(int argc, char *argv[]) { char src[1024]; char dst[1024]; - + TestConnections * Test = new TestConnections(argc, argv); for (int i = 0; i < Test->maxscales->N; i++) From 4dc9b56d290d0ee6e25ba8c9b470efe8f8b8d386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 10:41:34 +0200 Subject: [PATCH 06/17] Fix check_backend Fixed the core generation and removed obsolete files. --- maxscale-system-test/check_backend.cpp | 17 ++++------------- maxscale-system-test/mdbci/add_core_cnf.sh | 6 +++++- maxscale-system-test/mdbci/configure_core.sh | 11 ----------- 3 files changed, 9 insertions(+), 25 deletions(-) delete mode 100755 maxscale-system-test/mdbci/configure_core.sh diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index 83579ae48..e84c1ae82 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -8,23 +8,14 @@ int main(int argc, char *argv[]) { - char src[1024]; - char dst[1024]; TestConnections * Test = new TestConnections(argc, argv); - for (int i = 0; i < Test->maxscales->N; i++) - { - sprintf(src, "%s/mdbci/add_core_cnf.sh", test_dir); - Test->maxscales->ssh_node_f(i, false, "mkdir %s/ccore", Test->maxscales->access_homedir[i]); - sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); - Test->maxscales->copy_to_node(i, src, dst); - sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); - Test->maxscales->ssh_node_f(i, true, "%s/ccore/add_core_cnf.sh", Test->maxscales->access_homedir[i]); - } + std::string src = std::string(test_dir) + "/mdbci/add_core_cnf.sh"; + Test->copy_to_maxscale(src.c_str(), Test->maxscale_access_homedir); + Test->ssh_maxscale(true, "%s/add_core_cnf.sh %s", Test->maxscale_access_homedir, + Test->verbose ? "verbose" : ""); - /*Test->restart_maxscale(); - sleep(5);*/ Test->set_timeout(10); Test->tprintf("Connecting to Maxscale routers with Master/Slave backend\n"); diff --git a/maxscale-system-test/mdbci/add_core_cnf.sh b/maxscale-system-test/mdbci/add_core_cnf.sh index aa5db1828..21a69f460 100755 --- a/maxscale-system-test/mdbci/add_core_cnf.sh +++ b/maxscale-system-test/mdbci/add_core_cnf.sh @@ -1,4 +1,8 @@ -set -x +if [ "$1" == "verbose" ] +then + set -x +fi + chmod 777 /tmp/ echo 2 > /proc/sys/fs/suid_dumpable sed -i "s/start() {/start() { \n export DAEMON_COREFILE_LIMIT='unlimited'; ulimit -c unlimited; /" /etc/init.d/maxscale diff --git a/maxscale-system-test/mdbci/configure_core.sh b/maxscale-system-test/mdbci/configure_core.sh deleted file mode 100755 index 2c72b5ac9..000000000 --- a/maxscale-system-test/mdbci/configure_core.sh +++ /dev/null @@ -1,11 +0,0 @@ -#!/bin/bash -set -x - -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP '$maxscale_access_sudo service iptables stop' - -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP "$maxscale_access_sudo mkdir ccore; $maxscale_access_sudo chown $maxscale_access_user ccore" - -scp -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no ${script_dir}/add_core_cnf.sh $maxscale_access_user@$maxscale_IP:./ccore/ -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP "$maxscale_access_sudo /home/$maxscale_access_user/ccore/add_core_cnf.sh" - -set +x From 6068850b181e5af1451a6668a227eb97212965e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 17:47:24 +0200 Subject: [PATCH 07/17] MXS-1627: Only load users that use default auth plugin The list of users that is used for authentication shoudl only consist of users that do not use an explicit authentication plugin. This way authentication fails before any connections to the backend servers are done. --- server/modules/authenticator/MySQLAuth/dbusers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index c1981272b..09f056a0a 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -44,11 +44,11 @@ #define NEW_LOAD_DBUSERS_QUERY "SELECT u.user, u.host, d.db, u.select_priv, u.%s \ FROM mysql.user AS u LEFT JOIN mysql.db AS d \ - ON (u.user = d.user AND u.host = d.host) %s \ + ON (u.user = d.user AND u.host = d.host) WHERE u.plugin = '' %s \ UNION \ SELECT u.user, u.host, t.db, u.select_priv, u.%s \ FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t \ - ON (u.user = t.user AND u.host = t.host) %s" + ON (u.user = t.user AND u.host = t.host) WHERE u.plugin = '' %s" static int get_users(SERV_LISTENER *listener, bool skip_local); static MYSQL *gw_mysql_init(void); @@ -59,7 +59,7 @@ static bool get_hostname(DCB *dcb, char *client_hostname, size_t size); static char* get_new_users_query(const char *server_version, bool include_root) { const char* password = strstr(server_version, "5.7.") ? MYSQL57_PASSWORD : MYSQL_PASSWORD; - const char *with_root = include_root ? "" : "WHERE u.user NOT IN ('root')"; + const char *with_root = include_root ? "" : " AND u.user NOT IN ('root')"; size_t n_bytes = snprintf(NULL, 0, NEW_LOAD_DBUSERS_QUERY, password, with_root, password, with_root); char *rval = MXS_MALLOC(n_bytes + 1); From f9cc2d5bbbab0c571eb262a96d8fba63335bbb6a Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Tue, 30 Jan 2018 15:48:05 +0200 Subject: [PATCH 08/17] use 'mdbci destroy' instead of 'vagrant destroy' (#163) use mdbci destroy instead of vagrant destroy --- BUILD/mdbci/build.sh | 12 ++---------- BUILD/mdbci/upgrade_test.sh | 12 +++--------- maxscale-system-test/mdbci/create_config.sh | 4 +--- maxscale-system-test/mdbci/destroy.sh | 10 ---------- maxscale-system-test/mdbci/run_test.sh | 7 +++---- maxscale-system-test/mdbci/run_test_snapshot.sh | 9 ++++----- 6 files changed, 13 insertions(+), 41 deletions(-) delete mode 100755 maxscale-system-test/mdbci/destroy.sh diff --git a/BUILD/mdbci/build.sh b/BUILD/mdbci/build.sh index abdb0f4b2..a18c16f92 100755 --- a/BUILD/mdbci/build.sh +++ b/BUILD/mdbci/build.sh @@ -66,9 +66,7 @@ $(<${script_dir}/templates/build.json.template) # destroying existing box if [ -d "$MDBCI_VM_PATH/${name}" ]; then - cd $MDBCI_VM_PATH/${name} - vagrant destroy -f - cd ${dir} + ${mdbci_dir}/mdbci destroy $name fi # starting VM for build @@ -78,9 +76,7 @@ $(<${script_dir}/templates/build.json.template) ${mdbci_dir}/mdbci up --attempts=1 $name if [ $? != 0 ] ; then echo "Error starting VM" - cd $MDBCI_VM_PATH/${name} rm ~/vagrant_lock - cd $dir exit 1 fi echo "copying public keys to VM" @@ -122,11 +118,7 @@ if [ "$try_already_running" == "yes" ] ; then fi if [[ "$do_not_destroy_vm" != "yes" && "$try_already_running" != "yes" ]] ; then echo "Destroying VM" - vagrant destroy -f - cd .. - rm -rf $name - rm -rf ${name}.json - rm -rf ${name}_netwotk_config + ${mdbci_dir}/mdbci destroy $name fi cd $dir diff --git a/BUILD/mdbci/upgrade_test.sh b/BUILD/mdbci/upgrade_test.sh index 9ac3efe1f..c30069dcb 100755 --- a/BUILD/mdbci/upgrade_test.sh +++ b/BUILD/mdbci/upgrade_test.sh @@ -30,9 +30,7 @@ echo $JOB_NAME-$BUILD_NUMBER >> ~/vagrant_lock # destroying existing box if [ -d "install_$box" ]; then - cd $MDBCI_VM_PATH/$name - vagrant destroy -f - cd $dir + ${mdbci_dir}/mdbci destroy $name fi ${mdbci_dir}/repository-config/generate_all.sh repo.d @@ -44,11 +42,9 @@ ${mdbci_dir}/mdbci up $name --attempts=1 if [ $? != 0 ] ; then if [ $? != 0 ] ; then echo "Error starting VM" - cd ${MDBCI_VM_PATH}/$name if [ "x$do_not_destroy_vm" != "xyes" ] ; then - vagrant destroy -f + ${mdbci_dir}/mdbci destroy $name fi - cd $dir rm ~/vagrant_lock exit 1 fi @@ -126,9 +122,7 @@ scp $scpopt $sshuser@$IP:/var/log/maxscale/* $logs_publish_dir chmod a+r $logs_publish_dir/* if [ "x$do_not_destroy_vm" != "xyes" ] ; then - cd $MDBCI_VM_PATH/$name - vagrant destroy -f - cd $dir + ${mdbci_dir}/mdbci destroy $name fi kill $pid_to_kill exit $res diff --git a/maxscale-system-test/mdbci/create_config.sh b/maxscale-system-test/mdbci/create_config.sh index 6f176e92f..12f899323 100755 --- a/maxscale-system-test/mdbci/create_config.sh +++ b/maxscale-system-test/mdbci/create_config.sh @@ -20,10 +20,8 @@ if [ "$product" == "mysql" ] ; then export cnf_path=${script_dir}/cnf/mysql56 fi +${mdbci_dir}/mdbci destroy $name mkdir -p ${MDBCI_VM_PATH}/$name -cd ${MDBCI_VM_PATH}/$name -vagrant destroy -f -cd $dir mkdir ${MDBCI_VM_PATH}/$name/cnf cp -r ${cnf_path}/* ${MDBCI_VM_PATH}/$name/cnf/ diff --git a/maxscale-system-test/mdbci/destroy.sh b/maxscale-system-test/mdbci/destroy.sh deleted file mode 100755 index d12ec0764..000000000 --- a/maxscale-system-test/mdbci/destroy.sh +++ /dev/null @@ -1,10 +0,0 @@ -#!/bin/bash - -dir=`pwd` -cd ${MDBCI_VM_PATH}/${name} -vagrant destroy -f -cd $dir - -rm -rf ${MDBCI_VM_PATH}/${name} -rm -rf ${MDBCI_VM_PATH}/${name}.json -rm -rf ${MDBCI_VM_PATH}/${name}_network_config diff --git a/maxscale-system-test/mdbci/run_test.sh b/maxscale-system-test/mdbci/run_test.sh index c74ab8dc9..609d35e7c 100755 --- a/maxscale-system-test/mdbci/run_test.sh +++ b/maxscale-system-test/mdbci/run_test.sh @@ -63,7 +63,6 @@ res=$? ulimit -c unlimited if [ $res == 0 ] ; then -# . ${script_dir}/configure_backend.sh . ${script_dir}/set_env.sh $name cd ${script_dir}/.. cmake . -DBUILDNAME=$name -DCMAKE_BUILD_TYPE=Debug @@ -76,7 +75,7 @@ if [ $res == 0 ] ; then if [ $? != 0 ]; then echo "Backend broken!" if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name fi rm ~/vagrant_lock exit 1 @@ -90,13 +89,13 @@ if [ $res == 0 ] ; then else echo "Failed to create VMs, exiting" if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name fi rm ~/vagrant_lock exit 1 fi if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name echo "clean up done!" fi diff --git a/maxscale-system-test/mdbci/run_test_snapshot.sh b/maxscale-system-test/mdbci/run_test_snapshot.sh index 77d319121..cff4ce3fe 100755 --- a/maxscale-system-test/mdbci/run_test_snapshot.sh +++ b/maxscale-system-test/mdbci/run_test_snapshot.sh @@ -48,15 +48,14 @@ export repo_dir=$dir/repo.d/ ${mdbci_dir}/mdbci snapshot revert --path-to-nodes $name --snapshot-name $snapshot_name if [ $? != 0 ]; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name ${MDBCI_VM_PATH}/scripts/clean_vms.sh $name ${script_dir}/create_config.sh - checkExitStatus $? "Error creating configuration" $snapshot_lock_file - . ${script_dir}/configure_backend.sh - + checkExitStatus $? "Error creating configuration" $snapshot_lock_file + echo "Creating snapshot from new config" - $HOME/mdbci/mdbci snapshot take --path-to-nodes $name --snapshot-name $snapshot_name + ${mdbci_dir}/mdbci snapshot take --path-to-nodes $name --snapshot-name $snapshot_name fi . ${script_dir}/set_env.sh "$name" From 8dfb1d0113865d20325ea2790b57039504cde38a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 11:39:03 +0200 Subject: [PATCH 09/17] MXS-1621: Add ALTER TABLE ... [FIRST | AFTER `col` ] parsing The parser checks whether the FIRST or AFTER keywords are used and, if AFTER is used, extracts the relevant column name. Added a test case that checks that the parsing works and detects the correct column names. --- .../modules/routing/avrorouter/CMakeLists.txt | 4 + server/modules/routing/avrorouter/avro_file.c | 3 +- .../modules/routing/avrorouter/avro_schema.c | 105 +++++++++++++++++- .../routing/avrorouter/test/CMakeLists.txt | 3 + .../avrorouter/test/test_alter_parsing.c | 99 +++++++++++++++++ 5 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 server/modules/routing/avrorouter/test/CMakeLists.txt create mode 100644 server/modules/routing/avrorouter/test/test_alter_parsing.c diff --git a/server/modules/routing/avrorouter/CMakeLists.txt b/server/modules/routing/avrorouter/CMakeLists.txt index d852862bb..d23a4e986 100644 --- a/server/modules/routing/avrorouter/CMakeLists.txt +++ b/server/modules/routing/avrorouter/CMakeLists.txt @@ -6,6 +6,10 @@ if(AVRO_FOUND AND JANSSON_FOUND) set_target_properties(avrorouter PROPERTIES LINK_FLAGS -Wl,-z,defs) target_link_libraries(avrorouter maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro sqlite3 lzma) install_module(avrorouter core) + + if (BUILD_TESTS) + add_subdirectory(test) + endif() else() message(STATUS "No Avro C or Jansson libraries found, not building avrorouter.") endif() diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index f7bcdaa09..793376e07 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1040,12 +1040,13 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra db[dblen] = 0; size_t sqlsz = len, tmpsz = len; - char *tmp = MXS_MALLOC(len); + char *tmp = MXS_MALLOC(len + 1); MXS_ABORT_IF_NULL(tmp); remove_mysql_comments((const char**)&sql, &sqlsz, &tmp, &tmpsz); sql = tmp; len = tmpsz; unify_whitespace(sql, len); + sql[len] = '\0'; static bool warn_not_row_format = true; diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 24fe0adf8..610c27480 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -930,7 +930,6 @@ static void remove_extras(char* str) ss_dassert(strlen(str) == len); } - static void remove_backticks(char* src) { char* dest = src; @@ -1134,6 +1133,110 @@ static const char* get_tok(const char* sql, int* toklen, const char* end) return NULL; } +static void rskip_whitespace(const char* sql, const char** end) +{ + const char* ptr = *end; + + while (ptr > sql && isspace(*ptr)) + { + ptr--; + } + + *end = ptr; +} + +static void rskip_token(const char* sql, const char** end) +{ + const char* ptr = *end; + + while (ptr > sql && !isspace(*ptr)) + { + ptr--; + } + + *end = ptr; +} + +static bool get_placement_specifier(const char* sql, const char* end, const char** tgt, int* tgt_len) +{ + bool rval = false; + ss_dassert(end > sql); + end--; + + *tgt = NULL; + *tgt_len = 0; + + // Skip any trailing whitespace + rskip_whitespace(sql, &end); + + if (*end == '`') + { + // Identifier, possibly AFTER `column` + const char* id_end = end; + end--; + + while (end > sql && *end != '`') + { + end--; + } + + const char* id_start = end + 1; + ss_dassert(*end == '`' && *id_end == '`'); + + end--; + + rskip_whitespace(sql, &end); + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "AFTER", 5) == 0) + { + // This column comes after the specified column + rval = true; + *tgt = id_start; + *tgt_len = id_end - id_start; + } + } + else + { + // Something else, possibly FIRST or un-backtick'd AFTER + const char* id_end = end + 1; // Points to either a trailing space or one-after-the-end + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "FIRST", 5) == 0) + { + // Put this column first + rval = true; + } + else + { + const char* id_start = end + 1; + + // Skip the whitespace and until the start of the current token + rskip_whitespace(sql, &end); + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "AFTER", 5) == 0) + { + // This column comes after the specified column + rval = true; + *tgt = id_start; + *tgt_len = id_end - id_start; + } + } + } + + return rval; +} + static bool tok_eq(const char *a, const char *b, size_t len) { size_t i = 0; diff --git a/server/modules/routing/avrorouter/test/CMakeLists.txt b/server/modules/routing/avrorouter/test/CMakeLists.txt new file mode 100644 index 000000000..016461f38 --- /dev/null +++ b/server/modules/routing/avrorouter/test/CMakeLists.txt @@ -0,0 +1,3 @@ +add_executable(test_alter_parsing test_alter_parsing.c) +target_link_libraries(test_alter_parsing maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro sqlite3 lzma) +add_test(test_alter_parsing test_alter_parsing) \ No newline at end of file diff --git a/server/modules/routing/avrorouter/test/test_alter_parsing.c b/server/modules/routing/avrorouter/test/test_alter_parsing.c new file mode 100644 index 000000000..c622d6e75 --- /dev/null +++ b/server/modules/routing/avrorouter/test/test_alter_parsing.c @@ -0,0 +1,99 @@ +#include "../avro_schema.c" + +static struct +{ + const char* statement; + const char* target; + bool rval; +} data[] = +{ + {"/*!40000 ALTER TABLE `t1` DISABLE KEYS */", NULL, false}, + {"/*!40000 ALTER TABLE `t1` ENABLE KEYS */", NULL, false}, + {"ADD COLUMN `a` INT", NULL, false}, + {"ADD COLUMN `a`", NULL, false}, + {"ALTER TABLE `t1` ADD `account_id` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `amount` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `app_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` ADD `create_time` DATETIME", NULL, false}, + {"alter TABLE t1 add `end_time` varchar(10) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `t1` ADD `expire_time` DATETIME", NULL, false}, + {"ALTER TABLE `t1` ADD `id_a` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` ADD `id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` ADD `id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` ADD `node_state` INT(4)", NULL, false}, + {"ALTER TABLE `t1` ADD `no` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `order_id` INT", NULL, false}, + {"alter TABLE t1 add `start_time` varchar(10) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `t1` ADD `status` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `task_id` BIGINT(20)", NULL, false}, + {"alter TABLE t1 add `undo` int(1) DEFAULT '0' COMMENT 'this is a comment'", NULL, false}, + {"alter table `t1` add unique (`a`,`id`)", NULL, false}, + {"alter table `t1` add unique (`a`)", NULL, false}, + {"alter table `t1` add UNIQUE(`a`)", NULL, false}, + {"ALTER TABLE `t1` ADD UNIQUE `idx_id` USING BTREE (`id`, `result`)", NULL, false}, + {"ALTER TABLE `t1` ADD `update_time` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `username` VARCHAR(16)", NULL, false}, + {"ALTER TABLE `t1` AUTO_INCREMENT = 1", NULL, false}, + {"ALTER TABLE `t1` CHANGE `account_id` `account_id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `amount` `amount` DECIMAL(32,2)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `app_id` `app_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_id` `business_id` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_id` `business_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_unique_no` `business_unique_no` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `expire_time` `expire_time` DATETIME", NULL, false}, + {"ALTER TABLE `t1` CHANGE `id_a` `id_a` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `id` `id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `node_state` `node_state` INT(4)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `order_id` `order_id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `status` `status` INT(1)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `update_time` `update_time` TIMESTAMP", NULL, false}, + {"ALTER TABLE `t1` CHANGE `username` `username` VARCHAR(16)", NULL, false}, + {"ALTER TABLE `t1` COMMENT = 'a comment'", NULL, false}, + {"alter table `t1` drop index a", NULL, false}, + {"alter table t1 drop index t1_idx", NULL, false}, + {"alter table t1 index(account_id, business_id)", NULL, false}, + {"ALTER TABLE `t1` MODIFY COLUMN `expire_time` DATETIME DEFAULT NULL COMMENT 'this is a comment' AFTER `update_time`", "update_time", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `id_a` VARCHAR(128) CHARACTER SET utf8 COLLATE utf8_general_ci COMMENT 'this is a comment' AFTER `username`", "username", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `number` VARCHAR(64) CHARACTER SET utf8 COLLATE utf8_general_ci DEFAULT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `task_id` BIGINT(20) DEFAULT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `username` VARCHAR(16) CHARACTER SET utf8 COLLATE utf8_general_ci NOT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` RENAME `t2`", NULL, false}, + {"ALTER TABLE `db1`.`t1` ADD COLUMN `num` varchar(32) COMMENT 'this is a comment' AFTER `bank_name`", "bank_name", true}, + {"ALTER TABLE `db1`.`t1` ADD INDEX `idx_node_state` USING BTREE (`node_state`) comment ''", NULL, false}, + {"ALTER TABLE `db1`.`t1` CHANGE COLUMN `num` `code` varchar(32) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `db1`.`t1` DROP INDEX `a`, ADD INDEX `a` USING BTREE (`a`) comment ''", NULL, false}, + {"ALTER TABLE `db1`.`t1` DROP INDEX `a`, ADD INDEX `idx_a` USING BTREE (`a`) comment ''", NULL, false}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT AFTER `b`", "b", true}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT first", NULL, true}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT", NULL, false}, + {"ALTER TABLE `t1` MODIFY COLUMN `a` INT PRIMARY KEY", NULL, false}, + {NULL} +}; + +int main(int argc, char** argv) +{ + int rval = 0; + + for (int i = 0; data[i].statement; i++) + { + const char* target = NULL; + int len = 0; + const char* stmt = data[i].statement; + const char* end = data[i].statement + strlen(data[i].statement); + + if (get_placement_specifier(stmt, end, &target, &len) != data[i].rval) + { + const char* a = data[i].rval ? "true" : "false"; + const char* b = data[i].rval ? "false" : "true"; + printf("Expected '%s', got '%s' for '%s'\n", a, b, data[i].statement); + rval++; + } + else if (((bool)data[i].target != (bool)target) || (strncmp(target, data[i].target, len) != 0)) + { + printf("Expected '%s', got '%.*s' for '%s'\n", data[i].target, len, target, data[i].statement); + rval++; + } + } + + return rval; +} From c000b3186c61c8cd4398806398837067d7076420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 12:49:21 +0200 Subject: [PATCH 10/17] MXS-1575: Fix optional COLUMN keywork handling The COLUMN keyword is optional and cannot be assumed to exist. --- server/modules/routing/avrorouter/avro_schema.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 610c27480..8fcfd1b94 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -1334,9 +1334,14 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) if (tok) { - if (tok_eq(ptok, "add", plen) && tok_eq(tok, "column", len)) + if (tok_eq(tok, "column", len)) { + // Skip the optional COLUMN keyword tok = get_tok(tok + len, &len, end); + } + + if (tok_eq(ptok, "add", plen)) + { char avro_token[len + 1]; make_avro_token(avro_token, tok, len); bool is_new = true; @@ -1367,10 +1372,8 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) tok = get_next_def(tok, end); len = 0; } - else if (tok_eq(ptok, "drop", plen) && tok_eq(tok, "column", len)) + else if (tok_eq(ptok, "drop", plen)) { - tok = get_tok(tok + len, &len, end); - int idx = get_column_index(create, tok, len); if (idx != -1) @@ -1394,10 +1397,8 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) tok = get_next_def(tok, end); len = 0; } - else if (tok_eq(ptok, "change", plen) && tok_eq(tok, "column", len)) + else if (tok_eq(ptok, "change", plen)) { - tok = get_tok(tok + len, &len, end); - int idx = get_column_index(create, tok, len); if (idx != -1 && (tok = get_tok(tok + len, &len, end))) From b7e475f316c92fb0e4325c118dc1156d79cbeb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 16:37:49 +0200 Subject: [PATCH 11/17] =?UTF-8?q?MXS-1621:=20Detect=20TABLE=5FMAP=20?= =?UTF-8?q?=E2=86=94=20TABLE=5FCREATE=20column=20count=20mismatch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the TABLE_MAP and TABLE_CREATE have different column counts, an error is logged and the row events are skipped. --- server/core/mysql_binlog.c | 1 + server/modules/routing/avrorouter/avro_rbr.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/server/core/mysql_binlog.c b/server/core/mysql_binlog.c index 33f6dd395..86ea4065b 100644 --- a/server/core/mysql_binlog.c +++ b/server/core/mysql_binlog.c @@ -649,6 +649,7 @@ size_t unpack_numeric_field(uint8_t *src, uint8_t type, uint8_t *metadata, uint8 break; } + ss_dassert(size > 0); memcpy(dest, src, size); return size; } diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 8af455076..d3b5c6a87 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -210,6 +210,7 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) { bool rval = false; uint8_t *start = ptr; + uint8_t *end = ptr + hdr->event_size - BINLOG_EVENT_HDR_LEN; uint8_t table_id_size = router->event_type_hdr_lens[hdr->event_type] == 6 ? 4 : 6; uint64_t table_id = 0; @@ -274,8 +275,9 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) snprintf(table_ident, sizeof(table_ident), "%s.%s", map->database, map->table); AVRO_TABLE* table = hashtable_fetch(router->open_tables, table_ident); TABLE_CREATE* create = map->table_create; + ss_dassert(hashtable_fetch(router->created_tables, table_ident) == create); - if (table && create && ncolumns == map->columns) + if (table && create && ncolumns == map->columns && create->columns == map->columns) { avro_value_t record; avro_generic_value_new(table->avro_writer_iface, &record); @@ -292,7 +294,6 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) MXS_INFO("Row %lu", total_row_count++); /** Add the current GTID and timestamp */ - uint8_t *end = ptr + hdr->event_size - BINLOG_EVENT_HDR_LEN; int event_type = get_event_type(hdr->event_type); prepare_record(router, hdr, event_type, &record); ptr = process_row_event_data(map, create, &record, ptr, col_present, end); @@ -334,6 +335,12 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) "binary logs or the stored schema was not correct.", map->database, map->table); } + else if (ncolumns == map->columns && create->columns != map->columns) + { + MXS_ERROR("Table map event has a different column " + "count for table %s.%s than the CREATE TABLE statement.", + map->database, map->table); + } else { MXS_ERROR("Row event and table map event have different column " From 9146a215f7d1dc1e9508c8ba419899a12d9fa566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 13:50:06 +0200 Subject: [PATCH 12/17] Fix DDL table identifier parsing The parsing was inadequate and did not support all forms of quoted identifiers. --- server/modules/routing/avrorouter/avro_file.c | 30 +-- .../modules/routing/avrorouter/avro_schema.c | 173 ++++++++++++++---- .../modules/routing/avrorouter/avrorouter.h | 4 +- 3 files changed, 142 insertions(+), 65 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index 793376e07..dffb375d4 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1067,6 +1067,9 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } } + char ident[MYSQL_TABLE_MAXLEN + MYSQL_DATABASE_MAXLEN + 2]; + read_table_identifier(db, sql, sql + len, ident, sizeof(ident)); + if (is_create_table_statement(router, sql, len)) { TABLE_CREATE *created = NULL; @@ -1086,7 +1089,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - created = table_create_alloc(sql, len, db); + created = table_create_alloc(ident, sql, len, db); } if (created && !save_and_replace_table_create(router, created)) @@ -1096,30 +1099,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else if (is_alter_table_statement(router, sql, len)) { - char ident[MYSQL_TABLE_MAXLEN + MYSQL_DATABASE_MAXLEN + 2]; - read_alter_identifier(sql, sql + len, ident, sizeof(ident)); - - bool combine = (strnlen(db, 1) && strchr(ident, '.') == NULL); - - size_t ident_len = strlen(ident) + 1; // + 1 for the NULL - - if (combine) - { - ident_len += (strlen(db) + 1); // + 1 for the "." - } - - char full_ident[ident_len]; - full_ident[0] = 0; // Set full_ident to "". - - if (combine) - { - strcat(full_ident, db); - strcat(full_ident, "."); - } - - strcat(full_ident, ident); - - TABLE_CREATE *created = hashtable_fetch(router->created_tables, full_ident); + TABLE_CREATE *created = hashtable_fetch(router->created_tables, ident); if (created) { diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 8fcfd1b94..efc1bb832 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -720,46 +720,28 @@ int resolve_table_version(const char* db, const char* table) /** * @brief Handle a query event which contains a CREATE TABLE statement - * @param sql Query SQL - * @param db Database where this query was executed + * + * @param ident Table identifier in database.table format + * @param sql The CREATE TABLE statement + * @param len Length of @c sql + * * @return New CREATE_TABLE object or NULL if an error occurred */ -TABLE_CREATE* table_create_alloc(const char* sql, int len, const char* db) +TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len) { /** Extract the table definition so we can get the column names from it */ int stmt_len = 0; const char* statement_sql = get_table_definition(sql, len, &stmt_len); ss_dassert(statement_sql); + + char* tbl_start = strchr(ident, '.'); + ss_dassert(tbl_start); + *tbl_start++ = '\0'; + char table[MYSQL_TABLE_MAXLEN + 1]; char database[MYSQL_DATABASE_MAXLEN + 1]; - const char* err = NULL; - MXS_INFO("Create table: %.*s", len, sql); - - if (!statement_sql) - { - err = "table definition"; - } - else if (!get_table_name(sql, table)) - { - err = "table name"; - } - - if (get_database_name(sql, database)) - { - // The CREATE statement contains the database name - db = database; - } - else if (*db == '\0') - { - // No explicit or current database - err = "database name"; - } - - if (err) - { - MXS_ERROR("Malformed CREATE TABLE statement, could not extract %s: %.*s", err, len, sql); - return NULL; - } + strcpy(database, ident); + strcpy(table, tbl_start); int* lengths = NULL; char **names = NULL; @@ -1253,15 +1235,130 @@ static bool tok_eq(const char *a, const char *b, size_t len) return true; } -void read_alter_identifier(const char *sql, const char *end, char *dest, int size) +static void skip_whitespace(const char** saved) { - int len = 0; - const char *tok = get_tok(sql, &len, end); // ALTER - if (tok && (tok = get_tok(tok + len, &len, end)) // TABLE - && (tok = get_tok(tok + len, &len, end))) // Table identifier + const char* ptr = *saved; + + while (*ptr && isspace(*ptr)) { - snprintf(dest, size, "%.*s", len, tok); - remove_backticks(dest); + ptr++; + } + + *saved = ptr; +} + +static void skip_token(const char** saved) +{ + const char* ptr = *saved; + + while (*ptr && !isspace(*ptr)) + { + ptr++; + } + + *saved = ptr; +} + +static void skip_non_backtick(const char** saved) +{ + const char* ptr = *saved; + + while (*ptr && *ptr != '`') + { + ptr++; + } + + *saved = ptr; +} + +const char* keywords[] = +{ + "CREATE", + "DROP", + "ALTER", + "IF", + "EXISTS", + "REPLACE", + "OR", + "TABLE", + "NOT", + NULL +}; + +static bool token_is_keyword(const char* tok, int len) +{ + for (int i = 0; keywords[i]; i++) + { + if (strncasecmp(keywords[i], tok, len) == 0) + { + return true; + } + } + + return false; +} + +void read_table_identifier(const char* db, const char *sql, const char *end, char *dest, int size) +{ + const char* start; + int len = 0; + bool is_keyword = true; + + while (is_keyword) + { + skip_whitespace(&sql); // Leading whitespace + + if (*sql == '`') + { + // Quoted identifier, not a keyword + is_keyword = false; + sql++; + start = sql; + skip_non_backtick(&sql); + len = sql - start; + sql++; + } + else + { + start = sql; + skip_token(&sql); + len = sql - start; + is_keyword = token_is_keyword(start, len); + } + } + + skip_whitespace(&sql); // Space after first identifier + + if (*sql != '.') + { + // No explicit database + snprintf(dest, size, "%s.%.*s", db, len, start); + } + else + { + // Explicit database, skip the period + sql++; + skip_whitespace(&sql); // Space after first identifier + + const char* id_start; + int id_len = 0; + + if (*sql == '`') + { + sql++; + id_start = sql; + skip_non_backtick(&sql); + id_len = sql - id_start; + sql++; + } + else + { + id_start = sql; + skip_token(&sql); + id_len = sql - id_start; + } + + snprintf(dest, size, "%.*s.%.*s", len, start, id_len, id_start); } } diff --git a/server/modules/routing/avrorouter/avrorouter.h b/server/modules/routing/avrorouter/avrorouter.h index 9f4ebb1a5..02f1b8d6c 100644 --- a/server/modules/routing/avrorouter/avrorouter.h +++ b/server/modules/routing/avrorouter/avrorouter.h @@ -302,12 +302,12 @@ extern void read_table_info(uint8_t *ptr, uint8_t post_header_len, uint64_t *tab char* dest, size_t len); extern TABLE_MAP *table_map_alloc(uint8_t *ptr, uint8_t hdr_len, TABLE_CREATE* create); extern void table_map_free(TABLE_MAP *map); -extern TABLE_CREATE* table_create_alloc(const char* sql, int len, const char* db); +extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len, const char* db); extern TABLE_CREATE* table_create_copy(AVRO_INSTANCE *router, const char* sql, size_t len, const char* db); extern void table_create_free(TABLE_CREATE* value); extern bool table_create_save(TABLE_CREATE *create, const char *filename); extern bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end); -extern void read_alter_identifier(const char *sql, const char *end, char *dest, int size); +extern void read_table_identifier(const char* db, const char *sql, const char *end, char *dest, int size); extern int avro_client_handle_request(AVRO_INSTANCE *, AVRO_CLIENT *, GWBUF *); extern void avro_client_rotate(AVRO_INSTANCE *router, AVRO_CLIENT *client, uint8_t *ptr); extern bool avro_open_binlog(const char *binlogdir, const char *file, int *fd); From df86ee35791143d29625c9d87ca4f3a25f972f5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 14:02:53 +0200 Subject: [PATCH 13/17] Fix buffer overflow assertions The buffer overflow assertions were off by one: The data pointer can be equal to the end pointer when the last column of the row is processed. --- server/modules/routing/avrorouter/avro_rbr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index d3b5c6a87..4c506f284 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -564,7 +564,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value avro_value_set_string(&field, strval); sprintf(trace[i], "[%ld] ENUM: %lu bytes", i, bytes); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } else { @@ -600,7 +600,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value str[bytes] = '\0'; avro_value_set_string(&field, str); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } } else if (column_is_bit(map->column_types[i])) @@ -619,7 +619,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value avro_value_set_int(&field, value); sprintf(trace[i], "[%ld] BIT", i); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_decimal(map->column_types[i])) { @@ -627,7 +627,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value ptr += unpack_decimal_field(ptr, metadata + metadata_offset, &f_value); avro_value_set_double(&field, f_value); sprintf(trace[i], "[%ld] DECIMAL", i); - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_variable_string(map->column_types[i])) { @@ -650,7 +650,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value buf[sz] = '\0'; ptr += sz; avro_value_set_string(&field, buf); - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_blob(map->column_types[i])) { @@ -669,7 +669,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value uint8_t nullvalue = 0; avro_value_set_bytes(&field, &nullvalue, 1); } - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_temporal(map->column_types[i])) { @@ -681,7 +681,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value format_temporal_value(buf, sizeof(buf), map->column_types[i], &tm); avro_value_set_string(&field, buf); sprintf(trace[i], "[%ld] %s: %s", i, column_type_to_string(map->column_types[i]), buf); - check_overflow(ptr < end); + check_overflow(ptr <= end); } /** All numeric types (INT, LONG, FLOAT etc.) */ else @@ -692,7 +692,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value &metadata[metadata_offset], lval); set_numeric_field_value(&field, map->column_types[i], &metadata[metadata_offset], lval); sprintf(trace[i], "[%ld] %s", i, column_type_to_string(map->column_types[i])); - check_overflow(ptr < end); + check_overflow(ptr <= end); } ss_dassert(metadata_offset <= map->column_metadata_size); metadata_offset += get_metadata_len(map->column_types[i]); From e14710ab2bfd073ad31b394cb02a8e89a2514e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 14:04:19 +0200 Subject: [PATCH 14/17] Fix ALTER TABLE detection regex The regular expression expected that the COLUMN keyword was always present. --- server/modules/routing/avrorouter/avro.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/routing/avrorouter/avro.c b/server/modules/routing/avrorouter/avro.c index 2932b19b6..e9458c764 100644 --- a/server/modules/routing/avrorouter/avro.c +++ b/server/modules/routing/avrorouter/avro.c @@ -65,7 +65,7 @@ static const char* avro_index_name = "avro.index"; static const char* create_table_regex = "(?i)create[a-z0-9[:space:]_]+table"; static const char* alter_table_regex = - "(?i)alter[[:space:]]+table.*column"; + "(?i)alter[[:space:]]+table"; /* The router entry points */ static MXS_ROUTER *createInstance(SERVICE *service, char **options); From 93923acafb7b8dc6716a200deefde19e4a87fa99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 14:05:52 +0200 Subject: [PATCH 15/17] MXS-1621: Skip unneeded ALTER TABLE operations Some ALTER TABLE operations (e.g. ADD INDEX) are not useful to the avrorouter. These need to be detected and skipped. --- server/modules/routing/avrorouter/avro_file.c | 37 +++++++++++++++- server/modules/routing/avrorouter/avro_rbr.c | 6 +-- .../modules/routing/avrorouter/avro_schema.c | 44 ++++++++++++++++++- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index dffb375d4..ee9fd04f2 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1021,6 +1021,40 @@ void unify_whitespace(char *sql, int len) } } +/** + * A very simple function for stripping auto-generated executable comments + * + * Note that the string will not strip the trailing part of the comment, making + * the SQL invalid. + * + * @param sql String to modify + * @param len Pointer to current length of string, updated to new length if + * @c sql is modified + */ +static void strip_executable_comments(char *sql, int* len) +{ + if (strncmp(sql, "/*!", 3) == 0 || strncmp(sql, "/*M!", 4) == 0) + { + // Executable comment, remove it + char* p = sql + 3; + if (*p == '!') + { + p++; + } + + // Skip the versioning part + while (*p && isdigit(*p)) + { + p++; + } + + int n_extra = p - sql; + int new_len = *len - n_extra; + memmove(sql, sql + n_extra, new_len); + *len = new_len; + } +} + /** * @brief Handling of query events * @@ -1046,6 +1080,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra sql = tmp; len = tmpsz; unify_whitespace(sql, len); + strip_executable_comments(sql, &len); sql[len] = '\0'; static bool warn_not_row_format = true; @@ -1107,7 +1142,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - MXS_ERROR("Alter statement to a table with no create statement."); + MXS_ERROR("Alter statement to table '%s' has no preceding create statement.", ident); } } /* A transaction starts with this event */ diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 4c506f284..4b9f5d177 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -337,9 +337,9 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) } else if (ncolumns == map->columns && create->columns != map->columns) { - MXS_ERROR("Table map event has a different column " - "count for table %s.%s than the CREATE TABLE statement.", - map->database, map->table); + MXS_ERROR("Table map event has a different column count for table " + "%s.%s than the CREATE TABLE statement. Possible " + "unsupported DDL detected.", map->database, map->table); } else { diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index efc1bb832..aaacc4a32 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -1392,6 +1392,14 @@ int get_column_index(TABLE_CREATE *create, const char *tok, int len) char safe_tok[len + 2]; memcpy(safe_tok, tok, len); safe_tok[len] = '\0'; + + if (*safe_tok == '`') + { + int toklen = strlen(safe_tok) - 2; // Token length without backticks + memmove(safe_tok, safe_tok + 1, toklen); // Overwrite first backtick + safe_tok[toklen] = '\0'; // Null-terminate the string before the second backtick + } + fix_reserved_word(safe_tok); for (int x = 0; x < create->columns; x++) @@ -1406,6 +1414,35 @@ int get_column_index(TABLE_CREATE *create, const char *tok, int len) return idx; } +static bool not_column_operation(const char* tok, int len) +{ + const char* keywords[] = + { + "PRIMARY", + "UNIQUE", + "FULLTEXT", + "SPATIAL", + "PERIOD", + "PRIMARY", + "KEY", + "KEYS", + "INDEX", + "FOREIGN", + "CONSTRAINT", + NULL + }; + + for (int i = 0; keywords[i]; i++) + { + if (tok_eq(tok, keywords[i], strlen(keywords[i]))) + { + return true; + } + } + + return false; +} + bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) { const char *tbl = strcasestr(sql, "table"), *def; @@ -1431,7 +1468,12 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) if (tok) { - if (tok_eq(tok, "column", len)) + if (not_column_operation(tok, len)) + { + MXS_INFO("Statement doesn't affect columns, not processing: %s", sql); + return true; + } + else if (tok_eq(tok, "column", len)) { // Skip the optional COLUMN keyword tok = get_tok(tok + len, &len, end); From 66ec4792cd571b56578d1837583f750cb83a258a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 17:01:20 +0200 Subject: [PATCH 16/17] MXS-1575: Fix DATETIME handling DATETIME values in old formats should always be 8 bytes long. This is how MariaDB 10.2 stores them and only DATETIME2 values are stored with a fractional part. --- server/core/mysql_binlog.c | 55 +++++++++----------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/server/core/mysql_binlog.c b/server/core/mysql_binlog.c index 86ea4065b..fbd07179c 100644 --- a/server/core/mysql_binlog.c +++ b/server/core/mysql_binlog.c @@ -262,47 +262,18 @@ static void unpack_datetime(uint8_t *ptr, int length, struct tm *dest) uint64_t val = 0; uint32_t second, minute, hour, day, month, year; - if (length == -1) - { - val = gw_mysql_get_byte8(ptr); - second = val - ((val / 100) * 100); - val /= 100; - minute = val - ((val / 100) * 100); - val /= 100; - hour = val - ((val / 100) * 100); - val /= 100; - day = val - ((val / 100) * 100); - val /= 100; - month = val - ((val / 100) * 100); - val /= 100; - year = val; - } - else - { - // TODO: Figure out why DATETIME(0) doesn't work like it others do - val = unpack_bytes(ptr, datetime_sizes[length]); - val *= log_10_values[6 - length]; - - if (val < 0) - { - val = -val; - } - - int subsecond = val % 1000000; - val /= 1000000; - - second = val % 60; - val /= 60; - minute = val % 60; - val /= 60; - hour = val % 24; - val /= 24; - day = val % 32; - val /= 32; - month = val % 13; - val /= 13; - year = val; - } + val = gw_mysql_get_byte8(ptr); + second = val - ((val / 100) * 100); + val /= 100; + minute = val - ((val / 100) * 100); + val /= 100; + hour = val - ((val / 100) * 100); + val /= 100; + day = val - ((val / 100) * 100); + val /= 100; + month = val - ((val / 100) * 100); + val /= 100; + year = val; memset(dest, 0, sizeof(struct tm)); dest->tm_year = year - 1900; @@ -502,7 +473,7 @@ static size_t temporal_field_size(uint8_t type, uint8_t decimals, int length) return 3 + ((decimals + 1) / 2); case TABLE_COL_TYPE_DATETIME: - return length < 0 || length > 6 ? 8 : datetime_sizes[length]; + return 8; case TABLE_COL_TYPE_TIMESTAMP: return 4; From 6dcc71d8622295d6d6d7f8702d5a01683b3dc27a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 20:17:48 +0200 Subject: [PATCH 17/17] MXS-1621: Fix minor bugs caused by previous changes Used the correct value in table_create_alloc and remove unused parameter. Use the pre-calculated end pointer when looking for events. Always use the column count of the TABLE_MAP event as all mismatches are detected earlier. --- server/modules/routing/avrorouter/avro_file.c | 2 +- server/modules/routing/avrorouter/avro_rbr.c | 4 ++-- server/modules/routing/avrorouter/avro_schema.c | 4 ++-- server/modules/routing/avrorouter/avrorouter.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index ee9fd04f2..cbb011f85 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1124,7 +1124,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - created = table_create_alloc(ident, sql, len, db); + created = table_create_alloc(ident, sql, len); } if (created && !save_and_replace_table_create(router, created)) diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 4b9f5d177..dc72be06a 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -288,7 +288,7 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) int rows = 0; MXS_INFO("Row Event for '%s' at %lu", table_ident, router->current_pos); - while (ptr - start < hdr->event_size - BINLOG_EVENT_HDR_LEN) + while (ptr < end) { static uint64_t total_row_count = 1; MXS_INFO("Row %lu", total_row_count++); @@ -514,7 +514,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value { int npresent = 0; avro_value_t field; - long ncolumns = MXS_MIN(map->columns, create->columns); + long ncolumns = map->columns; uint8_t *metadata = map->column_metadata; size_t metadata_offset = 0; diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index aaacc4a32..962a394f7 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -755,13 +755,13 @@ TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len) { if ((rval = MXS_MALLOC(sizeof(TABLE_CREATE)))) { - rval->version = resolve_table_version(db, table); + rval->version = resolve_table_version(database, table); rval->was_used = false; rval->column_names = names; rval->column_lengths = lengths; rval->column_types = types; rval->columns = n_columns; - rval->database = MXS_STRDUP(db); + rval->database = MXS_STRDUP(database); rval->table = MXS_STRDUP(table); } diff --git a/server/modules/routing/avrorouter/avrorouter.h b/server/modules/routing/avrorouter/avrorouter.h index 02f1b8d6c..41a4f93fa 100644 --- a/server/modules/routing/avrorouter/avrorouter.h +++ b/server/modules/routing/avrorouter/avrorouter.h @@ -302,7 +302,7 @@ extern void read_table_info(uint8_t *ptr, uint8_t post_header_len, uint64_t *tab char* dest, size_t len); extern TABLE_MAP *table_map_alloc(uint8_t *ptr, uint8_t hdr_len, TABLE_CREATE* create); extern void table_map_free(TABLE_MAP *map); -extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len, const char* db); +extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len); extern TABLE_CREATE* table_create_copy(AVRO_INSTANCE *router, const char* sql, size_t len, const char* db); extern void table_create_free(TABLE_CREATE* value); extern bool table_create_save(TABLE_CREATE *create, const char *filename);