fixes for query_many and its variants

[greg-rychlewski]

This ticket is addressing the issue reported here: #151.

The underlying problem is that DDL commands return ok_packet results and these were not being considered when collecting the results in MyXQL.Connection.result. While investigating this I noticed some other cases that can cause issues:

1. Mixing statements that return resultset and ok_packet in text queries causes incorrect results.

   • Once an ok_packet response is received the results are returned. Any statements coming after it will be processed by the db but the result won't be returned to the user. This means a query such as create table; select 1; will. only return the result for create and not for the select.
   • A trailing ok_packet from a stored procedure can be confused with other statement combinations. For instance, the following 2 queries will process the same packets:
     • A stored procedure that executes select 1;
     • A text query such as select 1; create table;

   The issue is that trailing ok packets from stored procedures were handled specially, being thrown away and not returned to the user. In the second query above the result from create will similarly be thrown away.

   The proposal in this PR is to treat the trailing ok packet as a valid result. You can think of it as the result from the stored procedure execution. I'm not able to see another way to handle both scenarios correctly. The consequence of this is that it breaks a couple of the existing behaviours:

   • Can't use query/4 to execute stored procedures with a single select statement in them. Need to use the query_many variant.
   • Can't use stream/4 to execute stored procedures with a single select statement in them.

   To avoid breaking stream/4 without having an alternative method to perform the same action I could investigate what it would take to create stream_many and submit that before merging this change. Or I could see what it would take to alter the current cursor logic to allow for multiple statements, since it is already returning many (chunked) results.

2. The query_many variants weren't handling error packets. To be compliant with the current DBConnection behaviour it looks like there are only 2 options:

   • return the %MyXQL.Error{} struct in the list of results
   • return {:error, %MyXQL.Error{}}.

   The proposal in this PR is for the latter because it seems like the only way to allow disconnect_on_error_codes to work. If there is a desire to change the DBConnection behaviour to accommodate returning the successful results along with the error, I could investigate that.

[greg-rychlewski]

I investigated the quote below from my original post. This is about streaming a stored procedure that has a single select statement. It would break if we start treating the trailing ok packet as a valid result:

To avoid breaking stream/4 without having an alternative method to perform the same action I could investigate what it would take to create stream_many and submit that before merging this change. Or I could see what it would take to alter the current cursor logic to allow for multiple statements, since it is already returning many (chunked) results.

What I found is that stored procedures are not even using the cursor. It seems like MySQL receives the cursor flag but doesn't use it. At least on MySQL 8.0 on my laptop. For example, if we define a stored procedure like this:

DROP PROCEDURE IF EXISTS test_procedure;
DELIMITER $$
CREATE PROCEDURE test_procedure()
BEGIN
  SELECT * FROM integers;
END$$
DELIMITER ;

and then run this query:

MyXQL.query!(c.conn, "INSERT INTO integers VALUES (1), (2), (3), (4), (5)")

{:ok, results} =
   MyXQL.transaction(c.conn, fn conn ->
     stream = MyXQL.stream(conn, "CALL test_procedure()", [], max_rows: 2)
     Enum.to_list(stream)
   end)

IO.inspect(results)

we get the result:

[
  %MyXQL.Result{
    columns: ["x"],
    connection_id: 2553,
    last_insert_id: nil,
    num_rows: 5,
    num_warnings: 0,
    rows: [[1], [2], [3], [4], [5]]
  }
]

You can also test that the server_status_cursor_exists flag is not set while decoding the results.

Based on this it seems like there is a case to disallow streaming stored procedures. With the changes in this PR it will error with the {:error, :multiple_results} message. I could extend the message to give some guidance about this scenario, if the decision is to disallow this behaviour.

[wojtekmach]

What I found is that stored procedures are not even using the cursor.
Based on this it seems like there is a case to disallow streaming stored procedures.

Good catch, thank you. I had some tests for this but they were pretty bad, not actually checking we received rows in chunks. Yeah, if we can raise a good error message I think that'd be the best experience as otherwise users might think they are streaming.

[wojtekmach]

The issue is that trailing ok packets from stored procedures were handled specially, being thrown away and not returned to the user. In the second query above the result from create will similarly be thrown away.

Yeah, I think that was a mistake. The driver should never throw away any information as we don't know if it could be useful.

Can't use query/4 to execute stored procedures with a single select statement in them. Need to use the query_many variant.

Agreed.

Can't use stream/4 to execute stored procedures with a single select statement in them.

To avoid breaking stream/4 without having an alternative method to perform the same action I could investigate what it would take to create stream_many and submit that before merging this change. Or I could see what it would take to alter the current cursor logic to allow for multiple statements, since it is already returning many (chunked) results.

Given stream already returns multiple results I don't think we need a separate stream_many function.

[wojtekmach]

are the 3 assertions above duplicates from the test for non-bang version? If so I think we can remove them since query_many and query_many! are basically interchangeable.

[greg-rychlewski]

Ah yeah you're right. Thank you, i deleted the tests.

[wojtekmach]

on the wire, do we get an ok_packet for each of the 3 statements or we just get the trailing ok_packet that we get for all procedure calls?

[greg-rychlewski]

For stored procedures it seems like only statements that return result sets are sent through the wire. The way I checked was with these tests:

• https://github.com/elixir-ecto/myxql/pull/152/files#diff-25c00486ed7c3364911a13625fb201869b4221fc96834062c8abe5f099d334daR643
• https://github.com/elixir-ecto/myxql/pull/152/files#diff-25c00486ed7c3364911a13625fb201869b4221fc96834062c8abe5f099d334daR657
• https://github.com/elixir-ecto/myxql/pull/152/files#diff-25c00486ed7c3364911a13625fb201869b4221fc96834062c8abe5f099d334daR671

And then also to make sure the logic for accumulating the results doesn't have an error, I inspected the packets while running the tests.

For the text query I modified the protocol functions like this:

def decode_com_query_response(<<0x00, rest::binary>>, "", :initial) do
  IO.inspect "ok packet empty next"
  ok_packet(status_flags: status_flags) = ok_response = decode_ok_packet_body(rest)

  if has_status_flag?(status_flags, :server_more_results_exists) do
    {:cont, {:more_results, ok_response}}
  else
    {:halt, ok_response}
  end
end

def decode_com_query_response(<<0x00, rest::binary>>, _next_data, :initial) do
  IO.inspect "ok packet more data"
  {:cont, {:more_results, decode_ok_packet_body(rest)}}
end

def decode_com_query_response(<<0xFF, rest::binary>>, _next_data, :initial) do
  IO.inspect "error"
  {:halt, decode_err_packet_body(rest)}
end

def decode_com_query_response(payload, next_data, state) do
  IO.inspect "not ok/error"
  decode_resultset(payload, next_data, state, &Values.decode_text_row/2)
end

and the only thing that printed was this:

"ok packet empty next"

And similarly with the prepared statement:

def decode_com_stmt_execute_response(<<0x00, rest::binary>>, "", :initial) do
  IO.inspect "execute ok empty next"
  {:halt, decode_ok_packet_body(rest)}
end

def decode_com_stmt_execute_response(<<0xFF, rest::binary>>, "", :initial) do
  IO.inspect "execute error"
  {:halt, decode_err_packet_body(rest)}
end

def decode_com_stmt_execute_response(payload, next_data, state) do
  IO.inspect "execute not ok/error"
  decode_resultset(payload, next_data, state, &Values.decode_binary_row/2)
end

the only thing that got printed was this:

"execute ok empty next"

[greg-rychlewski]

@wojtekmach The idea here is that when streaming you can only get this type of error when using a stored procedure. The text query separated by semi-colons returns a syntax error, which I put a test for here: f04be98#diff-25c00486ed7c3364911a13625fb201869b4221fc96834062c8abe5f099d334daR775