Prevent PostgreSQL from creating duplicated index names within schema

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -216,7 +216,11 @@ protected function createTableSql($name, $table) {
       foreach ($table['indexes'] as $key_name => $key) {
-        $statements[] = $this->_createIndexSql($name, $key_name, $key);
+        // Prevent from creating an index twice. This could happen as PostgreSQL
+        // indexes are global.
+        if (!$this->indexExists($name, $key_name)) {
+          $statements[] = $this->_createIndexSql($name, $key_name, $key);

This looks like hack, maybe better solution to make index name generation "prefixed" somehow?

I have reviewed this code both on a manual installation on my laptop and using the drupalci test infrastructure, and this patch solves a number of failing simpletest tests and doesn't appear to cause any additional failures. I am comfortable marking this as RTBC and asking that it be committed.

Does this mean that we have indexes with the same name on different tables?

I'm concerned that this will cause us not to create important indexes.

Setting back to needs review to get answers to #3/#4

Does this mean that we have indexes with the same name on different tables?

Absolutely, it does -- and the more I think about it, this patch really would prevent us from building indices on important tables.

To show that we really do have indices with the same name on different tables, I ran the following query on MariaDB 10.0.15 after a fresh installation of 8.0.x HEAD (installed yesterday):

SELECT DISTINCT INDEX_NAME, TABLE_NAME FROM INFORMATION_SCHEMA.STATISTICS WHERE TABLE_SCHEMA = 'drupal8' ORDER BY INDEX_NAME, TABLE_NAME;

(Obviously, you can replace 'drupal8' with the name of your own database.)

If you look at the attached file, it shows that we have thirteen tables that all have an index named "revision_id".

Looks like we're back to the drawing board on this patch, perhaps by prefixing the index name with the table name.

Another attempt at a patch for this issue, this time by prefixing the index name with the table name, so that we don't have conflicting index names in PostgreSQL. Please let me know if this resolves the concerns from comments 1, 3, and 4.

I'm sorry, but this issue is not about forcing to create another (duplicate) index with a different name if an index with the same name already exists. This issue is about preventing from creating an index twice.

Before I get into the specifics of your last comment, let me just give some background so that we can all agree where we're coming from.

I wasn't aware until I looked into it further today that the ensureIdentifiersLength() function already concatenates the table name plus the index name plus 'idx' when creating an index in PostgreSQL, in addition to making sure the name of the table/index/constraint doesn't exceed PostgreSQL's limit. So, in short, it's already doing what my new patch proposed to do, so my patch is superfluous.

To answer the other comments on this issue after more investigation and reflection:

Comment #1: We already prefix the table name, so that's taken care of.
Comment #3: We do in fact in MySQL, but it doesn't really matter there. We don't in PostgreSQL.
Comment #4: Because we're fixing up the index names in PostgreSQL, this isn't a problem.

Which brings us all the way back around to @bzrudi71's patch, which really is the correct thing to do in this situation -- because we can now be sure that it really is an attempt to create a duplicate index on the same table, and not a naming conflict on the indices.

I'm setting this back to RTBC, as I don't see any other issues with @bzrudi71's patch.

Wow can we get a followup to give ensureIdentifiersLength() a better name. Also the variadic nature of the method seems unnecessarily fragile and complex.

So now my question is why are we trying to create duplicate indexes on a table? Since this has nothing to do with the global nature of postgres's indexes. (Setting back to needs review for this)

Plus can @bzrudi71's patch be re-uploaded since the rtbc retest only retests the latest patch - it doesn't check the hidden files property (afaik).

Add an indexExist check to createTableSql() to prevent PostgreSQL from creating indexes twice. This is because indexes in PostgreSQL are global.

According to the latest comments this is not why we're doing this change so this needs updating.

I think here should be a follow-up to extend schema generators to not produce a duplicated names for indexes, tables, constrains there's no reason to relay on storage and introduce hacks within drivers.

Oracle contrib driver exposes own identifier constraints. I'm pretty sure that mssql has the same issue.
So better to extend schema generator once it has previous state knowledge now.

I haven't really read this issue in depth, but I read over comment #12 and updated the issue summary. I downloaded the patch, copied it to a new file name, and ran interdiff and md5 to confirm the files were the same.

Created follow-up issues.

But why are duplicate indexes being created #14 still needs an answer.

So this is a problem with renaming tables in postgres. We need to rename any keys they have too. So the patch #17 is definitely wrong because the tables for the new fields will not have the correct indexes. Basically SqlContentEntityStorageSchema::onFieldStorageDefinitionDelete() calls Drupal\Core\Database\Driver\pgsql\Schema::renameTable() which should take care of renaming the indexes but, from @bzrudi71's report, this appears not to be working.

Let's postpone that on #1013034: PostgreSQL constraints do not get renamed by db_rename_table()