The standard logic we use in ::ensureTableExists() is wrong

Very nice find.

1. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php
   @@ -255,17 +255,17 @@ public function deleteAll() {
        }
        // If the table already exists, then attempting to recreate it will throw an
        // exception. In this case just catch the exception and do nothing.
   +    // @todo Should this be DatabaseException like the others?
        catch (SchemaObjectExistsException $e) {
   -      return TRUE;
        }
   

   I think it should, although you could also argue that all the others should use SchemaObjectExistsException? But either way we should use one consistently.

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -247,6 +247,7 @@ protected function safeExecuteSelect(SelectInterface $query) {
          // Some other failure that we can not recover from.
   +      // @todo this is actually unreachable code - should we fix this.
          throw $e;
        }
   

   Probably just in a follow-up.

It might be nice to put this into a trait and somehow notify modules that have copy&pasted this code.

There are two issues trying to do something like this already:

#2721271: Simplify exception handling for lazy-load pattern and #2371709: Move the on-demand-table creation into the database API.

To be able to use this in our tests where we initially ran into the problem, I backported it to 9.2.x

1. The impression that I get when I look at the patch is that we are doing the same thing or almost the same thing about 9 times in core. Maybe we should move the code to a trait (https://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
2. I am missing testing for this change/fix.

@alexpott: Good find!

@alexpott: thank for for adding the Drupal\KernelTests\Core\KeyValueStore\DatabaseStorageTest::testConcurrent() test. It was not the test I was looking for, as I did not know that we could do what your test is testing. Really great!

I have added the testing that I was looking for and I have removed a couple of small changes that to me are not necessary. More testing for the other changes is still necessary to me.

Fixed the style code violations.

1. +++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
   @@ -150,19 +150,19 @@ public function garbageCollection() {
   +      return TRUE;
   

   Why do we have this line here. As far as I can see it can be removed.

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -1163,21 +1163,17 @@ protected function treeDataRecursive(array &$links, array $parents, $depth) {
   -      throw new PluginException($e->getMessage(), 0, $e);
   +      return FALSE;
   

   If we make this change the docblock also need an update.

3. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -247,7 +247,7 @@ protected function safeExecuteSelect(SelectInterface $query) {
   -      throw $e;
   +      throw new PluginException($e->getMessage(), 0, $e);
   

   For this change the methods docblock needs to be updated.

4. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -100,7 +100,9 @@ public function dump(array $options = []) {
   -        $this->ensureTableExists();
   +        if (!$this->ensureTableExists()) {
   +          throw $e;
   +        }
   

   Can we do without this change? If not, then the methods docblock needs to be updated.

5. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
   @@ -47,7 +47,7 @@ protected function doInvalidateTags(array $tags) {
   -        $this->catchException($e);
   +        throw $e;
   
   @@ -63,7 +63,7 @@ protected function getTagInvalidationCounts(array $tags) {
   -        $this->catchException($e);
   +        throw $e;
   

   We are updating these methods. Can we then also update the docblock for the thrown exceptions.

6. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
   @@ -133,6 +130,7 @@ public function schemaDefinition() {
   +    @trigger_error('@todo', E_USER_DEPRECATED);
   

   The @todo needs to be fixed.

7. We are updating the method ensureTableExists() in 9 classes. We have only addeed testing for one class. Is that enough or do we need to add testing to all 9 classes? Can we add a dataprovider to the added test?

All code changes look good to me.
All the 9 methods ensureTableExists() are the same. In #2371709: Move the on-demand-table creation into the database API will these methods be replaced by something else.
The class for the database storage of keyvalue values has testing added for asynchronous table creation. According to @alexpott is this enough.
For the added deprecation of Drupal\Core\Cache\DatabaseCacheTagsChecksum::catchException() which is a protected method is a CR created. I have updated the IS for it.
For me it is RTBC.

Looks good to me now. I think the single point of test coverage is OK given we're going to remove all the copypasta in the follow-up, and this code runs implicitly thousands of times in the test suite overall.

Committed e0b4b7e and pushed to 9.3.x. Thanks!

It would be possible to backport this to 9.2.x, but given the new deprecation leaving this in 9.3.x