From cb93db4647790f2014f17155f28cf32082025275 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 25 Nov 2016 23:18:33 +0200 Subject: [PATCH 01/19] Make a copy of the client request If the request isn't copied, the buffer will be freed twice. --- server/modules/routing/avro/avro_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/routing/avro/avro_client.c b/server/modules/routing/avro/avro_client.c index 6ab00117a..a26047fdf 100644 --- a/server/modules/routing/avro/avro_client.c +++ b/server/modules/routing/avro/avro_client.c @@ -505,7 +505,7 @@ avro_client_process_command(AVRO_INSTANCE *router, AVRO_CLIENT *client, GWBUF *q { GWBUF *reply = gwbuf_alloc(5); memcpy(GWBUF_DATA(reply), "ECHO:", 5); - reply = gwbuf_append(reply, queue); + reply = gwbuf_append(reply, gwbuf_clone(queue)); client->dcb->func.write(client->dcb, reply); } } From 2fe13719bcc765c97186c5a4776deb993813a247 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 28 Nov 2016 10:45:34 +0200 Subject: [PATCH 02/19] MXS-1027: Add Upstart config file Various older systems use Upstart to control services. MaxScale should provide both init.d scripts and Upstart configurations for systems that don't support systemd. --- CMakeLists.txt | 5 +++++ etc/postinst.in | 5 +++++ etc/upstart/maxscale.conf.in | 26 ++++++++++++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 etc/upstart/maxscale.conf.in diff --git a/CMakeLists.txt b/CMakeLists.txt index c61ea96c6..8945e1952 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -169,6 +169,7 @@ configure_file(${CMAKE_SOURCE_DIR}/server/include/adminusers.h.in ${CMAKE_BINARY configure_file(${CMAKE_SOURCE_DIR}/server/test/maxscale_test.h.in ${CMAKE_BINARY_DIR}/server/include/maxscale_test.h @ONLY) configure_file(${CMAKE_SOURCE_DIR}/etc/postinst.in ${CMAKE_BINARY_DIR}/postinst @ONLY) configure_file(${CMAKE_SOURCE_DIR}/etc/postrm.in ${CMAKE_BINARY_DIR}/postrm @ONLY) +configure_file(${CMAKE_SOURCE_DIR}/etc/upstart/maxscale.conf.in ${CMAKE_BINARY_DIR}/upstart/maxscale.conf @ONLY) configure_file(${CMAKE_SOURCE_DIR}/server/test/maxscale_test.cnf ${CMAKE_BINARY_DIR}/maxscale.cnf @ONLY) set(FLAGS "-Wall -Wno-unused-variable -Wno-unused-function -Werror -fPIC" CACHE STRING "Compilation flags") @@ -284,16 +285,20 @@ if(WITH_SCRIPTS) message(STATUS "maxscale.conf will unpack to: /etc/ld.so.conf.d") message(STATUS "startup scripts will unpack to to: /etc/init.d") message(STATUS "systemd service files will unpack to to: /usr/lib/systemd/system") + message(STATUS "upstart files will unpack to: /etc/init/") install(PROGRAMS ${CMAKE_BINARY_DIR}/maxscale DESTINATION ${MAXSCALE_SHAREDIR} ) install(FILES ${CMAKE_BINARY_DIR}/maxscale.conf DESTINATION ${MAXSCALE_SHAREDIR}) install(FILES ${CMAKE_BINARY_DIR}/maxscale.service DESTINATION ${MAXSCALE_SHAREDIR}) + install(FILES ${CMAKE_BINARY_DIR}/upstart/maxscale.conf DESTINATION ${MAXSCALE_SHAREDIR}/upstart/) else() install(PROGRAMS ${CMAKE_BINARY_DIR}/maxscale DESTINATION /etc/init.d) install(FILES ${CMAKE_BINARY_DIR}/maxscale.conf DESTINATION /etc/ld.so.conf.d) install(FILES ${CMAKE_BINARY_DIR}/maxscale.service DESTINATION /usr/lib/systemd/system) + install(FILES ${CMAKE_BINARY_DIR}/upstart/maxscale.conf DESTINATION /etc/init/) message(STATUS "Installing maxscale.conf to: /etc/ld.so.conf.d") message(STATUS "Installing startup scripts to: /etc/init.d") message(STATUS "Installing systemd service files to: /usr/lib/systemd/system") + message(STATUS "Installing upstart files to: /etc/init/") endif() endif() diff --git a/etc/postinst.in b/etc/postinst.in index ed155b2b1..91c0e09c8 100755 --- a/etc/postinst.in +++ b/etc/postinst.in @@ -50,6 +50,11 @@ then systemctl daemon-reload fi else + if [ -d "/etc/init/" ] + then + cp @CMAKE_INSTALL_PREFIX@/@MAXSCALE_SHAREDIR@/upstart/maxscale.conf /etc/init/ + fi + # If systemd is not present, use init.d scripts if [ -f "@CMAKE_INSTALL_PREFIX@/@MAXSCALE_SHAREDIR@/maxscale" ] then diff --git a/etc/upstart/maxscale.conf.in b/etc/upstart/maxscale.conf.in new file mode 100644 index 000000000..bbbc89b5f --- /dev/null +++ b/etc/upstart/maxscale.conf.in @@ -0,0 +1,26 @@ +# MaxScale service + +description "MariaDB MaxScale" + +start on stopped rc RUNLEVEL=[2345] +stop on starting rc runlevel [!2345] + +# Respawn the process on abnormal exits +respawn + +# Uncomment this to limit respawns to two every five minutes +# respawn limit 2 5 + +# Unlimited open files +limit nofile unlimited unlimited + +# Make sure /var/run/maxscale exists +pre-start exec /usr/bin/install -d -o maxscale -g maxscale @MAXSCALE_VARDIR@/run/maxscale + +# Change the user to maxscale:maxscale +setuid maxscale +setgid maxscale + +# Start MaxScale +expect fork +exec /usr/bin/maxscale From 9362954e82b3621cd77e64c4c1204c15360637b3 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 28 Nov 2016 22:22:09 +0200 Subject: [PATCH 03/19] Add newlines to avrorouter JSON output JSON does not have a concept of streams and a common way to stream JSON is to separate each JSON object with a newline. Adding a newline makes it easier to parse as JSON values do not natively contain newlines. --- server/modules/protocol/cdc.c | 4 ++-- server/modules/routing/avro/avro_client.c | 12 ++++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/server/modules/protocol/cdc.c b/server/modules/protocol/cdc.c index c1281cf26..728a622e9 100644 --- a/server/modules/protocol/cdc.c +++ b/server/modules/protocol/cdc.c @@ -455,7 +455,7 @@ cdc_protocol_done(DCB* dcb) static void write_auth_ack(DCB *dcb) { - dcb_printf(dcb, "OK"); + dcb_printf(dcb, "OK\n"); } /** @@ -467,6 +467,6 @@ write_auth_ack(DCB *dcb) static void write_auth_err(DCB *dcb) { - dcb_printf(dcb, "ERR, code 11, msg: abcd"); + dcb_printf(dcb, "ERROR: Authentication failed\n"); } diff --git a/server/modules/routing/avro/avro_client.c b/server/modules/routing/avro/avro_client.c index a26047fdf..e7873f953 100644 --- a/server/modules/routing/avro/avro_client.c +++ b/server/modules/routing/avro/avro_client.c @@ -80,7 +80,7 @@ avro_client_handle_request(AVRO_INSTANCE *router, AVRO_CLIENT *client, GWBUF *qu if (avro_client_do_registration(router, client, queue) == 0) { client->state = AVRO_CLIENT_ERRORED; - dcb_printf(client->dcb, "ERR, code 12, msg: Registration failed"); + dcb_printf(client->dcb, "ERR, code 12, msg: Registration failed\n"); /* force disconnection */ dcb_close(client->dcb); rval = 0; @@ -88,7 +88,7 @@ avro_client_handle_request(AVRO_INSTANCE *router, AVRO_CLIENT *client, GWBUF *qu else { /* Send OK ack to client */ - dcb_printf(client->dcb, "OK"); + dcb_printf(client->dcb, "OK\n"); client->state = AVRO_CLIENT_REGISTERED; MXS_INFO("%s: Client [%s] has completed REGISTRATION action", @@ -558,11 +558,15 @@ const char* get_avrofile_name(const char *file_ptr, int data_len, char *dest) static int send_row(DCB *dcb, json_t* row) { char *json = json_dumps(row, JSON_PRESERVE_ORDER); - GWBUF *buf; + size_t len = strlen(json); + GWBUF *buf = gwbuf_alloc(len + 1); int rc = 0; - if (json && (buf = gwbuf_alloc_and_load(strlen(json), (void*)json))) + if (json && buf) { + uint8_t *data = GWBUF_DATA(buf); + memcpy(data, json, len); + data[len] = '\n'; rc = dcb->func.write(dcb, buf); } else From 8dae8136aaec17f75da391b8fae12168431d1c38 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 22:50:40 +0200 Subject: [PATCH 04/19] Add more logging to solve MXS-956 If a DCB is about to be closed but is has already been closed, a message is logged. This will allow us to figure out exactly where the DCB is closed. --- server/modules/include/readwritesplit.h | 1 + .../routing/readwritesplit/readwritesplit.c | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/server/modules/include/readwritesplit.h b/server/modules/include/readwritesplit.h index c58136b7f..7d108d070 100644 --- a/server/modules/include/readwritesplit.h +++ b/server/modules/include/readwritesplit.h @@ -237,6 +237,7 @@ typedef struct backend_ref_st #if defined(SS_DEBUG) skygw_chk_t bref_chk_tail; #endif + int closed_at; /** DEBUG: Line number where this backend reference was closed */ } backend_ref_t; /** diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 0bb306afc..0af46e0bc 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -69,6 +69,8 @@ MODULE_INFO info = * @endverbatim */ +#define RW_CHK_DCB(bref, dcb) do{if(dcb->state == DCB_STATE_DISCONNECTED){MXS_NOTICE("DCB was closed on line %d.", (bref) ? (bref)->closed_at : -1);}}while(false) + static char *version_str = "V1.1.0"; static ROUTER *createInstance(SERVICE *service, char **options); @@ -952,7 +954,9 @@ static void closeSession(ROUTER *instance, void *router_session) /** * closes protocol and dcb */ + RW_CHK_DCB(bref, dcb); dcb_close(dcb); + bref->closed_at = __LINE__; /** decrease server current connection counters */ atomic_add(&bref->bref_backend->backend_conn_count, -1); } @@ -2976,6 +2980,7 @@ bool connect_server(backend_ref_t *bref, SESSION *session, bool execute_history) if (bref->bref_dcb != NULL) { bref_clear_state(bref, BREF_CLOSED); + bref->closed_at = 0; if (!execute_history || execute_sescmd_history(bref)) { @@ -2994,7 +2999,9 @@ bool connect_server(backend_ref_t *bref, SESSION *session, bool execute_history) bref->bref_backend->backend_server->unique_name, bref->bref_backend->backend_server->name, bref->bref_backend->backend_server->port); + RW_CHK_DCB(bref, bref->bref_dcb); dcb_close(bref->bref_dcb); + bref->closed_at = __LINE__; bref->bref_dcb = NULL; } } @@ -3303,7 +3310,9 @@ static bool select_connect_backend_servers(backend_ref_t **p_master_ref, /** Decrease backend's connection counter. */ atomic_add(&backend_ref[i].bref_backend->backend_conn_count, -1); + RW_CHK_DCB(&backend_ref[i], backend_ref[i].bref_dcb); dcb_close(backend_ref[i].bref_dcb); + backend_ref[i].closed_at = __LINE__; } } } @@ -3545,7 +3554,9 @@ static GWBUF *sescmd_cursor_process_replies(GWBUF *replybuf, if (bref->bref_dcb) { + RW_CHK_DCB(bref, bref->bref_dcb); dcb_close(bref->bref_dcb); + bref->closed_at = __LINE__; } *reconnect = true; gwbuf_free(replybuf); @@ -3585,7 +3596,9 @@ static GWBUF *sescmd_cursor_process_replies(GWBUF *replybuf, if (ses->rses_backend_ref[i].bref_dcb) { + RW_CHK_DCB(&ses->rses_backend_ref[i], ses->rses_backend_ref[i].bref_dcb); dcb_close(ses->rses_backend_ref[i].bref_dcb); + ses->rses_backend_ref[i].closed_at = __LINE__; } *reconnect = true; MXS_WARNING("Disabling slave %s:%d, result differs from " @@ -4451,6 +4464,8 @@ static void handleError(ROUTER *instance, void *router_session, break; } + backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); + /** * If master has lost its Master status error can't be * handled so that session could continue. @@ -4458,7 +4473,6 @@ static void handleError(ROUTER *instance, void *router_session, if (rses->rses_master_ref && rses->rses_master_ref->bref_dcb == problem_dcb) { SERVER *srv = rses->rses_master_ref->bref_backend->backend_server; - backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); bool can_continue = false; if (rses->rses_config.rw_master_failure_mode != RW_FAIL_INSTANTLY && @@ -4505,7 +4519,12 @@ static void handleError(ROUTER *instance, void *router_session, *succp = handle_error_new_connection(inst, &rses, problem_dcb, errmsgbuf); } + RW_CHK_DCB(bref, problem_dcb); dcb_close(problem_dcb); + if (bref) + { + bref->closed_at = __LINE__; + } close_dcb = false; rses_end_locked_router_action(rses); break; @@ -4528,7 +4547,13 @@ static void handleError(ROUTER *instance, void *router_session, if (close_dcb) { + backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); + RW_CHK_DCB(bref, problem_dcb); dcb_close(problem_dcb); + if (bref) + { + bref->closed_at = __LINE__; + } } } @@ -4556,7 +4581,9 @@ static void handle_error_reply_client(SESSION *ses, ROUTER_CLIENT_SES *rses, if (BREF_IS_IN_USE(bref)) { close_failed_bref(bref, false); + RW_CHK_DCB(bref, backend_dcb); dcb_close(backend_dcb); + bref->closed_at = __LINE__; } } else From f8f400bdfd326f1fc3356adaed7ae3573539a0bc Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 28 Nov 2016 09:19:26 +0200 Subject: [PATCH 05/19] Only close the errored DCB if it is not in use It was theoretically possible that a DCB was closed while it was still in use. --- .../routing/readwritesplit/readwritesplit.c | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 0af46e0bc..b6f4d16b7 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -69,7 +69,7 @@ MODULE_INFO info = * @endverbatim */ -#define RW_CHK_DCB(bref, dcb) do{if(dcb->state == DCB_STATE_DISCONNECTED){MXS_NOTICE("DCB was closed on line %d.", (bref) ? (bref)->closed_at : -1);}}while(false) +#define RW_CHK_DCB(bref, dcb) do{if(dcb->state == DCB_STATE_DISCONNECTED){MXS_NOTICE("DCB was closed on line %d and another attempt to close it is made on line %d." , (bref) ? (bref)->closed_at : -1, __LINE__);}}while(false) static char *version_str = "V1.1.0"; @@ -3244,7 +3244,13 @@ static bool select_connect_backend_servers(backend_ref_t **p_master_ref, } } - ss_dassert(slaves_connected < max_nslaves); + ss_dassert(slaves_connected < max_nslaves || max_nslaves == 0); + + if (max_nslaves > 0 && slaves_connected == max_nslaves) + { + MXS_ERROR("Unexpected reconnection of slave servers. Found %d slaves with " + "a maximum of %d connected slaves.", slaves_found, max_nslaves); + } backend_ref_t *bref = get_slave_candidate(backend_ref, router_nservers, master_host, p); @@ -4520,11 +4526,28 @@ static void handleError(ROUTER *instance, void *router_session, } RW_CHK_DCB(bref, problem_dcb); - dcb_close(problem_dcb); - if (bref) + + if (bref == NULL || // No backend found for this DCB + bref->bref_dcb != problem_dcb || // The DCB was replaced + !BREF_IS_IN_USE(bref)) // The backend was closed { - bref->closed_at = __LINE__; + dcb_close(problem_dcb); + + if (bref && bref->bref_dcb == problem_dcb) + { + bref->closed_at = __LINE__; + } } + else + { + /** The DCB is not closed. This should not be reached unless + * an attempt to reconnect is make even though all connections + * are already connected. */ + const char *remote = problem_dcb->server ? + problem_dcb->server->unique_name : "not a server connection"; + MXS_ERROR("DCB at '%s' is still in use, not closing it.", remote); + } + close_dcb = false; rses_end_locked_router_action(rses); break; From 0bc68742a6e3f52cb0dff614c200184fb9dd7ca4 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 28 Nov 2016 13:13:17 +0200 Subject: [PATCH 06/19] Do error handling in one big locked action This is not the optimal way to do error handling but it should solve all problems that could rise from the multi-threaded model of MaxScale. By taking a lock at the start of handleError, we'll be able to modify the dcb error handling flag in a thread-safe manner. This should prevent double error handling for all DCBs. --- .../routing/readwritesplit/readwritesplit.c | 90 ++++++++----------- 1 file changed, 39 insertions(+), 51 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index b6f4d16b7..800209d0b 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -69,7 +69,15 @@ MODULE_INFO info = * @endverbatim */ -#define RW_CHK_DCB(bref, dcb) do{if(dcb->state == DCB_STATE_DISCONNECTED){MXS_NOTICE("DCB was closed on line %d and another attempt to close it is made on line %d." , (bref) ? (bref)->closed_at : -1, __LINE__);}}while(false) +#define RW_CHK_DCB(bref, dcb) \ +do{ \ + if(dcb->state == DCB_STATE_DISCONNECTED){ \ + MXS_NOTICE("DCB was closed on line %d and another attempt to close it is made on line %d." , \ + (bref) ? (bref)->closed_at : -1, __LINE__); \ + } \ +}while (false) + +#define RW_CLOSE_BREF(b) do{ if (bref){ bref->closed_at = __LINE__; } } while (false) static char *version_str = "V1.1.0"; @@ -956,7 +964,7 @@ static void closeSession(ROUTER *instance, void *router_session) */ RW_CHK_DCB(bref, dcb); dcb_close(dcb); - bref->closed_at = __LINE__; + RW_CLOSE_BREF(bref); /** decrease server current connection counters */ atomic_add(&bref->bref_backend->backend_conn_count, -1); } @@ -3001,7 +3009,7 @@ bool connect_server(backend_ref_t *bref, SESSION *session, bool execute_history) bref->bref_backend->backend_server->port); RW_CHK_DCB(bref, bref->bref_dcb); dcb_close(bref->bref_dcb); - bref->closed_at = __LINE__; + RW_CLOSE_BREF(bref); bref->bref_dcb = NULL; } } @@ -3318,7 +3326,7 @@ static bool select_connect_backend_servers(backend_ref_t **p_master_ref, atomic_add(&backend_ref[i].bref_backend->backend_conn_count, -1); RW_CHK_DCB(&backend_ref[i], backend_ref[i].bref_dcb); dcb_close(backend_ref[i].bref_dcb); - backend_ref[i].closed_at = __LINE__; + RW_CLOSE_BREF(&backend_ref[i]); } } } @@ -3562,7 +3570,7 @@ static GWBUF *sescmd_cursor_process_replies(GWBUF *replybuf, { RW_CHK_DCB(bref, bref->bref_dcb); dcb_close(bref->bref_dcb); - bref->closed_at = __LINE__; + RW_CLOSE_BREF(bref); } *reconnect = true; gwbuf_free(replybuf); @@ -3604,7 +3612,7 @@ static GWBUF *sescmd_cursor_process_replies(GWBUF *replybuf, { RW_CHK_DCB(&ses->rses_backend_ref[i], ses->rses_backend_ref[i].bref_dcb); dcb_close(ses->rses_backend_ref[i].bref_dcb); - ses->rses_backend_ref[i].closed_at = __LINE__; + RW_CLOSE_BREF(&ses->rses_backend_ref[i]); } *reconnect = true; MXS_WARNING("Disabling slave %s:%d, result differs from " @@ -4425,6 +4433,13 @@ static void handleError(ROUTER *instance, void *router_session, CHK_DCB(problem_dcb); + if (!rses_begin_locked_router_action(rses)) + { + /** Session is already closed */ + *succp = false; + return; + } + /** Don't handle same error twice on same DCB */ if (problem_dcb->dcb_errhandle_called) { @@ -4434,6 +4449,7 @@ static void handleError(ROUTER *instance, void *router_session, * be safe with the code as it stands on 9 Sept 2015 - MNB */ *succp = true; + rses_end_locked_router_action(rses); return; } else @@ -4443,6 +4459,7 @@ static void handleError(ROUTER *instance, void *router_session, session = problem_dcb->session; bool close_dcb = true; + backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); if (session == NULL || rses == NULL) { @@ -4461,17 +4478,6 @@ static void handleError(ROUTER *instance, void *router_session, { case ERRACT_NEW_CONNECTION: { - if (!rses_begin_locked_router_action(rses)) - { - close_dcb = false; /* With the assumption that if the router session is closed, - * then so is the dcb. - */ - *succp = false; - break; - } - - backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); - /** * If master has lost its Master status error can't be * handled so that session could continue. @@ -4516,12 +4522,11 @@ static void handleError(ROUTER *instance, void *router_session, "corresponding backend ref.", srv->name, srv->port); } } - else + else if (bref) { - /** - * This is called in hope of getting replacement for - * failed slave(s). - */ + /** We should reconnect only if we find a backend for this + * DCB. If this DCB is an older DCB that has been closed, + * we can ignore it. */ *succp = handle_error_new_connection(inst, &rses, problem_dcb, errmsgbuf); } @@ -4535,7 +4540,7 @@ static void handleError(ROUTER *instance, void *router_session, if (bref && bref->bref_dcb == problem_dcb) { - bref->closed_at = __LINE__; + RW_CLOSE_BREF(bref); } } else @@ -4549,7 +4554,6 @@ static void handleError(ROUTER *instance, void *router_session, } close_dcb = false; - rses_end_locked_router_action(rses); break; } @@ -4570,14 +4574,11 @@ static void handleError(ROUTER *instance, void *router_session, if (close_dcb) { - backend_ref_t *bref = get_bref_from_dcb(rses, problem_dcb); RW_CHK_DCB(bref, problem_dcb); dcb_close(problem_dcb); - if (bref) - { - bref->closed_at = __LINE__; - } + RW_CLOSE_BREF(bref); } + rses_end_locked_router_action(rses); } static void handle_error_reply_client(SESSION *ses, ROUTER_CLIENT_SES *rses, @@ -4592,35 +4593,22 @@ static void handle_error_reply_client(SESSION *ses, ROUTER_CLIENT_SES *rses, client_dcb = ses->client_dcb; spinlock_release(&ses->ses_lock); - if (rses_begin_locked_router_action(rses)) + if ((bref = get_bref_from_dcb(rses, backend_dcb)) != NULL) { - /** - * If bref exists, mark it closed - */ - if ((bref = get_bref_from_dcb(rses, backend_dcb)) != NULL) - { - CHK_BACKEND_REF(bref); + CHK_BACKEND_REF(bref); - if (BREF_IS_IN_USE(bref)) - { - close_failed_bref(bref, false); - RW_CHK_DCB(bref, backend_dcb); - dcb_close(backend_dcb); - bref->closed_at = __LINE__; - } - } - else + if (BREF_IS_IN_USE(bref)) { - // All dcbs should be associated with a backend reference. - ss_dassert(!true); + close_failed_bref(bref, false); + RW_CHK_DCB(bref, backend_dcb); + dcb_close(backend_dcb); + RW_CLOSE_BREF(bref); } - - rses_end_locked_router_action(rses); } else { - // The session has already been closed, hence the dcb has been - // closed as well. + // All dcbs should be associated with a backend reference. + ss_dassert(!true); } if (sesstate == SESSION_STATE_ROUTER_READY) From cc54d80a8b7811da06ddeb40c2402f957441eb1f Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Fri, 25 Nov 2016 12:53:15 +0200 Subject: [PATCH 07/19] Fix memory leaks in maxinfo (modified for 2.0 and develop-branch) MXS-1009. This commit adds a gwbuf_free after maxinfo_execute() to free a buffer with an sql-query after it has been processed. Also, the parse tree in maxinfo_execute_query() is now freed. The tree_free- function was renamed to maxinfo_tree_free, since it is now globally available. This commit has additional changes (in relation to the 1.4.4 branch) to remove errors caused by differences in the html and sql-sides of MaxInfo. --- server/modules/include/maxinfo.h | 1 + server/modules/routing/maxinfo/maxinfo.c | 189 +++++++++--------- server/modules/routing/maxinfo/maxinfo_exec.c | 36 ++-- .../modules/routing/maxinfo/maxinfo_parse.c | 116 ++++++----- 4 files changed, 186 insertions(+), 156 deletions(-) diff --git a/server/modules/include/maxinfo.h b/server/modules/include/maxinfo.h index 676a06ea7..c2d0d4b48 100644 --- a/server/modules/include/maxinfo.h +++ b/server/modules/include/maxinfo.h @@ -132,6 +132,7 @@ typedef enum extern MAXINFO_TREE *maxinfo_parse(char *, PARSE_ERROR *); +extern void maxinfo_free_tree(MAXINFO_TREE *); extern void maxinfo_execute(DCB *, MAXINFO_TREE *); extern void maxinfo_send_error(DCB *, int, char *); extern void maxinfo_send_parse_error(DCB *, char *, PARSE_ERROR); diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index d836b20d4..c5ce5d472 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -335,54 +335,60 @@ static void handleError( static int execute(ROUTER *rinstance, void *router_session, GWBUF *queue) { -INFO_INSTANCE *instance = (INFO_INSTANCE *)rinstance; -INFO_SESSION *session = (INFO_SESSION *)router_session; -uint8_t *data; -int length, len, residual; -char *sql; + INFO_INSTANCE *instance = (INFO_INSTANCE *)rinstance; + INFO_SESSION *session = (INFO_SESSION *)router_session; + uint8_t *data; + int length, len, residual; + char *sql; - if (GWBUF_TYPE(queue) == GWBUF_TYPE_HTTP) - { - return handle_url(instance, session, queue); - } - if (session->queue) - { - queue = gwbuf_append(session->queue, queue); - session->queue = NULL; - queue = gwbuf_make_contiguous(queue); - } - data = (uint8_t *)GWBUF_DATA(queue); - length = data[0] + (data[1] << 8) + (data[2] << 16); - if (length + 4 > GWBUF_LENGTH(queue)) - { - // Incomplete packet, must be buffered - session->queue = queue; - return 1; - } + if (GWBUF_TYPE(queue) == GWBUF_TYPE_HTTP) + { + return handle_url(instance, session, queue); + } + if (session->queue) + { + queue = gwbuf_append(session->queue, queue); + session->queue = NULL; + queue = gwbuf_make_contiguous(queue); + } + data = (uint8_t *)GWBUF_DATA(queue); + length = data[0] + (data[1] << 8) + (data[2] << 16); + if (length + 4 > GWBUF_LENGTH(queue)) + { + // Incomplete packet, must be buffered + session->queue = queue; + return 1; + } - // We have a complete request in a signle buffer - if (modutil_MySQL_Query(queue, &sql, &len, &residual)) - { - sql = strndup(sql, len); - int rc = maxinfo_execute_query(instance, session, sql); - free(sql); - return rc; - } - else - { - switch (MYSQL_COMMAND(queue)) - { - case COM_PING: - return maxinfo_ping(instance, session, queue); - case COM_STATISTICS: - return maxinfo_statistics(instance, session, queue); - default: - MXS_ERROR("maxinfo: Unexpected MySQL command 0x%x", - MYSQL_COMMAND(queue)); - } - } - - return 1; + int rc = 1; + // We have a complete request in a single buffer + if (modutil_MySQL_Query(queue, &sql, &len, &residual)) + { + sql = strndup(sql, len); + rc = maxinfo_execute_query(instance, session, sql); + free(sql); + } + else + { + switch (MYSQL_COMMAND(queue)) + { + case COM_PING: + rc = maxinfo_ping(instance, session, queue); + break; + case COM_STATISTICS: + rc = maxinfo_statistics(instance, session, queue); + break; + case COM_QUIT: + break; + default: + MXS_ERROR("maxinfo: Unexpected MySQL command 0x%x", + MYSQL_COMMAND(queue)); + break; + } + } + // MaxInfo doesn't route the data forward so it should be freed. + gwbuf_free(queue); + return rc; } /** @@ -601,51 +607,54 @@ uint8_t *ptr; static int maxinfo_execute_query(INFO_INSTANCE *instance, INFO_SESSION *session, char *sql) { -MAXINFO_TREE *tree; -PARSE_ERROR err; + MAXINFO_TREE *tree; + PARSE_ERROR err; - MXS_INFO("maxinfo: SQL statement: '%s' for 0x%p.", - sql, session->dcb); - if (strcmp(sql, "select @@version_comment limit 1") == 0) - { - respond_vercom(session->dcb); - return 1; - } - /* Below is a kludge for MonYog, if we see - * select unix_timestamp... as starttime - * just return the starttime of MaxScale - */ - if (strncasecmp(sql, "select UNIX_TIMESTAMP", - strlen("select UNIX_TIMESTAMP")) == 0 - && (strstr(sql, "as starttime") != NULL || strstr(sql, "AS starttime") != NULL)) - { - respond_starttime(session->dcb); - return 1; - } - if (strcasecmp(sql, "set names 'utf8'") == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "set session", 11) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "set autocommit", 14) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "SELECT `ENGINES`.`SUPPORT`", 26) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if ((tree = maxinfo_parse(sql, &err)) == NULL) - { - maxinfo_send_parse_error(session->dcb, sql, err); - MXS_NOTICE("Failed to parse SQL statement: '%s'.", sql); - } - else - maxinfo_execute(session->dcb, tree); - return 1; + MXS_INFO("maxinfo: SQL statement: '%s' for 0x%p.", + sql, session->dcb); + if (strcmp(sql, "select @@version_comment limit 1") == 0) + { + respond_vercom(session->dcb); + return 1; + } + /* Below is a kludge for MonYog, if we see + * select unix_timestamp... as starttime + * just return the starttime of MaxScale + */ + if (strncasecmp(sql, "select UNIX_TIMESTAMP", + strlen("select UNIX_TIMESTAMP")) == 0 + && (strstr(sql, "as starttime") != NULL || strstr(sql, "AS starttime") != NULL)) + { + respond_starttime(session->dcb); + return 1; + } + if (strcasecmp(sql, "set names 'utf8'") == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "set session", 11) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "set autocommit", 14) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "SELECT `ENGINES`.`SUPPORT`", 26) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if ((tree = maxinfo_parse(sql, &err)) == NULL) + { + maxinfo_send_parse_error(session->dcb, sql, err); + MXS_NOTICE("Failed to parse SQL statement: '%s'.", sql); + } + else + { + maxinfo_execute(session->dcb, tree); + maxinfo_free_tree(tree); + } + return 1; } /** diff --git a/server/modules/routing/maxinfo/maxinfo_exec.c b/server/modules/routing/maxinfo/maxinfo_exec.c index 884d65e42..235a8d400 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.c +++ b/server/modules/routing/maxinfo/maxinfo_exec.c @@ -881,22 +881,28 @@ variable_row(RESULTSET *result, void *data) static void exec_show_variables(DCB *dcb, MAXINFO_TREE *filter) { - RESULTSET *result; - VARCONTEXT context; + RESULTSET *result; + VARCONTEXT *context; + + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) + { + return; + } if (filter) { - context.like = filter->value; + context->like = filter->value; } else { - context.like = NULL; + context->like = NULL; } - context.index = 0; + context->index = 0; - if ((result = resultset_create(variable_row, &context)) == NULL) + if ((result = resultset_create(variable_row, context)) == NULL) { maxinfo_send_error(dcb, 0, "No resources available"); + free(context); return; } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); @@ -1167,22 +1173,28 @@ status_row(RESULTSET *result, void *data) static void exec_show_status(DCB *dcb, MAXINFO_TREE *filter) { - RESULTSET *result; - VARCONTEXT context; + RESULTSET *result; + VARCONTEXT *context; + + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) + { + return; + } if (filter) { - context.like = filter->value; + context->like = filter->value; } else { - context.like = NULL; + context->like = NULL; } - context.index = 0; + context->index = 0; - if ((result = resultset_create(status_row, &context)) == NULL) + if ((result = resultset_create(status_row, context)) == NULL) { maxinfo_send_error(dcb, 0, "No resources available"); + free(context); return; } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); diff --git a/server/modules/routing/maxinfo/maxinfo_parse.c b/server/modules/routing/maxinfo/maxinfo_parse.c index 437050255..556b33f4b 100644 --- a/server/modules/routing/maxinfo/maxinfo_parse.c +++ b/server/modules/routing/maxinfo/maxinfo_parse.c @@ -41,7 +41,7 @@ #include static MAXINFO_TREE *make_tree_node(MAXINFO_OPERATOR, char *, MAXINFO_TREE *, MAXINFO_TREE *); -static void free_tree(MAXINFO_TREE *); +void maxinfo_free_tree(MAXINFO_TREE *); // This function is needed by maxinfo.c static char *fetch_token(char *, int *, char **); static MAXINFO_TREE *parse_column_list(char **sql); static MAXINFO_TREE *parse_table_name(char **sql); @@ -62,44 +62,46 @@ char *ptr, *text; MAXINFO_TREE *tree = NULL; MAXINFO_TREE *col, *table; - *parse_error = PARSE_NOERROR; - while ((ptr = fetch_token(sql, &token, &text)) != NULL) - { - switch (token) - { - case LT_SHOW: - free(text); // not needed - ptr = fetch_token(ptr, &token, &text); - if (ptr == NULL || token != LT_STRING) - { - // Expected show "name" - *parse_error = PARSE_MALFORMED_SHOW; - return NULL; - } - tree = make_tree_node(MAXOP_SHOW, text, NULL, NULL); - if ((ptr = fetch_token(ptr, &token, &text)) == NULL) - return tree; - else if (token == LT_LIKE) - { - if ((ptr = fetch_token(ptr, &token, &text)) != NULL) - { - tree->right = make_tree_node(MAXOP_LIKE, - text, NULL, NULL); - return tree; - } - else - { - // Expected expression - *parse_error = PARSE_EXPECTED_LIKE; - free_tree(tree); - return NULL; - } - } - // Malformed show - free(text); - free_tree(tree); - *parse_error = PARSE_MALFORMED_SHOW; - return NULL; + *parse_error = PARSE_NOERROR; + while ((ptr = fetch_token(sql, &token, &text)) != NULL) + { + switch (token) + { + case LT_SHOW: + free(text); // not needed + ptr = fetch_token(ptr, &token, &text); + if (ptr == NULL || token != LT_STRING) + { + // Expected show "name" + *parse_error = PARSE_MALFORMED_SHOW; + return NULL; + } + tree = make_tree_node(MAXOP_SHOW, text, NULL, NULL); + if ((ptr = fetch_token(ptr, &token, &text)) == NULL) + { + return tree; + } + else if (token == LT_LIKE) + { + if ((ptr = fetch_token(ptr, &token, &text)) != NULL) + { + tree->right = make_tree_node(MAXOP_LIKE, + text, NULL, NULL); + return tree; + } + else + { + // Expected expression + *parse_error = PARSE_EXPECTED_LIKE; + maxinfo_free_tree(tree); + return NULL; + } + } + // Malformed show + free(text); + maxinfo_free_tree(tree); + *parse_error = PARSE_MALFORMED_SHOW; + return NULL; #if 0 case LT_SELECT: free(text); // not needed @@ -128,7 +130,7 @@ MAXINFO_TREE *col, *table; { /** Unknown token after SHUTDOWN MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } return tree; @@ -142,7 +144,7 @@ MAXINFO_TREE *col, *table; { /** Missing token for RESTART MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } tree->right = make_tree_node(MAXOP_LITERAL, text, NULL, NULL); @@ -152,7 +154,7 @@ MAXINFO_TREE *col, *table; /** Unknown token after RESTART MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; free(text); - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } return tree; @@ -270,20 +272,26 @@ MAXINFO_TREE *node; } /** - * Recusrsively free the storage associated with a parse tree + * Recursively free the storage associated with a parse tree * * @param tree The parse tree to free */ -static void -free_tree(MAXINFO_TREE *tree) +void +maxinfo_free_tree(MAXINFO_TREE *tree) { - if (tree->left) - free_tree(tree->left); - if (tree->right) - free_tree(tree->right); - if (tree->value) - free(tree->value); - free(tree); + if (tree->left) + { + maxinfo_free_tree(tree->left); + } + if (tree->right) + { + maxinfo_free_tree(tree->right); + } + if (tree->value) + { + free(tree->value); + } + free(tree); } /** @@ -410,8 +418,8 @@ MAXINFO_TREE* maxinfo_parse_literals(MAXINFO_TREE *tree, int min_args, char *ptr (node->right = make_tree_node(MAXOP_LITERAL, text, NULL, NULL)) == NULL) { *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); - if(ptr) + maxinfo_free_tree(tree); + if (ptr) { free(text); } From 9ede657a9bc5c24f964d065e2cfaa429008c19ec Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Wed, 9 Nov 2016 12:55:35 +0200 Subject: [PATCH 08/19] Change error message when permissions on .secrets are wrong If the user running MaxScale could open the .secrets-file and the file permissions were anything other than owner:read, the secrets_readkeys() would fail with error message "Ignoring secrets file , invalid permissions." Now the message is more accurate in stating the expected permissions. --- server/core/secrets.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/core/secrets.c b/server/core/secrets.c index d5edb927c..e459326f3 100644 --- a/server/core/secrets.c +++ b/server/core/secrets.c @@ -153,8 +153,8 @@ secrets_readKeys(const char* path) if (secret_stats.st_mode != (S_IRUSR | S_IFREG)) { close(fd); - MXS_ERROR("Ignoring secrets file " - "%s, invalid permissions.", + MXS_ERROR("Ignoring secrets file %s, invalid permissions." + "The only permission on the file should be owner:read.", secret_file); return NULL; } From 38e4a5902f81527c2b85c4011c8071b8a0fd9d39 Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Thu, 10 Nov 2016 12:21:03 +0200 Subject: [PATCH 09/19] MXS-576 Check for negative values for config settings Previously, negative values were allowed for persistpoolmax and persistmaxtime. Now they cause an error. Also, monitor_interval allowed negative (or zero) values, which were then implicitly cast to unsigned, causing unintended behaviour. Now this causes a warning and the default value is used. --- server/core/config.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/server/core/config.c b/server/core/config.c index f137fadb5..321a7ccdc 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -2459,22 +2459,32 @@ int create_new_server(CONFIG_CONTEXT *obj) const char *poolmax = config_get_value_string(obj->parameters, "persistpoolmax"); if (poolmax) { - server->persistpoolmax = strtol(poolmax, &endptr, 0); - if (*endptr != '\0') + long int persistpoolmax = strtol(poolmax, &endptr, 0); + if (*endptr != '\0' || persistpoolmax < 0) { MXS_ERROR("Invalid value for 'persistpoolmax' for server %s: %s", server->unique_name, poolmax); + error_count++; + } + else + { + server->persistpoolmax = persistpoolmax; } } const char *persistmax = config_get_value_string(obj->parameters, "persistmaxtime"); if (persistmax) { - server->persistmaxtime = strtol(persistmax, &endptr, 0); - if (*endptr != '\0') + long int persistmaxtime = strtol(persistmax, &endptr, 0); + if (*endptr != '\0' || persistmaxtime < 0) { MXS_ERROR("Invalid value for 'persistmaxtime' for server %s: %s", server->unique_name, persistmax); + error_count++; + } + else + { + server->persistmaxtime = persistmaxtime; } } @@ -2597,7 +2607,7 @@ int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, HASHTABLE* else { obj->element = NULL; - MXS_ERROR("Monitor '%s' is missing the require 'module' parameter.", obj->object); + MXS_ERROR("Monitor '%s' is missing the required 'module' parameter.", obj->object); error_count++; } @@ -2613,15 +2623,29 @@ int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, HASHTABLE* { monitorAddParameters(obj->element, obj->parameters); - char *interval = config_get_value(obj->parameters, "monitor_interval"); - if (interval) + char *interval_str = config_get_value(obj->parameters, "monitor_interval"); + if (interval_str) { - monitorSetInterval(obj->element, atoi(interval)); + char *endptr; + long interval = strtol(interval_str, &endptr, 0); + /* The interval must be >0 because it is used as a divisor. + Perhaps a greater minimum value should be added? */ + if (*endptr == '\0' && interval > 0) + { + monitorSetInterval(obj->element, (unsigned long)interval); + } + else + { + MXS_NOTICE("Invalid 'monitor_interval' parameter for monitor '%s', " + "using default value of %d milliseconds.", + obj->object, MONITOR_INTERVAL); + } } else { MXS_NOTICE("Monitor '%s' is missing the 'monitor_interval' parameter, " - "using default value of 10000 milliseconds.", obj->object); + "using default value of %d milliseconds.", + obj->object, MONITOR_INTERVAL); } char *connect_timeout = config_get_value(obj->parameters, "backend_connect_timeout"); From f83129584773d60d68d8479de9ea4b917ded4c44 Mon Sep 17 00:00:00 2001 From: MassimilianoPinto Date: Wed, 30 Nov 2016 10:05:00 +0100 Subject: [PATCH 10/19] In case of error ftruncate is called with router->binlog_position In case of error router->binlog_position is used with truncate and error message instead of router->last_written --- server/modules/routing/binlog/blr_file.c | 4 ++-- server/modules/routing/binlog/blr_master.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/modules/routing/binlog/blr_file.c b/server/modules/routing/binlog/blr_file.c index b6be11af8..c25478328 100644 --- a/server/modules/routing/binlog/blr_file.c +++ b/server/modules/routing/binlog/blr_file.c @@ -352,10 +352,10 @@ blr_write_binlog_record(ROUTER_INSTANCE *router, REP_HEADER *hdr, uint32_t size, router->binlog_name, strerror_r(errno, err_msg, sizeof(err_msg))); /* Remove any partial event that was written */ - if (ftruncate(router->binlog_fd, router->last_written)) + if (ftruncate(router->binlog_fd, router->binlog_position)) { MXS_ERROR("%s: Failed to truncate binlog record at %lu of %s, %s. ", - router->service->name, router->last_written, + router->service->name, router->binlog_position, router->binlog_name, strerror_r(errno, err_msg, sizeof(err_msg))); } diff --git a/server/modules/routing/binlog/blr_master.c b/server/modules/routing/binlog/blr_master.c index 53e251c83..ac4abeefb 100644 --- a/server/modules/routing/binlog/blr_master.c +++ b/server/modules/routing/binlog/blr_master.c @@ -2607,10 +2607,10 @@ blr_write_data_into_binlog(ROUTER_INSTANCE *router, uint32_t data_len, uint8_t * strerror_r(errno, err_msg, sizeof(err_msg))); /* Remove any partial event that was written */ - if (ftruncate(router->binlog_fd, router->last_written)) + if (ftruncate(router->binlog_fd, router->binlog_position)) { MXS_ERROR("%s: Failed to truncate binlog record at %lu of %s, %s. ", - router->service->name, router->last_written, + router->service->name, router->binlog_position, router->binlog_name, strerror_r(errno, err_msg, sizeof(err_msg))); } From 61cd9078d65dc598ef23a6b8e53b291ac757ad3e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 29 Nov 2016 17:03:30 +0200 Subject: [PATCH 11/19] Add note about strip_db_esc to configuration guide The default value was changed in version 2.0.1. --- Documentation/Getting-Started/Configuration-Guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index e89e996c1..c28eb1d08 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -563,7 +563,7 @@ grants when loading the users from the backend server. This parameter takes a boolean value and when enabled, will strip all backslash (`\`) characters from the database names. The default value for this parameter -is true. +is true since MaxScale 2.0.1. In previous version, the default value was false. Some visual database management tools automatically escape some characters and this might cause conflicts when MariaDB MaxScale tries to authenticate users. From 82ef1a9149beb0a4737e68d81ee4d8c523220bb4 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 29 Nov 2016 23:01:59 +0200 Subject: [PATCH 12/19] Document the addition of the Upstart config file The config file for Upstart is added in 2.0.3 which is now documented in the installation guide and the release notes. --- .../MariaDB-MaxScale-Installation-Guide.md | 2 + .../MaxScale-2.0.3-Release-Notes.md | 48 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 Documentation/Release-Notes/MaxScale-2.0.3-Release-Notes.md diff --git a/Documentation/Getting-Started/MariaDB-MaxScale-Installation-Guide.md b/Documentation/Getting-Started/MariaDB-MaxScale-Installation-Guide.md index a8e0f7bd2..9106066a2 100644 --- a/Documentation/Getting-Started/MariaDB-MaxScale-Installation-Guide.md +++ b/Documentation/Getting-Started/MariaDB-MaxScale-Installation-Guide.md @@ -54,6 +54,8 @@ If your system does not support systemd you can start MariaDB MaxScale using the service maxscale start ``` +Starting with version 2.0.3, MaxScale also supports Upstart. + An example configuration file is installed into the `/etc/` folder. This file should be changed according to your needs. ## Install MariaDB MaxScale Using a Tarball diff --git a/Documentation/Release-Notes/MaxScale-2.0.3-Release-Notes.md b/Documentation/Release-Notes/MaxScale-2.0.3-Release-Notes.md new file mode 100644 index 000000000..aaa94a3a7 --- /dev/null +++ b/Documentation/Release-Notes/MaxScale-2.0.3-Release-Notes.md @@ -0,0 +1,48 @@ +# MariaDB MaxScale 2.0.3 Release Notes + +Release 2.0.3 is a GA release. + +This document describes the changes in release 2.0.3, when compared to +release [2.0.2](MaxScale-2.0.2-Release-Notes.md). + +If you are upgrading from release 1.4.4, please also read the release +notes of release [2.0.0](./MaxScale-2.0.0-Release-Notes.md) and +release [2.0.1](./MaxScale-2.0.1-Release-Notes.md). + +For any problems you encounter, please submit a bug report at +[Jira](https://jira.mariadb.org). + +## Updated Features + +### [MXS-1027] (https://jira.mariadb.org/browse/MXS-1027) Add Upstart support (including respawn) for MaxScale + +MaxScale now provides an Upstart configuration file for systems that do not +support systemd. + +## Bug fixes + +[Here](https://jira.mariadb.org/issues/?jql=project%20%3D%20MXS%20AND%20issuetype%20%3D%20Bug%20AND%20status%20%3D%20Closed%20AND%20fixVersion%20%3D%202.0.3) +is a list of bugs fixed since the release of MaxScale 2.0.1. + +* [MXS-1009](https://jira.mariadb.org/browse/MXS-1009): maxinfo sigsegv in spinlock_release + +## Known Issues and Limitations + +There are some limitations and known issues within this version of MaxScale. +For more information, please refer to the [Limitations](../About/Limitations.md) document. + +## Packaging + +RPM and Debian packages are provided for the Linux distributions supported +by MariaDB Enterprise. + +Packages can be downloaded [here](https://mariadb.com/resources/downloads). + +## Source Code + +The source code of MaxScale is tagged at GitHub with a tag, which is derived +from the version of MaxScale. For instance, the tag of version `X.Y.Z` of MaxScale +is `maxscale-X.Y.Z`. + +The source code is available [here](https://github.com/mariadb-corporation/MaxScale). + From ac0f975f9c0274f2e20f3708c951c4ad583d24cb Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 30 Nov 2016 23:10:24 +0200 Subject: [PATCH 13/19] MXS-1033: Fix crash on 'maxadmin list clients' When the client connections were listed, the DCB state was not inspected. Only DCBs in the polling state should be printed as they are guaranteed to be in a valid state. --- server/core/dcb.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index c922e27c8..b3afd1084 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -2157,7 +2157,7 @@ dListDCBs(DCB *pdcb) dcb_printf(pdcb, "------------------+----------------------------+--------------------+----------\n"); while (dcb) { - if (dcb->dcb_is_in_use) + if (dcb->dcb_is_in_use && dcb->state == DCB_STATE_POLLING) { dcb_printf(pdcb, " %-16p | %-26s | %-18s | %s\n", dcb, gw_dcb_state2string(dcb->state), @@ -2189,7 +2189,8 @@ dListClients(DCB *pdcb) dcb_printf(pdcb, "-----------------+------------------+----------------------+------------\n"); while (dcb) { - if (dcb->dcb_is_in_use && dcb->dcb_role == DCB_ROLE_CLIENT_HANDLER) + if (dcb->dcb_is_in_use && dcb->dcb_role == DCB_ROLE_CLIENT_HANDLER && + dcb->state == DCB_STATE_POLLING) { dcb_printf(pdcb, " %-15s | %16p | %-20s | %10p\n", (dcb->remote ? dcb->remote : ""), From 0218ac9e9c58643788fa4ed73e41f3b754d8a939 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 2 Dec 2016 10:58:21 +0200 Subject: [PATCH 14/19] MXS-1043: Handle @@identity like @@last_insert_id The type of @@identity, @@last_insert_id and last_insert_id() is now the same, that is, QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ. --- .../qc_mysqlembedded/qc_mysqlembedded.cc | 22 ++++++++++++++----- query_classifier/qc_sqlite/qc_sqlite.c | 10 ++++++++- query_classifier/test/expected.sql | 3 +++ query_classifier/test/input.sql | 3 +++ 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc index b7383a2b1..2da28845f 100644 --- a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc +++ b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc @@ -913,11 +913,23 @@ static uint32_t resolve_query_type(parsing_info_t *pi, THD* thd) /** System session variable */ case Item_func::GSYSVAR_FUNC: - func_qtype |= QUERY_TYPE_SYSVAR_READ; - MXS_DEBUG("%lu [resolve_query_type] " - "functype GSYSVAR_FUNC, system " - "variable read.", - pthread_self()); + { + const char* name = item->name; + if (name && + ((strcasecmp(name, "@@last_insert_id") == 0) || + (strcasecmp(name, "@@identity") == 0))) + { + func_qtype |= QUERY_TYPE_MASTER_READ; + } + else + { + func_qtype |= QUERY_TYPE_SYSVAR_READ; + } + MXS_DEBUG("%lu [resolve_query_type] " + "functype GSYSVAR_FUNC, system " + "variable read.", + pthread_self()); + } break; /** User-defined variable read */ diff --git a/query_classifier/qc_sqlite/qc_sqlite.c b/query_classifier/qc_sqlite/qc_sqlite.c index d44c09949..71280297c 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.c +++ b/query_classifier/qc_sqlite/qc_sqlite.c @@ -670,7 +670,15 @@ static void update_affected_fields(QC_SQLITE_INFO* info, } else { - info->types |= QUERY_TYPE_SYSVAR_READ; + if ((strcasecmp(&zToken[2], "identity") == 0) || + (strcasecmp(&zToken[2], "last_insert_id") == 0)) + { + info->types |= QUERY_TYPE_MASTER_READ; + } + else + { + info->types |= QUERY_TYPE_SYSVAR_READ; + } } } else diff --git a/query_classifier/test/expected.sql b/query_classifier/test/expected.sql index fd67bcc91..0aa0a9739 100644 --- a/query_classifier/test/expected.sql +++ b/query_classifier/test/expected.sql @@ -12,3 +12,6 @@ QUERY_TYPE_BEGIN_TRX QUERY_TYPE_ROLLBACK QUERY_TYPE_COMMIT QUERY_TYPE_SESSION_WRITE +QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ +QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ +QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ diff --git a/query_classifier/test/input.sql b/query_classifier/test/input.sql index 450bf81c8..bfa445297 100644 --- a/query_classifier/test/input.sql +++ b/query_classifier/test/input.sql @@ -12,3 +12,6 @@ BEGIN; ROLLBACK; COMMIT; use X; +select last_insert_id(); +select @@last_insert_id; +select @@identity; From 6f7b5728228825bcb0a8d7aa2e0a172a577e9f5a Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 1 Dec 2016 15:54:01 +0200 Subject: [PATCH 15/19] Only close DCBs we know are valid If a DCB is passed to the error handler for which we cannot find the corresponding backend reference, it should not be closed. Added extra logging for situations where the backend reference can't be found or it is in the wrong state. --- .../routing/readwritesplit/readwritesplit.c | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 800209d0b..256b931dd 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4532,25 +4532,43 @@ static void handleError(ROUTER *instance, void *router_session, RW_CHK_DCB(bref, problem_dcb); - if (bref == NULL || // No backend found for this DCB - bref->bref_dcb != problem_dcb || // The DCB was replaced - !BREF_IS_IN_USE(bref)) // The backend was closed + if (bref) { - dcb_close(problem_dcb); + /** This is a valid DCB for a backend ref */ - if (bref && bref->bref_dcb == problem_dcb) + if (!BREF_IS_IN_USE(bref) || bref->bref_dcb != problem_dcb) { + /** The backend is closed or the reference was replaced */ + dcb_close(problem_dcb); RW_CLOSE_BREF(bref); } + else + { + MXS_ERROR("Backend '%s' is still in use and points to the problem DCB. Not closing.", + bref->bref_backend->backend_server->unique_name); + } } else { - /** The DCB is not closed. This should not be reached unless - * an attempt to reconnect is make even though all connections - * are already connected. */ - const char *remote = problem_dcb->server ? - problem_dcb->server->unique_name : "not a server connection"; - MXS_ERROR("DCB at '%s' is still in use, not closing it.", remote); + const char *remote = problem_dcb->state == DCB_STATE_POLLING && + problem_dcb->server ? problem_dcb->server->unique_name : "CLOSED"; + + MXS_ERROR("DCB connected to '%s' is not in use by the router " + "session, not closing it. DCB is in state '%s'", + remote, STRDCBSTATE(problem_dcb->state)); + MXS_ERROR("Backends currently in use:"); + + for (int i = 0; i < rses->rses_nbackends; i++) + { + dcb_state_t state = DCB_STATE_UNDEFINED; + if (BREF_IS_IN_USE(&rses->rses_backend_ref[i])) + { + state = rses->rses_backend_ref[i].bref_dcb->state; + } + + MXS_ERROR("%p: %s - %p", &rses->rses_backend_ref[i], STRDCBSTATE(state), + rses->rses_backend_ref[i].bref_dcb); + } } close_dcb = false; From 1d9d325a0113e1e21cf9de7acde2ceb2cbc0c717 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 2 Dec 2016 11:57:03 +0200 Subject: [PATCH 16/19] MXS-1026: Prevent crash with NullAuth The authenticator module will not crash but backend authentication will always fail. --- server/modules/authenticator/CMakeLists.txt | 2 +- server/modules/authenticator/null_auth.c | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/server/modules/authenticator/CMakeLists.txt b/server/modules/authenticator/CMakeLists.txt index bb8fa05d7..9adaf5e52 100644 --- a/server/modules/authenticator/CMakeLists.txt +++ b/server/modules/authenticator/CMakeLists.txt @@ -4,7 +4,7 @@ set_target_properties(MySQLAuth PROPERTIES VERSION "1.0.0") install(TARGETS MySQLAuth DESTINATION ${MAXSCALE_LIBDIR}) add_library(NullAuth SHARED null_auth.c) -target_link_libraries(NullAuth maxscale-common) +target_link_libraries(NullAuth maxscale-common MySQLClient) set_target_properties(NullAuth PROPERTIES VERSION "1.0.0") install(TARGETS NullAuth DESTINATION ${MAXSCALE_LIBDIR}) diff --git a/server/modules/authenticator/null_auth.c b/server/modules/authenticator/null_auth.c index c4ca31f08..0ca01de23 100644 --- a/server/modules/authenticator/null_auth.c +++ b/server/modules/authenticator/null_auth.c @@ -31,6 +31,9 @@ #include #include +/** MXS-1026: Without MySQL protocol data structures, the NullAuth authenticator will crash. */ +#include + /* @see function load_module in load_utils.c for explanation of the following * lint directives. */ @@ -124,6 +127,11 @@ null_auth_authenticate(DCB *dcb) static int null_auth_set_protocol_data(DCB *dcb, GWBUF *buf) { + /** MXS-1026: This will just prevent a crash when the NullAuth authenticator + * is used. This does not provide a way to use MaxScale with no authentication. */ + dcb->data = calloc(1, sizeof(MYSQL_session)); + dcb->protocol = mysql_protocol_init(dcb, dcb->fd); + return 0; } From 943aa48fb5b14aae27c84e579dbbc3f378e12233 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 1 Dec 2016 17:05:23 +0200 Subject: [PATCH 17/19] Make sure DCBs are OK before closing them Added a check for the validity of the backend DCBs before they are closed. This should guarantee that only valid DCBs are closed by readwritesplit. However, this is not the correct solution for the problem. The DCB should not be in an invalid state in the first place and this fix just removes the bad side effects of the double closing. With the added logging in the readwritesplit error handler, more detailed information should become available. --- .../routing/readwritesplit/readwritesplit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 256b931dd..7dcf5c867 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -959,12 +959,19 @@ static void closeSession(ROUTER *instance, void *router_session) } bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); - /** - * closes protocol and dcb - */ + RW_CHK_DCB(bref, dcb); - dcb_close(dcb); + + /** MXS-956: This will prevent closed DCBs from being closed twice. + * It should not happen but for currently unknown reasons, a DCB + * gets closed twice; first in handleError and a second time here. */ + if (dcb && dcb->state == DCB_STATE_POLLING) + { + dcb_close(dcb); + } + RW_CLOSE_BREF(bref); + /** decrease server current connection counters */ atomic_add(&bref->bref_backend->backend_conn_count, -1); } From 21098e0a267e347b1ffcbdc57e10b56f01d4602b Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 5 Dec 2016 14:25:18 +0200 Subject: [PATCH 18/19] MXS-1045: Delete default SIGCHLD handler If the default signal handler is not deleted for the original parent process, the forked daemon process never receives the signals. --- server/core/gateway.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/core/gateway.c b/server/core/gateway.c index c1901f8c4..a87fba03b 100644 --- a/server/core/gateway.c +++ b/server/core/gateway.c @@ -1037,6 +1037,11 @@ bool disable_signals(void) return false; } + if (!delete_signal(&sigset, SIGCHLD, "SIGCHLD")) + { + return false; + } + #ifdef SIGBUS if (!delete_signal(&sigset, SIGBUS, "SIGBUS")) { From 1a9232b152c1edbd1a7c83e797204aefbc8e7ba5 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 5 Dec 2016 17:26:21 +0200 Subject: [PATCH 19/19] Fix dbfwfilter rule syntax documentation The documentation listed the rules as a comma separated list when they were parsed as a whitespace separated list. The match specifiers were also defined as optional when in fact they were mandatory. --- Documentation/Filters/Database-Firewall-Filter.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/Filters/Database-Firewall-Filter.md b/Documentation/Filters/Database-Firewall-Filter.md index 4301ffacd..b715e1119 100644 --- a/Documentation/Filters/Database-Firewall-Filter.md +++ b/Documentation/Filters/Database-Firewall-Filter.md @@ -157,7 +157,7 @@ This limits the rule to be active only on certain types of queries. The possible The `users` directive defines the users to which the rule should be applied. -`users NAME ... match [any|all|strict_all] rules RULE [,...]` +`users NAME... match { any | all | strict_all } rules RULE...` The first keyword is `users`, which identifies this line as a user definition line.