performance icon indicating copy to clipboard operation
performance copied to clipboard

Module Proposal: Persistent database connections

Open XVII opened this issue 3 years ago • 40 comments

Overview

  • Proposed module name: Persistent Database Connections
  • Proposed module slug: persistent-db-conn
  • Proposed module owner GitHub username(s):

About the module

Purpose

Persistent database connections has the potential for improving WordPress performance by reducing the overhead of establishing new database connections.

Persistent connections do this by being able to leverage existing database connections from a pool rather than having to handshake/establish a new connection every time. The same theory is used in websockets (connect once, save connection overhead later).

This issue tends to more so affect installations using hosted database services where there's a higher latency compared with local databases.

Scope

  1. Allowing persistent connections to be switched on, or off (default)

  2. Find the best way from a code perspective implement the p: before the hostname to switch into persistent mode. People currently edit wp-db.php directly. It currently fails validation in wp-db.php if added to DB_HOST. image

A trac ticket and code suggestions already exist for this idea opened 8 years ago. The deprecation of older PHP versions does make this a little simpler to implement.

  • A re-opened enhancement that's stalled for a while https://core.trac.wordpress.org/ticket/31018
  • A proposed patch https://core.trac.wordpress.org/attachment/ticket/31018/31018.diff

Rationale

For some server setups persistent connections for the database could be the difference between mediocre and acceptable level of performance.

It's important to have this option available so it can be enabled by admins without directly editing core WordPress files.

Other

  • Some metrics would be useful to validate this as having an actual potential performance improvement. This should include different types of services with varying database configurations as some connections require more effort (remote server over SSL) than others (localhost socket).

  • Need to determine the best way to do a performance test to validate benefits of persistent vs. non-persistent connections.

  • #455 has a latency test which may be an indicator of whether this option would benefit or not.

  • Determine whether there are any unintended side-effects of using persistent connections with WordPress.

Relevant PHP docs

XVII avatar Jun 29 '22 13:06 XVII

:wave: @ShadowXVII I saw your comment on #455 about this issue (#403) being a suggestion for a new Site Health check. If this issue is indeed a proposal for a new module, can you please update the issue description to align with the Proposing a new module documentation here?

Once it's been updated, you can ping me here and we can discuss next steps, which include scheduling a discussion of the proposal as part of our weekly chats. Thank you!

mxbclang avatar Jul 31 '22 14:07 mxbclang

@bethanylang 👋 thanks for the info. Will see if I can restructure for the team.

I was hoping someone with more experience around connection persistence would be able to comment and had created this issue more so as a discussion piece. If you know anyone in the existing team, please let me know.

That database latency check might fall in its own health check, connection persistence aside.

Is the preference from the WP team to have multiple health checks per GitHub issue, or to try and group them into a suite (i.e database health checks)?

XVII avatar Jul 31 '22 15:07 XVII

@ShadowXVII Thanks for clarifying! I'm not aware of anyone on our team that has more experience with this particular issue, but you're welcome to attend one of our weekly performance chats on Tuesdays at 15:00 UTC and bring this up in the open floor portion at the end of our meeting. If that time doesn't work for your time zone, let me know and I can try to bring it up myself in a future meeting for you.

The preference would be to create/keep separate issues for different health checks, since ultimately each one is built as a standalone module and sometimes, upon discussion, we all decide that a particular health check may not be the best fit for Performance Lab and/or core. So keeping them separated allows us to track them accordingly.

Let me know if you have any other questions!

mxbclang avatar Jul 31 '22 23:07 mxbclang

Yes, let's please add this. Are there any old Trac tickets for it?

tillkruss avatar Aug 01 '22 02:08 tillkruss

I did a quick search and found these items:

  • A re-opened enhancement that's stalled for a while https://core.trac.wordpress.org/ticket/31018
  • & proposed patch https://core.trac.wordpress.org/attachment/ticket/31018/31018.diff

The other piece around doing a database latency check might be its own item. I'll try write up the module using the template.

XVII avatar Aug 01 '22 03:08 XVII

Just a heads' up that I re-rolled and (I think) improved the patch linked above with a 4th iteration since that patch was created. https://core.trac.wordpress.org/attachment/ticket/31018/3108-4.diff

lucyllewy avatar Aug 01 '22 04:08 lucyllewy

For what it's worth, my Database Performance health-check module for Performance Lab checks database connection latency. https://github.com/WordPress/performance/issues/455#issuecomment-1201217876

OllieJones avatar Aug 01 '22 13:08 OllieJones

@bethanylang alrighty, I popped the module template in. Please let me know if you need anything else ☺️

XVII avatar Aug 01 '22 14:08 XVII

@ShadowXVII Thanks so much!

Per the Proposing a new module documentation, the next step here is to discuss this proposal in one of our weekly Slack meetings. I can introduce you and link out to this issue and you can give a brief overview of it and why you're proposing this module. Then, we open the floor for 5-10 for discussion amongst the team to determine if this is something that would make sense for the Performance Lab plugin and, if so, what the next steps are.

We already have a full agenda for our meeting tomorrow, but would you be available for our Tuesday 9 August meeting? They're held at 15:00 UTC on Tuesdays in the #core-performance channel on the Making WordPress Slack.

mxbclang avatar Aug 01 '22 18:08 mxbclang

@bethanylang -- I'm based in Australia so a 1am meeting on a work night will be tricky. I'll pull a late one if we don't end up finding someone to initiate discussion.

@tillkruss , I know you have many other performance lab commitments but as this is a simple change conceptually, would you be willing to discuss on my behalf?

XVII avatar Aug 03 '22 07:08 XVII

Thanks, @ShadowXVII! Awaiting @tillkruss' reply.

mxbclang avatar Aug 03 '22 11:08 mxbclang

@tillkruss , I know you have many other performance lab commitments but as this is a simple change conceptually, would you be willing to discuss on my behalf?

I'm unfortunately rather slammed the next couple of weeks and can't take this on, but maybe @felixarntz knows someone?

tillkruss avatar Aug 03 '22 14:08 tillkruss

@diddledani , with your patch contribution, would you be willing to discuss this in one of the weekly slack meetings? That's assuming the time works better for you than me in AEDT.

XVII avatar Dec 05 '22 11:12 XVII

Circling back to this one, I have added to the Database focus area (please let me know if that is not correct). And added the Needs Dev label.

eclarke1 avatar Mar 03 '23 11:03 eclarke1

Will this have an impact on the SQLite implementation? 🤔

aristath avatar Mar 20 '23 10:03 aristath

Stupid question, can't you just add p: to the start of the host constant. Like this

define( 'DB_HOST', 'p:127.0.0.1' );

What else needs to change?

spacedmonkey avatar Mar 20 '23 11:03 spacedmonkey

Stupid question, can't you just add p: to the start of the host constant. Like this

define( 'DB_HOST', 'p:127.0.0.1' );

What else needs to change?

Unfortunately, this doesn't work :(

This either means that the username and password information in your wp-config.php file is incorrect or that contact with the database server at p:127.0.0.1 could not be established. This could mean your host’s database server is down.

This patch (it's 8 years old so it might not work) would enable p:

https://core.trac.wordpress.org/ticket/31018#comment:13

jordantrizz avatar Mar 22 '23 15:03 jordantrizz

@jordantrizz Have you tried using ludicrousdb? It a database plugin ( drop-in ) that allow for p: at the start of the connection. See.

You can config this line.

spacedmonkey avatar Mar 22 '23 17:03 spacedmonkey

@jordantrizz Have you tried using ludicrousdb? It a database plugin ( drop-in ) that allow for p: at the start of the connection. See.

You can config this line.

No, but thanks for sharing!

jordantrizz avatar Mar 24 '23 20:03 jordantrizz

https://www.php.net/manual/en/mysqli.configuration.php#ini.mysqli.allow-persistent

ddur avatar May 09 '23 05:05 ddur

https://www.php.net/manual/en/mysqli.construct.php

hostname

Can be either a host name or an IP address. When passing null, the value is retrieved from [mysqli.default_host](https://www.php.net/manual/en/mysqli.configuration.php#ini.mysqli.default-host). When possible, pipes will be used instead of the TCP/IP protocol. The TCP/IP protocol is used if a host name and port number are provided together e.g. localhost:3308.

Prepending host by p: opens a persistent connection. mysqli_change_user() is automatically called on connections opened from the connection pool.

ddur avatar May 09 '23 06:05 ddur

As this can be easily achieved via plugins ( drop-ins ), is there a place for this core or should we just close this ticket.

spacedmonkey avatar May 09 '23 11:05 spacedmonkey

As this can be easily achieved via plugins ( drop-ins ), is there a place for this core or should we just close this ticket.

The functionality inherently exists; the validation is stopping the functionality from being activated. The code change would correct the validation and allow "p:host" to be used. The amount of code required is smaller than available drop-ins and would eliminate an additional step to activate existing functionality.

Wouldn't this be considered a bug with the validation of DB_HOST and not a new feature or module?

jordantrizz avatar May 09 '23 12:05 jordantrizz

Wouldn't this be considered a bug with the validation of DB_HOST and not a new feature or module?

Validation may remain the same, but after validation, if PHP/MySQLi version and PHP .ini configuration supports, actual host may be prepended with p: (replaced) and as such modified, can be used in other code without other code changes.

I would call it automatic 'performance feature'. :sunglasses:

Anyways, MySQLi driver already (automatically) decides to use IP connection or PIPE, based on availability and host(name).

In addition, that automatic 'performance feature' may be enabled/disabled in wp-config.php via define CONSTANT.

ddur avatar May 10 '23 10:05 ddur

Validation may remain the same, but after validation, if PHP/MySQLi version and PHP .ini configuration supports, actual host may be prepended with p: (replaced) and as such modified, can be used in other code without other code changes.

I would call it automatic 'performance feature'. 😎

I agree, this is how it should be implemented. However, as trac shows, changes take time, 8 years for this issue. Your proposal might be the best way to tackle this issue. However, fixing the validation to allow someone to use "p:locahost" would be a minimal amount of effort, code, and testing it could reach the core sooner than later.

jordantrizz avatar May 10 '23 11:05 jordantrizz

A good start might be to create a new / refreshed PR / patch for core. There will increase the chances of getting this into core.

spacedmonkey avatar May 10 '23 11:05 spacedmonkey

I agree, this is how it should be implemented. However, as trac shows, changes take time, 8 years for this issue. Your proposal might be the best way to tackle this issue. However, fixing the validation to allow someone to use "p:locahost" would be a minimal amount of effort, code, and testing it could reach the core sooner than later.

I think, safest way is not to modify existing (tested) code. Adding, initially by default disabled, few lines to check and modify hostname would be safest way to distribute code to millions of WordPress installs without disturbing anyone and still allowing to be tested by anyone who knows about and is able to define (ie.) DB_PERSISTENT constant.

In addition, you may have performance health-check that may check availability, test performance, measure improvement and advise user that it is safe and it makes sense to define DB_PERSISTENT constant.

ddur avatar May 10 '23 11:05 ddur

In next 8 years it may be safe to define that enabling constant to true by default in wp-config(-sample).php :sunglasses:

ddur avatar May 10 '23 12:05 ddur

I guess that adding code in WP Core that is by default disabled (== 100% safe) is fastest way to PR acceptance.

ddur avatar May 10 '23 12:05 ddur

I think, safest way is not to modify existing (tested) code. Adding, initially by default disabled, few lines to check and modify hostname would be safest way to distribute code to millions of WordPress installs without disturbing anyone and still allowing to be tested by anyone who knows about and is able to define (ie.) DB_PERSISTENT constant.

In addition, you may have performance health-check that may check availability, test performance, measure improvement and advise user that it is safe and it makes sense to define DB_PERSISTENT constant.

Shall we try and make this happen? :)

jordantrizz avatar May 10 '23 17:05 jordantrizz