When index is added for content_translation_uid, the corresponding stored schema definition is not updated

This looks like a reasonable way to fix this, would a new addIndexIfNotExists() method make sense here? Or alternatively is there an easy way to persist whether the index has been updated during the earlier entity type update event?

Looks good!

Just to add to that, this issue fixes a bug in our upgrade path and is blocking a critical: #2555183: Fix the filled update tests, they are broken

Giving plach a chance to take a look.

It's a bit late here and I'd like to have a look to this with a fresh mind, but I'm not seeing additional test coverage: is this assuming implicit test coverage is provided by #2555183: Fix the filled update tests, they are broken?

Elevating to critical because #2555183: Fix the filled update tests, they are broken is a critical and is dependent on this being fixed.

It's the parent that needed an IS update, not this one...

Can you please explain the following sentence?

When translations are enabled for content with data, and index is added to the content_translation_uid field.

That field is defined by the Content Translation module, so when translations are enabled for a certain bundle already having data, the field should either exist or not exist. If I'm looking at the code right, it seems the ContentTranslationUpdatesManager only triggers updates for newly added fields, so what's the scenario where only an index is added?

I assume the update op is applied to the content_translation_uid field, but it's not clear to me how that can happen. Anyway, if that's the case, how can be sure the old index specification perfectly matches the current one? Shouldn't we drop the index and re-create it as we do in the other cases?

At least a quick reroll for now.

So looks like this does not have tests on its own to prove when is the problem happening. BUT https://qa.drupal.org/pifr/test/1133538 exposes the issue with just running the filled database dump through updates. Fails on

Index users__id__default_langcode__langcode properly created on the user_field_data table in SqlContentEntityStorageSchemaIndexTest.php:44

Among other things.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1867,4 +1872,25 @@ protected function getColumnSchemaRelevantKeys() {
+  protected function addIndexIfNotExists($table, $name, array $fields, array $spec) {
+    if (!$this->database->schema()->indexExists($table, $name)) {
+      $this->database->schema()->addIndex($table, $name, $fields, $spec);
+    }
+  }
+

What happens if the fields indexed have changed? As @plach mentioned in #19, we need to alter the index instead.

Agreed

Discussed with @webchick, @effulgentsia, @alexpott, and @catch. This issue is critical as a blocker for #2555183: Fix the filled update tests, they are broken and also independently a bug with entity update functionality.

Tagging beta target per #2341575-45: [meta] Provide a beta to beta/rc upgrade path.

@jhedstrom: that makes sense. Thanks for the explicit test coverage, looks good to me. More implicit test coverage will be added in #2555183: Fix the filled update tests, they are broken.

Looks ready to go.

I will have a another look to this anyway, but don't block commit on my further feedback.

Essentially, this is just adding a new method—addIndexIfNotExists()—and calling it in place of addIndex() to get around the problem. Looks straight-forward to me, and unblocks a blocker of a blocker. ;)

Committed and pushed to 8.0.x. Thanks!

@webchick:

I still don't get why the original problem manifests, but that's just the "Content Translation maintainer" inside me speaking.

The "release Drupal 8 for the love of God" guy inside me just says "yay, one sucker less!" ;)

Fix here wasn't correct after all, see #2561129: Composite indexes are not correctly deleted/re-created when updating a field storage definition.