From 16fc920d334bd3c48a65b1ca9cfbb744755840ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 28 Jan 2019 18:23:38 +0200 Subject: [PATCH] MXS-2299: Hints always take precedence Hints should override all statement level routing decisions that would otherwise be done based on the query type. --- include/maxscale/queryclassifier.hh | 3 +- server/core/queryclassifier.cc | 136 ++++++++++++++-------------- 2 files changed, 72 insertions(+), 67 deletions(-) diff --git a/include/maxscale/queryclassifier.hh b/include/maxscale/queryclassifier.hh index 467201b81..3af22b6a5 100644 --- a/include/maxscale/queryclassifier.hh +++ b/include/maxscale/queryclassifier.hh @@ -348,7 +348,8 @@ private: */ bool query_type_is_read_only(uint32_t qtype) const; - uint32_t get_route_target(uint8_t command, uint32_t qtype, HINT* pHints); + void process_routing_hints(HINT* pHints, uint32_t* target); + uint32_t get_route_target(uint8_t command, uint32_t qtype); MXS_SESSION* session() const { diff --git a/server/core/queryclassifier.cc b/server/core/queryclassifier.cc index 18d87d929..2e3c2c7a2 100644 --- a/server/core/queryclassifier.cc +++ b/server/core/queryclassifier.cc @@ -433,7 +433,73 @@ bool QueryClassifier::query_type_is_read_only(uint32_t qtype) const return rval; } -uint32_t QueryClassifier::get_route_target(uint8_t command, uint32_t qtype, HINT* pHints) +void QueryClassifier::process_routing_hints(HINT* pHints, uint32_t* target) +{ + HINT* pHint = pHints; + + while (pHint) + { + if (m_pHandler->supports_hint(pHint->type)) + { + switch (pHint->type) + { + case HINT_ROUTE_TO_MASTER: + // This means override, so we bail out immediately. + *target = TARGET_MASTER; + MXS_DEBUG("Hint: route to master"); + pHint = NULL; + break; + + case HINT_ROUTE_TO_NAMED_SERVER: + // The router is expected to look up the named server. + *target |= TARGET_NAMED_SERVER; + MXS_DEBUG("Hint: route to named server: %s", (char*)pHint->data); + break; + + case HINT_ROUTE_TO_UPTODATE_SERVER: + // TODO: Add generic target type, never to be seem by RWS. + mxb_assert(false); + break; + + case HINT_ROUTE_TO_ALL: + // TODO: Add generic target type, never to be seem by RWS. + mxb_assert(false); + break; + + case HINT_ROUTE_TO_LAST_USED: + MXS_DEBUG("Hint: route to last used"); + *target = TARGET_LAST_USED; + break; + + case HINT_PARAMETER: + if (strncasecmp((char*)pHint->data, + "max_slave_replication_lag", + strlen("max_slave_replication_lag")) == 0) + { + *target |= TARGET_RLAG_MAX; + } + else + { + MXS_ERROR("Unknown hint parameter '%s' when " + "'max_slave_replication_lag' was expected.", + (char*)pHint->data); + } + break; + + case HINT_ROUTE_TO_SLAVE: + *target = TARGET_SLAVE; + MXS_DEBUG("Hint: route to slave."); + } + } + + if (pHint) + { + pHint = pHint->next; + } + } +} + +uint32_t QueryClassifier::get_route_target(uint8_t command, uint32_t qtype) { bool trx_active = session_trx_is_active(m_pSession); uint32_t target = TARGET_UNDEFINED; @@ -533,70 +599,6 @@ uint32_t QueryClassifier::get_route_target(uint8_t command, uint32_t qtype, HINT target = TARGET_MASTER; } - /** Process routing hints */ - HINT* pHint = pHints; - - while (pHint) - { - if (m_pHandler->supports_hint(pHint->type)) - { - switch (pHint->type) - { - case HINT_ROUTE_TO_MASTER: - // This means override, so we bail out immediately. - target = TARGET_MASTER; - MXS_DEBUG("Hint: route to master"); - pHint = NULL; - break; - - case HINT_ROUTE_TO_NAMED_SERVER: - // The router is expected to look up the named server. - target |= TARGET_NAMED_SERVER; - MXS_DEBUG("Hint: route to named server: %s", (char*)pHint->data); - break; - - case HINT_ROUTE_TO_UPTODATE_SERVER: - // TODO: Add generic target type, never to be seem by RWS. - mxb_assert(false); - break; - - case HINT_ROUTE_TO_ALL: - // TODO: Add generic target type, never to be seem by RWS. - mxb_assert(false); - break; - - case HINT_ROUTE_TO_LAST_USED: - MXS_DEBUG("Hint: route to last used"); - target = TARGET_LAST_USED; - break; - - case HINT_PARAMETER: - if (strncasecmp((char*)pHint->data, - "max_slave_replication_lag", - strlen("max_slave_replication_lag")) == 0) - { - target |= TARGET_RLAG_MAX; - } - else - { - MXS_ERROR("Unknown hint parameter '%s' when " - "'max_slave_replication_lag' was expected.", - (char*)pHint->data); - } - break; - - case HINT_ROUTE_TO_SLAVE: - target = TARGET_SLAVE; - MXS_DEBUG("Hint: route to slave."); - } - } - - if (pHint) - { - pHint = pHint->next; - } - } - return target; } @@ -999,9 +1001,11 @@ QueryClassifier::RouteInfo QueryClassifier::update_route_info( type_mask = ps_get_type(stmt_id); } - route_target = get_route_target(command, type_mask, pBuffer->hint); + route_target = get_route_target(command, type_mask); } + process_routing_hints(pBuffer->hint, &route_target); + if (session_trx_is_ending(m_pSession) || qc_query_is_type(type_mask, QUERY_TYPE_BEGIN_TRX)) {