Postgres schema doesn't support keylength on a unique index

Indeed.

Triaging, bumping to 8.x to fix it there first.

The patch contains debug statements that need to be removed, but otherwise looks good.

Rerolled patch for 8.x without debug statements.
Let get this in, for 7.x asap as well.

Actually, no, this needs work.

We need to use functional indexes here, and there is already a DatabaseSchema_pgsql::_createKeySql() function to do that. Simply ignoring the column prefixes is just not an option for the unique and primary keys, because it would change the behavior of the key.

For unique indexes I agree but I doubt whether Postgres allows functional primary keys.

This version uses _createKeySql() in unique indexes. Since Postgres does not allow functions to be used in primary key indexes it still ignores the length limit for those.
In the rare case that a length is provided for a primary key, an alternative would be to add an extra functional unique key constraint. Any thoughts on that?

#5: 1148856-pgsql_schema_fix_issue_of_mysql_key_length-5.patch queued for re-testing.

How the changes are affrected #1852896: Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL

Postgresql does not have limits on key size!

This sounds like a pretty critical bug for postgres environments... In fact, the only reason I'm leaving it at normal is that no one seems to have posted on it in years. Are there duplicates of this issue about? Does this problem still occur in D8 whenever you install a something with the index length restricted, like for example when you install, oh, taxonomy? #1871032: Taxonomy module fails to install on MyISAM due to too long schema index

Can someone manually test installing Postgres and the standard profile on D8?

@xjm: this is only a problem in practice when you try to use a UNIQUE key on a prefix... but this is really not something we should support in Drupal in the first place.

Ah, thanks @Damien Tournoud. Sounds like we just need a summary update then.

I ran into this during the DrupalCamp Atlanta Code Sprint. The comment module is now using keylength for the comment_entity_statistics table, so Drupal8 won't install on PostgreSQL without this being fixed. And the PostgreSQL driver code has been moved to core/lib/Drupal/Core/Database/Driver/pgsql/ since the patch in #5. Attached is patch #5 redone for the new code layout.

Fixing the Component that was erroneously set to ajax system

Not sure it's possible to write tests special for PostgreSQL
But the patch fixes installing.
{comment} table also have 'comment_num_new' index with 'entity_type' and 'field_name' "shorten" to 32 bites same as {locales_source} for 'source_context' index

Well, there is a test client with a PostgreSQL/PHP5.3 environment in the test system, so it seems it might be possible. I haven't dug enough into the test system to know how to write tests for that, though.

Backport to D7

Part of the patch in #12 was done in #2181283: Prevent pgsql driver from trying to implode primary key fields that are defined by an array.

Patch needs to be modified to remove _createPrimaryKeySql as that became createPrimaryKeySql, and is already there.

_createKeySql could be renamed to createKeySql as that was a code review issue in the above mentioned issue.

Next try, missed $this->ensureIdentifiersLength ;-)

The changes here look pretty random. If we want to support this properly, we should generate a functional index here.

http://www.postgresql.org/docs/9.1/static/indexes-expressional.html

@Damien Thanks for the link! The first thing that made me thinking is the requirement of PostgreSQL version >= 9.x. Do we really wan't to break 8.x because of this? The second is about performance.

Index expressions are relatively expensive to maintain, because the derived expression(s) must be computed for each row upon insertion and whenever it is updated.

Yes, I agree that I don't think we should support key length as a constraint mechanism for indexes. It seems like it's adding functionality that is not relevant for other drivers and that may confuse developers.

Index expressions are supported since 7.4: http://www.postgresql.org/docs/7.4/static/indexes-expressional.html

only docs gate

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -372,7 +372,7 @@ function getFieldTypeMap() {
   }
 
-  protected function _createKeySql($fields) {
+  protected function createKeySql($fields) {

needs doc block once we are on that

Updating title to make it clear why this is a minor use case (at best).

Not sure how to test this so not sure is it actually still an issue.

Re-roll plus comment for createKeySql(). This problem still exists – ran into this in #3228210: Fix current test failures, where I now have the choice between fixing the code for MySQL or Postgres.

Still needs tests, though.

@drunken monkey: If you add the testing, then I will do the reviewing.

Thanks for the quick reaction!
Here you go – was (luckily) pretty simple anyways. (Interdiff is the same as the tests-only patch.)

Can we instead of doing a unittest, do a kerneltest. Then we have a database. Use the Drupal\KernelTests\Core\Database\DatabaseTestBase to extend from. Then use one of the tables with data in it. Create the unique indexes on that table and test those indexes. Then we test if it really works on a PostgreSQL database.

OK, then how about this? (I don’t have Postgres set up locally, though, so not 100% sure it will work.)

Also, as our SQLite driver ignores the prefix length for key column specifiers, we need to test differently for that DB.

IMHO this should should be wontfix, for the same reason as #3068310-12: Composite primary key with specified index length fails the NOT NULL check

IMHO this should should be wontfix, for the same reason as #3068310-12: Composite primary key with specified index length fails the NOT NULL check

I strongly disagree: the functionality of specifying a prefix length for any index key (primary, unique or index) is part of the API (unless #3082239: Forbid limited length primary and unique keys, allow only in indexes actually gets accepted), so having a driver where actually using this functionality leads to an error isn’t acceptable.

We can discuss whether we want to actually comply with the specified prefix length, or ignore it like SQLite, but the current behavior is definitely a bug. Furthermore, note that MySQL (or at least some versions of it) requires a prefix length when using TEXT or BLOB columns as part of a key specification (see here), so having this functionality in the Schema API can be crucial in some (though admittedly probably rather rare) cases.

1. Let me first start with that the patch fails for PostgreSQL.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   @@ -0,0 +1,84 @@
   +  protected function supportsPrefixLength(): bool {
   +    return $this->connection->driver() !== 'sqlite';
   

   Could we instead do in the function setup():

   if ($this->connection->driver() === 'sqlite') {
     $this->markTestSkipped("SQLite does not do support prefix lengths for keys.");
   }
   

   Edit: The Is is saying that the bug that we are fixing does not exists with SQLite??? What is it?

3. I am missing testing for the change in Schema::addField().
4. I am missing testing for the change in Schema::changeField().

5. +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   @@ -0,0 +1,84 @@
   +    // First, clear the table
   

   Why is this comment here? Can it be removed?

6. +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   @@ -0,0 +1,84 @@
   +class UniqueKeysTest extends DatabaseTestBase {
   

   Can we add in the docblock a @coversDefaultClass \Drupal\Core\Database\Schema

7. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   --- /dev/null
   +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   

   Can we rename the file to: SchemaUniquePrefixedKeysIndexTest.php.

8. +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   @@ -0,0 +1,84 @@
   +  public function testCreateTable(): void {
   

   Can we add to the docblock: "@covers ::createTable" and "@covers ::CreateTableSql".

9. +++ b/core/tests/Drupal/KernelTests/Core/Database/UniqueKeysTest.php
   @@ -0,0 +1,84 @@
   +  public function testAddUniqueKey(): void {
   

   Can we add to the docblock: "@covers ::addUniqueKey".

10. Does this problem also exists with not unique indexes? If so, can we fix that too in this issue?

Let me first start with that the patch fails for PostgreSQL.

Shoot. As said, I don’t have Postgres set up, so I just followed the proposed patch in #29. Seems Postgres (or at least some versions of it?) doesn’t like that syntax after all. Then I’d say we just ignore prefix length, like we already do for SQLite. Then it’s only supported in MySQL, but that’s also (apparently) where it’s most needed, and at least this won’t trigger an error anymore.

Edit: The Is is saying that the bug that we are fixing does not exists with SQLite??? What is it?

The bug is that using this functionality on Postgres causes an error – so, no, the bug doesn’t exist in SQLite. SQLite ignores the setting, which is also not ideal (I’d say), but not the issue at hand. Therefore, even if SQLite doesn’t recognize the setting, it should still definitely be part of the test, as we still want to make sure using the setting doesn’t cause an error. That’s what the regression would be, too, in Postgres.

I am missing testing for the change in Schema::addField().
I am missing testing for the change in Schema::changeField().

Neither of those have changes? Same goes for _createIndexSql() now, by the way. (And before it was only a method rename, so wouldn’t have said it required extra tests.)

Can we add in the docblock a @coversDefaultClass \Drupal\Core\Database\Schema
…

I’m not sure that’s appropriate, as we really just test that class’ sub-classes? Also, there isn’t even a \Drupal\Core\Database\Schema::createTableSql() method.

But fine by me either way.

Does this problem also exists with not unique indexes?

No.

I have searched the PostgreSQL documentation and I could not find anywhere that this database supports unique indexes with prefix limits. Also SQLite does not support it. Therefor it will be an MySQL only thing. It would be however very nice if the database drivers of SQLite and PostgreSQL do not choke when there is a prefix limit is set.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaUniquePrefixedKeysIndexTest.php
   @@ -0,0 +1,82 @@
   +    $this->expectException(DatabaseException::class);
   +    $value = '1234567890 ' . ($this->supportsPrefixLength() ? 'bar' : 'foo');
   +    $this->connection->insert('test_unique')
   +      ->fields([
   +        'field' => $value,
   +      ])
   +      ->execute();
   ...
   +    $this->expectException(DatabaseException::class);
   +    $value = '1234567890 ' . ($this->supportsPrefixLength() ? 'bar' : 'foo');
   +    $this->connection->insert('test_people')
   +      ->fields([
   +        'job' => $value,
   +      ])
   +      ->execute();
   

   Can we rewrite these 2 cases to:

   if ($this->connection->databaseType() === 'mysql') {
     // Some documentation about why for for MySQL the exception is thrown.
     $this->expectException(DatabaseException::class);
     $this->connection->insert('test_unique' or 'test_people')
       ->fields([
        'field' => '1234567890 bar',
       ])
      ->execute();
   }
   

2. With Schema::addField() and Schema::changeField() you can add a field with an unique index with a prefix limit. Can we add testing that that works. If necessary can we add a fix for PostgreSQL.

Can we rewrite these 2 cases to:

Why? I’d say it’s still better to test something than nothing.
E.g., current code would catch it if the (SQLite/Postgres) driver would just ignore any UNIQUE keys with prefix length altogether (maybe try with wrong syntax and then fail silently), instead of just ignoring the prefix length.

With Schema::addField() and Schema::changeField() you can add a field with an unique index with a prefix limit. Can we add testing that that works. If necessary can we add a fix for PostgreSQL.

Sure, makes sense. Patch attached.

I have only 2 remarks, after that it is RTBC for me.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaUniquePrefixedKeysIndexTest.php
   @@ -0,0 +1,147 @@
   +    $this->connection->insert('test_people')
   +      ->fields([
   +        'job' => '1234567890 foo',
   +      ])
   +      ->execute();
   +
   +    $this->expectException(DatabaseException::class);
   +    $value = '1234567890 ' . ($this->supportsPrefixLength() ? 'bar' : 'foo');
   +    $this->connection->insert('test_people')
   +      ->fields([
   +        'job' => $value,
   +      ])
   +      ->execute();
   

   Now we have this piece of code 4 times. Can we move it to its own method.

2. In Drupal core we use Code Coverage Analysis. Could you add that to the test (@covers).

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -294,7 +294,7 @@ protected function createTableSql($name, $table) {
+        $sql_keys[] = 'CONSTRAINT ' . $this->ensureIdentifiersLength($name, $key_name, 'key') . ' UNIQUE (' . $this->createPrimaryKeySql($key) . ')';

@@ -810,7 +810,7 @@ public function addUniqueKey($table, $name, $fields) {
+    $this->connection->query('ALTER TABLE {' . $table . '} ADD CONSTRAINT ' . $this->ensureIdentifiersLength($table, $name, 'key') . ' UNIQUE (' . $this->createPrimaryKeySql($fields) . ')');

The call to createPrimaryKeySql() sounds a bit strange in the context of unique keys; however, reading the code in the method it is clear that what it does is actually disregard the partial length and implode the fields names, quoted with the appropriate identifiers.

I would suggest to either (a) add a comment on why we're using this method here, or (b) adjust the Schema::fieldNames method in the base class to take in also the identifiers if needed and return quoted field names as well as unquoted ones.

Should all be fixed in the attached patch revision.

I also attempted to bring the issue summary up to date. Is that what you meant, daffie?

The bug is fixed and testing haas been added for the fix.
The patch without the fix is correctly failing for PostgreSQL.
All points of @mondrake and myself have been addressed.
For me it is RTBC.

@drunken_monkey: Good work!

I've went back on forth about silently ignoring the index length configuration and whether this is the correct thing to do. However, we already do this for primary keys on postrges and all keys on SQLite. So I think the approach is correct. However, I think we should improve documentation as part of this change.

1. The docs for \Drupal\Core\Database\Driver\pgsql\Schema::createPrimaryKeySql() currently say:
   * Create the SQL expression for primary keys.
   This should say:
   * Create the SQL expression for primary and unique keys.
2. It would be good to document the length part of a key column specifier can be ignored depending on the database type and the type of key in the documentation. In core/lib/Drupal/Core/Database/database.api.php we have:

    * A key column specifier is either a string naming a column or an array of two
    * elements, column name and length, specifying a prefix of the named column.
   

   This can be improved.

#62.2 is actually being done in #3082239: Forbid limited length primary and unique keys, allow only in indexes which could be a follow up of this issue?

@mondrake I think we should fix the docs here to represent HEAD where different DBs do it differently. We have no idea how long #3082239: Forbid limited length primary and unique keys, allow only in indexes will take to land.

Here are the doc improvements requested in #62. I went for this addition to the second paragraph:

Note that some DBMS drivers may opt to ignore the prefix length configuration and still use the whole field value for the key. Code should therefore not rely on this functionality.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Oops, wrong patch file.

My bad, the postgres test failures need addressing.

This should do it.
Escaping of the field names already happens in createPrimaryKeySql(), so no need to call escapeField(), too.

Changes looks good and covered with tests, queued tests for pgsql and sqlite

Updating issue credits

Committed to 10.1.x and 10.0.x

Doesn't apply to 9.5.x, marking as patch to be ported.

Thanks folks!

Rolled this back from 10.0.x, ::getPrefix doens't exist there so HEAD was broken

Since we don't have the ::getPrefix method in 10.0.x, I think we should leave this in 10.1.x - makes it not a straight backport.

Given the conditional

   protected function supportsPrefixLength(): bool {
	
    return $this->connection->driver() === 'mysql';
	
  } 

the test should have been a DriverSpecificDatabaseTestBase extending test, no?

Sorry for late feedback

@mondrake: I have created the followup for extending DriverSpecificDatabaseTestBase. See: #3351886: Change SchemaUniquePrefixedKeysIndexTest to extend DriverSpecificDatabaseTestBase