WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 18 months ago

#31018 reopened enhancement

Persistent database connections with mysqli

Reported by: blobaugh Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: needs-refresh needs-unit-tests has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

WordPress currently does not allow support for persistent database connections. This can be accomplished by prepending "p:" to the hostname with mysqli however with its current configuration WordPress will be confused by the ":" and think the hostname is actually a port if specified in the wp-config.php file. This patch add support for a constant that allows persistent connections to be turned on or off.

Why should this be added to core? Because persistent connections are useful :P. But really, we have seen requests for this in other tickets such as #27933. Additionally I am involved in a project where 10,000+ sites are requiring persistent db connection, not an insignificant number. Persistent db connections are also needed to ensure proper performance on IIS and Azure installations.

In short a couple lines of code is all it takes to ensure WordPress continues to work well across other platforms and project requirements.

Props to awoods and bradparbs for helping identify the issue and solution.

Attachments (3)

31018.diff (845 bytes) - added by blobaugh 3 years ago.
Adds support for persistent database connections
31018-2.diff (2.5 KB) - added by jtsternberg 3 years ago.
While I'm still a +1 on the original patch (I think persistent connections are useful enough to warrant config override), I still think some abstraction is needed in these methods. This patch abstracts mysqli_real_connect and mysql_pconnect, for easy replacement by drop-in replacements.
31018-3.diff (967 bytes) - added by MrGregWaugh 2 years ago.
Enable pass through 'p:' from DB_HOST to mysqli_real_connect.

Download all attachments as: .zip

Change History (20)

@blobaugh
3 years ago

Adds support for persistent database connections

#1 @blobaugh
3 years ago

  • Keywords has-patch needs-testing added

#2 @awoods
3 years ago

I think this is a good idea. There are some hosts, where it might not have much of an impact. But other hosts it will have a great positive impact on performance. It doesn't seem like a big deal until you need it and you can't use it. I like the idea of using by setting a constant in their wp-config.php to enable persistent connections. It's a great way to empower developers without them having to hack core.

We should ensure that a check for the mysql driver case is also covered -- for older hosts that might not be using mysqli. which is just an additional couple of lines of code.

#3 follow-ups: @pento
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Thanks for the feature request, Ben!

While persistent connections can be useful under some limited circumstances, I don't think this is appropriate for core, for a couple of reasons:

  • We really don't want to add new wp-config.php settings, especially for something as edge case as this.
  • Persistent connections sound like a really good idea, but have bunch of gotchas that most users won't be aware of, or know how to mitigate. We don't want to encourage users to do things that will break their site, and persistent connections will do that in fairly obscure ways.

For situations like this, I'd recommend creating a DB drop-in that replaces wpdb::db_connect() with a persistent version.

#4 @bradparbs
3 years ago

Microsoft Azure recommends modifying core with a similar change for all of the WordPress installations on their. And we've seen pretty big performance increases with allowing persistant connections, due to the way the servers are set up.

I think allowing an option for the user to enable this is a good idea, even if it is an edge case.

#5 in reply to: ↑ 3 @jtsternberg
3 years ago

  • Keywords dev-feedback added

Replying to pento:

For situations like this, I'd recommend creating a DB drop-in that replaces wpdb::db_connect() with a persistent version.

Since azure is slowly becoming a viable option, and things like this exist https://wordpress.org/plugins/persistent-database-connection-updater/, it would seem we should think about a better solution. Hacking core on every upgrade? Ouch.
A) Gary, I'm guessing something like this would be a proper db drop-in replacement: https://gist.github.com/jtsternberg/eec4ab95e11ce9be4807
B) It seems we should be abstracting some of the functionality in wpdb::db_connect() to smaller methods. This would allow more precision in drop-in replacements. I'll submit a patch for at least that.

@jtsternberg
3 years ago

While I'm still a +1 on the original patch (I think persistent connections are useful enough to warrant config override), I still think some abstraction is needed in these methods. This patch abstracts mysqli_real_connect and mysql_pconnect, for easy replacement by drop-in replacements.

#6 @bradparbs
3 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

+1 to Justin's latest patch.

#8 @DrewAPicture
3 years ago

  • Milestone set to Awaiting Review

#9 in reply to: ↑ 3 ; follow-up: @blobaugh
3 years ago

Replying to pento:

  • We really don't want to add new wp-config.php settings, especially for something as edge case as this.

Could you explain why this is? It seems new settings are warranted as there is no other way to modify how the database connection works.

  • Persistent connections sound like a really good idea, but have bunch of gotchas that most users won't be aware of, or know how to mitigate. We don't want to encourage users to do things that will break their site, and persistent connections will do that in fairly obscure ways.

Uh huh, so does setting DB_HOST or DB_CHARSET or WP_CONTENT_DIR or any number of other settings. They can all break a user's site. That is why they are in the settings file and not exposed in the UI for users to play with. That argument does not seem to stand up to the test of other settings.

For situations like this, I'd recommend creating a DB drop-in that replaces wpdb::db_connect() with a persistent version.

Yes this is one option, however it is not a good option because core changes how connections work periodically. The site owner would need to be aware that core was changing so they could hire a dev to update their drop-in file. It is not realistic for a site owner to track that. They should only have to enjoy their site. We are already seeing many instances of this happening with IIS based installs. The plugin jtsternberg mentioned goes to some length to capture this but is ultimately a bad hack and very fragile but it is currently how I have seen adding persistent connections to WordPress recommended (though I do agree it is horrible).

#10 in reply to: ↑ 9 @pento
3 years ago

  • Keywords dev-feedback removed

Replying to blobaugh:

Could you explain why this is? It seems new settings are warranted as there is no other way to modify how the database connection works.

New settings are almost never warranted - I think the last one we added was WP_MAX_MEMORY_LIMIT, added in r17749, released in WordPress 3.2.

Uh huh, so does setting DB_HOST or DB_CHARSET or WP_CONTENT_DIR or any number of other settings. They can all break a user's site. That is why they are in the settings file and not exposed in the UI for users to play with. That argument does not seem to stand up to the test of other settings.

Of course you can break your site by messing up your other settings. The difference is that DB_PERSISTENT would allow you to break your site in an inconsistent manner, just by enabling it. Persistent connections are kind of like MySQL's query cache in this regard - they were very useful to improve performance in MySQL 4.0, and can even give performance benefits today. They'll keep giving performance benefits, right up until they explode.

In the case of persistent connections, that explosion point is when a PHP process dies without releasing all of its locks. Those locks will be held until the connection is recycled, and mysqli forces all locks to be released. In the intervening time, your site is locked with no obvious reason why. For this reason alone, I'd want to actively discourage users from enabling persistent connections, by specifically not supporting it in core.

Yes this is one option, however it is not a good option because core changes how connections work periodically. The site owner would need to be aware that core was changing so they could hire a dev to update their drop-in file. It is not realistic for a site owner to track that. They should only have to enjoy their site. We are already seeing many instances of this happening with IIS based installs. The plugin jtsternberg mentioned goes to some length to capture this but is ultimately a bad hack and very fragile but it is currently how I have seen adding persistent connections to WordPress recommended (though I do agree it is horrible).

That's the deal with DB drop-ins - they need to be maintained.

Well, that's not entirely true. When we change WPDB, we keep drop-ins in mind, so that we don't suddenly cause them to stop functioning. They really only need to be updated to take advantage of new features (see HyperDB, which functions quite well without having added mysqli support).

Regarding 31018-2.diff, this will create severe maintenance problems - even if we were to switch these functions to protected, instead of public (so that plugins don't start calling them), we'd still need to support them for DB drop-ins, like the one being suggested. If we were to move to a DB driver system, for example, we'd likely be stuck with these driver-specific functions on the global API.

This ticket comes down entirely to where we want the maintenance burden to rest - on core, to maintain support for persistent connections, or on a DB drop-in, to maintain feature parity with core. Given the extremely limited audience of this feature, it seems to me that core isn't the right place for this.

#11 @SergeyBiryukov
3 years ago

  • Description modified (diff)

#12 @pento
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

@MrGregWaugh
2 years ago

Enable pass through 'p:' from DB_HOST to mysqli_real_connect.

#13 @MrGregWaugh
2 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

So I'm going to make one more stab at this. I agree with most here that while persistent database connections are generally not needed, there are specific cases where they are of great benefit, the example of using ClearDB being one. Seeing "solutions" like the "persistent-database-connection-updater" make me really sad.

The only thing really preventing one from being able to use valid mysqli host syntax in wp-config.php for persistent connections (prepending a 'p:') is the port_or_socket() code in wp-db.php, which according to the comments "duplicates how mysql_connect detects a port and/or socket file". My proposed solution is to enable that to correctly pass through a 'p:' from DB_HOST to mysqli_real_connect.

For example, you would be able to do:

define('DB_HOST', 'p:db-host.expensive-connection.com');

Note: this patch is intended to enable this functionality with absolutely minimal changes to the core code.

Patch_31018-3 (raw)

Here is the complete code segment, from wp-db.php line 1431

			// mysqli_real_connect doesn't support the host param including a port or socket
			// like mysql_connect does. This duplicates how mysql_connect detects a port and/or socket file.
			$port = null;
			$socket = null;
			$host = $this->dbhost;
			$pre_host = '';
			// If DB_HOST begins with a 'p:', allow it to be passed to mysqli_real_connect().
			// mysqli supports persistent connections starting with PHP 5.3.0.
			if (version_compare( phpversion(), '5.3.0', '>=' ) && 0 === strpos( $host, 'p:' )) {
				$host = substr( $host, 2 );
				$pre_host = 'p:';
			}
			$port_or_socket = strstr( $host, ':' );
			if ( ! empty( $port_or_socket ) ) {
				$host = substr( $host, 0, strpos( $host, ':' ) );
				$port_or_socket = substr( $port_or_socket, 1 );
				if ( 0 !== strpos( $port_or_socket, '/' ) ) {
					$port = intval( $port_or_socket );
					$maybe_socket = strstr( $port_or_socket, ':' );
					if ( ! empty( $maybe_socket ) ) {
						$socket = substr( $maybe_socket, 1 );
					}
				} else {
					$socket = $port_or_socket;
				}
			}
			$host = $pre_host . $host;

			if ( WP_DEBUG ) {
				mysqli_real_connect( $this->dbh, $host, $this->dbuser, $this->dbpassword, null, $port, $socket, $client_flags );
			} else {
				@mysqli_real_connect( $this->dbh, $host, $this->dbuser, $this->dbpassword, null, $port, $socket, $client_flags );
			}

#14 @netweb
2 years ago

  • Milestone set to Awaiting Review

#15 @blobaugh
2 years ago

I think this is a good solution. It avoids the creation of a new constant and still allows advanced users/dev the ability to have their persistent connections. Thanks @MrGregWaugh

#16 @lisota
2 years ago

+1 for this solution.

WordPress needs an option to support persistent MySQL connections for folks that need it and know what they are doing. There are valid cases to use persistent connections to MySQL, and hosters out there who recommend its use.

Supporting persistent connections should not require rewriting core files, namely wp-db.php, which are easily overwritten by core updates.

The fix is a trivial pass-thru of the "p:" parameter. Anyone sophisticated enough to add that parameter to the DB_HOST does not need hand holding.

#17 @pento
18 months ago

  • Keywords needs-refresh needs-unit-tests added; needs-testing removed

31018-3.diff is a direction I like.

I'd like to see support for ext/mysql added to the patch before we commit to anything, though.

If someone feels like picking it up, dusting it off, and adding some unit tests, I think we might be onto something.

Note: See TracTickets for help on using tickets.