WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 7 weeks ago

#5932 closed task (blessed) (fixed)

wpdb should reconnect and retry query when "MySQL server has gone away"

Reported by: dtc Owned by: pento
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.0
Component: Database Keywords: needs-patch
Focuses: Cc:

Description

Using 2.3.3, here are the type of errors that crop up in error_log everyday.

[25-Jan-2008 08:37:35] WordPress database error MySQL server has gone away for query UPDATE wp_options SET option_value = '0' WHERE option_name = 'doing_cron'
[25-Jan-2008 09:23:19] WordPress database error MySQL server has gone away for query UPDATE wp_options SET option_value = '0' WHERE option_name = 'doing_cron'
[26-Jan-2008 00:03:54] WordPress database error MySQL server has gone away for query UPDATE wp_options SET option_value = '0' WHERE option_name = 'doing_cron'
[26-Jan-2008 00:04:29] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_pingme' LIMIT 1
[26-Jan-2008 00:04:29] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_encloseme' LIMIT 1
[26-Jan-2008 00:04:29] WordPress database error MySQL server has gone away for query SELECT ID FROM wp_posts WHERE CHAR_LENGTH(TRIM(to_ping)) > 7 AND post_status = 'publish'
[26-Jan-2008 00:05:09] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_pingme' LIMIT 1
[26-Jan-2008 00:05:09] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_encloseme' LIMIT 1
[26-Jan-2008 00:05:09] WordPress database error MySQL server has gone away for query SELECT ID FROM wp_posts WHERE CHAR_LENGTH(TRIM(to_ping)) > 7 AND post_status = 'publish'
[26-Jan-2008 00:05:47] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_pingme' LIMIT 1
[26-Jan-2008 00:05:47] WordPress database error MySQL server has gone away for query SELECT * FROM wp_posts, wp_postmeta WHERE wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = '_encloseme' LIMIT 1
[26-Jan-2008 00:05:47] WordPress database error MySQL server has gone away for query SELECT ID FROM wp_posts WHERE CHAR_LENGTH(TRIM(to_ping)) > 7 AND post_status = 'publish'
.................

Attachments (16)

wp-mysql-ping.php (2.5 KB) - added by Mrasnika 6 years ago.
This plugin is designed to help you deal with the "MySQL Server Has Gone Away" error.
check-connection.diff (2.2 KB) - added by pento 21 months ago.
check-connection.2.diff (3.3 KB) - added by pento 21 months ago.
check-connection.3.diff (3.3 KB) - added by pento 21 months ago.
check-connection.4.diff (4.4 KB) - added by pento 21 months ago.
check-connection.5.diff (4.3 KB) - added by pento 21 months ago.
check-connection.6.diff (4.3 KB) - added by pento 19 months ago.
5932.diff (4.3 KB) - added by pento 7 months ago.
5932.2.diff (4.4 KB) - added by wonderboymusic 5 months ago.
5932.3.diff (5.2 KB) - added by pento 3 months ago.
5932.4.diff (5.2 KB) - added by pento 3 months ago.
5932.5.diff (5.5 KB) - added by pento 3 months ago.
5932.6.diff (5.6 KB) - added by pento 3 months ago.
5932.7.diff (5.9 KB) - added by pento 3 months ago.
5932.8.diff (1.1 KB) - added by pento 8 weeks ago.
5932.9.diff (2.2 KB) - added by pento 8 weeks ago.

Download all attachments as: .zip

Change History (59)

comment:1 darkdragon6 years ago

  • Milestone 2.3.4 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Not a WordPress error. Please check the mysql server settings to increase either the timeout or the amount of connections that can be made to the mysql server. If you are on a shared host, then consider either WP-Cache or WP Super Cache.

Mrasnika6 years ago

This plugin is designed to help you deal with the "MySQL Server Has Gone Away" error.

comment:2 andy4 years ago

  • Cc andy added
  • Component changed from General to Database
  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from 2.3.3 to 3.0

comment:3 andy4 years ago

  • Summary changed from WordPress database error MySQL server has gone away to wpdb should reconnect and retry query when "MySQL server has gone away"

WordPress should try to recover from certain kinds of errors from MySQL. This error in particular. There has been much written about this error, including variations in PHP versions.

comment:4 hakre3 years ago

I know errors of that kind from some sites as well, but I assume that this is a configuration issue.

I suggest to close as wontfix or invalid for the moment.

comment:5 andy3 years ago

If you know of a configuration that fixes this then it's a configuration issue. Until then, your assumption is no reason to close this ticket.

Typically this error occurs when the mysql client connection has timed out due to inactivity. This is correct behavior and not necessarily within the power of the WP admin to configure.

It is also easy to recover from: identify the error code, reconnect, and retry the query. I have implemented this in wpdb-based scripts that commonly see deadlocks (another recoverable error). A do-while loop works well.

comment:6 hakre3 years ago

  • Keywords reporter-feedback added; database error mysql server removed

Well, I'm willing to pick up the configuration argument.

Please provide steps to reproduce the problem so it's possible to look into this issue with more detail.

comment:7 andy3 years ago

Write a script that does one query, then sleeps a while, then does another query. Increase the sleep time until the error appears.

comment:8 gazouteast3 years ago

This error also appears when wp_options has hit circa 100,000+ rows (on shared and VPS hosting) - I'm not sure if the limitation is from MySQL or simply a "query taking too long to get answer" issue.

Related - if a SQL table has crashed, MySQL has the ability to return that in the error, WP doesn't seem capable of returning that reason - might be worth a look see to check if could be added to verbosity?

comment:9 scribu3 years ago

Marked #15298 as dup.

pento21 months ago

comment:10 pento21 months ago

  • Keywords has-patch added; reporter-feedback removed

This patch checks the connection whenever wpdb::query() is run. This will add a slight delay to each query (approximately the ping time to the MySQL server).

Here's a testing snippet.

global $wpdb;
$wpdb->query( 'SET SESSION wait_timeout = 10;' );
echo $wpdb->get_var( 'select id from wp_posts limit 1;' );
sleep( 12 );
echo $wpdb->get_var( 'select id from wp_posts limit 1,1;' );

comment:11 scribu21 months ago

Checking the connection before every single query seems wasteful to me.

Ideally, it would work like this:

  1. Do query.
  2. Check some flag that says if the query actually reached the server.
  3. Re-connect, if needed.

comment:12 pento21 months ago

A little wasteful.

I'll have another pass and see what the code looks like when reconnecting after a query failed because the server went away.

pento21 months ago

pento21 months ago

comment:13 pento21 months ago

Patch 3 does the check after a query fails with error 2006 (server gone away).

I also added a 1 second sleep between connection retries, to give the server 5 seconds to come back.

comment:14 scribu21 months ago

  • Owner anonymous deleted
  • Status changed from reopened to assigned

Now that's more like it. :)

I think the actual connection logic, i.e. mysql_connect() + $this->select() should be moved to a helper method, as it's probably duplicated currently.

Last edited 21 months ago by scribu (previous) (diff)

pento21 months ago

comment:15 pento21 months ago

Good point, patch 4 makes wpdb::db_connect() more generic so the code duplication can be removed.

Also, the way diff created that patch is kind of weird.

comment:16 nacin21 months ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 3.5
  • Owner set to pento

I'm game for this.

I discussed with pento how we could clean up check-connection.4.diff's while() loop a bit. Otherwise, looks interesting and good.

pento21 months ago

comment:17 pento21 months ago

  • Keywords needs-refresh removed

Cleaned up the loop in wpdb::check_connection(), per discussion with nacin.

comment:18 nacin19 months ago

The db_connect() call inside check_connection() doesn't pass $allow_bail = false, as I imagine it is supposed to.

pento19 months ago

comment:19 pento19 months ago

New patch adds the correct parameter, updated to patch cleanly against trunk.

comment:20 scribu18 months ago

  • Keywords early added
  • Milestone changed from 3.5 to Future Release
  • Type changed from defect (bug) to enhancement

I think it's too late in the cycle to mess with such a critical component.

pento7 months ago

comment:21 pento7 months ago

  • Milestone changed from Future Release to 3.7

attachment:5932.diff updates the patch against trunk. Let's do this.

comment:22 follow-up: nacin7 months ago

I'm noticing in 5932.diff that there is a return false when the connection fails. But it's possible that the bail() doesn't result in a wp_die(). Then $wpdb->last_error isn't set, $wpdb->insert_id isn't reset, and we have various potential problems. How exactly should this work?

comment:23 nacin7 months ago

  • Milestone changed from 3.7 to 3.8
  • Type changed from enhancement to task (blessed)

This is really cool but it needs some time to soak. Going to tackle in 3.8, probably with mysqli at the same time.

comment:24 follow-up: markoheijnen7 months ago

MySQLi really should discussed at #21663. It makes it now really hard to track if things are being discussed in multiple tickets.

comment:25 in reply to: ↑ 24 nacin7 months ago

Replying to markoheijnen:

MySQLi really should discussed at #21663. It makes it now really hard to track if things are being discussed in multiple tickets.

That's what I was referring to, yes.

comment:26 valeriosza6 months ago

  • Keywords needs-patch close added; has-patch removed
  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #21663.

comment:27 markoheijnen6 months ago

  • Keywords close removed
  • Resolution duplicate deleted
  • Status changed from closed to reopened

This isn't a duplicate of that ticket.

comment:28 jdgrimes6 months ago

  • Cc jdg@… added

wonderboymusic5 months ago

comment:29 wonderboymusic5 months ago

5932.2.diff​ updates the patch and changes the @since docs to 3.9.0. Should be rebooted and paired with #21663. I think we are pushing to try to drop these in at the end of the cycle, and I'm the king of wanting to drop stuff in at the end of the cycle.

comment:30 SergeyBiryukov5 months ago

  • Keywords has-patch 3.9-early added; early needs-patch removed
  • Milestone changed from 3.8 to Future Release

comment:31 iandunn4 months ago

  • Cc ian.dunn@… added

comment:32 netweb4 months ago

  • Cc stephen@… added

pento3 months ago

pento3 months ago

comment:33 in reply to: ↑ 22 pento3 months ago

  • Milestone changed from Future Release to 3.9

attachment:5932.4.diff

  • Updates patch to apply cleanly against trunk
  • Changes a couple of the mysql_*() function calls to @-calls, as they generate error messages before we've had a chance to reconnect
  • Add some curly brackets to match WordPress coding style.
  • Add a unit test

Replying to nacin:

I'm noticing in 5932.diff that there is a return false when the connection fails. But it's possible that the bail() doesn't result in a wp_die(). Then $wpdb->last_error isn't set, $wpdb->insert_id isn't reset, and we have various potential problems. How exactly should this work?

The patch doesn't change existing behaviour. If bail() is called and $wpdb->show_errors is false, $wpdb->error is set, then the calling functions will return up to wp-settings.php. The next function call is wp_set_wpdb_vars(), which checks $wpdb->error, then will call dead_db(), which throws up the <h1>Error establishing a database connection</h1> message we all know and love.

It wouldn't be a bad idea to revisit bail() and decide if it could be made to work nicer, but that seems out of scope for this ticket.

This patch is feeling pretty close to ready, so some extra scrutiny would be greatly appreciated.

comment:34 ircbot3 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

pento3 months ago

comment:35 pento3 months ago

attachment:5932.5.diff

  • Tidies up the check_connection() logic in query()
  • Unsuppresses the mysql_errno() call in query() (it had previously been causing problems with the unit test)
  • When WP_DEBUG is true, make sure we only show one warning if the db_connect() fails multiple times
  • When WP_DEBUG is false, fall back to dead_db() if the reconnect fails completely

Still to do:

  • The error message in check_connection() needs to be more grammatically cromulent
  • PHPDocs need a once over

pento3 months ago

comment:36 pento3 months ago

attachment:5932.6.diff

  • Fixes the PHPDoc for check_connection no longer being accurate (it doesn't return false anymore, it'll always die)
  • Tweaking the "Error reconnecting to the database" message.

pento3 months ago

comment:37 pento3 months ago

attachment:5932.7.diff

When trying to reconnect, only enable warnings for the last attempt. This will prevent "unable to connect" warnings from showing on the page, if it manages to reconnect.

comment:38 nacin3 months ago

In 27075:

When the MySQL server has "gone away," attempt to reconnect and retry the query.

props pento.
see #5932.

comment:39 nacin3 months ago

  • Keywords needs-patch added; has-patch 3.9-early removed

I've committed this but I don't think it's quite done.

The one remaining concern I have here is that the page could already be building when this error occurs. At the moment, the query would fail, as would all after it, but the page could still hypothetically finish building. With this patch, though, two things occur: we cease execution immediately and we call wp_die(), which could potentially ugly up a pre-existing page.

So, a few options:

  • Should wp_die() and dead_db() be smarter about whether a pageload is underway? wp_die() already checks for admin_head() to avoid re-issuing some things, but that's particularly because we sometimes use wp_die() in the admin after the header is printed. That wouldn't quite be the case here.
  • Perhaps we should wp_die() only if we've yet to hit the wp_head action, otherwise just silently fail the query?

Also, I left the string untranslated for the moment, so we can make some adjustments if necessary.

Last edited 8 weeks ago by markjaquith (previous) (diff)

comment:40 markjaquith8 weeks ago

Perhaps we should wp_die() only if we've yet to hit the wp_head action, otherwise just silently fail the query?

Seems reasonable.

pento8 weeks ago

pento8 weeks ago

comment:41 pento8 weeks ago

attachment:5932.9.diff only fires wp_die()/dead_db() before wp_head happens.

If you want to test this, note that there are some warnings caused by bugs in [21663].

comment:42 nacin8 weeks ago

In 27279:

When failing to reconnect to a server that has gone away, simply fail the query once we've passed template_redirect, rather than wp_die().

props pento.
see #5932.

comment:43 nacin7 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.