WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
362 stars 98 forks source link

Module Proposal: Persistent database connections #403

Open XVII opened 2 years ago

XVII commented 2 years ago

Overview

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.

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

Relevant PHP docs

mxbclang commented 2 years ago

: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!

XVII commented 2 years ago

@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)?

mxbclang commented 2 years ago

@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!

tillkruss commented 2 years ago

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

XVII commented 2 years ago

I did a quick search and found these items:

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

lucyllewy commented 2 years ago

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

OllieJones commented 2 years ago

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

XVII commented 2 years ago

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

mxbclang commented 2 years ago

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

XVII commented 2 years ago

@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?

mxbclang commented 2 years ago

Thanks, @ShadowXVII! Awaiting @tillkruss' reply.

tillkruss commented 2 years ago

@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?

XVII commented 1 year ago

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

eclarke1 commented 1 year ago

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.

aristath commented 1 year ago

Will this have an impact on the SQLite implementation? 🤔

spacedmonkey commented 1 year ago

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?

jordantrizz commented 1 year ago

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

spacedmonkey commented 1 year ago

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

jordantrizz commented 1 year ago

@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!

ddur commented 1 year ago

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

ddur commented 1 year ago

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.

spacedmonkey commented 1 year ago

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

jordantrizz commented 1 year ago

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?

ddur commented 1 year ago

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.

jordantrizz commented 1 year ago

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.

spacedmonkey commented 1 year ago

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

ddur commented 1 year ago

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 commented 1 year ago

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

ddur commented 1 year ago

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

jordantrizz commented 1 year ago

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? :)

ddur commented 1 year ago

Shall we try and make this happen? :)

If your team makes persistence health-check as above, value of improvement would be obvious to site owners and to WP Core team. Instead of recommending site owners to define constant, send them to vote for your WP Core improvement PR. :)

ddur commented 1 year ago

I don't see any other way to implement persistent connection in WordPress than to modify WP Core. Either to relax hostname validation as you suggested or another ways or give up.

ddur commented 1 year ago

Yes, there is another way. You may modify or propose to modify MySQLi driver? :)

XVII commented 1 year ago

I'm in favour of a CONST to allow a default = off situation, then we can provide doco for turning it on.

The CONST is proposed here, 4 years ago: https://core.trac.wordpress.org/attachment/ticket/31018/3108-4.diff

I raised this ticket to bring awareness to it, (I don't think this is necessarily required as a Performance module or drop in).

Is someone willing to provide an updated/new PR/Patch?

ddur commented 1 year ago

@ShadowXVII

1) I guess nobody here want to mess with hostname validation, as that involves hostname parsing.

2) You may try to implement persistent connections on your (test) server in file /wp-includes/class-wpdb.php

AFTER OR BEFORE

            /*
             * If using the `mysqlnd` library, the IPv6 address needs to be enclosed
             * in square brackets, whereas it doesn't while using the `libmysqlclient` library.
             * @see https://bugs.php.net/bug.php?id=67563
             */
            if ( $is_ipv6 && extension_loaded( 'mysqlnd' ) ) {
                $host = "[$host]";
            }

ADD / INSERT

            if ( defined( 'DB_PERSIST' ) && true === DB_PERSIST ) {
                $host = "p:$host";
            }

To activate by p: prefixed $host, you only need to define DB_PERSIST constant in your wp-config.php

define( 'DB_PERSIST', true ); // enabled

OR

define( 'DB_PERSIST', false ); // disabled, OR REMOVE ENTIRE LINE TO DISABLE

NOTE: WordPress does not use mysqli_connect but mysqli_real_connect. Prefixing $host with p: is documented only in mysqli_connect but NOT in mysqli_real_connect. Anyways, I didn't get any error, looks like that part of documentation is missing from mysqli_real_connect.

NOTE: In the case when your hostname is IPv6 and MySQLi is using native driver (mysqlnd), I'm not sure if addition should be before or after referenced code. Depends on syntax native driver supports, p:[ipv6] or [p:ipv6], if any.

Now you can test if your test site is faster :sunglasses:. Please let me know your findings.

Recommended reading: https://www.php.net/manual/en/mysqli.quickstart.connections.php

And: https://www.php.net/manual/en/mysqlnd.persist.php

ddur commented 1 year ago

More about persistent connections

ddur commented 1 year ago

More about persistent connections

Document above looks like is at least 20 years old.

There is another newer (not mentioned) persistent way since, to connect Server to PHP and indirectly to MySQL : Fast-CGI.

Document doesn't mention MySQL driver/connect, I guess because at the time the document was written, MySQL didn't had transactions or table locking implemented.

Transaction risks mentioned in the document shouldn't apply to WordPress, because WordPress Core doesn't use transactions or table locking, AFAIK and as far as I can search.

jordantrizz commented 1 year ago

There is another newer (not mentioned) persistent way since, to connect Server to PHP and indirectly to MySQL : Fast-CGI.

Link? Also, what about non-Fast-CGI stacks, like Litespeed's LSPHP?

ddur commented 1 year ago

There is another newer (not mentioned) persistent way since, to connect Server to PHP and indirectly to MySQL : Fast-CGI.

Link? Also, what about non-Fast-CGI stacks, like Litespeed's LSPHP?

Yeah, tell us about.

robheffo79 commented 11 months ago

@ShadowXVII

  1. I guess nobody here want to mess with hostname validation, as that involves hostname parsing.
  2. You may try to implement persistent connections on your (test) server in file /wp-includes/class-wpdb.php

AFTER OR BEFORE

          /*
           * If using the `mysqlnd` library, the IPv6 address needs to be enclosed
           * in square brackets, whereas it doesn't while using the `libmysqlclient` library.
           * @see https://bugs.php.net/bug.php?id=67563
           */
          if ( $is_ipv6 && extension_loaded( 'mysqlnd' ) ) {
              $host = "[$host]";
          }

ADD / INSERT

          if ( defined( 'DB_PERSIST' ) && true === DB_PERSIST ) {
              $host = "p:$host";
          }

To activate by p: prefixed $host, you only need to define DB_PERSIST constant in your wp-config.php

define( 'DB_PERSIST', true ); // enabled

OR

define( 'DB_PERSIST', false ); // disabled, OR REMOVE ENTIRE LINE TO DISABLE

NOTE: WordPress does not use mysqli_connect but mysqli_real_connect. Prefixing $host with p: is documented only in mysqli_connect but NOT in mysqli_real_connect. Anyways, I didn't get any error, looks like that part of documentation is missing from mysqli_real_connect.

NOTE: In the case when your hostname is IPv6 and MySQLi is using native driver (mysqlnd), I'm not sure if addition should be before or after referenced code. Depends on syntax native driver supports, p:[ipv6] or [p:ipv6], if any.

Now you can test if your test site is faster 😎. Please let me know your findings.

Recommended reading: https://www.php.net/manual/en/mysqli.quickstart.connections.php

And: https://www.php.net/manual/en/mysqlnd.persist.php

I made this change and it works a TREAT!

Honestly, this is all that is needed to resolve this issue.