Make WordPress Core

Opened 17 years ago

Closed 11 years ago

Last modified 10 years ago

#5932 closed task (blessed) (fixed)

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

Reported by: dtc's profile dtc Owned by: pento's profile 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 17 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 12 years ago.
check-connection.2.diff (3.3 KB) - added by pento 12 years ago.
check-connection.3.diff (3.3 KB) - added by pento 12 years ago.
check-connection.4.diff (4.4 KB) - added by pento 12 years ago.
check-connection.5.diff (4.3 KB) - added by pento 12 years ago.
check-connection.6.diff (4.3 KB) - added by pento 12 years ago.
5932.diff (4.3 KB) - added by pento 11 years ago.
5932.2.diff (4.4 KB) - added by wonderboymusic 11 years ago.
5932.3.diff (5.2 KB) - added by pento 11 years ago.
5932.4.diff (5.2 KB) - added by pento 11 years ago.
5932.5.diff (5.5 KB) - added by pento 11 years ago.
5932.6.diff (5.6 KB) - added by pento 11 years ago.
5932.7.diff (5.9 KB) - added by pento 11 years ago.
5932.8.diff (1.1 KB) - added by pento 11 years ago.
5932.9.diff (2.2 KB) - added by pento 11 years ago.

Download all attachments as: .zip

Change History (60)

#1 @darkdragon
17 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.

@Mrasnika
17 years ago

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

#2 @andy
14 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

#3 @andy
14 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.

#4 @hakre
14 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.

#5 @andy
14 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.

#6 @hakre
14 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.

#7 @andy
14 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.

#8 @gazouteast
14 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?

#9 @scribu
14 years ago

Marked #15298 as dup.

#10 @pento
12 years 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;' );

#11 @scribu
12 years 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.

#12 @pento
12 years 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.

#13 @pento
12 years 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.

#14 @scribu
12 years 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 12 years ago by scribu (previous) (diff)

#15 @pento
12 years 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.

#16 @nacin
12 years 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.

#17 @pento
12 years ago

  • Keywords needs-refresh removed

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

#18 @nacin
12 years ago

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

#19 @pento
12 years ago

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

#20 @scribu
12 years 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.

@pento
11 years ago

#21 @pento
11 years ago

  • Milestone changed from Future Release to 3.7

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

#22 follow-up: @nacin
11 years 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?

#23 @nacin
11 years 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.

#24 follow-up: @markoheijnen
11 years ago

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

#25 in reply to: ↑ 24 @nacin
11 years 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.

#26 @valeriosza
11 years ago

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

Duplicate of #21663.

#27 @markoheijnen
11 years ago

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

This isn't a duplicate of that ticket.

#28 @jdgrimes
11 years ago

  • Cc jdg@… added

#29 @wonderboymusic
11 years 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.

#30 @SergeyBiryukov
11 years ago

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

#31 @iandunn
11 years ago

  • Cc ian.dunn@… added

#32 @netweb
11 years ago

  • Cc stephen@… added

@pento
11 years ago

@pento
11 years ago

#33 in reply to: ↑ 22 @pento
11 years 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.

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


11 years ago

@pento
11 years ago

#35 @pento
11 years 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

@pento
11 years ago

#36 @pento
11 years 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.

@pento
11 years ago

#37 @pento
11 years 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.

#38 @nacin
11 years ago

In 27075:

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

props pento.
see #5932.

#39 @nacin
11 years 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 11 years ago by markjaquith (previous) (diff)

#40 @markjaquith
11 years 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.

@pento
11 years ago

@pento
11 years ago

#41 @pento
11 years 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].

#42 @nacin
11 years 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.

#43 @nacin
11 years ago

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

#44 @pento
10 years ago

#20178 was marked as a duplicate.

Note: See TracTickets for help on using tickets.