WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#28155 closed defect (bug) (fixed)

mysqli flush issues

Reported by: soulseekah Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.9
Component: Database Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

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.

Attachments (5)

28155.diff (693 bytes) - added by soulseekah 6 years ago.
28155.2.diff (2.3 KB) - added by soulseekah 5 years ago.
patch and unit tests
28155.3.diff (788 bytes) - added by soulseekah 5 years ago.
28155.4.diff (797 bytes) - added by soulseekah 5 years ago.
28155.5.diff (627 bytes) - added by soulseekah 5 years ago.

Download all attachments as: .zip

Change History (24)

@soulseekah
6 years ago

#1 @soulseekah
5 years ago

  • Keywords has-patch needs-unit-tests added

#2 @kovshenin
5 years ago

  • Keywords dev-feedback added

#3 @soulseekah
5 years ago

  • Keywords needs-unit-tests removed

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.

Last edited 5 years ago by soulseekah (previous) (diff)

@soulseekah
5 years ago

patch and unit tests

#4 @SergeyBiryukov
5 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.1

This ticket was mentioned in Slack in #core by soulseekah. View the logs.


5 years ago

#6 @pento
5 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

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.

#7 @boonebgorges
5 years ago

  • Keywords needs-patch added; has-patch dev-feedback commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[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.

#8 @soulseekah
5 years ago

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
5 years ago

@soulseekah
5 years ago

#9 @soulseekah
5 years ago

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.

@soulseekah
5 years ago

This ticket was mentioned in Slack in #core by soulseekah. View the logs.


5 years ago

#11 @pento
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#12 @pento
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#13 @boonebgorges
5 years ago

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?

#14 @soulseekah
5 years ago

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

#15 @boonebgorges
5 years ago

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.)

#16 @pento
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

#17 @boonebgorges
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#18 @boonebgorges
5 years ago

  • Owner changed from pento to boonebgorges
  • Status changed from reopened to accepted

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

#19 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.