PostgreSQL: Fix system\Tests\Cache\DatabaseBackendUnitTest

This is in the critical path just like cache get, where we explicitly avoid using the select method ( see DatabaseBackend).

Why is this a problem but that one isn't? Why doesn't every single test fail because of this? Maybe they implicitly rely on set() being called first?

It all looks good to me.
I can confirm that the test fails for postgreSQL and with the patch the test passes for postgreSQL.
As far as I can see the code changes but does the same.
So for me it is RTBC.

My questions have not been answered yet.

This would've been fixed by #2371709: Move the on-demand-table creation into the database API :/

Also using select here is performance regression because query alter overhead.
The change should be measured.

Would wrapping it in a transaction be worse for performance? If the query is not guaranteed to succeed, then it should be wrapped in a transaction for PostgreSQL (and MySQL strict mode).

I looked into driver-specific code, but I think that's too expensive for PostgreSQL because I had to preg_match all select queries run through query. This is just one query. High-performance sites hopefully will be running a non-database backend for caching.

I think band-aid fixing this now is fine because the other issue stalled out.

As commented before, I would still like to understand why this fails but the actual backend works, which uses an absolutely identical pattern? And why does it *only* fail in that kernel test, and not everywhere? We have to run into this scenario after *every* installation?

If you look at GenericCacheBackendUnitTestBase::getCacheBackend(), that explicitly calls deleteAll(), which should be not needed, because we should be able to assume that any backend will respect the test environment and be empty already. However, if you look at deleteAll(), it does a truncate query and contains the table-creation logic as well, my guess is that creates the cache table?

But that doesn't explain what creates the for web tests and real installations?

I spent about 9 months going around and around debugging the install sending garbage queries before I gave up and wrapped Select queries in implicit commit save points (#2001350: [meta] Drupal cannot be installed on PostgreSQL and #2181291: Prevent a query from aborting the entire transaction in pgsql).

If I had to guess I think that because KernelTestBase doesn't run through the install it doesn't emulate the same behavior, and then it tries to get cachetags table, which fails, aborts the transaction because there's no implicit commit, and the test fails. DatabaseBackendTagTest calls db_select() on cachetags which will be wrapped in the savepoint handling by Select class so that's why that does not fail.

I think you're correct and we could also add a transaction around the actual cache table select to be safe, but the less amount of these transactions the better in my opinion. Could it be possible that a contrib module that wants to help manage cache could be in the same situation, switch to database back end, and try to get cache table immediately? I don't see a specific test for swapping out the cache backends and immediately trying to get cache, but I don't know if we already have coverage for that somewhere else.

I'd rather stick with the approach of adding transaction/savepoint for now until the cache backend schema situation gets a permanent solution (probably not until after 8.1.x development cycle).

Not sure, but anyway, we need to profile this, for example with the internal page cache.

As mentioned, this is one of the 2-3 queries that is executed there, even small changes can have quite an impact there, and it is also a query that is called a lot.

I'll try and get setup with Apache Benchmark and YSlow for phantom.js on Saturday and Sunday. Next step would be to add the transaction handling around the query('select' in the cache DatabaseBackend class as well.

I talked with @Crell yesterday about this, and another solution may be to create tagged services for PostgreSQL, MySQL, and SQLite for these back ends so that each backend can extend parent for performance / niche bugs.

Yes, possibly, but that will require manual configuration to actually use those replacement services AFAIK, it won't happen automatically.

It's easier if there's a factory, which is the case for the cache DatabaseBackend (We're adding a mysql specific implementation in #2336627: Deadlock on cache_config (DatabaseBackend::setMultiple()))

@mradcliffe please elaborate the idea with tagged services.
I don't get how cachetags table creation relates to cache backend... does it mean a sort of PgsqlCacheBackend that could be useful somehow

These fails are caused in Postgres because setMultiple() is called before anything else within testSetMultiple. If $backend = $this->getCacheBackend(); didn't delete all data from the cache bins under test then we'd get an error on creating the cache table too.

Patch attached fixes the fails in POstgres and I think will be performance neutral.

Different approach to the earlier patches so no interdiff.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -213,37 +245,11 @@ public function setMultiple(array $items) {
+        // Only pass the values to values since the order of $fields matches the
+        // order of the insert fields.
+        $query->values(array_values($fields));

Not sure I understand the array_values(), I get that we don't need the keys but it doesn't hurt if we pass them in?

If that fixes PostgreSQL, then that's fine with me.

@Berdir it is a performance optimisation - save unnecessary looping...

  public function values(array $values) {
    if (is_numeric(key($values))) {
      $this->insertValues[] = $values;
    }
    else {
      // Reorder the submitted values to match the fields array.
      foreach ($this->insertFields as $key) {
        $insert_values[$key] = $values[$key];
      }
      // For consistency, the values array is always numerically indexed.
      $this->insertValues[] = array_values($insert_values);
    }
    return $this;
  }

If we pass in the keys we have to loop for every call to ->values(). Seems pointless here.

I ran Drupal\system\Tests\Cache\DatabaseBackendUnitTest on SQLite with this patch applied and we're good :)

Test summary
------------

Drupal\system\Tests\Cache\DatabaseBackendUnitTest  160 passes                                      

Test run duration: 2 sec

Thanks @amateescu - note that this also fixes #2443647: PostgreSQL: Fix system\Tests\Cache\ChainedFastBackendUnitTest.

After discussing with @catch improved the comments to explain why this function is as it is. Just in case anyone gets the urge to optimise before we have PostgreSQL testbots.

+1 for the comments and thanks for the SQLite run.

Seems like a catch patch.

Committed/pushed to 8.0.x, thanks!