From 6b3c7041e3880fbdec8dff2de6d68806d123086d Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Tue, 5 Aug 2014 09:31:10 +0300 Subject: [PATCH 01/17] Bug #468, http://bugs.skysql.com/show_bug.cgi?id=468, Query classifier accessed freed thread context. If parsing fails thd doesn't need to be freed because it holds correct information about command type. session.c:session_setup_filters : fixed memory leak hintparser.c: added token_free for HINT_TOKENs and fixed a few memory leaks. mysql_client_server_protocol.h: added mysql_protocol_done which frees memory blocks pointed to by protocol members. Those can't be freed in dcb.c because dcb.c doesn't know about protocol's members. mysql_backend.c:gw_backend_close: fixed memory leak mysql_client.c: gw_client_close: fixed memory leak mysql_common.c: added implementation of mysql_protocol_done :protocol_archive_srv_command: tried to fix memory leak. Some memory is still leaking according to valgrind. Removed use of uninitialized local variable len. readwritesplit.c:execute_sescmd_in_backend: fixed a memory leak - visible only in DEBUG=Y build. --- query_classifier/query_classifier.cc | 16 +++++----- server/core/session.c | 1 + .../include/mysql_client_server_protocol.h | 1 + server/modules/protocol/mysql_backend.c | 2 ++ server/modules/protocol/mysql_client.c | 1 + server/modules/protocol/mysql_common.c | 31 ++++++++++++++++--- .../routing/readwritesplit/readwritesplit.c | 19 +++++++----- 7 files changed, 52 insertions(+), 19 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index eccc35a31..889940e00 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -149,17 +149,17 @@ skygw_query_type_t skygw_query_classifier_get_type( thd = get_or_create_thd_for_parsing(mysql, query_str); if (thd == NULL) - { - skygw_query_classifier_free(mysql); - } - /** Create parse_tree inside thd */ - failp = create_parse_tree(thd); - - if (failp) { skygw_query_classifier_free(mysql); *p_mysql = NULL; + goto return_qtype; } + /** + * Create parse_tree inside thd. + * thd and even lex are readable even if parser failed so let it + * continue despite failure. + */ + failp = create_parse_tree(thd); qtype = resolve_query_type(thd); if (p_mysql == NULL) @@ -464,7 +464,7 @@ static skygw_query_type_t resolve_query_type( type |= QUERY_TYPE_DISABLE_AUTOCOMMIT; type |= QUERY_TYPE_BEGIN_TRX; } - /** + /** * REVOKE ALL, ASSIGN_TO_KEYCACHE, * PRELOAD_KEYS, FLUSH, RESET, CREATE|ALTER|DROP SERVER */ diff --git a/server/core/session.c b/server/core/session.c index 80767f9ff..62e1015e4 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -667,6 +667,7 @@ int i; session->filters[i].session = head->session; session->filters[i].instance = head->instance; session->head = *head; + free(head); } for (i = 0; i < service->n_filters; i++) diff --git a/server/modules/include/mysql_client_server_protocol.h b/server/modules/include/mysql_client_server_protocol.h index f7d6030c1..9e2147d05 100644 --- a/server/modules/include/mysql_client_server_protocol.h +++ b/server/modules/include/mysql_client_server_protocol.h @@ -299,6 +299,7 @@ typedef struct { void gw_mysql_close(MySQLProtocol **ptr); MySQLProtocol* mysql_protocol_init(DCB* dcb, int fd); +void mysql_protocol_done (DCB* dcb); MySQLProtocol *gw_mysql_init(MySQLProtocol *data); void gw_mysql_close(MySQLProtocol **ptr); int gw_receive_backend_auth(MySQLProtocol *protocol); diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 43cd97bcd..5d8088d4e 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -996,6 +996,8 @@ gw_backend_close(DCB *dcb) /** Send COM_QUIT to the backend being closed */ mysql_send_com_quit(dcb, 0, quitbuf); + + mysql_protocol_done(dcb); if (session != NULL && session->state == SESSION_STATE_STOPPING) { diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index db61084eb..6cc9027de 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1325,6 +1325,7 @@ gw_client_close(DCB *dcb) CHK_PROTOCOL(protocol); } #endif + mysql_protocol_done(dcb); session = dcb->session; /** diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index 4ce34452b..70050a13c 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -96,6 +96,31 @@ return_p: } +/** + * mysql_protocol_done + * + * free protocol allocations. + * + * @param dcb owner DCB + * + */ +void mysql_protocol_done ( + DCB* dcb) +{ + server_command_t* scmd = ((MySQLProtocol *)dcb->protocol)->protocol_cmd_history; + server_command_t* scmd2; + + while (scmd != NULL) + { + scmd2 = scmd->scom_next; + free(scmd); + scmd = scmd2; + } +} + + + + /** * gw_mysql_close * @@ -1601,7 +1626,7 @@ void protocol_archive_srv_command( { server_command_t* s1; server_command_t** s2; - int len; + int len = 0; spinlock_acquire(&p->protocol_lock); @@ -1617,9 +1642,7 @@ void protocol_archive_srv_command( s2 = &p->protocol_cmd_history; if (*s2 != NULL) - { - len = 0; - + { while ((*s2)->scom_next != NULL) { *s2 = (*s2)->scom_next; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 90132a9f9..7fcf22974 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1132,6 +1132,8 @@ static int routeQuery( break; case MYSQL_COM_STMT_EXECUTE: + /** Parsing is not needed for this type of packet */ +#if defined(NOT_USED) plainsqlbuf = gwbuf_clone_transform(querybuf, GWBUF_TYPE_PLAINSQL); len = GWBUF_LENGTH(plainsqlbuf); @@ -1140,7 +1142,8 @@ static int routeQuery( memcpy(querystr, startpos, len); memset(&querystr[len], 0, 1); qtype = skygw_query_classifier_get_type(querystr, 0, &mysql); - qtype |= QUERY_TYPE_EXEC_STMT; +#endif + qtype = QUERY_TYPE_EXEC_STMT; break; case MYSQL_COM_SHUTDOWN: /**< 8 where should shutdown be routed ? */ @@ -1923,7 +1926,8 @@ static bool select_connect_backend_servers( } } } - } + } /*< log only */ + /** * Choose at least 1+min_nslaves (master and slave) and at most 1+max_nslaves * servers from the sorted list. First master found is selected. @@ -2666,8 +2670,9 @@ static bool execute_sescmd_in_backend( pthread_self(), dcb->fd, STRPACKETTYPE(cmd)))); + gwbuf_free(tmpbuf); } -#endif +#endif /*< SS_DEBUG */ switch (scur->scmd_cur_cmd->my_sescmd_packet_type) { case MYSQL_COM_CHANGE_USER: rc = dcb->func.auth( @@ -3065,7 +3070,7 @@ static bool router_option_configured( } return succp; } -#endif +#endif /*< NOT_USED */ static void rwsplit_process_router_options( ROUTER_INSTANCE* router, @@ -3346,7 +3351,7 @@ static void print_error_packet( { while ((buf = gwbuf_consume(buf, GWBUF_LENGTH(buf))) != NULL); } -#endif +#endif /*< SS_DEBUG */ } static int router_get_servercount( @@ -3568,10 +3573,10 @@ static prep_stmt_t* prep_stmt_init( if (pstmt != NULL) { - #if defined(SS_DEBUG) +#if defined(SS_DEBUG) pstmt->pstmt_chk_top = CHK_NUM_PREP_STMT; pstmt->pstmt_chk_tail = CHK_NUM_PREP_STMT; - #endif +#endif pstmt->pstmt_state = PREP_STMT_ALLOC; pstmt->pstmt_type = type; From dbfaa5a8eaa1d62def7dce5503ca24faefe46cd2 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Tue, 5 Aug 2014 16:28:26 +0300 Subject: [PATCH 02/17] Fix to http://bugs.skysql.com/show_bug.cgi?id=469, connection counter leaks in master. Removed redundant counter increments. --- server/modules/routing/readwritesplit/readwritesplit.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 7fcf22974..218972ab4 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -381,7 +381,7 @@ static void refreshInstance( * used in slave selection. */ if (!rlag_enabled) - { + { if (rlag_limited) { LOGIF(LE, (skygw_log_write_flush( @@ -1985,7 +1985,7 @@ static bool select_connect_backend_servers( backend_ref[i].bref_state = 0; bref_set_state(&backend_ref[i], BREF_IN_USE); - /** + /** * Increase backend connection counter. * Server's stats are _increased_ in * dcb.c:dcb_alloc ! @@ -2023,7 +2023,7 @@ static bool select_connect_backend_servers( session, b->backend_server->protocol); - if (backend_ref[i].bref_dcb != NULL) + if (backend_ref[i].bref_dcb != NULL) { master_connected = true; /** @@ -2040,8 +2040,6 @@ static bool select_connect_backend_servers( bref_set_state(&backend_ref[i], BREF_IN_USE); /** Increase backend connection counters */ - atomic_add(&b->backend_server->stats.n_current, 1); - atomic_add(&b->backend_server->stats.n_connections, 1); atomic_add(&b->backend_conn_count, 1); } else From 28cc98d33ae0116f399131974d6d5ad88dcd1a01 Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Thu, 14 Aug 2014 23:22:21 +0300 Subject: [PATCH 03/17] add gcov patch file --- gcov.diff | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 gcov.diff diff --git a/gcov.diff b/gcov.diff new file mode 100644 index 000000000..c310d1b08 --- /dev/null +++ b/gcov.diff @@ -0,0 +1,138 @@ +diff --git a/makefile.inc b/makefile.inc +index f2d93bf..c7dbffa 100644 +--- a/makefile.inc ++++ b/makefile.inc +@@ -24,8 +24,8 @@ endif + + # -O2 -g -pipe -Wformat -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector --param=ssp-buffer-size=4 -fPIC + +-CFLAGS := $(CFLAGS) -Wall +-LDLIBS := $(LDLIBS) -pthread ++CFLAGS := $(CFLAGS) -Wall -fprofile-arcs -ftest-coverage ++LDLIBS := $(LDLIBS) -pthread -lgcov + LDMYSQL := -lmysqld + CPP_LDLIBS := -lstdc++ + +diff --git a/server/core/Makefile b/server/core/Makefile +index 9bf650c..9df75a7 100644 +--- a/server/core/Makefile ++++ b/server/core/Makefile +@@ -75,7 +75,7 @@ POBJS=maxpasswd.o secrets.o utils.o + LIBS=-L$(EMBEDDED_LIB) \ + -lmysqld \ + -lz -lm -lcrypt -lcrypto -ldl -laio -lrt -pthread -llog_manager \ +- -L../inih/extra -linih -lssl -lstdc++ ++ -L../inih/extra -linih -lssl -lstdc++ -lgcov + + all: maxscale maxkeys maxpasswd + +diff --git a/server/modules/filter/Makefile b/server/modules/filter/Makefile +index 931c35a..d5dcca9 100644 +--- a/server/modules/filter/Makefile ++++ b/server/modules/filter/Makefile +@@ -25,7 +25,7 @@ UTILSPATH := $(ROOT_PATH)/utils + + CC=cc + CFLAGS=-c -fPIC -I/usr/include -I../include -I../../include -I$(LOGPATH) \ +- -I$(UTILSPATH) -Wall -g ++ -I$(UTILSPATH) -Wall -g -fprofile-arcs -ftest-coverage + + include ../../../makefile.inc + +@@ -44,7 +44,7 @@ TEESRCS=tee.c + TEEOBJ=$(TEESRCS:.c=.o) + SRCS=$(TESTSRCS) $(QLASRCS) $(REGEXSRCS) $(TOPNSRCS) $(TEESRCS) + OBJ=$(SRCS:.c=.o) +-LIBS=$(UTILSPATH)/skygw_utils.o -lssl -llog_manager ++LIBS=$(UTILSPATH)/skygw_utils.o -lssl -llog_manager -lgcov + MODULES= libtestfilter.so libqlafilter.so libregexfilter.so libtopfilter.so libtee.so + + +diff --git a/server/modules/monitor/Makefile b/server/modules/monitor/Makefile +index 7fdbc58..bca01de 100644 +--- a/server/modules/monitor/Makefile ++++ b/server/modules/monitor/Makefile +@@ -28,7 +28,7 @@ CFLAGS=-c -fPIC -I. -I/usr/include -I../include -I../../include -I$(LOGPATH) \ + + LDFLAGS=-shared -L$(LOGPATH) -Wl,-rpath,$(DEST)/lib \ + -Wl,-rpath,$(LOGPATH) -Wl,-rpath,$(UTILSPATH) \ +- -Wl,-rpath,$(EMBEDDED_LIB) ++ -Wl,-rpath,$(EMBEDDED_LIB) -fprofile-arcs -ftest-coverage + + + +@@ -39,7 +39,7 @@ GALERAOBJ=$(GALERASRCS:.c=.o) + SRCS=$(MYSQLSRCS) + OBJ=$(SRCS:.c=.o) + LIBS=$(UTILSPATH)/skygw_utils.o -llog_manager \ +- -L$(EMBEDDED_LIB) -lmysqld ++ -L$(EMBEDDED_LIB) -lmysqld -lgcov + MODULES=libmysqlmon.so libgaleramon.so + + +diff --git a/server/modules/protocol/Makefile b/server/modules/protocol/Makefile +index 54a8f8c..c8913ab 100644 +--- a/server/modules/protocol/Makefile ++++ b/server/modules/protocol/Makefile +@@ -31,7 +31,7 @@ UTILSPATH := $(ROOT_PATH)/utils + + CC=cc + CFLAGS=-c -fPIC -I/usr/include -I../include -I../../include -I$(LOGPATH) \ +- -I$(UTILSPATH) -Wall -g ++ -I$(UTILSPATH) -Wall -g -fprofile-arcs -ftest-coverage + + include ../../../makefile.inc + +@@ -51,7 +51,7 @@ MAXSCALEDOBJ=$(MAXSCALEDSRCS:.c=.o) + SRCS=$(MYSQLCLIENTSRCS) $(MYSQLBACKENDSRCS) $(TELNETDSRCS) $(HTTPDSRCS) \ + $(MAXSCALEDSRCS) + OBJ=$(SRCS:.c=.o) +-LIBS=$(UTILSPATH)/skygw_utils.o ++LIBS=$(UTILSPATH)/skygw_utils.o -lgcov + MODULES=libMySQLClient.so libMySQLBackend.so libtelnetd.so libHTTPD.so \ + libmaxscaled.so + +diff --git a/server/modules/routing/Makefile b/server/modules/routing/Makefile +index 4feac68..afd1da7 100644 +--- a/server/modules/routing/Makefile ++++ b/server/modules/routing/Makefile +@@ -29,7 +29,7 @@ UTILSPATH := $(ROOT_PATH)/utils + + CC=cc + CFLAGS=-c -fPIC -I/usr/include -I../include -I../../include -I$(LOGPATH) \ +- -I$(UTILSPATH) -Wall -g ++ -I$(UTILSPATH) -Wall -g -fprofile-arcs -ftest-coverage + + include ../../../makefile.inc + +@@ -46,7 +46,7 @@ CLISRCS=cli.c debugcmd.c + CLIOBJ=$(CLISRCS:.c=.o) + SRCS=$(TESTSRCS) $(READCONSRCS) $(DEBUGCLISRCS) cli.c + OBJ=$(SRCS:.c=.o) +-LIBS=$(UTILSPATH)/skygw_utils.o -lssl -llog_manager ++LIBS=$(UTILSPATH)/skygw_utils.o -lssl -llog_manager -lgcov + MODULES= libdebugcli.so libreadconnroute.so libtestroute.so libcli.so + + +diff --git a/server/modules/routing/readwritesplit/Makefile b/server/modules/routing/readwritesplit/Makefile +index c60f2ff..a3a643e 100644 +--- a/server/modules/routing/readwritesplit/Makefile ++++ b/server/modules/routing/readwritesplit/Makefile +@@ -27,7 +27,7 @@ QCLASSPATH := $(ROOT_PATH)/query_classifier + CC=cc + CFLAGS=-c -fPIC -I/usr/include -I../../include -I../../../include \ + -I$(LOGPATH) -I$(UTILSPATH) -I$(QCLASSPATH) \ +- $(MYSQL_HEADERS) -Wall -g ++ $(MYSQL_HEADERS) -Wall -g -fprofile-arcs -ftest-coverage + + include ../../../../makefile.inc + +@@ -38,7 +38,7 @@ LDFLAGS=-shared -L$(LOGPATH) -L$(QCLASSPATH) -L$(EMBEDDED_LIB) \ + + SRCS=readwritesplit.c + OBJ=$(SRCS:.c=.o) +-LIBS=-lssl -pthread -llog_manager -lquery_classifier -lmysqld ++LIBS=-lssl -pthread -llog_manager -lquery_classifier -lmysqld -lgcov + MODULES=libreadwritesplit.so + + all: $(MODULES) From 65b25a825ad4449abe044c12133dea57dd52079d Mon Sep 17 00:00:00 2001 From: Mark Riddoch Date: Wed, 20 Aug 2014 14:50:44 +0100 Subject: [PATCH 04/17] Addition of adminusers unit test Fix to filters unit test --- server/core/test/makefile | 12 +- server/core/test/testadminusers.c | 278 ++++++++++++++++++++++++++++++ server/core/test/testfilter.c | 2 + server/include/adminusers.h | 2 + 4 files changed, 292 insertions(+), 2 deletions(-) create mode 100644 server/core/test/testadminusers.c diff --git a/server/core/test/makefile b/server/core/test/makefile index 446027948..7f6831082 100644 --- a/server/core/test/makefile +++ b/server/core/test/makefile @@ -8,7 +8,7 @@ include ../../../makefile.inc include ../../../test.inc CC=cc -TESTLOG := $(shell pwd)/testhash.log +TESTLOG := $(shell pwd)/testcore.log LOGPATH := $(ROOT_PATH)/log_manager UTILSPATH := $(ROOT_PATH)/utils @@ -21,7 +21,7 @@ LDFLAGS=-rdynamic -L$(LOGPATH) \ LIBS= -lz -lm -lcrypt -lcrypto -ldl -laio -lrt -pthread -llog_manager \ -L../../inih/extra -linih -lssl -lstdc++ -TESTS=testhash testspinlock testfilter +TESTS=testhash testspinlock testfilter testadminusers cleantests: - $(DEL) *.o @@ -40,17 +40,25 @@ testhash: testhash.c -I$(ROOT_PATH)/server/include \ -I$(ROOT_PATH)/utils \ testhash.c ../hashtable.o ../atomic.o ../spinlock.o -o testhash + testspinlock: testspinlock.c $(CC) $(CFLAGS) \ -I$(ROOT_PATH)/server/include \ -I$(ROOT_PATH)/utils \ testspinlock.c ../spinlock.o ../atomic.o ../thread.o -o testspinlock + testfilter: testfilter.c libcore.a $(CC) $(CFLAGS) $(LDFLAGS) \ -I$(ROOT_PATH)/server/include \ -I$(ROOT_PATH)/utils \ testfilter.c libcore.a $(UTILSPATH)/skygw_utils.o $(LIBS) -o testfilter +testadminusers: testadminusers.c libcore.a + $(CC) $(CFLAGS) $(LDFLAGS) \ + -I$(ROOT_PATH)/server/include \ + -I$(ROOT_PATH)/utils \ + testadminusers.c libcore.a $(UTILSPATH)/skygw_utils.o $(LIBS) -o testadminusers + libcore.a: ../*.o ar rv libcore.a ../*.o diff --git a/server/core/test/testadminusers.c b/server/core/test/testadminusers.c new file mode 100644 index 000000000..00ec3a452 --- /dev/null +++ b/server/core/test/testadminusers.c @@ -0,0 +1,278 @@ +/* + * This file is distributed as part of MaxScale. It 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. + * + * 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 Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Copyright SkySQL Ab 2014 + */ + +/** + * + * @verbatim + * Revision History + * + * Date Who Description + * 20-08-2014 Mark Riddoch Initial implementation + * + * @endverbatim + */ + +#include +#include +#include + +#include + + +/** + * test1 default user + * + * Test that the username password admin/skysql is accepted if no users + * have been created and that no other users are accepted + * + * WARNING: $MAXSCALE_HOME/etc/passwd must be removed before this test is run + */ +static int +test1() +{ + if (admin_verify("admin", "skysql") == 0) + { + fprintf(stderr, "admin_verify: test 1.1 (default user) failed.\n"); + return 1; + } + if (admin_verify("bad", "user")) + { + fprintf(stderr, "admin_verify: test 1.2 (wrong user) failed.\n"); + return 1; + } + + return 0; +} + +/** + * test2 creating users + * + * Create a user + * Try to create a duplicate user - expects a failure + * Remove that user - expected to fail as one user must always remain + */ +static int +test2() +{ +char *err; + + if ((err = admin_add_user("user0", "passwd0")) != NULL) + { + fprintf(stderr, "admin_add_user: test 2.1 (add user) failed, %s.\n", err); + + return 1; + } + if (admin_add_user("user0", "passwd0") == NULL) + { + fprintf(stderr, "admin_add_user: test 2.2 (add user) failed, du;plicate.\n"); + + return 1; + } + + /* Deleting the last user is forbidden so we expect this to fail */ + if ((err = admin_remove_user("user0", "passwd0")) == NULL) + { + fprintf(stderr, "admin_remove_user: test 2.3 (add user) failed, %s.\n", err); + + return 1; + } + return 0; +} + +/** + * test3 search/verify users + * + * Create a user + * Search for that user + * Search for a non-existant user + * Remove the user + * Search for the user that was removed + */ +static int +test3() +{ +char *err; + + if ((err = admin_add_user("user1", "passwd1")) != NULL) + { + fprintf(stderr, "admin_add_user: test 3.1 (add user) failed, %s.\n", err); + + return 1; + } + + if (admin_search_user("user1") == 0) + { + fprintf(stderr, "admin_search_user: test 3.2 (search user) failed.\n"); + + return 1; + } + if (admin_search_user("user2") != 0) + { + fprintf(stderr, "admin_search_user: test 3.3 (search user) failed, unexpeted user found.\n"); + + return 1; + } + + if ((err = admin_remove_user("user1", "passwd1")) != NULL) + { + fprintf(stderr, "admin_remove_user: test 3.4 (add user) failed, %s.\n", err); + + return 1; + } + + if (admin_search_user("user1")) + { + fprintf(stderr, "admin_search_user: test 3.5 (search user) failed - user was deleted.\n"); + + return 1; + } + return 0; +} + +/** + * test4 verify users + * + * Create a numebr of users + * search for each user in turn + * verify each user in turn (password verification) + * Verify each user in turn with incorrect password + * Randomly verify each user + * Remove each user + */ +static int +test4() +{ +char *err, user[40], passwd[40]; +int i, n_users = 50; + + for (i = 1; i < n_users; i++) + { + sprintf(user, "user%d", i); + sprintf(passwd, "passwd%d", i); + if ((err = admin_add_user(user, passwd)) != NULL) + { + fprintf(stderr, "admin_add_user: test 4.1 (add user) failed, %s.\n", err); + + return 1; + } + } + + for (i = 1; i < n_users; i++) + { + sprintf(user, "user%d", i); + if (admin_search_user(user) == 0) + { + fprintf(stderr, "admin_search_user: test 4.2 (search user) failed.\n"); + + return 1; + } + } + for (i = 1; i < n_users; i++) + { + sprintf(user, "user%d", i); + sprintf(passwd, "passwd%d", i); + if (admin_verify(user, passwd) == 0) + { + fprintf(stderr, "admin_verify: test 4.3 (search user) failed.\n"); + + return 1; + } + } + + for (i = 1; i < n_users; i++) + { + sprintf(user, "user%d", i); + sprintf(passwd, "badpasswd%d", i); + if (admin_verify(user, passwd) != 0) + { + fprintf(stderr, "admin_verify: test 4.4 (search user) failed.\n"); + + return 1; + } + } + srand(time(0)); + for (i = 1; i < 1000; i++) + { + int j; + j = rand() % n_users; + if (j == 0) + j = 1; + sprintf(user, "user%d", j); + sprintf(passwd, "passwd%d", j); + if (admin_verify(user, passwd) == 0) + { + fprintf(stderr, "admin_verify: test 4.5 (random) failed.\n"); + + return 1; + } + } + + for (i = 1; i < n_users; i++) + { + sprintf(user, "user%d", i); + sprintf(passwd, "passwd%d", i); + if ((err = admin_remove_user(user, passwd)) != NULL) + { + fprintf(stderr, "admin_remove_user: test 4.6 (add user) failed, %s.\n", err); + + return 1; + } + } + return 0; +} + +/** + * test5 remove first user + * + * Create a user so that user0 may be removed + * Remove the first user created (user0) + */ +static int +test5() +{ +char *err; + + if ((err = admin_add_user("user", "passwd")) != NULL) + { + fprintf(stderr, "admin_add_user: test 5.1 (add user) failed, %s.\n", err); + + return 1; + } + if ((err = admin_remove_user("user0", "passwd0")) != NULL) + { + fprintf(stderr, "admin_remove_user: test 5.2 (add user) failed, %s.\n", err); + + return 1; + } + return 0; +} + +int +main(int argc, char **argv) +{ +int result = 0; + + result += test1(); + result += test2(); + result += test3(); + result += test4(); + result += test5(); + + exit(result); +} + diff --git a/server/core/test/testfilter.c b/server/core/test/testfilter.c index eefc8ecfa..55f7fadf3 100644 --- a/server/core/test/testfilter.c +++ b/server/core/test/testfilter.c @@ -140,6 +140,8 @@ int i, n_filters = 1000; return 0; } + +int main(int argc, char **argv) { int result = 0; diff --git a/server/include/adminusers.h b/server/include/adminusers.h index d46570e38..07afa5390 100644 --- a/server/include/adminusers.h +++ b/server/include/adminusers.h @@ -29,6 +29,8 @@ * * @endverbatim */ +#include + #define ADMIN_SALT "MS" extern int admin_verify(char *, char *); From a853b72baf2e12dcf21c88c724d83025e62484f5 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 20 Aug 2014 17:22:32 +0100 Subject: [PATCH 05/17] Modify build_gateway.inc so that variables are used, thus avoiding a need for editing. Please review the file to see the variables that are used - they should be obvious. --- build_gateway.inc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/build_gateway.inc b/build_gateway.inc index 83ffaf762..0e1d6b75f 100644 --- a/build_gateway.inc +++ b/build_gateway.inc @@ -12,7 +12,7 @@ # # Set debug flags # -DEBUG := +DEBUG := ${MAXSCALE_DEBUG} # # Set build env @@ -22,7 +22,7 @@ UNIX := Y # # Set MaxScale branch directory # -ROOT_PATH := $(HOME)/src/bazaar/tmp/maxscale +ROOT_PATH := $(HOME)/${MAXSCALE_SOURCE} INC_PATH := $(HOME)/usr/include # @@ -38,7 +38,7 @@ MYSQL_HEADERS := -I$(INC_PATH) -I$(MYSQL_ROOT)/ -I$(MYSQL_ROOT)/private/ -I$(MYS # # Set DYNLIB=Y if you want to link MaxScale with dynamic embedded lib # -DYNLIB := +DYNLIB := ${MAXSCALE_DYNLIB} # # Set path to Embedded MySQL Server @@ -51,3 +51,4 @@ endif # Set path to MySQL errors file # ERRMSG := $(HOME)/usr/share/mysql + From 3476558f526d2e788b42445169ba8d747241838a Mon Sep 17 00:00:00 2001 From: Mark Riddoch Date: Fri, 22 Aug 2014 14:25:27 +0100 Subject: [PATCH 06/17] Fixed soem errors from a cppcheck run mbrampton@martin-office:~/Dropbox/development/skygit/MaxScale/server$ cppcheck -q core/*.c [core/adminusers.c:302]: (error) Resource leak: fp_tmp [core/filter.c:382]: (error) Uninitialized variable: me [core/service.c:1071]: (error) Uninitialized variable: succp --- server/core/adminusers.c | 2 ++ server/core/filter.c | 2 +- server/core/service.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/server/core/adminusers.c b/server/core/adminusers.c index 6ed70ed3c..61cd7c077 100644 --- a/server/core/adminusers.c +++ b/server/core/adminusers.c @@ -298,6 +298,7 @@ char* admin_remove_user( fname, err))); fclose(fp); + fclose(fp_tmp); unlink(fname_tmp); return ADMIN_ERR_PWDFILEACCESS; } @@ -325,6 +326,7 @@ char* admin_remove_user( fname, err))); fclose(fp); + fclose(fp_tmp); unlink(fname_tmp); return ADMIN_ERR_PWDFILEACCESS; } diff --git a/server/core/filter.c b/server/core/filter.c index 405a01470..99525f7f6 100644 --- a/server/core/filter.c +++ b/server/core/filter.c @@ -359,7 +359,7 @@ DOWNSTREAM *me; UPSTREAM * filterUpstream(FILTER_DEF *filter, void *fsession, UPSTREAM *upstream) { -UPSTREAM *me; +UPSTREAM *me = NULL; /* * The the filter has no setUpstream entry point then is does diff --git a/server/core/service.c b/server/core/service.c index 1102dabb4..1b79db346 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -1008,7 +1008,7 @@ bool service_set_param_value ( { char* p; int valint; - bool succp; + bool succp = true; /** * Find out whether the value is numeric and ends with '%' or '\0' From c133c6ef4affe1075cb4922a12c34e5f7f022795 Mon Sep 17 00:00:00 2001 From: Mark Riddoch Date: Fri, 22 Aug 2014 14:33:14 +0100 Subject: [PATCH 07/17] Fix for bug 479 - Undefined filter reference in MaxScale.cnf causes a crash --- server/core/filter.c | 3 +++ server/core/session.c | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/server/core/filter.c b/server/core/filter.c index 99525f7f6..3b3ef4dd6 100644 --- a/server/core/filter.c +++ b/server/core/filter.c @@ -318,6 +318,9 @@ filterApply(FILTER_DEF *filter, SESSION *session, DOWNSTREAM *downstream) { DOWNSTREAM *me; + if (filter == NULL) + return NULL; + if (filter->obj == NULL) { /* Filter not yet loaded */ diff --git a/server/core/session.c b/server/core/session.c index 62e1015e4..db59f9e6e 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -333,13 +333,15 @@ bool session_free( { for (i = 0; i < session->n_filters; i++) { - session->filters[i].filter->obj->closeSession( + if (session->filters[i].filter) + session->filters[i].filter->obj->closeSession( session->filters[i].instance, session->filters[i].session); } for (i = 0; i < session->n_filters; i++) { - session->filters[i].filter->obj->freeSession( + if (session->filters[i].filter) + session->filters[i].filter->obj->freeSession( session->filters[i].instance, session->filters[i].session); } @@ -653,6 +655,14 @@ int i; session->n_filters = service->n_filters; for (i = service->n_filters - 1; i >= 0; i--) { + if (service->filters[i] == NULL) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Service '%s' contians an unresolved filter.\n", + service->name))); + return 0; + } if ((head = filterApply(service->filters[i], session, &session->head)) == NULL) { From 493feb49ba2dda1c53c9fc1055cae75e9118fe97 Mon Sep 17 00:00:00 2001 From: Mark Riddoch Date: Fri, 22 Aug 2014 14:46:26 +0100 Subject: [PATCH 08/17] Fix for bug 410 - MaxScale.cnf server option is not parsed for spaces --- server/core/config.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/server/core/config.c b/server/core/config.c index f2c9185dc..1a8689bfb 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -65,6 +65,29 @@ static char *config_file = NULL; static GATEWAY_CONF gateway; char *version_string = NULL; + +/** + * Trim whitespace from the front and rear of a string + * + * @param str String to trim + * @return Trimmed string, changes are done in situ + */ +static char * +trim(char *str) +{ +char *ptr; + + while (isspace(*str)) + str++; + + /* Point to last character of the string */ + ptr = str + strlen(str) - 1; + while (ptr > str && isspace(*ptr)) + *ptr-- = 0; + + return str; +} + /** * Config item handler for the ini file reader * @@ -508,7 +531,7 @@ int error_count = 0; CONFIG_CONTEXT *obj1 = context; while (obj1) { - if (strcmp(s, obj1->object) == 0 && + if (strcmp(trim(s), obj1->object) == 0 && obj->element && obj1->element) { serviceAddBackend( From 531d8d7b47cefc1abe9df60a5da044a232a6a2a4 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Fri, 29 Aug 2014 10:08:48 +0300 Subject: [PATCH 09/17] query_classifier.cc: added detection for CREATE TEMPORARY TABLE and setting a new query type QUERY_TYPE_CREATE_TMP_TABLE for it. query_classifier.h: added QUERY_TYPE_CREATE_TMP_TABLE and QUERY_TYPE_READ_TMP_TABLE for use of temporary table support. hashtable.c:Added variant of hashtable which is 'flat', that is, stored to existing memory instead of allocating memory as a part of the call. Existing function declarations don't change but added hashtable_alloc_flat for the purpose. Both hashtable_alloc and hashtable_alloc_flat now call the real allocation function, hashtable_alloc_real. hashtable_free only frees memory which is allocated in hashtable_alloc_real. hashtable.h: added a flag to HASHTABLE struct to indicate whether hashtable owns its memory or not. readwritesplit.h: Added RSES_PROP_TYPE_TMPTABLES property type to be used for keeping the hashtable for tablenames. readwritesplit.c: Added comments about temporary table support implementation. --- query_classifier/query_classifier.cc | 42 +++++++- query_classifier/query_classifier.h | 6 +- server/core/hashtable.c | 102 +++++++++++++++++- server/include/hashtable.h | 5 + server/modules/include/readwritesplit.h | 4 +- .../routing/readwritesplit/readwritesplit.c | 24 ++++- 6 files changed, 175 insertions(+), 8 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 889940e00..fe712008c 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -494,8 +494,15 @@ static skygw_query_type_t resolve_query_type( force_data_modify_op_replication) { type |= QUERY_TYPE_SESSION_WRITE; - } else { + } + else + { type |= QUERY_TYPE_WRITE; + + if (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE) + { + type |= QUERY_TYPE_CREATE_TMP_TABLE; + } } goto return_qtype; @@ -692,6 +699,17 @@ static skygw_query_type_t resolve_query_type( break; } } /**< for */ +#if defined(TEMPORARY_TABLES) + if ((skygw_query_type_t)type == QUERY_TYPE_READ) + { + /** + * Find out the database name and all tables the query + * uses. Create a hashvalue from each and if any of the + * values can be found from property's hashtable, set + * query type to QUERY_TYPE_READ_TMP_TABLE. + */ + } +#endif } /**< if */ return_qtype: qtype = (skygw_query_type_t)type; @@ -816,3 +834,25 @@ char* skygw_query_classifier_get_stmtname( return ((THD *)(mysql->thd))->lex->prepared_stmt_name.str; } + +/** + * Finds the head of the list of tables affected by the current select statement. + * @param thd Pointer to a valid thread descriptor structure + * @return Head of the TABLE_LIST chain or NULL in case of an error + */ +void* skygw_get_affected_tables(void* thdp) +{ + THD* thd = (THD*)thdp; + + if(thd == NULL || + thd->lex == NULL || + thd->lex->current_select == NULL) + { + ss_dassert(thd != NULL && + thd->lex != NULL && + thd->lex->current_select != NULL); + return NULL; + } + + return (void*)thd->lex->current_select->table_list.first; +} \ No newline at end of file diff --git a/query_classifier/query_classifier.h b/query_classifier/query_classifier.h index 31f9cf44e..52bfea4f2 100644 --- a/query_classifier/query_classifier.h +++ b/query_classifier/query_classifier.h @@ -43,7 +43,9 @@ typedef enum { QUERY_TYPE_COMMIT = 0x0200, /*< COMMIT */ QUERY_TYPE_PREPARE_NAMED_STMT = 0x0400, /*< Prepared stmt with name from user */ QUERY_TYPE_PREPARE_STMT = 0x0800, /*< Prepared stmt with id provided by server */ - QUERY_TYPE_EXEC_STMT = 0x1000 /*< Execute prepared statement */ + QUERY_TYPE_EXEC_STMT = 0x1000, /*< Execute prepared statement */ + QUERY_TYPE_CREATE_TMP_TABLE = 0x2000, /*< Create temporary table */ + QUERY_TYPE_READ_TMP_TABLE = 0x4000 /*< Read temporary table */ } skygw_query_type_t; #define QUERY_IS_TYPE(mask,type) ((mask & type) == type) @@ -60,6 +62,8 @@ skygw_query_type_t skygw_query_classifier_get_type( /** Free THD context and close MYSQL */ void skygw_query_classifier_free(MYSQL* mysql); char* skygw_query_classifier_get_stmtname(MYSQL* mysql); +void* skygw_get_affected_tables(void* thdp); + EXTERN_C_BLOCK_END diff --git a/server/core/hashtable.c b/server/core/hashtable.c index 50857bfec..b56f5d169 100644 --- a/server/core/hashtable.c +++ b/server/core/hashtable.c @@ -63,6 +63,10 @@ static void hashtable_read_lock(HASHTABLE *table); static void hashtable_read_unlock(HASHTABLE *table); static void hashtable_write_lock(HASHTABLE *table); static void hashtable_write_unlock(HASHTABLE *table); +static HASHTABLE *hashtable_alloc_real(HASHTABLE* target, + int size, + int (*hashfn)(), + int (*cmpfn)()); /** * Special null function used as default memory allfunctions in the hashtable @@ -81,11 +85,75 @@ nullfn(void *data) /** * Allocate a new hash table * + * @param target The address where hashtable is to be initialized, if NULL then allocate * @param size The size of the hash table * @param hashfn The user supplied hash function * @param cmpfn The user supplied key comparison function * @return The hashtable table */ +#if 1 +HASHTABLE * +hashtable_alloc(int size, int (*hashfn)(), int (*cmpfn)()) +{ + return hashtable_alloc_real(NULL, size, hashfn, cmpfn); +} + +HASHTABLE* hashtable_alloc_flat( + HASHTABLE* target, + int size, + int (*hashfn)(), + int (*cmpfn)()) +{ + return hashtable_alloc_real(target, size, hashfn, cmpfn); +} + +static HASHTABLE * +hashtable_alloc_real( + HASHTABLE* target, + int size, + int (*hashfn)(), + int (*cmpfn)()) +{ + HASHTABLE *rval; + + if (target == NULL) + { + if ((rval = malloc(sizeof(HASHTABLE))) == NULL) + return NULL; + rval->ht_isflat = false; + } + else + { + rval = target; + rval->ht_isflat = true; + } + +#if defined(SS_DEBUG) + rval->ht_chk_top = CHK_NUM_HASHTABLE; + rval->ht_chk_tail = CHK_NUM_HASHTABLE; +#endif + rval->hashsize = size; + rval->hashfn = hashfn; + rval->cmpfn = cmpfn; + rval->kcopyfn = nullfn; + rval->vcopyfn = nullfn; + rval->kfreefn = nullfn; + rval->vfreefn = nullfn; + rval->n_readers = 0; + rval->writelock = 0; + spinlock_init(&rval->spin); + if ((rval->entries = (HASHENTRIES **)calloc(size, sizeof(HASHENTRIES *))) == NULL) + { + free(rval); + return NULL; + } + memset(rval->entries, 0, size * sizeof(HASHENTRIES *)); + + return rval; +} + +#else + HASHTABLE * hashtable_alloc(int size, int (*hashfn)(), int (*cmpfn)()) { @@ -117,18 +185,49 @@ HASHTABLE *rval; return rval; } - +#endif /** * Delete an entire hash table * * @param table The hash table to delete */ +#if 1 + void hashtable_free(HASHTABLE *table) { int i; HASHENTRIES *entry, *ptr; + hashtable_write_lock(table); + for (i = 0; i < table->hashsize; i++) + { + entry = table->entries[i]; + while (entry) + { + ptr = entry->next; + table->kfreefn(entry->key); + table->vfreefn(entry->value); + free(entry); + entry = ptr; + } + } + free(table->entries); + + if (!table->ht_isflat) + { + free(table); + } +} + +#else + +void +hashtable_free(HASHTABLE *table) +{ + int i; + HASHENTRIES *entry, *ptr; + hashtable_write_lock(table); for (i = 0; i < table->hashsize; i++) { @@ -146,6 +245,7 @@ HASHENTRIES *entry, *ptr; free(table); } +#endif /** * Provide memory management functions to the hash table. This allows * function pointers to be registered that can make copies of the diff --git a/server/include/hashtable.h b/server/include/hashtable.h index 92bc71b8e..175bd5d32 100644 --- a/server/include/hashtable.h +++ b/server/include/hashtable.h @@ -84,12 +84,17 @@ typedef struct hashtable { SPINLOCK spin; /**< Internal spinlock for the hashtable */ int n_readers; /**< Number of clients reading the table */ int writelock; /**< The table is locked by a writer */ + bool ht_isflat; /**< Indicates whether hashtable is in stack or heap */ #if defined(SS_DEBUG) skygw_chk_t ht_chk_tail; #endif } HASHTABLE; extern HASHTABLE *hashtable_alloc(int, int (*hashfn)(), int (*cmpfn)()); +HASHTABLE *hashtable_alloc_flat(HASHTABLE* target, + int size, + int (*hashfn)(), + int (*cmpfn)()); /**< Allocate a hashtable */ extern void hashtable_memory_fns(HASHTABLE *, HASHMEMORYFN, HASHMEMORYFN, HASHMEMORYFN, HASHMEMORYFN); /**< Provide an interface to control key/value memory diff --git a/server/modules/include/readwritesplit.h b/server/modules/include/readwritesplit.h index 6fc639005..3530dc086 100644 --- a/server/modules/include/readwritesplit.h +++ b/server/modules/include/readwritesplit.h @@ -78,7 +78,8 @@ typedef enum rses_property_type_t { RSES_PROP_TYPE_UNDEFINED=-1, RSES_PROP_TYPE_SESCMD=0, RSES_PROP_TYPE_FIRST = RSES_PROP_TYPE_SESCMD, - RSES_PROP_TYPE_LAST=RSES_PROP_TYPE_SESCMD, + RSES_PROP_TYPE_TMPTABLES, + RSES_PROP_TYPE_LAST=RSES_PROP_TYPE_TMPTABLES, RSES_PROP_TYPE_COUNT=RSES_PROP_TYPE_LAST+1 } rses_property_type_t; @@ -143,6 +144,7 @@ struct rses_property_st { rses_property_type_t rses_prop_type; union rses_prop_data { mysql_sescmd_t sescmd; + HASHTABLE tmp_table_hash; void* placeholder; /*< to be removed due new type */ } rses_prop_data; rses_property_t* rses_prop_next; /*< next property of same type */ diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 1c22d4979..7fdd5d8ef 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1263,6 +1263,9 @@ static int routeQuery( ss_dassert(succp); goto return_ret; } + /** + * qtype is QUERY_TYPE_WRITE or QUERY_TYPE_READ_TMP_TABLE + */ else { bool succp = true; @@ -1287,7 +1290,15 @@ static int routeQuery( if (!rses_begin_locked_router_action(router_cli_ses)) { goto return_ret; - } + } +#if defined(TEMPORARY_TABLES) + /** + * If query is of type QUERY_TYPE_CREATE_TMP_TABLE then find out + * the database and table name, create a hashvalue and + * add it to the router client session's property. If property + * doesn't exist then create it first. + */ +#endif if (master_dcb == NULL) { @@ -2682,8 +2693,13 @@ static bool execute_sescmd_in_backend( sescmd_cursor_clone_querybuf(scur)); break; - case MYSQL_COM_QUERY: - case MYSQL_COM_INIT_DB: + case MYSQL_COM_INIT_DB: +#if defined(TEMPORARY_TABLES) + /** + * Record database name and store to session. + */ +#endif + case MYSQL_COM_QUERY: default: /** * Mark session command buffer, it triggers writing @@ -2969,7 +2985,7 @@ static bool route_session_write( rses_property_done(prop); succp = false; goto return_succp; - } + } /** * Additional reference is created to querybuf to * prevent it from being released before properties From 7629c455a63c56e428b581d2e9cb944f502204eb Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 30 Aug 2014 08:27:05 +0300 Subject: [PATCH 10/17] partial implementation --- query_classifier/query_classifier.cc | 110 +++++++++- query_classifier/query_classifier.h | 2 + server/modules/include/readwritesplit.h | 2 +- .../routing/readwritesplit/readwritesplit.c | 202 ++++++++++++++++-- 4 files changed, 292 insertions(+), 24 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 3f2c18eb4..14fa7ec44 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -736,15 +736,17 @@ static skygw_query_type_t resolve_query_type( } } /**< for */ #if defined(TEMPORARY_TABLES) - if ((skygw_query_type_t)type == QUERY_TYPE_READ) - { - /** - * Find out the database name and all tables the query - * uses. Create a hashvalue from each and if any of the - * values can be found from property's hashtable, set - * query type to QUERY_TYPE_READ_TMP_TABLE. - */ - } + if ((skygw_query_type_t)type == QUERY_TYPE_READ) + { + /** + * Find out the database name and all tables the query + * uses. Create a hashvalue from each and if any of the + * values can be found from property's hashtable, set + * query type to QUERY_TYPE_READ_TMP_TABLE. + */ + + + } #endif } /**< if */ return_qtype: @@ -874,8 +876,8 @@ char* skygw_query_classifier_get_stmtname( /** * Finds the head of the list of tables affected by the current select statement. - * @param thd Pointer to a valid thread descriptor structure - * @return Head of the TABLE_LIST chain or NULL in case of an error + * @param thd Pointer to a valid THD + * @return Pointer to the head of the TABLE_LIST chain or NULL in case of an error */ void* skygw_get_affected_tables(void* thdp) { @@ -893,6 +895,92 @@ void* skygw_get_affected_tables(void* thdp) return (void*)thd->lex->current_select->table_list.first; } + + +/** + * Reads the parsetree and lists all the affected tables and views in the query. + * In the case of an error, the size of the table is set to zero and no memory is allocated. + * The caller must free the allocated memory. + * + * @param querybuf GWBUF where the table names are extracted from + * @param tblsize Pointer where the number of tables is written + * @return Array of null-terminated strings with the table names + */ +char** skygw_get_table_names(GWBUF* querybuf,int* tblsize) +{ + parsing_info_t* pi; + MYSQL* mysql; + THD* thd; + TABLE_LIST* tbl; + SELECT_LEX*slx; + int i = 0, currtblsz = 0; + char**tables,**tmp; + + if (!GWBUF_IS_PARSED(querybuf)) + { + tables = NULL; + goto retblock; + } + pi = (parsing_info_t *)gwbuf_get_buffer_object_data(querybuf, + GWBUF_PARSING_INFO); + + if (pi == NULL) + { + tables = NULL; + goto retblock; + } + + if (pi->pi_query_plain_str == NULL || + (mysql = (MYSQL *)pi->pi_handle) == NULL || + (thd = (THD *)mysql->thd) == NULL) + { + ss_dassert(pi->pi_query_plain_str != NULL && + mysql != NULL && + thd != NULL); + tables = NULL; + goto retblock; + } + + thd->lex->current_select = thd->lex->all_selects_list; + + while(thd->lex->current_select){ + + tbl = (TABLE_LIST*)skygw_get_affected_tables(thd); + + while (tbl) + { + if(i >= currtblsz){ + + tmp = (char**)malloc(sizeof(char*)*(currtblsz*2+1)); + + if(tmp){ + if(currtblsz > 0){ + int x; + for(x = 0;xalias); + tbl=tbl->next_local; + } + thd->lex->current_select = thd->lex->current_select->next_select_in_list(); + } + + retblock: + *tblsize = i; + return tables; +} + + /* * Replace user-provided literals with question marks. Return a copy of the * querystr with replacements. diff --git a/query_classifier/query_classifier.h b/query_classifier/query_classifier.h index 6f5ecd803..595ba502b 100644 --- a/query_classifier/query_classifier.h +++ b/query_classifier/query_classifier.h @@ -73,6 +73,8 @@ skygw_query_type_t query_classifier_get_type(GWBUF* querybuf); /** Free THD context and close MYSQL */ char* skygw_query_classifier_get_stmtname(MYSQL* mysql); +void* skygw_get_affected_tables(void* thdp); +char** skygw_get_table_names(GWBUF* querybuf,int* tblsize); char* skygw_get_canonical(GWBUF* querybuf); bool parse_query (GWBUF* querybuf); parsing_info_t* parsing_info_init(void (*donefun)(void *)); diff --git a/server/modules/include/readwritesplit.h b/server/modules/include/readwritesplit.h index 3530dc086..f8024fde6 100644 --- a/server/modules/include/readwritesplit.h +++ b/server/modules/include/readwritesplit.h @@ -145,7 +145,7 @@ struct rses_property_st { union rses_prop_data { mysql_sescmd_t sescmd; HASHTABLE tmp_table_hash; - void* placeholder; /*< to be removed due new type */ + HASHTABLE* temp_tables; } rses_prop_data; rses_property_t* rses_prop_next; /*< next property of same type */ #if defined(SS_DEBUG) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index aa118f005..dbc99d801 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -273,6 +273,33 @@ static bool have_enough_servers( static SPINLOCK instlock; static ROUTER_INSTANCE* instances; +static int hashkeyfun(void* key); +static int hashcmpfun (void *, void *); + +static int hashkeyfun( + void* key) +{ + unsigned int hash = 0,c = 0; + char* ptr = (char*)key; + while((c = *ptr++)){ + hash = c + (hash << 6) + (hash << 16) - hash; + } + return *(int *)key; +} + +static int hashcmpfun( + void* v1, + void* v2) +{ + int i1; + int i2; + + i1 = *(int *)v1; + i2 = *(int *)v2; + + return (i1 < i2 ? -1 : (i1 > i2 ? 1 : 0)); +} + /** * Implementation of the mandatory version entry point * @@ -1034,12 +1061,21 @@ static int routeQuery( skygw_query_type_t qtype = QUERY_TYPE_UNKNOWN; mysql_server_cmd_t packet_type; uint8_t* packet; - int ret = 0; + int ret = 0, + tsize = 0, + klen = 0, + i = 0; DCB* master_dcb = NULL; DCB* slave_dcb = NULL; ROUTER_INSTANCE* inst = (ROUTER_INSTANCE *)instance; ROUTER_CLIENT_SES* router_cli_ses = (ROUTER_CLIENT_SES *)router_session; + rses_property_t* rses_prop; bool rses_is_closed = false; + bool target_tmp_table = false; + char *dbname,*hkey; + char** tbl; + HASHTABLE* h; + MYSQL_session* data; CHK_CLIENT_RSES(router_cli_ses); @@ -1053,6 +1089,7 @@ static int routeQuery( packet = GWBUF_DATA(querybuf); packet_type = packet[4]; + rses_prop = *router_cli_ses->rses_properties; if (rses_is_closed) { @@ -1083,6 +1120,9 @@ static int routeQuery( master_dcb = router_cli_ses->rses_master_ref->bref_dcb; CHK_DCB(master_dcb); + + data = (MYSQL_session*)master_dcb->session->data; + dbname = data->db; switch(packet_type) { case MYSQL_COM_QUIT: /*< 1 QUIT will close all sessions */ @@ -1128,6 +1168,51 @@ static int routeQuery( break; } /**< switch by packet type */ + + /** + *Check if the query targets a temporary table + */ + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_READ)){ + + + tbl = skygw_get_table_names(querybuf,&tsize); + + if(tsize > 0) + { /**Query targets at least one table*/ + for(i = 0;irses_prop_data.temp_tables){ + if((target_tmp_table = hashtable_fetch(rses_prop->rses_prop_data.temp_tables,(void *)hkey))) + { + /**Query target is a temporary table*/ + + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Query targets a temporary table: %s",hkey))); + } + } + + + + } + + for(i = 0;irses_transaction_active) + !router_cli_ses->rses_transaction_active && + !target_tmp_table) { bool succp; @@ -1263,19 +1349,98 @@ static int routeQuery( { goto return_ret; } -#if defined(TEMPORARY_TABLES) + + //#if defined(TEMPORARY_TABLES) + + if(!query_is_parsed(querybuf) && !parse_query(querybuf)){ + LOGIF(LE, (skygw_log_write( + LOGFILE_ERROR, + "Error: Unable to parse query."))); + } + qtype = query_classifier_get_type(querybuf); + + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE)){ + + + + bool is_temp = true; + + + if(rses_prop == NULL){ + if((rses_prop = + (rses_property_t*)calloc(1,sizeof(rses_property_t)))){ + rses_prop->rses_prop_rsession = router_cli_ses; + rses_prop->rses_prop_refcount = 1; + rses_prop->rses_prop_next = NULL; + rses_prop->rses_prop_type = RSES_PROP_TYPE_SESCMD; + *router_cli_ses->rses_properties = rses_prop; + } + + } + + if( rses_prop->rses_prop_data.temp_tables == NULL){ + + h = hashtable_alloc(10000, hashkeyfun, hashcmpfun); + + if(h){ + rses_prop->rses_prop_data.temp_tables = h; + } + + } + + tbl = skygw_get_table_names(querybuf,&tsize); + + if(tsize == 1 && tbl[0]) + { /**One valid table created*/ + + klen = strlen(dbname) + strlen(tbl[0]) + 2; + hkey = calloc(klen,sizeof(char)); + strcpy(hkey,dbname); + strcat(hkey,"."); + strcat(hkey,tbl[0]); + + if( + hashtable_add( + rses_prop->rses_prop_data.temp_tables, + (void *)hkey, + (void *)&is_temp + ) + == 0) /**Conflict in hash table*/ + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Temporary table conflict in hashtable: %s",hkey))); + } + char* retkey = hashtable_fetch( + rses_prop->rses_prop_data.temp_tables, + hkey); + if(retkey){ +LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Temporary table added: %s",retkey))); + } + } + + if(tsize > 0){ + for(i = 0;isession->data; + tmpbuf = scur->scmd_cur_cmd->my_sescmd_buf; + qlen = MYSQL_GET_PACKET_LEN((unsigned char*)tmpbuf->start); + strncpy(data->db,tmpbuf->start+5,qlen - 1); + + } + //#endif case MYSQL_COM_QUERY: default: /** From 45faa388774175d46d05285d51fd36832c45b553 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 30 Aug 2014 19:51:59 +0300 Subject: [PATCH 11/17] added temporary table detection for reads --- .../routing/readwritesplit/readwritesplit.c | 163 +++++++++--------- 1 file changed, 85 insertions(+), 78 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index dbc99d801..05afbf587 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -291,15 +291,26 @@ static int hashcmpfun( void* v1, void* v2) { - int i1; - int i2; + char* i1 = (char*) v1; + char* i2 = (char*) v2; - i1 = *(int *)v1; - i2 = *(int *)v2; - - return (i1 < i2 ? -1 : (i1 > i2 ? 1 : 0)); + return strcmp(i1,i2); } +static void* hstrdup(void* fval) +{ + char* str = (char*)fval; + return strdup(str); +} + + +static void* hfree(void* fval) +{ + free (fval); + return NULL; +} + + /** * Implementation of the mandatory version entry point * @@ -1069,7 +1080,7 @@ static int routeQuery( DCB* slave_dcb = NULL; ROUTER_INSTANCE* inst = (ROUTER_INSTANCE *)instance; ROUTER_CLIENT_SES* router_cli_ses = (ROUTER_CLIENT_SES *)router_session; - rses_property_t* rses_prop; + rses_property_t* rses_prop_tmp; bool rses_is_closed = false; bool target_tmp_table = false; char *dbname,*hkey; @@ -1089,7 +1100,7 @@ static int routeQuery( packet = GWBUF_DATA(querybuf); packet_type = packet[4]; - rses_prop = *router_cli_ses->rses_properties; + rses_prop_tmp = router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES]; if (rses_is_closed) { @@ -1172,9 +1183,9 @@ static int routeQuery( /** *Check if the query targets a temporary table */ + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_READ)){ - tbl = skygw_get_table_names(querybuf,&tsize); if(tsize > 0) @@ -1185,18 +1196,18 @@ static int routeQuery( strcpy(hkey,dbname); strcat(hkey,"."); strcat(hkey,tbl[0]); - if(rses_prop && rses_prop->rses_prop_data.temp_tables){ - if((target_tmp_table = hashtable_fetch(rses_prop->rses_prop_data.temp_tables,(void *)hkey))) + if(rses_prop_tmp && rses_prop_tmp->rses_prop_data.temp_tables){ + if((target_tmp_table = (bool)hashtable_fetch(rses_prop_tmp->rses_prop_data.temp_tables,(void *)hkey))) { + /**Query target is a temporary table*/ - - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, + qtype = QUERY_TYPE_READ_TMP_TABLE; + LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, "Query targets a temporary table: %s",hkey))); } } - + free(hkey); } @@ -1207,10 +1218,6 @@ static int routeQuery( } - - - - } /** @@ -1274,8 +1281,7 @@ static int routeQuery( goto return_ret; } else if (QUERY_IS_TYPE(qtype, QUERY_TYPE_READ) && - !router_cli_ses->rses_transaction_active && - !target_tmp_table) + !router_cli_ses->rses_transaction_active) { bool succp; @@ -1350,40 +1356,42 @@ static int routeQuery( goto return_ret; } - //#if defined(TEMPORARY_TABLES) - - if(!query_is_parsed(querybuf) && !parse_query(querybuf)){ - LOGIF(LE, (skygw_log_write( - LOGFILE_ERROR, - "Error: Unable to parse query."))); - } - qtype = query_classifier_get_type(querybuf); - if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE)){ - - - - bool is_temp = true; - + /** + * If query is of type QUERY_TYPE_CREATE_TMP_TABLE then find out + * the database and table name, create a hashvalue and + * add it to the router client session's property. If property + * doesn't exist then create it first. + */ - if(rses_prop == NULL){ - if((rses_prop = + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE)){ + + bool is_temp = true; + + if(rses_prop_tmp == NULL){ + if((rses_prop_tmp = (rses_property_t*)calloc(1,sizeof(rses_property_t)))){ - rses_prop->rses_prop_rsession = router_cli_ses; - rses_prop->rses_prop_refcount = 1; - rses_prop->rses_prop_next = NULL; - rses_prop->rses_prop_type = RSES_PROP_TYPE_SESCMD; - *router_cli_ses->rses_properties = rses_prop; + +#if defined(SS_DEBUG) + rses_prop_tmp->rses_prop_chk_top = CHK_NUM_ROUTER_PROPERTY; + rses_prop_tmp->rses_prop_chk_tail = CHK_NUM_ROUTER_PROPERTY; +#endif + + rses_prop_tmp->rses_prop_rsession = router_cli_ses; + rses_prop_tmp->rses_prop_refcount = 1; + rses_prop_tmp->rses_prop_next = NULL; + rses_prop_tmp->rses_prop_type = RSES_PROP_TYPE_TMPTABLES; + router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES] = rses_prop_tmp; } } - if( rses_prop->rses_prop_data.temp_tables == NULL){ + if( rses_prop_tmp->rses_prop_data.temp_tables == NULL){ - h = hashtable_alloc(10000, hashkeyfun, hashcmpfun); - + h = hashtable_alloc(7, hashkeyfun, hashcmpfun); + hashtable_memory_fns(h,hstrdup,NULL,hfree,NULL); if(h){ - rses_prop->rses_prop_data.temp_tables = h; + rses_prop_tmp->rses_prop_data.temp_tables = h; } } @@ -1393,32 +1401,34 @@ static int routeQuery( if(tsize == 1 && tbl[0]) { /**One valid table created*/ - klen = strlen(dbname) + strlen(tbl[0]) + 2; - hkey = calloc(klen,sizeof(char)); - strcpy(hkey,dbname); - strcat(hkey,"."); - strcat(hkey,tbl[0]); + klen = strlen(dbname) + strlen(tbl[0]) + 2; + hkey = calloc(klen,sizeof(char)); + strcpy(hkey,dbname); + strcat(hkey,"."); + strcat(hkey,tbl[0]); - if( - hashtable_add( - rses_prop->rses_prop_data.temp_tables, - (void *)hkey, - (void *)&is_temp - ) - == 0) /**Conflict in hash table*/ - { + if( + hashtable_add( + rses_prop_tmp->rses_prop_data.temp_tables, + (void *)hkey, + (void *)is_temp + ) + == 0) /**Conflict in hash table*/ + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Temporary table conflict in hashtable: %s",hkey))); + } +#if defined(SS_DEBUG) + bool retkey = hashtable_fetch( + rses_prop_tmp->rses_prop_data.temp_tables, + hkey); + if(retkey){ LOGIF(LT, (skygw_log_write( LOGFILE_TRACE, - "Temporary table conflict in hashtable: %s",hkey))); + "Temporary table added: %s",hkey))); } - char* retkey = hashtable_fetch( - rses_prop->rses_prop_data.temp_tables, - hkey); - if(retkey){ -LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Temporary table added: %s",retkey))); - } +#endif } if(tsize > 0){ @@ -1426,16 +1436,9 @@ LOGIF(LT, (skygw_log_write( free(tbl[i]); } free(tbl); + free(hkey); } } - /** - * If query is of type QUERY_TYPE_CREATE_TMP_TABLE then find out - * the database and table name, create a hashvalue and - * add it to the router client session's property. If property - * doesn't exist then create it first. - */ - - //#endif if (master_dcb == NULL) { @@ -2436,6 +2439,9 @@ static void rses_property_done( case RSES_PROP_TYPE_SESCMD: mysql_sescmd_done(&prop->rses_prop_data.sescmd); break; + case RSES_PROP_TYPE_TMPTABLES: + hashtable_free(prop->rses_prop_data.temp_tables); + break; default: LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, @@ -2836,7 +2842,7 @@ static bool execute_sescmd_in_backend( break; case MYSQL_COM_INIT_DB: - //#if defined(TEMPORARY_TABLES) + /** * Record database name and store to session. */ @@ -2850,10 +2856,11 @@ static bool execute_sescmd_in_backend( data = dcb->session->data; tmpbuf = scur->scmd_cur_cmd->my_sescmd_buf; qlen = MYSQL_GET_PACKET_LEN((unsigned char*)tmpbuf->start); + memset(data->db,0,MYSQL_DATABASE_MAXLEN+1); strncpy(data->db,tmpbuf->start+5,qlen - 1); } - //#endif + case MYSQL_COM_QUERY: default: /** From ecc89a823b917b4a160abb05d8dacdf87eec8078 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sun, 31 Aug 2014 19:30:00 +0300 Subject: [PATCH 12/17] added tests for temporary tables --- .../routing/readwritesplit/test/rwsplit.sh | 26 ++++++++++++++++--- .../test/test_temporary_table.sql | 5 ++++ 2 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 server/modules/routing/readwritesplit/test/test_temporary_table.sql diff --git a/server/modules/routing/readwritesplit/test/rwsplit.sh b/server/modules/routing/readwritesplit/test/rwsplit.sh index c2e403b16..f55960009 100755 --- a/server/modules/routing/readwritesplit/test/rwsplit.sh +++ b/server/modules/routing/readwritesplit/test/rwsplit.sh @@ -230,6 +230,16 @@ if [ "$a" != "$TRETVAL" ]; then else echo "$TINPUT PASSED">>$TLOG ; fi + +TINPUT=test_temporary_table.sql +a=`$RUNCMD < ./$TINPUT` +TRETVAL=1 +if [ "$a" != "$TRETVAL" ]; then + echo "$TINPUT FAILED, return value $a when $TRETVAL was expected">>$TLOG; +else + echo "$TINPUT PASSED">>$TLOG ; +fi + echo "-----------------------------------" >> $TLOG echo "Session variables: Stress Test 1" >> $TLOG echo "-----------------------------------" >> $TLOG @@ -238,6 +248,10 @@ RUNCMD=mysql\ --host=$THOST\ -P$TPORT\ -u$TUSER\ -p$TPWD\ --unbuffered=true\ --d TINPUT=test_sescmd2.sql for ((i = 0;i<1000;i++)) do + if [[ $(( i % 50 )) -eq 0 ]] + then + printf "." + fi a=`$RUNCMD < $TINPUT 2>&1` if [[ "`echo "$a"|grep -i 'error'`" != "" ]] then @@ -255,15 +269,19 @@ fi echo "-----------------------------------" >> $TLOG echo "Session variables: Stress Test 2" >> $TLOG echo "-----------------------------------" >> $TLOG - +echo "" err="" TINPUT=test_sescmd3.sql for ((j = 0;j<1000;j++)) do - b=`$RUNCMD < $TINPUT 2>&1` - if [[ "`echo "$b"|grep -i 'null'`" != "" ]] + if [[ $(( j % 50 )) -eq 0 ]] then - err=`echo "$b" | grep -i null` + printf "." + fi + b=`$RUNCMD < $TINPUT 2>&1` + if [[ "`echo "$b"|grep -i 'null|error'`" != "" ]] + then + err=`echo "$b" | grep -i null|error` break fi done diff --git a/server/modules/routing/readwritesplit/test/test_temporary_table.sql b/server/modules/routing/readwritesplit/test/test_temporary_table.sql new file mode 100644 index 000000000..374510389 --- /dev/null +++ b/server/modules/routing/readwritesplit/test/test_temporary_table.sql @@ -0,0 +1,5 @@ +use test; +drop table if exists t1; +create temporary table t1 (id integer); +insert into t1 values(1); +select id from t1; From 58e8c05c8a035fb1d73660adef0a83dace4671e2 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sun, 31 Aug 2014 20:19:47 +0300 Subject: [PATCH 13/17] added detection of drop table targeting a temporary table --- query_classifier/query_classifier.cc | 6 +++ query_classifier/query_classifier.h | 3 +- .../routing/readwritesplit/readwritesplit.c | 50 ++++++++++++------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 14fa7ec44..92931e7ab 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -540,7 +540,13 @@ static skygw_query_type_t resolve_query_type( { type |= QUERY_TYPE_CREATE_TMP_TABLE; } + } + + if(lex->sql_command == SQLCOM_DROP_TABLE) + { + type |= QUERY_TYPE_DROP_TABLE; + } goto return_qtype; } diff --git a/query_classifier/query_classifier.h b/query_classifier/query_classifier.h index 595ba502b..86d5033d4 100644 --- a/query_classifier/query_classifier.h +++ b/query_classifier/query_classifier.h @@ -46,7 +46,8 @@ typedef enum { QUERY_TYPE_PREPARE_STMT = 0x0800, /*< Prepared stmt with id provided by server */ QUERY_TYPE_EXEC_STMT = 0x1000, /*< Execute prepared statement */ QUERY_TYPE_CREATE_TMP_TABLE = 0x2000, /*< Create temporary table */ - QUERY_TYPE_READ_TMP_TABLE = 0x4000 /*< Read temporary table */ + QUERY_TYPE_READ_TMP_TABLE = 0x4000, /*< Read temporary table */ + QUERY_TYPE_DROP_TABLE = 0x8000 } skygw_query_type_t; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 05afbf587..72fe0c03a 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1364,6 +1364,31 @@ static int routeQuery( * doesn't exist then create it first. */ + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_DROP_TABLE)){ + + tbl = skygw_get_table_names(querybuf,&tsize); + + if(tsize == 1 && tbl[0]) + { /**One valid table created*/ + + klen = strlen(dbname) + strlen(tbl[0]) + 2; + hkey = calloc(klen,sizeof(char)); + strcpy(hkey,dbname); + strcat(hkey,"."); + strcat(hkey,tbl[0]); + } + + + if(tsize > 0){ + for(i = 0;irses_prop_data.temp_tables, @@ -1431,15 +1445,15 @@ static int routeQuery( #endif } - if(tsize > 0){ - for(i = 0;irses_prop_data.temp_tables){ + hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey); } } - + + free(hkey); + if (master_dcb == NULL) { succp = get_dcb(&master_dcb, router_cli_ses, BE_MASTER); From bc939501e9cf567d93b0b40dab3b7897129cb14a Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 1 Sep 2014 10:11:04 +0300 Subject: [PATCH 14/17] minor bugfix to memory allocations --- .../routing/readwritesplit/readwritesplit.c | 118 +++++++++--------- 1 file changed, 62 insertions(+), 56 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 72fe0c03a..ba4f70489 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1377,83 +1377,89 @@ static int routeQuery( strcpy(hkey,dbname); strcat(hkey,"."); strcat(hkey,tbl[0]); - } + } - if(tsize > 0){ - for(i = 0;irses_prop_chk_top = CHK_NUM_ROUTER_PROPERTY; - rses_prop_tmp->rses_prop_chk_tail = CHK_NUM_ROUTER_PROPERTY; + rses_prop_tmp->rses_prop_chk_top = CHK_NUM_ROUTER_PROPERTY; + rses_prop_tmp->rses_prop_chk_tail = CHK_NUM_ROUTER_PROPERTY; #endif - rses_prop_tmp->rses_prop_rsession = router_cli_ses; - rses_prop_tmp->rses_prop_refcount = 1; - rses_prop_tmp->rses_prop_next = NULL; - rses_prop_tmp->rses_prop_type = RSES_PROP_TYPE_TMPTABLES; - router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES] = rses_prop_tmp; + rses_prop_tmp->rses_prop_rsession = router_cli_ses; + rses_prop_tmp->rses_prop_refcount = 1; + rses_prop_tmp->rses_prop_next = NULL; + rses_prop_tmp->rses_prop_type = RSES_PROP_TYPE_TMPTABLES; + router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES] = rses_prop_tmp; + } + } - } + if( rses_prop_tmp->rses_prop_data.temp_tables == NULL){ - if( rses_prop_tmp->rses_prop_data.temp_tables == NULL){ - - h = hashtable_alloc(7, hashkeyfun, hashcmpfun); - hashtable_memory_fns(h,hstrdup,NULL,hfree,NULL); - if(h){ - rses_prop_tmp->rses_prop_data.temp_tables = h; + h = hashtable_alloc(7, hashkeyfun, hashcmpfun); + hashtable_memory_fns(h,hstrdup,NULL,hfree,NULL); + if(h){ + rses_prop_tmp->rses_prop_data.temp_tables = h; + } + } - } - - if( - hashtable_add( - rses_prop_tmp->rses_prop_data.temp_tables, - (void *)hkey, - (void *)is_temp - ) - == 0) /**Conflict in hash table*/ - { - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Temporary table conflict in hashtable: %s",hkey))); - } -#if defined(SS_DEBUG) - bool retkey = hashtable_fetch( - rses_prop_tmp->rses_prop_data.temp_tables, - hkey); - if(retkey){ + if( + hashtable_add( + rses_prop_tmp->rses_prop_data.temp_tables, + (void *)hkey, + (void *)is_temp + ) + == 0) /**Conflict in hash table*/ + { LOGIF(LT, (skygw_log_write( LOGFILE_TRACE, - "Temporary table added: %s",hkey))); + "Temporary table conflict in hashtable: %s",hkey))); } + +#if defined(SS_DEBUG) + bool retkey = hashtable_fetch( + rses_prop_tmp->rses_prop_data.temp_tables, + hkey); + if(retkey){ + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Temporary table added: %s",hkey))); + } #endif + + } + + /**Check if DROP TABLE... targets a temporary table*/ + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_DROP_TABLE)) + { + if(rses_prop_tmp && rses_prop_tmp->rses_prop_data.temp_tables) + { + hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey); + } + } + + free(hkey); + + if(tsize > 0) + { + for(i = 0;irses_prop_data.temp_tables){ - hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey); - } } - free(hkey); - if (master_dcb == NULL) { succp = get_dcb(&master_dcb, router_cli_ses, BE_MASTER); From 067ce3c8866003f1889a717aa6e6d15e51c7119b Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 1 Sep 2014 10:35:38 +0300 Subject: [PATCH 15/17] removed unneeded QUERY_TYPE_DROP_TABLE type from query_classifier.h --- query_classifier/query_classifier.cc | 17 ----------------- query_classifier/query_classifier.h | 1 - .../routing/readwritesplit/readwritesplit.c | 4 ++-- 3 files changed, 2 insertions(+), 20 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 92931e7ab..1e8916208 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -542,11 +542,6 @@ static skygw_query_type_t resolve_query_type( } } - - if(lex->sql_command == SQLCOM_DROP_TABLE) - { - type |= QUERY_TYPE_DROP_TABLE; - } goto return_qtype; } @@ -741,19 +736,7 @@ static skygw_query_type_t resolve_query_type( break; } } /**< for */ -#if defined(TEMPORARY_TABLES) - if ((skygw_query_type_t)type == QUERY_TYPE_READ) - { - /** - * Find out the database name and all tables the query - * uses. Create a hashvalue from each and if any of the - * values can be found from property's hashtable, set - * query type to QUERY_TYPE_READ_TMP_TABLE. - */ - - } -#endif } /**< if */ return_qtype: qtype = (skygw_query_type_t)type; diff --git a/query_classifier/query_classifier.h b/query_classifier/query_classifier.h index 86d5033d4..a0dccf988 100644 --- a/query_classifier/query_classifier.h +++ b/query_classifier/query_classifier.h @@ -47,7 +47,6 @@ typedef enum { QUERY_TYPE_EXEC_STMT = 0x1000, /*< Execute prepared statement */ QUERY_TYPE_CREATE_TMP_TABLE = 0x2000, /*< Create temporary table */ QUERY_TYPE_READ_TMP_TABLE = 0x4000, /*< Read temporary table */ - QUERY_TYPE_DROP_TABLE = 0x8000 } skygw_query_type_t; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index ba4f70489..10d738d6c 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1365,7 +1365,7 @@ static int routeQuery( */ if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_DROP_TABLE)){ + packet_type == MYSQL_COM_DROP_DB){ tbl = skygw_get_table_names(querybuf,&tsize); @@ -1439,7 +1439,7 @@ static int routeQuery( } /**Check if DROP TABLE... targets a temporary table*/ - if(QUERY_IS_TYPE(qtype, QUERY_TYPE_DROP_TABLE)) + if(packet_type == MYSQL_COM_DROP_DB) { if(rses_prop_tmp && rses_prop_tmp->rses_prop_data.temp_tables) { From 164d8b1e32fed38031eb9ffd85d45b70425c15e0 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 1 Sep 2014 13:40:52 +0300 Subject: [PATCH 16/17] Fixed various memory leaks dbuser.c: key.user value was never freed skygw_utils.cc: replace_literal values were not always freed --- server/core/dbusers.c | 2 +- utils/skygw_utils.cc | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/core/dbusers.c b/server/core/dbusers.c index dd36d683c..2e4dcb18b 100644 --- a/server/core/dbusers.c +++ b/server/core/dbusers.c @@ -388,7 +388,7 @@ getUsers(SERVICE *service, struct users *users) memcpy(users->cksum, hash, SHA_DIGEST_LENGTH); free(users_data); - + free(key.user); mysql_free_result(result); mysql_close(con); mysql_thread_end(); diff --git a/utils/skygw_utils.cc b/utils/skygw_utils.cc index 8297ca5ba..dab614f25 100644 --- a/utils/skygw_utils.cc +++ b/utils/skygw_utils.cc @@ -1912,6 +1912,7 @@ char* replace_literal( fprintf(stderr, "Regex memory allocation failed : %s\n", strerror(errno)); free(search_re); + free(newstr); newstr = haystack; goto retblock; } @@ -1928,6 +1929,7 @@ char* replace_literal( search_re, error_message); free(search_re); + free(newstr); newstr = haystack; goto retblock; } @@ -1936,6 +1938,7 @@ char* replace_literal( if (rc != 0) { free(search_re); + free(newstr); regfree(&re); newstr = haystack; goto retblock; @@ -1947,6 +1950,7 @@ char* replace_literal( regfree(&re); free(haystack); + free(search_re); retblock: return newstr; } From 52f3adbf20780978dd3e6da1c0c7782e830bebbf Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 1 Sep 2014 19:50:25 +0300 Subject: [PATCH 17/17] fixed temporary tables looking for database drops instead of table drops --- query_classifier/query_classifier.cc | 76 ++++++++++++++- query_classifier/query_classifier.h | 2 + .../routing/readwritesplit/readwritesplit.c | 96 ++++++++++--------- 3 files changed, 127 insertions(+), 47 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index d722c2b16..4e3befaa3 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -544,7 +544,8 @@ static skygw_query_type_t resolve_query_type( { type |= QUERY_TYPE_WRITE; - if (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE) + if (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE && + lex->sql_command == SQLCOM_CREATE_TABLE) { type |= QUERY_TYPE_CREATE_TMP_TABLE; } @@ -870,7 +871,6 @@ char* skygw_query_classifier_get_stmtname( } /** - * Finds the head of the list of tables affected by the current select statement. * @param thd Pointer to a valid THD * @return Pointer to the head of the TABLE_LIST chain or NULL in case of an error @@ -908,7 +908,6 @@ char** skygw_get_table_names(GWBUF* querybuf,int* tblsize) MYSQL* mysql; THD* thd; TABLE_LIST* tbl; - SELECT_LEX*slx; int i = 0, currtblsz = 0; char**tables,**tmp; @@ -975,7 +974,78 @@ char** skygw_get_table_names(GWBUF* querybuf,int* tblsize) *tblsize = i; return tables; } +/** + * Extract the name of the created table. + * @param querybuf Buffer to use. + * @return A pointer to the name if a table was created, otherwise NULL + */ +char* skygw_get_created_table_name(GWBUF* querybuf) +{ + parsing_info_t* pi; + MYSQL* mysql; + THD* thd; + if (!GWBUF_IS_PARSED(querybuf)) + { + return NULL; + } + pi = (parsing_info_t *)gwbuf_get_buffer_object_data(querybuf, + GWBUF_PARSING_INFO); + + if (pi == NULL) + { + return NULL; + } + + if ((mysql = (MYSQL *)pi->pi_handle) == NULL || + (thd = (THD *)mysql->thd) == NULL) + { + ss_dassert(mysql != NULL && + thd != NULL); + return NULL; + } + + if(thd->lex->create_last_non_select_table && + thd->lex->create_last_non_select_table->table_name){ + char* name = strdup(thd->lex->create_last_non_select_table->table_name); + return name; + }else{ + return NULL; + } + +} +/** + * Checks whether the buffer contains a DROP TABLE... query. + * @param querybuf Buffer to inspect + * @return true if it contains the query otherwise false + */ +bool is_drop_table_query(GWBUF* querybuf) +{ + parsing_info_t* pi; + MYSQL* mysql; + THD* thd; + + if (!GWBUF_IS_PARSED(querybuf)) + { + return false; + } + pi = (parsing_info_t *)gwbuf_get_buffer_object_data(querybuf, + GWBUF_PARSING_INFO); + if (pi == NULL) + { + return false; + } + + if ((mysql = (MYSQL *)pi->pi_handle) == NULL || + (thd = (THD *)mysql->thd) == NULL) + { + ss_dassert(mysql != NULL && + thd != NULL); + return false; + } + + return thd->lex->sql_command == SQLCOM_DROP_TABLE; +} /* * Replace user-provided literals with question marks. Return a copy of the diff --git a/query_classifier/query_classifier.h b/query_classifier/query_classifier.h index 10098ff14..ccf08a6ea 100644 --- a/query_classifier/query_classifier.h +++ b/query_classifier/query_classifier.h @@ -74,6 +74,8 @@ skygw_query_type_t query_classifier_get_type(GWBUF* querybuf); /** Free THD context and close MYSQL */ char* skygw_query_classifier_get_stmtname(MYSQL* mysql); +char* skygw_get_created_table_name(GWBUF* querybuf); +bool is_drop_table_query(GWBUF* querybuf); void* skygw_get_affected_tables(void* thdp); char** skygw_get_table_names(GWBUF* querybuf,int* tblsize); char* skygw_get_canonical(GWBUF* querybuf); diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index aca14218d..93fd3fd36 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -287,6 +287,9 @@ static int hashcmpfun (void *, void *); static int hashkeyfun( void* key) { + if(key == NULL){ + return 0; + } unsigned int hash = 0,c = 0; char* ptr = (char*)key; while((c = *ptr++)){ @@ -1246,7 +1249,6 @@ static int routeQuery( HASHTABLE* h; MYSQL_session* data; size_t len; - MYSQL* mysql = NULL; route_target_t route_target; @@ -1541,40 +1543,35 @@ static int routeQuery( } target_dcb = master_dcb; } - /** Lock router session */ - /*if (!rses_begin_locked_router_action(router_cli_ses)) - { - goto return_ret; - } */ + /** * If query is of type QUERY_TYPE_CREATE_TMP_TABLE then find out * the database and table name, create a hashvalue and * add it to the router client session's property. If property - * doesn't exist then create it first. + * doesn't exist then create it first. If the query is DROP TABLE... + * then see if it targets a temporary table and remove it from the hashtable + * if it does. */ - - if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE) || - packet_type == MYSQL_COM_DROP_DB){ - - tbl = skygw_get_table_names(querybuf,&tsize); - - if(tsize == 1 && tbl[0]) - { /**One valid table created*/ - - klen = strlen(dbname) + strlen(tbl[0]) + 2; + + if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE)){ + + bool is_temp = true; + char* tblname = NULL; + + tblname = skygw_get_created_table_name(querybuf); + if(tblname && strlen(tblname) > 0){ + klen = strlen(dbname) + strlen(tblname) + 2; hkey = calloc(klen,sizeof(char)); strcpy(hkey,dbname); strcat(hkey,"."); - strcat(hkey,tbl[0]); + strcat(hkey,tblname); + }else{ + hkey = NULL; + } - } - if(QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE)){ - - bool is_temp = true; - if(rses_prop_tmp == NULL){ if((rses_prop_tmp = (rses_property_t*)calloc(1,sizeof(rses_property_t)))){ @@ -1603,7 +1600,7 @@ static int routeQuery( } - if( + if( hkey && hashtable_add( rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey, @@ -1616,40 +1613,51 @@ static int routeQuery( "Temporary table conflict in hashtable: %s",hkey))); } + #if defined(SS_DEBUG) - bool retkey = hashtable_fetch( - rses_prop_tmp->rses_prop_data.temp_tables, + bool retkey = hashtable_fetch(rses_prop_tmp->rses_prop_data.temp_tables, hkey); - if(retkey){ - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Temporary table added: %s",hkey))); - } + if(retkey) + { + LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, + "Temporary table added: %s",hkey))); + } #endif - + free(hkey); } /**Check if DROP TABLE... targets a temporary table*/ - if(packet_type == MYSQL_COM_DROP_DB) + if(is_drop_table_query(querybuf)) { - if(rses_prop_tmp && rses_prop_tmp->rses_prop_data.temp_tables) - { - hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey); - } - } - - free(hkey); - if(tsize > 0) - { + tbl = skygw_get_table_names(querybuf,&tsize); + for(i = 0;irses_prop_data.temp_tables) + { + if(hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, (void *)hkey)) + { + LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, + "Temporary table dropped: %s",hkey))); + } + } + free(tbl[i]); + free(hkey); } free(tbl); - } - } + } + + if (master_dcb == NULL) {