Merge close and prepare statement packets in unnamed mode

[hkrutzer]

After #83 we saw an increase in performance in for repeated queries, but a decrease for other queries. The second type of query has a more strict performance requirement for us, and this was no longer being met (by a significant amount). This patch fixes that by removing a database roundtrip.

Unfortunately it was easier for me to create this patch than to investigate why exactly things were slowed down so much. It has been running in our prod environment for around two weeks.

This is the version we are running but it should be identical

diff --git a/lib/myxql/client.ex b/lib/myxql/client.ex
index 4477b1a..6688c17 100644
--- a/lib/myxql/client.ex
+++ b/lib/myxql/client.ex
@@ -114,6 +114,13 @@ defmodule MyXQL.Client do
     end
   end
 
+  def com_stmt_close_prepare(client, statement, close_statement_id) do
+    with :ok <- send_com(client, [{:com_stmt_close, close_statement_id},
+                                  {:com_stmt_prepare, statement}]) do
+      recv_packets(client, &decode_com_stmt_prepare_response/3, :initial)
+    end
+  end
+
   def com_stmt_execute(client, statement_id, params, cursor_type) do
     with :ok <- send_com(client, {:com_stmt_execute, statement_id, params, cursor_type}) do
       recv_packets(client, &decode_com_stmt_execute_response/3, :initial)
@@ -142,6 +149,12 @@ defmodule MyXQL.Client do
     :ok
   end
 
+  def send_com(client, coms) when is_list(coms) do
+    sequence_num = 0
+    payload = Enum.reduce(coms, [], &[&2 | encode_packet(encode_com(&1), sequence_num, @default_max_packet_size)])
+    send_data(client, payload)
+  end
+
   def send_com(client, com) do
     payload = encode_com(com)
     send_packet(client, payload, 0)
diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex
index 637bcf4..409977a 100644
--- a/lib/myxql/connection.ex
+++ b/lib/myxql/connection.ex
@@ -77,8 +77,6 @@ defmodule MyXQL.Connection do
     if cached_query = queries_get(state, query) do
       {:ok, cached_query, %{state | last_query: cached_query}}
     else
-      state = maybe_close(query, state)
-
       case prepare(query, state) do
         {:ok, query, state} ->
           {:ok, query, state}
@@ -99,8 +97,6 @@ defmodule MyXQL.Connection do
 
   @impl true
   def handle_execute(%Query{} = query, params, _opts, state) do
-    state = maybe_close(query, state)
-
     with {:ok, query, state} <- maybe_reprepare(query, state),
          result =
            Client.com_stmt_execute(
@@ -465,7 +461,16 @@ defmodule MyXQL.Connection do
   defp cache_key(%MyXQL.Query{cache: :statement, statement: statement}), do: statement
 
   defp prepare(%Query{ref: ref, statement: statement} = query, state) when is_reference(ref) do
-    case Client.com_stmt_prepare(state.client, statement) do
+    res =
+      if state.prepare == :unnamed and not is_nil(state.last_query) and
+           state.last_query.ref != ref do
+        queries_delete(state, state.last_query)
+        Client.com_stmt_close_prepare(state.client, statement, state.last_query.statement_id)
+      else
+        Client.com_stmt_prepare(state.client, statement)
+      end
+
+    case res do
       {:ok, com_stmt_prepare_ok(statement_id: statement_id, num_params: num_params)} ->
         query = %{query | num_params: num_params, statement_id: statement_id}
         queries_put(state, query)
@@ -494,15 +499,6 @@ defmodule MyXQL.Connection do
     end
   end
 
-  defp maybe_close(%{ref: ref}, %{last_query: %{ref: ref}} = state), do: state
-
-  defp maybe_close(_query, %{prepare: :unnamed, last_query: last_query} = state)
-       when last_query != nil do
-    close(last_query, state)
-  end
-
-  defp maybe_close(_query, state), do: state
-
   defp close(query, state) do
     :ok = Client.com_stmt_close(state.client, query.statement_id)
     queries_delete(state, query)

[wojtekmach]

Hi @hkrutzer, we received more reports about issues so we reverted your previous patch to unblock folks. We're keen on moving forward with this PR, could you please rebase on master?

[hkrutzer]

Done

[josevalim]

We need to understand why the previous patch (and likely this patch) are causing statements to leak though. Any ideas on the root cause will be welcome.

[hkrutzer]

are causing statements to leak

I am not sure this is the case. The prepared statement count we are seeing is stable:

It could be that #93 is not running prepare: :unnamed and there is an issue with prepare: :named that my patch introduced.

[josevalim]

Right. It is most likely that other scenarios are causing the leakage, which we were not yet able to reproduce!

[hkrutzer]

I tried

  test "produce the expected amount of prepared statements in named mode" do
    {:ok, conn} = MyXQL.start_link(@opts)
    assert prepared_stmt_count() == 0

    MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42")
    assert prepared_stmt_count() == 1

    MyXQL.query!(conn, "SELECT 1337", [], cache_statement: "69")
    assert prepared_stmt_count() == 2

    MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42")
    assert prepared_stmt_count() == 2

    MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42")
    assert prepared_stmt_count() == 2
  end

but this passes. I will try running our test suite with different driver versions next week.

[josevalim]

Hi @hkrutzer! We have found another potential source for the leaked statements, that will be fixed on #104. If you want to rebase this on master so we can give it another try, it would be very appreciated.

[hkrutzer]

I'll take a look tomorrow!

[hkrutzer]

I rebased my older commit but it is broken. I don't know enough about where cache_statement comes from to determine how this should work right know. I might take a look at a later time.

[hkrutzer]

Another attempt here

[josevalim]

Closing in favor of more recent PR.