Create the database driver for MySQLi for async queries

Nice writeup re differences between PDOMYSql and mysqli, https://websitebeaver.com/php-pdo-vs-mysqli

From the github repo, it looks like any core inclusion is currently blocked on #3110546: Allow contributed modules (mostly database drivers) to override tests in core → and ✨ Introduce database driver extensions and autoload database drivers' dependencies Fixed , so I'm going to mark this PP-2.

None of this would stop us trying out some proof of concept stuff with PHP Fibers though.

Actually, #3110546: Allow contributed modules (mostly database drivers) to override tests in core → is only necessary on GitHub to skip running the ApcuBackendTest on GHA - I couldn't find a away to enable APCu properly there, so I am still skipping it.

However, I would say that to be on a preferrable scenario, we would need two more issues in first:

📌 Convert select query extenders to backend-overrideable services RTBC - this left harbour one year ago, then sank couple of weeks later, with only limited SAR activities afterwards. It's tagged for framework manager review.

📌 Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead Fixed - to reduce boilerplate empty classes, which is blocked on the above

Actually, 🐛 Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding Fixed too.

Having committed 📌 Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead Fixed , now 📌 Convert select query extenders to backend-overrideable services RTBC is not strictly necessary to get this moving on.

Last blocker is in.

From the issue summary:

Create the MySQLi database driver which should extend the MySQL database driver.
Add the PHP MySQLi extension as a requirement to the core MySQL and MySQLi database drivers.

Why would we need the MySQLi extension for the current PDO-based MySQL driver?

Very probably it means mysqlnd extension which is primary driver for Mysql and it's reused by PDO_mysql at least

I'm still doing some changes on the mysqli module on GitHub - I'd like to have it pass tests there before moving the code here. Also, I think we should try to refactor there the parser code that was taken from doctrine/dbal to manage converting SQL statements from having named placeholders (':placeholder') to have positional placeholders ('?') - it's overkill for the specific purpose here.

Question: do we want to introduce a new 'mysqli' module, or shall we try to have a single 'mysql' module with two drivers, 'mysql' and 'mysqli'?

@lauriii posted some question in slack, added them, and the answers to the issue summary.

Question: do we want to introduce a new 'mysqli' module, or shall we try to have a single 'mysql' module with two drivers, 'mysql' and 'mysqli'?

Two drivers in module might be simpler, but:

1. Can we make it work where only one option is available due to installed extensions? And also not show drivers that won't work to people?
2. Would there be any need for the new driver to be experimental?

btw depending on the answers to this, there might be issue summary updates needed now.

Lauri pinged me to provide feedback as a product manager. These two statements are contradictory and are critical to asses the impact of the proposal I think :)

We need to continue to support PDO since not all hosting has php_mysqli enabled, so it has to be a new driver. However it might be fine to have both drivers in the same module.

vs

Add the PHP MySQLi extension as a requirement to the core MySQL and MySQLi database drivers.

IMHO there's no need to add the PHP MySQLi extension as a requirement to the core (PDO, existing) MySQL database driver.

That will obviously have to be a requirement for the NEW MySQLi driver, though.

The installability of a driver is driven by the installable() method in the Install/Tasks class of each driver.

For the existing PDO/MySQL driver it is inheriting from the base class

  public function installable() {
    return $this->hasPdoDriver() && empty($this->error);
  }

For the new driver it will be something like

  public function installable() {
    return extension_loaded('mysqli');
  }

I think in case if both drivers are available some description should explain benefits using mysqli
So on the most of hostings user could contact support to ask more performant driver

If benefits is obvious then we can add some logic to set "preferred driver" at install screen

I'll start adding the code from GitHub as a separate module, since I understand the driver may start as experimental, and I do not think we can have a module with a released driver and an experimental one in the same base.

OK, now code checks pass. I suppose the next step is to get tests run using the new driver, but I have no clue how this can happen. I assume infra need to do infra things?

I filed #3384661: Upgrade PHP 8.2 image to 8.2.10 → and as the new image will pass tests we can and mysqli extension

The extension already in the CI image

$ docker run --rm -it drupalci/php-8.2-apache:production php --ri mysqli

mysqli

MysqlI Support => enabled
Client API library version => mysqlnd 8.2.0
Active Persistent Links => 0
Inactive Persistent Links => 0
Active Links => 0

Directive => Local Value => Master Value
mysqli.max_links => Unlimited => Unlimited
mysqli.max_persistent => Unlimited => Unlimited
mysqli.allow_persistent => On => On
mysqli.rollback_on_cached_plink => Off => Off
mysqli.default_host => no value => no value
mysqli.default_user => no value => no value
mysqli.default_pw => no value => no value
mysqli.default_port => 3306 => 3306
mysqli.default_socket => no value => no value
mysqli.allow_local_infile => Off => Off
mysqli.local_infile_directory => no value => no value

@mondrake I think you can edit drupalci.yml to run only new module's testgroup

#50 no I think we need first to be able to install using the new driver. I.e. a new test environment option like Test PHP 8.1 & MySQL 5.7 via MySQLi. Editing drupalci.yml we can only get the new module test to run with the other PDO drivers under the hood.

Attention - the driver requires minimum PHP 8.1.2 where this was fixed.

@mondrake as I see last CI run https://dispatcher.drupalci.org/job/drupal_patches/201116/testReport/Dat...
there's mysqli tests running fine)

Posgres also supports async queries
https://www.php.net/manual/en/function.pg-send-query.php

Is there a chance that Drupal some day would have a driver for it?

I've updated the issue summary to remove the bit about the mysqli requirement on the PDO driver and to say we'll start with an experimental module but eventually merge the stable driver into the main mysql module.

@Chi a pgsql driver should be able to build on the work done here, so that seems possible if someone's motivated to work on it.

I think mysqli module should implement hook_requirements() and check if PHP MySQLI extension is available.

#56 maybe, but I do not think that's relevant. During early installation there's no module concept yet, and installability of a driver is checked as described in #43.

We already recommend PHP 8.1.6 or above. Easy to add a hard requirement on PHP 8.1.6 to the experimental module, but we could probably only add a hook_requirements() warning checking the database settings + PHP version once it's incorporated into the main mysql module.

I wonder if we should try to do the mysqli testing directly on gitlab CI rather than getting a new environment on DrupalCI since it's getting a lot closer to the point where core can switch over entirely - not sure exactly how close though and wouldn't want to artificially hold this up on that either.

I did some googling about PDO support for async and found the PDO brainstorming page: https://wiki.php.net/internals/pdo/brainstorming

If that's up-to-date, then it's been discussed, but nothing has happened for years. It's possibly that PHP Fibers support will increase demand and might spur some activity, but I think we're looking at more years before it would be supported, if ever.

Should we open a follow-up on how to handle the actual async support? For the BigPipe/Renderer implementations of fibers the main use case is to fire off a single async query, suspend the fiber so the calling code can do something else, then when we're resumed see if it came back. It looks like this might require multiple database connections (since 'something else' could also be executing database queries) so could be a bit tricky.

I can't see us needing or wanting to execute multiple queries async in core although support for that would be good to add at some point, maybe its own issue.

##6 Yeah I think better have that as a follow-up, the issue here I think it's to get a mysqli driver that is like-for-like the pdo_mysql one.

@mondrake: It looks good. Are you going to be the official maintainer of this new database ddriver?

Probably it will be easier to fix credentials in gitlabci → as drupalci_testbot has a lot of things hardcoded

• https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa...
• https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa...

Replied via slack, copying here too:

This is the way we are defining the database and variables for GitlabCI for core: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/b...

Note that this uses the images generated from https://git.drupalcode.org/project/drupalci_environments but does not use https://git.drupalcode.org/project/drupalci_testbot to run the tests.

We connect the images via the .gitlab-ci.yml file and just call the core/scripts/run-tests.sh file to run the tests, so everything you need should be able to be set here: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/b...

I *think* now on GitlabCI we can run tests on the new driver by only changing the database URL. Just for the sake of getting it run I changed in the MR from the PDO MySQL driver to mysqli, obviously the change would have to be structured moving forward.

Yes, a failure like

    PHPUnit Test failed to complete; Error: PHPUnit 9.6.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\Tests\migrate_drupal\Kernel\MigrateMissingDatabaseTest
    E                                                                   1 / 1
    (100%)R
    
    Time: 00:01.211, Memory: 4.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\migrate_drupal\Kernel\MigrateMissingDatabaseTest::testMissingDatabase
    mysqli_sql_exception: php_network_getaddresses: getaddrinfo for
    does_not_exist failed: Name or service not known
    

tells us that the pipeline used the new driver for testing.

There are quite a few things to fix looking at the pipeline failures.

Found a blocker. Filed 🐛 migrate\Plugin\migrate\id_map\Sql assumes a PDO db driver Active .

There are update tests failing because the database dumps contain the 'mysql' module installed, but since the test are run with 'mysqli', when updating we get a requirement error 'install the mysqli module that contains the database driver'.

Not sure how to tackle that.

it could use update hook to pass upgrade tests, or it will need common fixture to apply to all this tests

There are update tests failing because the database dumps contain the 'mysql' module installed, but since the test are run with 'mysqli', when updating we get a requirement error 'install the mysqli module that contains the database driver'.

Not sure how to tackle that.

I think we've had the same problem when the database driver modules were introduced in the first place, the way to fix it is to re-import the database dump into a real site, install the mysqli module, then re-export the database dump. Then it gets picked up when you try to update from the dump and everything works (or hopefully). You have to rewrite history to make it seem as if the mysqli module existed at the time of the dump - so it might mean manually copying the module folder back to the old commit hash that you import the database dump to, or potentially manually editing the uncompressed dump to add the correct lines etc.

Thanks for #71 (that's what I was afraid of :))

Tried manually updating the dump files, just to add 'mysqli' to the system.schema key_value collection (should suffice? module has no config), then compressing them with
$ tar -czf drupal-9.4.0.bare.standard.php.gz drupal-9.4.0.bare.standard.php

But now the import fails during the update test with
ParseError: syntax error, unexpected character 0x00, expecting end of file

It could be some wrong way of calling tar?

Just throwing here if anyone has faced this earlier and has a ready-made answer

#72 actually found #3305003: Create standard steps for creating database dumps → .

FTR, the changes to the database dumps:

1. gunzip core/modules/system/tests/fixtures/update/drupal-9.4.0.[type].standard.php.gz ([type] three of them, bare, filled and phpass)

2. Edited and manually added the module: add 'mysqli' to the system.schema key_value collection and to the extension.core collection

3. gzip core/modules/system/tests/fixtures/update/drupal-9.4.0.[type].standard.php

Note - do not use tar for the purpose as it fills files with null characters at the end

We are almost there. There are still a handful of installer test failures, again because an attempted uninstall of the 'mysql' module fails since it is a dependency of the new module, and 2 kernel tests that fail on MySql 8 but not on MySql 5.7.

So, this is passing on MySQL 5.7 now, but not on MySQL 8 because of this test failing:

PHPUnit 9.6.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\KernelTests\Core\Database\BasicSyntaxTest
    ...E...                                                             7 / 7
    (100%)
    
    Time: 00:05.293, Memory: 4.00 MB
    
    There was 1 error:
    
    1) Drupal\KernelTests\Core\Database\BasicSyntaxTest::testConcatWsFields
    Drupal\Core\Database\DatabaseExceptionWrapper: Illegal mix of collations
    for operation 'concat_ws': SELECT CONCAT_WS('-', ?, "name", ?, "age") FROM
    "test48178452test" WHERE "age" = ?; Array
    (
        [:a1] => name
        [:a2] => age
        [:age] => 25
    )
    
    
    /builds/issue/drupal-3259709/core/modules/mysqli/src/Driver/Database/mysqli/ExceptionHandler.php:56
    /builds/issue/drupal-3259709/core/lib/Drupal/Core/Database/Connection.php:858
    /builds/issue/drupal-3259709/core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:67
    /builds/issue/drupal-3259709/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    Caused by
    mysqli_sql_exception: Illegal mix of collations for operation 'concat_ws'
    
    /builds/issue/drupal-3259709/core/modules/mysqli/src/Driver/Database/mysqli/Statement.php:84
    /builds/issue/drupal-3259709/core/lib/Drupal/Core/Database/Connection.php:826
    /builds/issue/drupal-3259709/core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:67
    /builds/issue/drupal-3259709/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    ERRORS!
    Tests: 7, Assertions: 8, Errors: 1.

I cannot reproduce this locally; can it be it's some setting of MySQL8 on the runner?

Also, Drupal\Tests\mysqli\Kernel\mysqli\LargeQueryTest behaves erratically, if I remove the dump() in it then the statement errors under destruction (?).

Apart from these two, I think this can go through more heavy-lifting reviewing.

Some help would be appreciated on the GitlabCI set-up to make this run independently from the PDO-Mysql tests; the current changes are only a shortcut to make test run with mysqli instead of pdo_mysql.

I added the async job as a separate job and left the others as they were. Note that this issue contains 🐛 _TARGET_DB_TYPE does not exist Active that will be committed shortly, so we might get a conflict on the rebase. I can take care of it.

We might want to call it 'MYSQLI' instead of 'ASYNC' but otherwise that looks great.

Now I understand the logic, thanks a lot @fjgarlin.

How about generalizing more, and instead of DB_ASYNC, add two more variables

_TARGET_DB_DRIVER
_TARGET_DB_DRIVER_MODULE

and use them too in building the database URL?

@mondrake - we initially had some more variables around the database type and version, but wanted to simplify the job definition as much as possible, so it's easy to understand and to create new jobs. Having said that, it's an option.

Right now, we only added an optional variable, which I think keeps the simplicity of new jobs. I renamed it as suggested in #78 but feel free to rename it again if there is a better name.

I had some misgivings on user space parsing of SQL but there's nothing better we can do: I hoped we could reuse the parser in PDO. But as far as I can tell, this is only called on execute and even then only when PDO::ATTR_EMULATE_PREPARES is set to true and then the placeholders are replaced by the arguments not ?. There's no doubt PDO creates the ? placeholder'd query because MySQL API only supports that but if it is exposed anywhere, I can't figure out where, PDOStatement::debugDumpParams does not display it. (I ran all four combinations of MYSQL_ATTR_DIRECT_QUERY / ATTR_EMULATE_PREPARES )

working on a unit test for NamedPlaceholderConverter

Added tests for NamedPlaceholderConverter.

@chx thanks for #81.

Clearly, NamedPlaceholderConverter is not optimal: it destroys and rebuilds a SQL statement that was built already.

A solution to try and retrieve from pdo_mysql something to be used by mysqli is for me ... weird. It'd keep PDO as a required PHP extension, and would just move the parsing from our class to PHP internals. If (if, I dunno) that would also require establishing a client connection it would be even more weird.

On the other side, IIUC mysqli is meant to be a very thin client. Expecting it to implement positional placeholders would be against its concept.

Personally, I think we should aim at trying to embed alternative placeholder generation (positional/named) into dynamic queries instead - and avoid reparsing post-factum. It would definitely increase performance. Then, we should be able to still do the placeholder conversion for static queries - allowing an option to be passed to ::query(). But that would be D11 material IMHO.

I have talked to @longwave, @alexpott and @catch about the new MySQLi database driver. The plan that we came up with is the following:

1. The MySQLi module goes into core as experimental. The is important because we need the freedom to change anything in this module that we would like to change without triggering a BC break. Also when users install a new Drupal site they need to see this module as experimental instead of it being a regular module.
2. The next phase for the module is to add all the parallel query functionality to make the PHP Fibers part work for the database queries. Maybe add other improvements? After this is done we are going to ask expert Drupal users to test out the database driver.
3. When a number of expert Drupal users have been using this database driver without any problems and all other bugs have been fixed that are the result of using the MySQLi database driver and the PHP mysqli extension, the module can be marked as stable.
4. When the module is marked as stable, we can start informing site owners that we have a new database driver and what is improved compared to the existing MySQL driver. The only thing that existing site owners need to do start using the MySQLi driver is to update their database driver settings in settings.php. Change 'driver' => 'mysql to 'driver' => 'mysqli'. Just adding the i to mysql. For new site installs the MySQLi database driver becomes the default option.
5. When the MySQLi database driver is ready for becoming the default driver for MySQL, the are going to remove all code from the MySQL module that is overridden in the MySQLi module and all code from the MySQLi module is copied to the MySQL module. All override code in MySQLi module will be removed. The MySQL and the MySQLi module are now the same. The MySQL module will stay the default module for a MySQL database. The MySQLi module will now be marked as deprecated. Now all site owners who are using a MySQL or equivalent are now using the code from the original MySQLi module. Existing site owners who did not change to the MySQLi driver do not have to do anything and they are now using the code from the original MySQLi driver. Site owners who changed their site to switch to the MySQLi module shall need to change back to the default MySQL driver. We are now finished!

To do in this patch:
- Make it is experimental. Add testing for this.
- Add in the autoloading of database driver module dependencies an override to when the driver is "mysqli" to also autoload the "mysql" module. This has the advantage that existing site owners can switch to the MySQLi driver without adding the autoload dependency to the database driver settings to the settings.php file. Do the same for the "autoload" and "namespace" settings. Lets make it easy for site owners to try out the MySQLi driver and revert back to MySQL driver. Add testing for this.

From a release management point of view the plan in #85 looks good to me. I suggest also opening a plan issue for the mysqli driver with the steps from #85 so we have them documented as what is left after this issue is committed.

I have talked to @laurii in person and as long as the module is hidden for new site install as long as the module is experimental, am I allowed to remove the "Needs product manager review". Updated comment #85.

Thanks for #85! Looks clear to me. Re. #85.2, I suggest to add #84

embed alternative placeholder generation (positional/named) into dynamic queries

to the meta plan suggested in #86.

Now, we have a failure in the recently fixed SqlContentEntityStorageRevisionDataCleanupTest.

This is due to this code

    $result = $connection->query('SELECT nid, vid, langcode FROM {node_field_revision} WHERE nid = :nid', [
      'nid' => 8,
    ])->fetchAll();

The arguments in the queries of this test DO NOT HAVE the colon in front, i.e. it should be ':nid' => 8.

Apparently the PDO-based drivers do not care, even if https://www.drupal.org/docs/drupal-apis/database-api/static-queries#plac... → examples clearly indicate placeholders should have the colon.

I'm not sure whether we should fix the test, or relax NamedPlaceholderConverter to also allow placeholder's array keys not to have a colon.

Done the two points from #85 that are for this issue,

• Make it is experimental.
• Add in the autoloading of database driver module dependencies an override to when the driver is "mysqli" to also autoload the "mysql" module.

but tests are missing. Any idea/reference on how to do those?

Just a note I have ✨ [PP-1] Async database query + fiber support Active open for the next step (or next two steps, depending on how much async support we add in one issue vs. two).

Updated IS with a list of todos remaining here; feel free to add what is necessary.

Also help on the points would be appreciated.

Re #89 I eventually thought that even if SqlContentEntityStorageRevisionDataCleanupTest should be probably fixed, nevertheless it's better allow parameter arrays with keys missing the initial colon, given that PDO seems to be supporting both. So we can support contrib code that might have missed this detail.

Fixed the collation bug affecting BasicSyntaxTest in MySQL 8. I had to move the test to driver-specific, and for mysqli force conversion of the parameters' values to utf8mb4. Not sure it's the right thing to do but could not find other options. It's an edge case anyway, and could not replicate the issue locally either.

Added test to ensure module is experimental and hidden.

Postponing on the referenced issues, that will make the management of the lack of transactional DDL support consistent across the two drivers.

Maybe the module should not be hidden and removed from the database selection during the install process. We want to allow site owners to experiment with the mysqli database driver and therefor the module should be visible on the module list.

Uhm... I don't remember how it works. Definitely, a 'module' extension is different from a 'database_driver' extension. The list of database to be presented for installation is independent from the modules, simply because there's is even no notion of the 'module' at that stage of the installation process. So, it could be the database driver 'mysqli' is presented for (a grassroot) installation if the PHP mysqli extension is available, even if the module itself is hidden. Then, during the (grassroot) installation, the mysqli module is installed automatically (even if hidden). But since it's hidden, there will be no way to uninstall/install it from the UI (that, when it's accessed, presumes all the installation, config and bootstrap to be complete).

Worth giving a manual test to all this process, I guess.

Then, if the expectation is to just allow changing 'driver' => 'mysql', to 'driver' => 'mysqli', in settings.php to play around, then we have more to do - i.e. disallow reading the namespace from settings.php in this case (because that will be still the mysql one, and an existing 'namespace' key in the settings takes priority on the 'driver' key), and allow manually installing the mysqli module (so removing the hidden: true from the module yaml).

Thanks for your reviews and input, @daffie. Would you mind checking back the inline comments and replies and not which ones we can mark as solved from your POV, so we can just keep the open ones to work on?

Filed 📌 [PP-2] Enable dynamic queries to produce SQL with positional placeholders Postponed  for follow up.

Rebased, with the exception of database dumps which is a pain in the neck to do; that update IMHO should be done just before commit.

The mysqli construction approach has limited ability to define an SSL connection, but leveraging the procedural approach allows you to establish options prior to making a connection.

For example:

      $mysqli = \mysqli_init();
      // Reusing PDO keys for simplicity and commonality with the `mysql` driver
      $mysqli->ssl_set(
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_KEY],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CERT],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CA],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CAPATH],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CIPHER]
      );

      $mysqli->real_connect(
        $connection_options['host'],
        $connection_options['username'],
        $connection_options['password'],
        $connection_options['database'] ?? '',
        !empty($connection_options['port']) ? (int) $connection_options['port'] : 3306,
        $connection_options['unix_socket'] ?? '',
        MYSQLI_CLIENT_SSL // This can likely be made through a connection option
      );

Which issue is this postponed on? I can't see anything listed in the issue summary.

It's currently postponed on ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active per #97.

Thanks added a note to the issue summary.

✨ [PP-1] Introduce a Connection::executeDdlStatement method Active has landed.

Looks it still blocked on 📌 Replace \PDO::FETCH_* constants to indicate fetch mode with an enumeration Active