From 1fdb531189f1687d563ddeff87af8a61b059371a Mon Sep 17 00:00:00 2001 From: Hans Krutzer Date: Wed, 21 Oct 2020 14:31:43 +0200 Subject: [PATCH 1/6] Add tests for statement count with named mode on --- test/myxql/sync_test.exs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/myxql/sync_test.exs b/test/myxql/sync_test.exs index e17e46a..07f0943 100644 --- a/test/myxql/sync_test.exs +++ b/test/myxql/sync_test.exs @@ -12,6 +12,26 @@ defmodule MyXQL.SyncTest do assert prepared_stmt_count() == 0 end + 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 + + MyXQL.query!(conn, "SELECT 43", [], cache_statement: "43") + assert prepared_stmt_count() == 3 + end + test "do not leak statements with rebound :cache_statement" do {:ok, conn} = MyXQL.start_link(@opts) assert prepared_stmt_count() == 0 From 306836444a663883896e29ad02fd1c5525c5c333 Mon Sep 17 00:00:00 2001 From: Hans Krutzer Date: Wed, 21 Oct 2020 15:01:03 +0200 Subject: [PATCH 2/6] Cache last query instead of ref --- lib/myxql/connection.ex | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex index 52aa573..40e13b2 100644 --- a/lib/myxql/connection.ex +++ b/lib/myxql/connection.ex @@ -13,7 +13,7 @@ defmodule MyXQL.Connection do prepare: :named, queries: nil, transaction_status: :idle, - last_ref: nil + last_query: nil ] @impl true @@ -64,7 +64,7 @@ defmodule MyXQL.Connection do query = rename_query(state, query) if cached_query = queries_get(state, query) do - {:ok, cached_query, %{state | last_ref: cached_query.ref}} + {:ok, cached_query, %{state | last_query: cached_query}} else case prepare(query, state) do {:ok, _, _} = ok -> @@ -469,14 +469,14 @@ defmodule MyXQL.Connection do ref = make_ref() query = %{query | num_params: num_params, statement_id: statement_id, ref: ref} queries_put(state, query) - {:ok, query, %{state | last_ref: ref}} + {:ok, query, %{state | last_query: query}} result -> result(result, query, state) end end - defp maybe_reprepare(%{ref: ref} = query, %{last_ref: ref} = state), do: {:ok, query, state} + defp maybe_reprepare(query, %{last_query: query} = state), do: {:ok, query, state} defp maybe_reprepare(query, state) do if query_member?(state, query) do @@ -494,8 +494,8 @@ defmodule MyXQL.Connection do defp maybe_close(%{name: ""} = query, state), do: close(query, state) defp maybe_close(_query, state), do: {:ok, state} - defp close(%{ref: ref} = query, %{last_ref: ref} = state) do - close(query, %{state | last_ref: nil}) + defp close(query, %{last_query: query} = state) do + close(query, %{state | last_query: nil}) end defp close(query, state) do From efc96d88583daf75721a81bae3bb3e332c6ae144 Mon Sep 17 00:00:00 2001 From: Hans Krutzer Date: Wed, 21 Oct 2020 16:45:49 +0200 Subject: [PATCH 3/6] Reuse repeat prepared statements in unnamed mode --- lib/myxql/connection.ex | 27 ++++++++++++++++++++++----- test/myxql/sync_test.exs | 25 +++++++++++++++++++++---- test/myxql_test.exs | 3 ++- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex index 40e13b2..2e5a566 100644 --- a/lib/myxql/connection.ex +++ b/lib/myxql/connection.ex @@ -66,6 +66,8 @@ defmodule MyXQL.Connection do if cached_query = queries_get(state, query) do {:ok, cached_query, %{state | last_query: cached_query}} else + {:ok, state} = maybe_close(query, state) + case prepare(query, state) do {:ok, _, _} = ok -> ok @@ -96,6 +98,8 @@ defmodule MyXQL.Connection do end def handle_execute(query, params, _opts, state) do + {:ok, state} = maybe_close(query, state) + with {:ok, query, state} <- maybe_reprepare(query, state) do result = Client.com_stmt_execute( @@ -106,9 +110,17 @@ defmodule MyXQL.Connection do result_state(query) ) - with {:ok, state} <- maybe_close(query, state) do - result(result, query, state) - end + state = + case result do + {:ok, err_packet()} -> + {:ok, state} = close(query, state) + state + + _ -> + state + end + + result(result, query, state) end end @@ -490,8 +502,13 @@ defmodule MyXQL.Connection do %{state | cursors: Map.delete(state.cursors, cursor.ref)} end - # Close unnamed queries after executing them - defp maybe_close(%{name: ""} = query, state), do: close(query, state) + # Close a previous unnamed query if the current query is different + defp maybe_close(_query, %{prepare: :unnamed, last_query: last_query} = state) + when last_query != nil do + close(last_query, state) + end + + defp maybe_close(%{ref: ref}, %{last_query: %{ref: ref}} = state), do: {:ok, state} defp maybe_close(_query, state), do: {:ok, state} defp close(query, %{last_query: query} = state) do diff --git a/test/myxql/sync_test.exs b/test/myxql/sync_test.exs index 07f0943..95293b6 100644 --- a/test/myxql/sync_test.exs +++ b/test/myxql/sync_test.exs @@ -9,10 +9,16 @@ defmodule MyXQL.SyncTest do assert prepared_stmt_count() == 0 MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") - assert prepared_stmt_count() == 0 + assert prepared_stmt_count() == 1 + + MyXQL.query!(conn, "SELECT 1337", [], cache_statement: "69") + assert prepared_stmt_count() == 1 + + MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") + assert prepared_stmt_count() == 1 end - test "produce the expected amount of prepared statements in named mode" do + test "produce the expected amount of prepared statements with prepare: :named" do {:ok, conn} = MyXQL.start_link(@opts) assert prepared_stmt_count() == 0 @@ -43,13 +49,24 @@ defmodule MyXQL.SyncTest do assert prepared_stmt_count() == 1 end - test "do not leak statements with insert and failed insert" do + test "do not leak statements with insert and failed insert with prepare: :named" do {:ok, conn} = MyXQL.start_link(@opts) assert prepared_stmt_count() == 0 {:ok, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (1)") - assert prepared_stmt_count() == 0 + assert prepared_stmt_count() == 1 {:error, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (1)") + assert prepared_stmt_count() == 1 + end + + test "do not leak statements with insert and failed insert with prepare: :unnamed" do + {:ok, conn} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) + assert prepared_stmt_count() == 0 + {:ok, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (2)") + assert prepared_stmt_count() == 1 + {:error, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (2)") assert prepared_stmt_count() == 0 + MyXQL.query!(conn, "SELECT 123", [], cache_statement: "123") + assert prepared_stmt_count() == 1 end test "do not leak statements on multiple executions of the same name in prepare_execute" do diff --git a/test/myxql_test.exs b/test/myxql_test.exs index 1351fe8..78eae26 100644 --- a/test/myxql_test.exs +++ b/test/myxql_test.exs @@ -209,7 +209,8 @@ defmodule MyXQLTest do {:ok, pid} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) {:ok, query} = MyXQL.prepare(pid, "1", "SELECT 1") {:ok, query2, _} = MyXQL.execute(pid, query, []) - assert query == query2 + assert query.ref != query2.ref + assert query.statement_id != query2.statement_id {:ok, query3, _} = MyXQL.execute(pid, query, []) assert query2.ref != query3.ref assert query2.statement_id != query3.statement_id From bf51b9474d0b3e0465e6f5edf1f101bf672e7d69 Mon Sep 17 00:00:00 2001 From: Hans Krutzer Date: Tue, 10 Aug 2021 11:07:37 +0200 Subject: [PATCH 4/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Valim --- lib/myxql/connection.ex | 10 +++------- test/myxql/sync_test.exs | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex index 2e5a566..10613f4 100644 --- a/lib/myxql/connection.ex +++ b/lib/myxql/connection.ex @@ -110,14 +110,10 @@ defmodule MyXQL.Connection do result_state(query) ) - state = + {:ok, state} = case result do - {:ok, err_packet()} -> - {:ok, state} = close(query, state) - state - - _ -> - state + {:ok, err_packet()} -> close(query, state) + _ -> {:ok, state} end result(result, query, state) diff --git a/test/myxql/sync_test.exs b/test/myxql/sync_test.exs index 95293b6..2a9ac1a 100644 --- a/test/myxql/sync_test.exs +++ b/test/myxql/sync_test.exs @@ -25,7 +25,7 @@ defmodule MyXQL.SyncTest do MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") assert prepared_stmt_count() == 1 - MyXQL.query!(conn, "SELECT 1337", [], cache_statement: "69") + MyXQL.query!(conn, "SELECT 1337", [], cache_statement: "1337") assert prepared_stmt_count() == 2 MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") From e52f347fe52ae042dd6b4e58a6a6d048e22a3243 Mon Sep 17 00:00:00 2001 From: Greg Rychlewski Date: Fri, 21 Jan 2022 16:52:13 -0500 Subject: [PATCH 5/6] refactor --- lib/myxql/connection.ex | 54 +++++++++++++++++++++------------------- test/myxql/sync_test.exs | 27 ++++++++++++-------- test/myxql_test.exs | 21 ++++++++++++---- 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex index 10613f4..193193b 100644 --- a/lib/myxql/connection.ex +++ b/lib/myxql/connection.ex @@ -29,7 +29,7 @@ defmodule MyXQL.Connection do prepare: prepare, disconnect_on_error_codes: Keyword.fetch!(opts, :disconnect_on_error_codes), ping_timeout: ping_timeout, - queries: queries_new() + queries: queries_new(prepare) } {:ok, state} @@ -64,10 +64,9 @@ defmodule MyXQL.Connection do query = rename_query(state, query) if cached_query = queries_get(state, query) do - {:ok, cached_query, %{state | last_query: cached_query}} + %{ref: ref, statement_id: statement_id} = cached_query + {:ok, cached_query, %{state | last_query: {ref, statement_id}}} else - {:ok, state} = maybe_close(query, state) - case prepare(query, state) do {:ok, _, _} = ok -> ok @@ -98,8 +97,6 @@ defmodule MyXQL.Connection do end def handle_execute(query, params, _opts, state) do - {:ok, state} = maybe_close(query, state) - with {:ok, query, state} <- maybe_reprepare(query, state) do result = Client.com_stmt_execute( @@ -110,13 +107,9 @@ defmodule MyXQL.Connection do result_state(query) ) - {:ok, state} = - case result do - {:ok, err_packet()} -> close(query, state) - _ -> {:ok, state} - end - - result(result, query, state) + with {:ok, state} <- maybe_close(query, state) do + result(result, query, state) + end end end @@ -477,14 +470,21 @@ defmodule MyXQL.Connection do ref = make_ref() query = %{query | num_params: num_params, statement_id: statement_id, ref: ref} queries_put(state, query) - {:ok, query, %{state | last_query: query}} + {:ok, query, %{state | last_query: {ref, statement_id}}} result -> result(result, query, state) end end - defp maybe_reprepare(query, %{last_query: query} = state), do: {:ok, query, state} + defp maybe_reprepare(%{ref: ref} = query, %{last_query: {ref, _statement_id}} = state) do + {:ok, query, state} + end + + defp maybe_reprepare(query, %{queries: nil, last_query: {_ref, statement_id}} = state) do + Client.com_stmt_close(state.client, statement_id) + prepare(query, state) + end defp maybe_reprepare(query, state) do if query_member?(state, query) do @@ -498,16 +498,15 @@ defmodule MyXQL.Connection do %{state | cursors: Map.delete(state.cursors, cursor.ref)} end - # Close a previous unnamed query if the current query is different - defp maybe_close(_query, %{prepare: :unnamed, last_query: last_query} = state) - when last_query != nil do - close(last_query, state) - end - - defp maybe_close(%{ref: ref}, %{last_query: %{ref: ref}} = state), do: {:ok, state} + # When prepare is :named, close unnamed queries after executing them. + # When prepare is :unnamed, queries will be closed the next time a + # query is prepared or a different query is executed. This allows us + # to re-execute the same unnamed query without preparing it again. + defp maybe_close(_query, %{queries: nil} = state), do: {:ok, state} + defp maybe_close(%Query{name: ""} = query, state), do: close(query, state) defp maybe_close(_query, state), do: {:ok, state} - defp close(query, %{last_query: query} = state) do + defp close(%{ref: ref} = query, %{last_query: {ref, _statement_id}} = state) do close(query, %{state | last_query: nil}) end @@ -529,7 +528,8 @@ defmodule MyXQL.Connection do ## Cache query handling - defp queries_new(), do: :ets.new(__MODULE__, [:set, :public]) + defp queries_new(:unnamed), do: nil + defp queries_new(_), do: :ets.new(__MODULE__, [:set, :public]) defp queries_put(%{queries: nil}, _), do: :ok defp queries_put(_state, %{name: ""}), do: :ok @@ -583,7 +583,11 @@ defmodule MyXQL.Connection do end end - defp queries_get(%{queries: nil}, _), do: nil + defp queries_get(%{queries: nil, last_query: {_ref, statement_id}} = state, _query) do + Client.com_stmt_close(state.client, statement_id) + nil + end + defp queries_get(_state, %{name: ""}), do: nil defp queries_get(state, %{cache: :reference, name: name}) do diff --git a/test/myxql/sync_test.exs b/test/myxql/sync_test.exs index 2a9ac1a..57da216 100644 --- a/test/myxql/sync_test.exs +++ b/test/myxql/sync_test.exs @@ -8,17 +8,22 @@ defmodule MyXQL.SyncTest do {:ok, conn} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) assert prepared_stmt_count() == 0 + # Multiple queries do not increase statement count MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") assert prepared_stmt_count() == 1 - MyXQL.query!(conn, "SELECT 1337", [], cache_statement: "69") + MyXQL.query!(conn, "SELECT 43", [], cache_statement: "43") assert prepared_stmt_count() == 1 - MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") + # Multiple preparations don't increase statement count + {:ok, _query} = MyXQL.prepare(conn, "1", "SELECT 1") + assert prepared_stmt_count() == 1 + + {:ok, _query} = MyXQL.prepare(conn, "2", "SELECT 2") assert prepared_stmt_count() == 1 end - test "produce the expected amount of prepared statements with prepare: :named" do + test "do not leak statements with prepare: :named" do {:ok, conn} = MyXQL.start_link(@opts) assert prepared_stmt_count() == 0 @@ -31,11 +36,8 @@ defmodule MyXQL.SyncTest do MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") assert prepared_stmt_count() == 2 - MyXQL.query!(conn, "SELECT 42", [], cache_statement: "42") + MyXQL.query!(conn, "SELECT 43", []) assert prepared_stmt_count() == 2 - - MyXQL.query!(conn, "SELECT 43", [], cache_statement: "43") - assert prepared_stmt_count() == 3 end test "do not leak statements with rebound :cache_statement" do @@ -52,19 +54,24 @@ defmodule MyXQL.SyncTest do test "do not leak statements with insert and failed insert with prepare: :named" do {:ok, conn} = MyXQL.start_link(@opts) assert prepared_stmt_count() == 0 + {:ok, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (1)") - assert prepared_stmt_count() == 1 + assert prepared_stmt_count() == 0 + {:error, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (1)") - assert prepared_stmt_count() == 1 + assert prepared_stmt_count() == 0 end test "do not leak statements with insert and failed insert with prepare: :unnamed" do {:ok, conn} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) assert prepared_stmt_count() == 0 + {:ok, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (2)") assert prepared_stmt_count() == 1 + {:error, _} = MyXQL.query(conn, "INSERT INTO uniques(a) VALUES (2)") - assert prepared_stmt_count() == 0 + assert prepared_stmt_count() == 1 + MyXQL.query!(conn, "SELECT 123", [], cache_statement: "123") assert prepared_stmt_count() == 1 end diff --git a/test/myxql_test.exs b/test/myxql_test.exs index 78eae26..bdcb910 100644 --- a/test/myxql_test.exs +++ b/test/myxql_test.exs @@ -205,15 +205,26 @@ defmodule MyXQLTest do assert query == query3 end - test ":unnamed" do + test ":unnamed re-executes last query without preparing" do {:ok, pid} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) {:ok, query} = MyXQL.prepare(pid, "1", "SELECT 1") {:ok, query2, _} = MyXQL.execute(pid, query, []) - assert query.ref != query2.ref - assert query.statement_id != query2.statement_id + assert query == query2 {:ok, query3, _} = MyXQL.execute(pid, query, []) - assert query2.ref != query3.ref - assert query2.statement_id != query3.statement_id + assert query2 == query3 + end + + test ":unnamed re-prepares if last query is not the same" do + {:ok, pid} = MyXQL.start_link(@opts ++ [prepare: :unnamed]) + + {:ok, query1} = MyXQL.prepare(pid, "1", "SELECT 1") + {:ok, query2} = MyXQL.prepare(pid, "2", "SELECT 2") + + {:ok, query1b, _} = MyXQL.execute(pid, query1, []) + {:ok, query2b, _} = MyXQL.execute(pid, query2, []) + + assert query1 != query1b + assert query2 != query2b end test ":force_named" do From 51c862c355aa4cf9679adcd705b013c7216887ad Mon Sep 17 00:00:00 2001 From: Greg Rychlewski Date: Fri, 21 Jan 2022 17:05:05 -0500 Subject: [PATCH 6/6] typo --- lib/myxql/connection.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/myxql/connection.ex b/lib/myxql/connection.ex index 193193b..b18c5e4 100644 --- a/lib/myxql/connection.ex +++ b/lib/myxql/connection.ex @@ -503,7 +503,7 @@ defmodule MyXQL.Connection do # query is prepared or a different query is executed. This allows us # to re-execute the same unnamed query without preparing it again. defp maybe_close(_query, %{queries: nil} = state), do: {:ok, state} - defp maybe_close(%Query{name: ""} = query, state), do: close(query, state) + defp maybe_close(%{name: ""} = query, state), do: close(query, state) defp maybe_close(_query, state), do: {:ok, state} defp close(%{ref: ref} = query, %{last_query: {ref, _statement_id}} = state) do