Composite indexes are not correctly deleted/re-created when updating a field storage definition

What's awesome is that Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexFilledTest passes... so this is a difference between what occurs when the db is empty and when it is has data.

In node_update_8002()...

  // Regenerate entity type indexes, this should drop "node__default_langcode".
  $manager->updateEntityType($manager->getEntityType('node'));
  // Regenerate "langcode" indexes, this should drop "node_field__langcode".
  $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node'));

So the index is created correctly during the first update but then deleted in the next one... weird and scary this is different between SQL engines.

Those are different indexes. We're dropping 2 old indexes and creating one new one with more fields. The error message in the issue summary indicates that it is the index creation that is broken (in the presence of existing data). I don't have experience with Postgres to know if that's a common problem.

Oh, unless you're saying that the new index, node__id__default_langcode__langcode, is being attempted to get created twice: once for each line of code in #5, and MySQL is more forgiving of that than Postgres? I haven't debugged to see if any of this true, just speculating based on what's written in #5.

So the problem occurs because when $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node')); fires on an empty db it ends up dropping and recreating the langcode column. In postgres land this causes the index added on the entity level to be dropped because the column is part of the index. In mysql land this doesn't happen.

I think what we need to do is to have something that checks that all the entity level indexes exist if EM::updateFieldStorageDefinition() is called. Discussed with @plach and @jhedstrom on IRC.

@effulgentsia:

@alexpott found out what's going, I'm working on a fix. See the following chat:

alexpott - plach: it's a real head scratcher
plach - alexpott: good to know :)
plach - at least I was able to replicate the failure
alexpott - plach: it's really great that version with the filled database works :)
plach - alexpott: we have two different code paths, so it seems what's not working is dropping the entire schema and re-creating it, right?
alexpott - plach: that works
alexpott - plach: what is going wrong is  $manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition('langcode', 'node'));
alexpott - plach: the schema is dropped and recreated in $manager->updateEntityType($manager->getEntityType('node')); as we expect
plach - ok - alexpott - plach: but then the next line of code messes it up
alexpott - plach: in node_update_8002
plach - alexpott: are you sure just the first line would be enough?
alexpott - plach: no idea... I just know that it is the second line that is breaking the test
alexpott - plach: and I know that the test's expectations are correct :)
alexpott - plach: my hunch would be that the second line is needed when we have data
alexpott - plach: but that is just a gut feeling
plach - I see
plach - alexpott: yep, commenting that line fixes the test
plach - and I bet breaks the filled test

alexpott - plach: still very weird that mysql is different
plach - ... and, yes, breaks the filled test
plach - alexpott: maybe dropping and re-creating the index is not exactly the same on PG
plach - but yes it's weird
plach - I'll manually test the upgrade
alexpott - plach: cause we could just wrap with a db_index_exists() check
plach - alexpott: we don't know whether the old spec if up-to-date
plach - * is
alexpott - plach: well $manager->updateEntityType($manager->getEntityType('node')); updates the installed definitions
plach - alexpott: ok, in this case, but in general?
plach has the feeling he already had this discussion
plach - jhedstrom: ;)
alexpott - plach: I know why...
alexpott - plach: postgres will delete the index when the language column is deleted and added again
alexpott - plach: i bet mysql doesn't
jhedstrom - alexpott: plach so can we wrap our drop index calls in an exists, the same way our add index calls are now wrapped?
alexpott - jhedstrom: I think so
jibran is now known as jibran|afk
alexpott - jhedstrom: although I think this hints at larger problems to come
alexpott - jhedstrom: this is going to be hard to spot
alexpott - plach, jhedstrom: yep this is exactly what is going on
plach - alexpott: so basically we should ensure the index is re-created rather than not dropping it
plach - jhedstrom: ^
alexpott - plach: yep
plach - otherwise hoe can we be sure it's the most current spec?
plach - when non preceded by a full entity schema update
plach - I mean
alexpott - plach: yep
plach - alexpott++
plach - great detective work

Let's see whether this works.

This probably needs some additional test coverage, for the PG-specific changes.

Silly me

It'd be nice if one of the Postgres peeps had a look at this one.

Fixed the last MySQL failure and improved test coverage. The test-only patch should fail also on MySQL.

I'm working on some test coverage for the PG-specific stuff.

Btw, I tested all the remaining failures on PG locally (VM with Ubuntu 15.04, PHP 5.6.4, and PG 9.4.4) and all tests are green...

@plach all the other postgres fails are fixed by #2561121: Installer fails on postgresql due to uncaught exception and #2561229: Upgrade content tests fails on postgres for translated comment

Ok, this should be more or less complete. Reviews welcome :)

Attaching also a previous interdiff I was about to post, when I noticed the last review. This fixes also a few notices introduced by the latest patch.

@catch:

3: This was happening before introducing the optimization that skips dropping/creating indexes when creating the entity schema. Theoretically I guess it could happen also if multiple fields are added and a composite index is added at the same time. I added a comment to explain this.

4: Well, creation is handled in the previous ::applyUpdates() call, but one more cannot hurt.

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -588,13 +588,25 @@ public function indexExists($table, $name) {
   +    if ($name == 'pkey') {
   +      $suffix = $name;
   +      $name = '';
   +    }
   +    else {
   +      $pos = strrpos($name, '__');
   +      $suffix = substr($name, $pos + 2);
   +      $name = substr($name, 0, $pos);
   +    }
   

   Needs a comment, why we are doing this here.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -328,42 +328,12 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
   +      $this->deleteEntitySchemaIndexes($this->loadEntitySchemaData($entity_type));
   ...
   +      $this->createEntitySchemaIndexes($entity_schema);
   

   It is great to move things into separate methods for readability!

Well, it's not only for readability, the method is being invoked in two places actually :)

Assigning credit to catch (as a reviewer) and myself (as cause of the problem finder)

This has been my concerns

Hm, should we believe the postgres fail? (I heard those bots are not predictable(?))

@Gábor Hojtsy:

The PG fail was expected, it should be fixed by #2561229: Upgrade content tests fails on postgres for translated comment now. Re-testing.

Btw, #2561121: Installer fails on postgresql due to uncaught exception fixed most of the PG bots randomosity, although I saw one random failure in https://www.drupal.org/pift-ci-job/26901. @alexpott++

@plach there is a feeling that the first test on postgres might fail because of a DrupalCI timing issue - but it does not happen very often.. I has not occurred on the latest run. So https://www.drupal.org/pift-ci-job/27075 should be the first ever green postgres run.

Yay, for first green run :)

Committed a173031 and pushed to 8.0.x. Thanks!

Yay, thanks all!

The primary key of the entity tables was forgotten in this patch, here's a followup for that: #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field