mysqli flush issues

[soulseekah]

The flush method in wpdb is not correct for mysqli usage.
First of all is_resource does not work with mysqli_result instances.

Next, I'm getting a "Commands out of sync; you can't run this command now" error when using resultsets in stored procedures. This is a known issue with MySQLi and seems to occurr when trying to initiate a new query when the old resultset has not been freed. So obviously I was expecting flush to take care of the quirk.

Here's the test procedure;

-- Test procedure for out of sync mysqli commands

DROP PROCEDURE IF EXISTS `mysqli_sync_procedure`;
DELIMITER ;;

CREATE PROCEDURE `mysqli_sync_procedure`()
BEGIN
    SELECT * FROM `wp_posts` LIMIT 1;
END;;
DELIMITER ;

Next, try running the following code:

global $wpdb;
$wpdb->get_results( 'CALL mysqli_sync_procedure' );
$wpdb->get_results( 'SELECT * FROM wp_posts LIMIT 1' );

You should get a bunch of WordPress database error: [Commands out of sync; you can't run this command now] from other queries being executed.

This is because stored procedures have a second resultset which contains the status: 'OK' or 'ERR'.

flush doesn't work as expected, first of all because is_resource returns false for mysqli results, checking should probably be made using $this->result instanceof mysqli_result instead.

Regardless, you'll find yourself against a new error "Couldn't fetch mysqli_result" when trying to free any of the results. This happens even if you don't use the procedure, but just call flush() between two queries. This may have to do with how the results are never stored or used to begin with (mysqli_use_result, mysqli_store_result).

A primitive fix in this case would be to use while ( $this->dbh->more_results() ) $this->dbh->next_result();, so that all results are fetched, leaving no results to linger about.

A better fix would be sort out the correct usage of use/store and free with MySQLi. See resultmode in ​http://ru2.php.net/manual/en/mysqli.query.php where by default you can't even call free on the result, because it's always STORED vs. USED. ("If you use MYSQLI_USE_RESULT all subsequent calls will return error Commands out of sync unless you call mysqli_free_result()").

So, to summarize, we have three issues with mysqli:

1. is_resource is not the correct way to identify mysqli results
2. stored procedures are not flushed correctly because of a second query
3. free cannot be called on results that were fetched with MYSQLI_STORE_RESULT mode

Thoughts, ideas? I'm attaching a patch that flushes mysqli results by fetching any lingering rows.

[soulseekah]

is_resource issues apart, since those are evident, here are some further thoughts on mysqli_free_result.

MYSQLI_STORE_RESULT is the default mode when using mysqli_query without the resultmode set. This is currently the way it's being done in WordPress.

mysqli_free_result must be called when resultmode is MYSQLI_USE_RESULT, but judging by the source code for the mysqlnd freeing a stored result is not useless. So we can call mysqli_free_result either way on the current result.

Moving on to calling procedures using mysqli_query (which is what WordPress will gladly do.

For non-DML queries (not INSERT, UPDATE or DELETE), this function is similar to calling mysqli_real_query() followed by either mysqli_use_result() or mysqli_store_result().

​http://ru2.php.net/manual/en/mysqli.query.php

Do a search for "stored procedure" to read more comments.

​https://bugs.php.net/bug.php?id=35203
​https://bugs.php.net/bug.php?id=32882

Since procedures can return more than one resultset (and in many cases do, especially the additional 'OK' or 'ERR' resultset) it is strongly recommended to use mysqli_multi_query.

So as it stands, in order to properly call a procedure we can't use the WordPress Database API (not without the suggested hacks of iterating over the resultsets). So this is partially a misuse of mysqli by $wpdb on at least CALL procedure queries. This is not critical, since multiple resultsets can still be fetched using next_result and use/store_result without any apparent issues.

However, since the WordPress Database API does not currently expose the aforementioned functions it is impossible to retrieve multiple resultsets by using it. This is, however, a separate issue that I have created a new ticket for: #29938

Note that more_results will only return true inside of $wpdb->flush if there are indeed more resultsets waiting to be used or stored, resulting in sync errors unless looped over.

[soulseekah]

patch and unit tests

[pento]

In 30297:

wpdb::flush() was not flushing results correctly when using mysqli.

This change also allows stored procedures or queries made with mysqli_multi_query() to be flushed.

Includes unit tests.

Fixes #28155.

Props soulseekah.

[boonebgorges]

[30297] appears to have broken a couple of tests. See eg ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/40509364. Glancing at the failing tests, it looks like we're missing a flush somewhere - too much data is being returned.

[soulseekah]

The wrong changeset has been applied for wpdb.php. It's missing the !empty check for dbh in flush.

Compare: https://core.trac.wordpress.org/attachment/ticket/28155/28155.2.diff and https://core.trac.wordpress.org/changeset/30297

Explains the failed test.

[soulseekah]

Sorry for all the changsets, trying to upload a minimal fix and noticing several changes made in my original patch. All tests appear to pass with the latest patch.

[pento]

In 30299:

wpdb: When flushing results on a mysqli connection, make sure that wpdb::$dbh is a valid mysqli connection handle.

Fixes a unit test failure introduced in [30297].

Fixes #28155.

Props soulseekah.

[pento]

Unit tests are still failing. Interestingly, it seems to be the test_mysqli_flush_sync unit test that is causing the failures, the patch to wpdb::flush() itself works just fine.

[boonebgorges]

Interestingly, it seems to be the test_mysqli_flush_sync unit test that is causing the failures

Hm. Well, the symptom is that the post being created on line 485 is sticking around in the database, which suggests that the transaction is being committed?

[soulseekah]

I'm not getting any failed tests, what's the error?

[boonebgorges]

Here are the tests that fail: ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/40614196

But it's really strange - the tests are passing in the latest build, though nothing relevant has been changed. The failing seems to be intermittent, and I don't see a clear way to reproduce. (Even rolling back to [30299] doesn't seem to trigger the errors.)

[pento]

I still can't reproduce this, so I'm not sure what happened. We can reopen this ticket if it starts happening again.

[boonebgorges]

Found it. As I suspected, DROP PROCEDURE causes an implicit commit: ​http://dev.mysql.com/doc/refman/5.0/en/implicit-commit.html. The easy workaround is to manually delete the created post, which I will do :-D

[boonebgorges]

In 30320:

Manually delete fixture in test_mysqli_flush_sync().

This test creates a dummy post and subsequently runs a query containing
DROP PROCEDURE. This latter query implies a COMMIT, which means that the
post is not cleaned up for later tests. Manually deleting the post with
wp_delete_post() solves this problem.

Fixes #28155.