0.7.1 unnamed prepared statements aren't closed

[eraserhd]

We've just had to reduce our connection pool sizes because we hit the prepared statement limit.

Observations:

1. AFAICT, open connections keep their prepared statements indefinitely
2. Our connection pool connections stay open indefinitely
3. Statements are re-prepared for each connection, although sometimes more.
4. We are using Ecto.all, and we have a GraphQL interface that allows arbitrary filters and heavily uses dynamic, so we have a potentially unbounded number of distinct queries over time.

Conclusion:

We will eventually need to restart our system or the database, as we will eventually have too many prepared statements.

mysql> select count(1), sql_text from performance_schema.prepared_statements_instances group by sql_text order by count(1) desc;
+----------+-----------------------------------
| count(1) | sql_text                          
+----------+-----------------------------------
|     1762 | SELECT s0.`id`, s0.`uid`, s0.`id`,
|     1200 | SELECT s0.`id`, s0.`name`, s0.`api
|     1200 | SELECT s0.`salesforce_object_id`, 
|     1200 | SELECT o0.`id`, o0.`code`, o0.`cre
|     1187 | SELECT s0.`id`, s0.`name`, s0.`api
|      641 | SELECT s0.`id`, s0.`uid`, s0.`id`,
|      600 | SELECT a0.`id`, a0.`ability`, a0.`
|      600 | SELECT s0.`id`, s0.`uid`, s0.`id`,
|      600 | SELECT o4.`name`, s3.`field`, o4.`
|      600 | INSERT INTO `sf_objects_values` (`
|      600 | SELECT o0.`type_api_name_override`

This is a snapshot about an hour after reducing the connections and restarting. There are 6 pods, each with a connection pool limit of 100 connections each for writing and for reading (we don't have a read replica any more, but the code is still there)... so the 1200 and 600 counts are statements prepared for every connection.

I'm not sure why the 1762 query is not capped at 1200. Maybe because it uses dynamic?

All prepared statements are unnamed, at least according to MySQL:

mysql> select (statement_name is null), count(1) from performance_schema.prepared_statements_instances group by statement_name is null;
+--------------------------+----------+
| (statement_name is null) | count(1) |
+--------------------------+----------+
|                        1 |    22326 |
+--------------------------+----------+
1 row in set (0.03 sec)

Perhaps there needs to be an LRU for prepared statements? It seems like we don't want to close them after every query.

Thoughts?

[greg-rychlewski]

Hi @eraserhd,

Thanks for the report. There are a couple things that come to mind when reading your message:

1. Since you are using Ecto, there is a good chance it is using named prepared statements unless you are explicitly setting the prepared: :unnamed option in all of your functions. Do you know if you are doing that?
2. It seems statement_name is always null for the binary protocol (source). So most likely this is not a sign the prepared statements are unnamed. It seems those names come from the text protocol when you explicitly write a query like PREPARE statement_name ...

So given all this, it seems likely your app is generating a lot of different named prepared statements. We can try to help you reduce that but we'd need to know more about the types of queries you are using. This tends to happen when placeholders are not being used. For example you are doing from t in Table, where: t.column = "some_value1", from t in Table, where: t.column = "some_value2", ... instead of from t in Table, where: t.column = ^some_value where some_value is a variable that can change.

[greg-rychlewski]

so we have a potentially unbounded number of distinct queries over time.

It might also be the case that using prepare: :unnamed is what you need. If you are not re-using things very often. Unnamed prepared statements are closed immediately. Which might not be a big deal if you are not re-using things.

[v0idpwn]

On an additional note, if you don't want to use prepare: :unnamed, you can periodically disconnect. See the discussion on elixir-ecto/db_connection#99 for more info on how to.

[eraserhd]

1. Since you are using Ecto, there is a good chance it is using named prepared statements unless you are explicitly setting the prepared: :unnamed option in all of your functions. Do you know if you are doing that?

The word prepared does not appear in our code or config anywhere.

2. It seems statement_name is always null for the binary protocol (source). So most likely this is not a sign the prepared statements are unnamed. It seems those names come from the text protocol when you explicitly write a query like PREPARE statement_name ...

Ahh, good to know.

The primary bit of code that builds the queries from the GraphQL filters is this:

  def filter_with(query, module, filters) do
    with {:ok, query, field_map} <- add_all_fields(query, module, filters) do
      expr =
        filters
        |> normalize()
        |> cata(&remove_object_clauses/1)
        |> cata(fn
          %{op: :and, clauses: clauses} ->
            build_logical_grouping(clauses, true, &dynamic(^&2 and ^&1))

          %{op: :or, clauses: clauses} ->
            build_logical_grouping(clauses, false, &dynamic(^&2 or ^&1))

          %{op: op, field: field} = filter ->
            {binding, column} = Map.get(field_map, field)
            value = module.query_value(value(filter), field)

            case op do
              :eq ->
                dynamic([{^binding, b}], field(b, ^column) == ^value)

              :neq ->
                dynamic([{^binding, b}], field(b, ^column) != ^value)

              :gt ->
                dynamic([{^binding, b}], field(b, ^column) > ^value)

              :lt ->
                dynamic([{^binding, b}], field(b, ^column) < ^value)

              :gte ->
                dynamic([{^binding, b}], field(b, ^column) >= ^value)

              :lte ->
                dynamic([{^binding, b}], field(b, ^column) <= ^value)

              :match ->
                dynamic([{^binding, b}], fragment("? LIKE ?", field(b, ^column), ^value))

              :in ->
                dynamic([{^binding, b}], fragment("? IN (?)", field(b, ^column), splice(^value)))

              :nin ->
                dynamic(
                  [{^binding, b}],
                  fragment("? NOT IN (?)", field(b, ^column), splice(^value))
                )
            end
        end)

      from(query, where: ^expr)
    else
      err -> err
    end
  end

It seems like we can't just add prepare: :unnamed to the query here, or to the Repo.all() call AFAICT? It seems like we have to do this for the whole app?

Is there any way to not sacrifice the performance of the dozens of other, non-dynamic queries?

[josevalim]

Setting it to prepare: :unnamed is the way to go. Perhaps open up an issue in Absinthe to make this customizable somehow?

[greg-rychlewski]

@eraserhd We have added the ability to add prepare: :unnamed per repo call to MyXQL here: elixir-ecto/ecto_sql#650

This means you could do Repo.all(dynamic_query, prepare: :unnamed) on the queries you don't want to create prepared statements. Please give it a try if you think it would help you.

[eraserhd]

Thanks! I like this. Will test it out today.

[eraserhd]

This is working for us. The queries we want cached are cached, the ones we don't aren't. I'm OK with closing this issue if you are.

[greg-rychlewski]

Thanks for testing, we're glad it is working :).

I'll close this issue but please feel free to reach out again if anything else pops up.