Make SQLite faster by combining multiple inserts and updates in a single query

13: drupal-8.x-2031261-13.patch queued for re-testing.

This is related to the new drupalci_testbot project, which stores simpletest data in sqlite. Discovered this when the postgresql test bot couldn't stuff all the errors into simpletest table.

# --sqlite database is used for the test runner only (and only contains the simpletest module database schema)

The new drupalci_testbot project can use both options, installing via "drush" or "none". This last one runs tests with sqlite.

DCI_INSTALLER="none" # Try to use core non install tests. The other option is "drush".

so i'm running a test with :

~/drupalci_testbot# ./scripts/build_all.sh refresh pgsql-9.1
~/drupalci_testbot# sudo \
DCI_PATCH="https://www.drupal.org/files/issues/drupal-2181291-pgsql-aborted-transaction-15.patch,." \
DCI_DBTYPE='pgsql' \
DCI_DBVER='9.1'  \
DCI_TESTGROUPS='--all' \
DCI_DRUPALBRANCH='8.0.x' \
DCI_VERBOSE="true" \
DCI_CONCURRENCY="8" \
DCI_INSTALLER="drush" \
./containers/web/run.sh

Would it be possible to write a test that illustrates the current failing behavior?

patch #12 worked for me ... at first
but then, after a drush cc all, I got errors on rules & WSOD

 137032  21/Sep 09:28  warnin  php  Warning: Creating default object from empty value in RulesEntityController->attachLoad() (line 57 of 
                       g            /var/www/drupal/sites/all/modules/contrib/rules/includes/rules.core.inc).                     
 137033  21/Sep 09:28  notice  php  Notice: unserialize(): Error at offset 17 of 20 bytes in EntityAPIController->load() (line 261 of    
                                    /var/www/drupal/sites/all/modules/contrib/entity/entity/includes/entity.controller. 

I tried drush rr but in vain.
If I remove the patch, the error disapear.

I am having this issue with D8.1.x-dev

https://www.drupal.org/node/2746027#comment-11281037

This is really serious. Can someone please help? I have a site that needs to go live and now the thing has this major issue. I'm in a serious situation here having spend many many hours building the site to this point only for this to happen. There is no way I can start from scratch and need to know how to fix this.

Thank you.

m

@lilbebel: If this issue is so important to you, can you do a review.

Daffie, thank you for comment. I cannot do a review as I am not a developer and it is beyond my skill set. I was referred to this page by a senior member of Drupal and advised to push the matter as it is serious. It won't only affect me. This is already established as being a major issue by senior developers. This is not just about me. This is a major issue that will affect many. The tone of your comment seems curt which is not helpful or appropriate. I am simply asking for help from those who know more than I do. If you did not intend it to sound so, I apologize. I wish that I could do such coding tasks but we are not all coders.

@lilbebel: It is normal for people on Drupal to work on issues that they find interesting to them or issues they want to get fixed. With that being the normal work ethic on Drupal, I suggested that you can help this issue by doing a review. I did not know your skill set and I was not out to be negative to you in any way. I am sorry if I came over to you in a negative way. Also I would like to mentioning that English is not my native language. So my expression in English is not always as good as I would like it to be.

@lilbebel English is my native language and I write things like #31 all the time. That is a normal part of working in an open source community. I did not interpret it as snarky at all.

Daffie, I apologize. My bad. Sometimes, things come across differently in writing than they are intended. I am doing my best to try to find solutions and contribute but am not equipped to write patches and such. I have a theming and creative background. I would love to contribute more if I could.

My server provider looked into things on my end and, I don't know if it helps but, they said the following:

I was able to recreate the error, but nothing is wrong with the database as per the MySQL checks that I've performed via the phpMyAdmin. Also as far as I can see the error is registered in the Apache error log which is rather odd since MySQL has nothing to do with Apache error log.

I've tried changing the PHP versions, but nothing has fixed this issue.

Maybe this information is relevant? If there is anything I can do to help, I am more than happy to.

Thank you all.

M

@lilbebel - if you are using MySQL then this issue is not relevant to you. You are using MySQL or sqlite as the database? In general I don't think sqlite is really thought to be suited for production sites.

@pwolanin - I'm using:

Database system SQLite
Database system version 3.8.10.2

On my dev desktop version, it would only allow me to use SQLite. Is it possible to change it now without redoing the site?

Many thanks.

M

@axel.rutz: I would like to help you get this issue fixed. If you work on the patch I will do the reviewing. What I found in the first review:

1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
   @@ -24,64 +24,75 @@ class Statement extends StatementPrefetch implements StatementInterface {
   +    $quote_value = function($value) use($dbh) {
   ...
   +        $get_replacement = function() use($args, $quote_value) {
   ...
   +        $get_replacement = function($match) use($args, $quote_value) {
   

   I am not a great fan of inline functions. I find them difficult to read. Do we really need 3 of them in the function getStatement().

2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
   @@ -24,64 +24,75 @@ class Statement extends StatementPrefetch implements StatementInterface {
   +        $query = preg_replace_callback('/:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\b/u', $get_replacement, $query);
   

   Can you explain how this line of code works and why it is necessary.

3. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/SqliteStatementTest.php
   @@ -0,0 +1,89 @@
   +class SqliteStatementTest extends UnitTestCase {
   

   Can we use KernelTestBase instead of UnitTestBase. KernelTestBase has already a database connection.

If somebody else wants to work on this patch that would also be great.

Lets move this to the SQLite driver.

@alex.rutz: I think that you are right with that it is difficult/impossible to distinct between strings and BLOB's.
I have updated your test patch from comment #27 to use KernelTestBase. The problem is that it passes without any changes to core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php. So am I doing something wrong or is there no problem?

I found out why the test where passing on my local machine. I am using Ubuntu and they have raised the value of SQLITE_MAX_VARIABLE_NUMBER to 250000. The default is 999 and that is the root cause of this issue.
Created a new patch with tests for integers, floats, strings and blobs. I also added a small change to the Statement class to see if it helps.

The tests only patch should fail. So increasing the number of variables to 2000.

@axel.rutz: Do you have any insight why the tests only patch is not failing?

The testbots are also running Ubuntu. Could that be it?

Static variables should be addressed differently.

The tests are now fixed with the patch from comment #49.

New patch with the fix from comment #23 made by alex.rutz and the new tests.

Tests were not good: "Fatal error: Undefined class constant 'number_of_variables'".

Just to make sure that the fix from comment #43 does not work.

The tests only patch from comment #54 is the one we need for further testing any possible fixes.

And maybe we just use this to decide string/blob quoting.
mb_check_encoding($s,"UTF-8")

I think Unicode::validateUTF8 would work for this if Multibyte support is not available. It uses PCRE though.

If someone writes a patch for the fix, I will do the reviewing.

@alex.rutz: Can you give some more directions for the extra tests that you would like.

This blocks usage of *config_installer*

INSERT OR REPLACE INTO
{cache_config} (cid, expire, created, tags, checksum, data,
serialized) VALUES (:db_insert_placeholder_0,........

No very big site but have a 2429 variables, including config translations

Looks last comments about utf-detection needs separate issue, let's make this issue focused of variables

Re-rolled.

I think that we should use a change of tactics to fix this problem. I think that we should split up a insert or update statement with a large number of variables in multiple statements with less then 999 variables each.

I think this might be the cause of #3049948: SQLSTATE[HY000]: General error: 1 too many SQL variables: INSERT OR REPLACE INTO {cache_content} (cid, expire, created, tags, checksum, data, serialized)?

@davidwbarratt yep, all of them face with limit

Ah. Thanks.

Unfortunately the work-around doesn't work if you are using the Drupal Docker Image because there isn't a way to recompile PHP without starting an image from scratch. :(

That's a matter of sqlite configuration of build so vary per os package, you can always rebuild sqlite with needed options as workaround

I’m hitting this too. One possible mitigation—not a fix—that occurs to me is maybe we could make SQLite insert/update queries “batchable” under the hood. So, if you are about to run a query that exceeds 999 variables, the query is internally split up into two separate queries, 500 variables each, and executed one after the other. Not sure if this would work at all, or even be desirable, but it’s a thought.

@phenaproxima not sure it will help because each distro has own limits (SQLITE_MAX_VARIABLE_NUMBER) and I found no way to get this limit at runtime

The good news is that most of distros using to increase this limit to 250k

Moving this to 9.0.x to fix there first, then we can discuss backporting to 8.

Here's an approach that I think will work. Just posting the patch for now, and will follow up tomorrow with a comment explaining it.

As I'm using sqlite everyday I will prefer to have 999 is not hardcoded limit.
I don't wanna patch every app to replace this limit in code for sane OS I'm using or for docker php image

@effulgentsia interesting approach! Just a question of how to make it opt-in/opt-out

When we create the connection to the SQLite database we can read the max variable limit and use that value instead of the 999 as the hardcoded value.

@effulgentsia: Great to see a new approach.

In #3107155: Discuss lowering SQLite version requirement from 3.26 to 3.22 in Drupal 9 we were discussing warning people (in system_requirements()) about the low variable limit. Looks like even with the workaround in place, performance would be more optimal if the variable limit would not be in place(?) If so, adding that warning alongside the workaround here would be best IMHO.

@effulgentsia: Great new solution idea!

1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Condition.php
   @@ -0,0 +1,36 @@
   +    if (count($values) > 50) {
   

   Can we create a protected class variable for this?

2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Condition.php
   @@ -0,0 +1,36 @@
   +      $value_fragment[] = "select value from json_each($placeholder)";
   

   Could we add some documentation about this line and what it does.

3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Upsert.php
   @@ -27,4 +27,32 @@ public function __toString() {
   +    // SQLite has a default limit of 999 placeholders, so upsert in batches
   +    // less than that.
   

   Can we use the json variable solution for this instead of using batches?

4. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
   @@ -70,6 +70,42 @@ public function testMultiInsert() {
   +   * This test is motivated by SQLite's default limit of 999 placeholders, but
   +   * it's good to ensure that all database drivers can handle large inserts.
   

   The limit of 999 variables for SQLite can be set higher. Lets change the test by getting the actual maximum number of variables from the database + 1 and use that number for testing.

5. I am missing testing for the merge queries.

If I add the following tests to Drupal\KernelTests\Core\Database\SelectTest::testLargeInCondition(), those extra tests should pass.
However they do not. See patch 2031261-91-nested-condition-should-fail.patch

+    // Find the above 3 Beatles with a nested condition.
+    $condition = new Condition('AND');
+    $condition->condition('name', $names, 'IN');
+    $num_records = $this->connection->select('test')
+      ->condition($condition)
+      ->countQuery()
+      ->execute()
+      ->fetchField();
+    $this->assertEqual($num_records, 3);
+
+    // Find the other Beatle with a nested condition.
+    $condition = new Condition('AND');
+    $condition->condition('name', $names, 'NOT IN');
+    $num_records = $this->connection->select('test')
+      ->condition($condition)
+      ->countQuery()
+      ->execute()
+      ->fetchField();
+    $this->assertEqual($num_records, 1);

The patch from @effulgentsia from comment #86 adds driver specific code to the Drupal\Core\Database\Query\Condition class.
The problem is that the database sub-system add the moment does not do driver specific conditions.
And we need that to fix this issue in the correct way. In #3113403: Make Drupal\Core\Database\Query\Condition driver overridable we make the condition driver overridable.
Added the patch 2031261-91-combined-with-3113403-16.patch to see that a combined patch fixes the problem.

Postponing this issue on #3113403: Make Drupal\Core\Database\Query\Condition driver overridable.

Is there a minimum number of parameters that is considered universally acceptable? Microsoft SQL server has a limit of 2100. Would this driver also need extra parameter handling? I believe it currently performs some “emulate prepares” operations manually in order to overcome this limit. I’d like to simplify the driver wherever possible.

Microsoft SQL server has a limit of 2100

@Beakerboy: I do not think that will be enough. I am working on getting a testbot for custom database drivers. So that we can see what the problems are for your driver. These 2 issues are needed for such a testbot: #3109761: Allow custom db, php and chrome docker images from non-drupalci docker hub profiles and #3110803: Allow contrib modules to run the tests for Drupal core. Both are ready for a review and have an explanation of how to test the patch.

I do not think that will be enough

Is there a test for this somewhere? The driver is able to fully install core, and passes all but 2 database kernel tests, and it passes all of the Entity kernel tests. The two failures are related to casting varchar to binary fields. Are there other part of the test suite that I should run on my Travis-CI VM?

@Beakerboy: The problems are with the importing of configuration data. Not sure which test it is.

Sorry wrong issue. Please ignore.

#3113403: Make Drupal\Core\Database\Query\Condition driver overridable has landed.

Reroll needed.

#3113403: Make Drupal\Core\Database\Query\Condition driver overridable has been unlanded.

#3113403 is ready for review again, if anyone wants to keep moving this forward.

#3113403: Make Drupal\Core\Database\Query\Condition driver overridable has landed.

Rerolled the patch.

Back to 9.0.x, because #3113403: Make Drupal\Core\Database\Query\Condition driver overridable has been landed on 9.0.x

Used the same trick for upsert queries as @effulgentsia used for the select queries.

Nice!

What do you think of this, as a way to make the execute() method clearer?

@effulgentsia: Yes, you are right your changes to the execute() method are an improvement. Thanks. What else needs to be done to get this issue committed?

Would like to have an 8.8.x patch to test against with.

Fix coding standards only in #110

(Wrong interdiff file extension, sorry!)

missed one

-      $json_value = json_encode($values, 0 ,1);
+      $json_value = json_encode($values, 0, 1);

Added testing that counts the number of placeholders used in the query.

@jungle: Thanks for the coding standards errors.

Minor fix.

To answer my own review from comment #90:

For #90.1: Lets keep 50 as a sensible default.

For #90.2: Done.

For #90.3: Done.

For #90.4: Added testing that the queries one use one placeholder.

For #90.5: Was not necessary.

The patch is ready for a review from somebody else.

Back to needs review

It needs profiling because more code executed each query

Also contrib fallback driver could be another workaround

Looks like we can go all the way back to 8.9.x with this as well based on #3113403: Make Drupal\Core\Database\Query\Condition driver overridable.

The XHProf testing for the patch contains 2 parts: one for the select query and the second for the upsert query.

XHProf testing the select query

The part of the code that I used for XHProf testing the select query:

  public function testLargeInCondition() {
    tideways_xhprof_enable(TIDEWAYS_XHPROF_FLAGS_MEMORY | TIDEWAYS_XHPROF_FLAGS_CPU);
    $names = [];
    $names[] = 'John';
    for ($i = 1; $i < 500; $i++) {
      $names[] = "Name $i";
    }
    $names[] = 'George';
    for ($i = 501; $i < 1000; $i++) {
      $names[] = "Name $i";
    }
    $names[] = 'Ringo';

    // Find the above 3 Beatles.
    $num_records = $this->connection->select('test')
      ->condition('name', $names, 'IN')
      ->countQuery()
      ->execute()
      ->fetchField();
    $this->assertEqual($num_records, 3);

    // Find the other Beatle.
    $num_records = $this->connection->select('test')
      ->condition('name', $names, 'NOT IN')
      ->countQuery()
      ->execute()
      ->fetchField();
    $this->assertEqual($num_records, 1);

    // Find the above 3 Beatles with a nested condition.
    $condition = $this->connection->condition('AND');
    $condition->condition('name', $names, 'IN');
    $num_records = $this->connection->select('test')
      ->condition($condition)
      ->countQuery()
      ->execute()
      ->fetchField();
    $this->assertEqual($num_records, 3);

    // Find the other Beatle with a nested condition.
    $condition = $this->connection->condition('AND');
    $condition->condition('name', $names, 'NOT IN');
    $num_records = $this->connection->select('test')
      ->condition($condition)
      ->countQuery()
      ->execute()
      ->fetchField();
    $this->assertEqual($num_records, 1);

    file_put_contents(
        sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.sqlite-variable-limit-selecttest-with-patch.xhprof',
        serialize(tideways_xhprof_disable())
    );

I ran the XHProf testing 5 time with and without the fix. For the select testing without the patch, I removed the method Drupal\Core\Database\Driver\sqlite\Condition::compileValueList().
The median result for XHProf testing with the patch is:

Overall Summary	
Total Incl. Wall Time (microsec):	16,317 microsecs
Total Incl. CPU (microsecs):	15,537 microsecs
Total Incl. MemUse (bytes):	110,808 bytes
Total Incl. PeakMemUse (bytes):	108,088 bytes
Number of Function Calls:	587

The median result for XHProf testing without the patch is:

Overall Summary	
Total Incl. Wall Time (microsec):	147,901 microsecs
Total Incl. CPU (microsecs):	144,112 microsecs
Total Incl. MemUse (bytes):	110,824 bytes
Total Incl. PeakMemUse (bytes):	634,832 bytes
Number of Function Calls:	4,587

My conclusion is that the select test with the patch is significantly faster.

XHProf testing the upsert query

The part of the code that I used for XHProf testing the select query:

  public function testLargeUpsert() {
    tideways_xhprof_enable(TIDEWAYS_XHPROF_FLAGS_MEMORY | TIDEWAYS_XHPROF_FLAGS_CPU);
    $num_records_before = $this->connection->query('SELECT COUNT(*) FROM {test_people}')->fetchField();

    $upsert = $this->connection->upsert('test_people')
      ->key('job')
      ->fields(['job', 'age', 'name']);

    for ($i = 0; $i < 1000; $i++) {
      $values = [
        'job' => "Job $i",
        'age' => $i,
        'name' => "Name $i",
      ];
      $upsert->values($values);
    }

    $upsert_cloned = clone $upsert;

    $upsert->execute();

    $num_records_after = $this->connection->query('SELECT COUNT(*) FROM {test_people}')->fetchField();
    $this->assertEqual($num_records_before + 1000, $num_records_after, 'Rows were inserted properly.');

    $person = $this->connection->query('SELECT * FROM {test_people} WHERE job = :job', [':job' => 'Job 0'])->fetch();
    $this->assertEqual($person->job, 'Job 0', 'First job set correctly.');
    $this->assertEqual($person->age, 0, 'First age set correctly.');
    $this->assertEqual($person->name, 'Name 0', 'First name set correctly.');

    $person = $this->connection->query('SELECT * FROM {test_people} WHERE job = :job', [':job' => 'Job 999'])->fetch();
    $this->assertEqual($person->job, 'Job 999', 'Last job set correctly.');
    $this->assertEqual($person->age, 999, 'Last age set correctly.');
    $this->assertEqual($person->name, 'Name 999', 'Last name set correctly.');

    file_put_contents(
      sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.sqlite-variable-limit-upserttest-without-patch.xhprof',
      serialize(tideways_xhprof_disable())
    );

I ran the XHProf testing 5 time with and without the fix. For the upsert testing without the patch, I removed the method Drupal\Core\Database\Driver\sqlite\Upsert::execute().
The median result for XHProf testing with the patch is:

Overall Summary	
Total Incl. Wall Time (microsec):	36,639 microsecs
Total Incl. CPU (microsecs):	30,819 microsecs
Total Incl. MemUse (bytes):	534,344 bytes
Total Incl. PeakMemUse (bytes):	940,064 bytes
Number of Function Calls:	1,208

The median result for XHProf testing without the patch is:

Overall Summary	
Total Incl. Wall Time (microsec):	206,110 microsecs
Total Incl. CPU (microsecs):	188,928 microsecs
Total Incl. MemUse (bytes):	531,592 bytes
Total Incl. PeakMemUse (bytes):	1,633,552 bytes
Number of Function Calls:	1,206

My conclusion is that the upsert test with the patch is significantly faster.

For both the select testing and the upsert testing is the solution with the patch significantly faster.

Great news! 3.32 released and brings limit increase to 32766, also new opcache for prepared statements https://sqlite.org/releaselog/3_32_0.html

Microsoft SQL Server has a limit of 2100 parameters. One way the driver gets around this is is performs upsert queries in batches and within a transaction. If each statement has 10 fields, then the batch size is 200 statements.

@Beakerboy: Can you use the solution from this issue for the SQL Server?

I’ll have to look at the patch closer so see how it works. I don’t get it, but I’ve never used json in sql before.

The large IN clause test will work on SQL Server because it uses EMULATE_PREPARES when a SELECT statement has >2100 parameters.

Hopefully someone has filed a bug report with the PHP group regarding the PDO EMULATE bug. I found a PDO but with the way it manages the ESCAPE part of LIKE statements. They said the fix would have to wait for PHP8!

It could use follow-up to make 50 configurable via connection options, often it much easier to build sqlite for own needs in docker image this days

Profiling has been added.

To prevent excessive memory allocations, the maximum value of a host parameter number is SQLITE_MAX_VARIABLE_NUMBER, which defaults to 999 for SQLite versions prior to 3.32.0 (2020-05-22) or 32766 for SQLite versions after 3.32.0.

-- From/See https://www.sqlite.org/limits.html

The default value is 32766 nowadays. So should this be a won't fix?

(Edited: @andypost mentioned it in #128 already)

@jungle I would still very much like land this issue. I know that 32766 is a high number, only the patch fixes it for any number of variables.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Upsert.php
@@ -17,17 +24,76 @@ public function __toString() {
+    // SQLite has a default limit of 999 placeholders. Assuming fewer than 20
+    // fields per row, if fewer than 50 rows are being upserted, then the

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
@@ -614,4 +614,89 @@ public function testEmptyInCondition() {
+   * Tests queries with >1000 items in an IN list.
...
+   * This test is motivated by SQLite's default limit of 999 placeholders, but
...
+    for ($i = 501; $i < 1000; $i++) {

Thanks, @daffie. If so, I think the test needs updating, the default is not always 999. It might be 32766.

Because #3159073: Use the new UPSERT capability from SQLite 3.24 landed, the patch needed to be rerolled and updated (the query changed).

@jungle: The default limit of 999 placeholders is the current limit for the minimum supported version of Drupal. It will probably change in D10. Only that is still more then a year from now and it is also not that important as the test makes sure that the query one uses 1 placeholder. The other reason why I would like this patch to land is because when working with config queries, they are with this patch a lot quicker. See comment #124. Could I interest you in doing a review of the patch?

Additionally to #134 not clear why ATTR_EMULATE_PREPARES makes sense to use (pgsql and mysql drivers using it) for sqlite.

Original bug is fixed via unbundling of sqlite in PHP 7.4 https://bugs.php.net/bug.php?id=79269

The related still needs work https://bugs.php.net/bug.php?id=79272 (reproducible)

moreover the error message has change in php8 https://3v4l.org/oke3S

#137 is failing in 9.3.x branch. Is there any latest patch available for this?

Needs a reroll

Re-rolled patch against 9.4.x

Several phpcs failures

Fixed coding standard failures

#145 is failing for drupal 9.4.0-rc2 version. Please let me know if any other patch is compatible with 9.4.0-rc2.

Try this patch, rerolled for version above 9.4.0

Moving for review.
Thanks

Fixed failed commands on #152
Attached patch against Drupal 9.5.x

Thought I would fix the tests.

This patch fixes two namespaces errors, adds types to some @params and adds a constructor in \Drupal\sqlite\Driver\Database\sqlite\Upsert to remove the return option.