From 5b1f8afcd9c8035f3478b9669300a6799156bb92 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= <markus.makela@mariadb.com>
Date: Mon, 21 Aug 2017 10:31:07 +0300
Subject: [PATCH] MXS-1366: Validate closed connections before pooling them

When a session is being closed in a controlled manner, i.e. a COM_QUIT is
received from the client, it is possible to deduce from this fact that the
backend connections are very likely to be idle. This can be used as an
additional qualification that must be met by all connections before they
can be candidates for connection pooling.

This assumption will not hold with batched and asynchronous queries. In
this case it is possible that the COM_QUIT is received from the client
before even the first result from the backend is read. For this to work,
the protocol module would need to track the number and state of expected
responses.
---
 include/maxscale/session.h                        | 15 +++++++++++++++
 server/core/dcb.c                                 |  2 ++
 server/core/session.c                             | 12 ++++++++++++
 .../protocol/MySQL/MySQLClient/mysql_client.c     | 13 +++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/include/maxscale/session.h b/include/maxscale/session.h
index a85b9292a..f55639772 100644
--- a/include/maxscale/session.h
+++ b/include/maxscale/session.h
@@ -145,6 +145,7 @@ typedef struct session
         GWBUF *buffer; /**< Buffer containing the statement */
         const struct server *target; /**< Where the statement was sent */
     } stmt;  /**< Current statement being executed */
+    bool qualifies_for_pooling; /**< Whether this session qualifies for the connection pool */
     skygw_chk_t     ses_chk_tail;
 } MXS_SESSION;
 
@@ -365,4 +366,18 @@ bool session_take_stmt(MXS_SESSION *session, GWBUF **buffer, const struct server
  */
 void session_clear_stmt(MXS_SESSION *session);
 
+/**
+ * Qualify the session for connection pooling
+ *
+ * @param session Session to qualify
+ */
+void session_qualify_for_pool(MXS_SESSION* session);
+
+/**
+ * Check if the session qualifies for connection pooling
+ *
+ * @param session
+ */
+bool session_valid_for_pool(const MXS_SESSION* session);
+
 MXS_END_DECLS
diff --git a/server/core/dcb.c b/server/core/dcb.c
index adc16afaa..81182ab3b 100644
--- a/server/core/dcb.c
+++ b/server/core/dcb.c
@@ -1662,6 +1662,8 @@ dcb_maybe_add_persistent(DCB *dcb)
         && (dcb->func.established == NULL || dcb->func.established(dcb))
         && strlen(dcb->user)
         && dcb->server
+        && dcb->session
+        && session_valid_for_pool(dcb->session)
         && dcb->server->persistpoolmax
         && (dcb->server->status & SERVER_RUNNING)
         && !dcb->dcb_errhandle_called
diff --git a/server/core/session.c b/server/core/session.c
index fff7f9832..e7da16789 100644
--- a/server/core/session.c
+++ b/server/core/session.c
@@ -112,6 +112,7 @@ session_alloc(SERVICE *service, DCB *client_dcb)
     session->stats.connect = time(0);
     session->stmt.buffer = NULL;
     session->stmt.target = NULL;
+    session->qualifies_for_pooling = false;
     /*<
      * Associate the session to the client DCB and set the reference count on
      * the session to indicate that there is a single reference to the
@@ -947,3 +948,14 @@ void session_clear_stmt(MXS_SESSION *session)
     session->stmt.buffer = NULL;
     session->stmt.target = NULL;
 }
+
+void session_qualify_for_pool(MXS_SESSION* session)
+{
+    session->qualifies_for_pooling = true;
+}
+
+bool session_valid_for_pool(const MXS_SESSION* session)
+{
+    ss_dassert(session->state != SESSION_STATE_DUMMY);
+    return session->qualifies_for_pooling;
+}
diff --git a/server/modules/protocol/MySQL/MySQLClient/mysql_client.c b/server/modules/protocol/MySQL/MySQLClient/mysql_client.c
index e19fcc2d6..a25b329d0 100644
--- a/server/modules/protocol/MySQL/MySQLClient/mysql_client.c
+++ b/server/modules/protocol/MySQL/MySQLClient/mysql_client.c
@@ -991,6 +991,19 @@ gw_read_finish_processing(DCB *dcb, GWBUF *read_buffer, uint64_t capabilities)
     /** Reset error handler when routing of the new query begins */
     dcb->dcb_errhandle_called = false;
 
+    if (proto->current_command == MYSQL_COM_QUIT)
+    {
+        /** The client is closing the connection. We know that this will be the
+         * last command the client sends so the backend connections are very likely
+         * to be in an idle state.
+         *
+         * If the client is pipelining the queries (i.e. sending N request as
+         * a batch and then expecting N responses) then it is possible that
+         * the backend connections are not idle when the COM_QUIT is received.
+         * In most cases we can assume that the connections are idle. */
+        session_qualify_for_pool(session);
+    }
+
     if (rcap_type_required(capabilities, RCAP_TYPE_STMT_INPUT))
     {
         /**