Avoid GC run between mysql_stmt_execute and mysql_stmt_store_result

[kamipo]

This fixes a regression caused by #912 due to calling rb_funcall
between mysql_stmt_execute and mysql_stmt_store_result, it will
cause mysql_stmt_close to be called in wrong order.

Fixes #956.

[sodabrew]

Nice find! I am worried that this will be fragile, and some other call into Ruby space can still creep in here. I'm looking at whether there is a missing refcount.

[sodabrew]

In result.c, rb_mysql_result_to_obj, there's a comment I left a while back:

 /* Keep a handle to the Statement to ensure it doesn't get garbage collected first */
 wrapper->statement = statement;
 if (statement != Qnil) {
   wrapper->stmt_wrapper = DATA_PTR(statement);
   wrapper->stmt_wrapper->refcount++;
 } else {
   wrapper->stmt_wrapper = NULL;
 }

I am wondering how the statement is possibly getting garbage collected at the point of this rb_funcall, if later down in the file, self is passed into rb_mysql_result_to_obj. I tried adding RB_GC_GUARD(self) at several location in rb_mysql_stmt_execute but that did not help.

Your solution avoids rb_funcall - but I read through rb_hash_aref and found that it is possible to call rb_funcall if the hash has a default value callback and the key is missing. This is admittedly not likely, but I was able to reproduce the break by adding this to the repro script:

connection_1.query_options.default_proc = ->(h, k) { puts "Hello #{k}" }

The output Hello stream prints a few times and then fails with Commands out of sync again.

I put together a different movement of code and was able to protect against this failure as well.

Bottom line, a GC run between mysql_stmt_execute and mysql_stmt_store_result is collecting the statement handle, and I don't understand why.

[sodabrew]

I found a commit to the sqlite gem showing another case of a hard GC workaround: sparklemotion/sqlite3-ruby@380287a

[sodabrew]

Aha, I get it now. The issue is that once a statement begins executing, any other command is invalid such as freeing a much older statement handle, which requires a notification from the client library to the server to release the handle, but isn't valid if the server is in results-streaming-mode.

[sodabrew]

Which leads me to wonder... what about streaming results? Yep, that breaks, too! In the repro script:

result_1    = statement_1.execute(1, stream: true, cache_rows: false).to_a

Even with both of our proposed fixes:

outofsync.rb:17:in `each': Row retrieval was canceled by mysql_stmt_close() call (Mysql2::Error)

[sodabrew]

According to the last comment on https://dev.mysql.com/doc/refman/5.7/en/commands-out-of-sync.html, and confirmed locally, setting a cursor for the statement execution allows other commands to be processed:

const unsigned long type = (unsigned long) CURSOR_TYPE_READ_ONLY;
mysql_stmt_attr_set (stmt, STMT_ATTR_CURSOR_TYPE, (void*) &type);

I'm looking at whether the cursor makes sense in all cases, or only in the streaming case.

Edit: seems to be fine adding the cursor in both cases, and it completely alleviates the GC problem.

[kamipo]

Yeah, originally fixing the case of stream: true is out of target in my fix since I didn't know whether it is possible or not.
If possible that, probably it will solve both cases.

I'm closing this in favor of #958.