Problem/Motivation

As discussed in #1008128: Do not use a single underscore as table and index separator on PostgreSQL and SQLite, naming collisions can occur between indexes and table names in SQLite and PostgresSQL.
That issue was closed in favor of #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL, but only the PostgresSQL part was fixed. This issue still affects SQLite

We wanted to add an index called search_api_language on the search_api_db_default_index resulting in the index name
search_api_db_default_index_search_api_language. This conflicted because we already have a table with that name.

I discovered this issue when fixing tests for Search API with SQLite enabled. SQLite gave us a conflicts because table names and index names have to be unique. They weren't and Drupal Core didn't solve this properly, as it does with the other SQL backends.

Proposed resolution

A patch attached in #2 implements the same fix as originally proposed in #1008128: Do not use a single underscore as table and index separator on PostgreSQL and SQLite.

Remaining tasks

Discuss, possibly write an upgrade path?

User interface changes

None

API changes

None

Data model changes

A double underscore is added to index names.

CommentFileSizeAuthor
#38 2625664-38--fix_sqlite_index_names.patch10.18 KBdrunken monkey
#38 2625664-38--fix_sqlite_index_names--interdiff.txt586 bytesdrunken monkey
#35 2625664-35--fix_sqlite_index_names.patch10.18 KBdrunken monkey
#35 2625664-35--fix_sqlite_index_names--interdiff.txt930 bytesdrunken monkey
#34 2625664-34.patch10.23 KBandypost
#25 2625664-25.patch10.35 KBdaffie
#23 2625664-23.patch10.25 KBdaffie
#23 interdiff-2625664-22-23.txt1.39 KBdaffie
#22 2625664-22.patch10.35 KBdaffie
#22 interdiff-2625664-18-22.txt2.22 KBdaffie
#20 2625664-20.patch10.28 KBdaffie
#20 interdiff-2625664-18-20.txt2.16 KBdaffie
#18 interdiff-2625664-15-18.txt1.41 KBdaffie
#18 2625664-18.patch8.13 KBdaffie
#15 2625664-15.patch6.7 KBdaffie
#15 2625664-15-test-only-should-fail.patch3.17 KBdaffie
#12 2625664-12-test-only-should-fail.patch3.18 KBdaffie
#12 2625664-12.patch6.72 KBdaffie
#7 2625664-interdiff.txt5.41 KBmollux
#7 drupal-sqlite-index-naming-2625664-7.patch6.72 KBmollux
#2 drupal-sqlite-index-naming-2625664-2.patch6.56 KBmollux
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mollux created an issue. See original summary.

mollux’s picture

This patch fixes this by adding a double underscore in the index name.
The test is based on one found in the patches in #1008128: Do not use a single underscore as table and index separator on PostgreSQL and SQLite.

borisson_’s picture

Updated IS and added the Search API issue as a related issue.

Nick_vh’s picture

Issue summary: View changes
Nick_vh’s picture

Priority: Normal » Major

Upgrade path here is tricky as this is something that is broken in sqlite. It won't be the case that there is an index and a table with the same name, so we hardly can update any index. But reading the code, this changes existing indexes? I think this is a great fix but right now I am not sure we can get this in without renaming all the existing indexes to add that prefix. Does core even have tests or use cases where the index and table name would be equal? If so, then this looks like a Major issue to me. Changing it accordingly.

So the upgrade path should be something along the lines of:

1) Check if sqlite
2) Loop over all indexes and find out if they adhere to our NonTablePrefix standard.
3) Change if they don't adhere
4) Done.

A changelog should be written with the impact for sites that already run SQLite and that potentially not all indexes were added if they were using contrib or custom code.

amateescu’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -726,4 +726,14 @@ public function findTables($table_expression) {
    +  function prefixNonTable($table) {
    +    $args = func_get_args();
    +    $info = $this->getPrefixInfo($table);
    +    $args[0] = $info['table'];
    +    return implode('__', $args);
    +  }
    

    Wouldn't it be more clear if we make this new helper method specific to generating a prefixed index name given a table name and the index? Are there any use cases for having it more generic like it is implemented now?

    Also, it should be a protected method :)

  2. +++ b/core/modules/system/src/Tests/Database/SchemaTest.php
    @@ -793,4 +793,88 @@ public function testFindTables() {
    +    try {
    ...
    +    catch (\Exception $e) {
    +      $exception = TRUE;
    +      $this->exceptionHandler($e);
    +    }
    +    $this->assertFalse($exception, t('The creation of a table without a primary key succeeded.'));
    

    We usually put $this->pass() and $this->fail() messages inside the try / catch blocks instead of asserting that there is no exception.

So the upgrade path should be something along the lines of:

1) Check if sqlite
2) Loop over all indexes and find out if they adhere to our NonTablePrefix standard.
3) Change if they don't adhere
4) Done.

Given that the current rule for generating index names is well defined (<$table_name>_<$index_name>), I think an upgrade path following those steps is not tricky at all :)

and that potentially not all indexes were added if they were using contrib or custom code.

So I'm not sure what happens in HEAD when we have this situation with a table name that collides with an index name. If a PDO exception is thrown when it happens, that site is already in a known broken state, no?

mollux’s picture

#6.1 : I don't see a use case for the generic approach, so I changed the method to generate prefixed indexes and made it protected.
#6.2 : I changed the tests to use $this->pass() and $this->fail().

So I'm not sure what happens in HEAD when we have this situation with a table name that collides with an index name. If a PDO exception is thrown when it happens, that site is already in a known broken state, no?

A PDO exception exception is thrown, and can lead to a broken state (e.g. when creating a unique index fails).

amateescu’s picture

Great! Now we just need the upgrade path and tests for it :)

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
@@ -726,4 +726,20 @@ public function findTables($table_expression) {
+   * @param string $table
+   *   The table name.

We should specify that this is an unprefixed table name.

alexpott’s picture

Issue tags: +Triaged core major

Discussed with @xjm and @catch. We agreed that this is a major bug as this causes an unrecoverable error on SQLite - the index will never exist.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 7: drupal-sqlite-index-naming-2625664-7.patch, failed testing.

daffie’s picture

Rerolled the patch from comment #7. It needed a reroll because the SchemaTest was moved to a different location. No other changed made. Added a second patch with only the added test and that one should fail.

The last submitted patch, 12: 2625664-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2625664-12-test-only-should-fail.patch, failed testing.

daffie’s picture

The new test-function in SchemaTest was protected. The SchemaTest class is now a sub-class of KernelTestBase and the test-functions should not be protected. The only change with the patches from comment #12 is that the function testSchemaCornerCases() is no longer a protected function.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, and verified it also allows us to remove our custom workaround for Search API (cf. #2625722: Remove sqlite fixing code in search api.).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@drunken monkey I think we still need an upgrade path.

daffie’s picture

Status: Needs review » Needs work

There needs to be an UpdatePathTest.

daffie’s picture

Status: Needs review » Needs work

The last submitted patch, 20: 2625664-20.patch, failed testing.

daffie’s picture

daffie’s picture

daffie’s picture

If I run the test on my local machine with a SQLite database they pass. Any help is much appreciated.

daffie’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost’s picture

drunken monkey’s picture

Some minor code clean-up, otherwise I’d have set it to RTBC. Still works perfectly.

borisson_’s picture

The code cleanup that @drunken monkey did was what I was going to mention on this issue as well, this now looks very good.

amateescu’s picture

+++ b/core/modules/system/src/Tests/Update/SqliteIndexUpdateTest.php
@@ -3,7 +3,7 @@
+use \Drupal\FunctionalTests\Update\UpdatePathTestBase;

If someone does a reroll of this patch at some point, we should remove the leading slash here :)

drunken monkey’s picture

If someone does a reroll of this patch at some point, we should remove the leading slash here :)

Fixed.
If you are all otherwise fine with the patch, could you then please set to RTBC so we have a chance of finally getting this in?

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Yes, looks good and the only issue that @amateescu mentioned is now fixed as well.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

I'm sorry if it was confusing, but my remark from #37 was only about the interdiff from #35.

Here's a full review for the patch:

  1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -69,12 +69,12 @@ protected function createIndexSql($tablename, $schema) {
           foreach ($schema['unique keys'] as $key => $fields) {
    ...
    +        $sql[] = 'CREATE UNIQUE INDEX ' . $info['schema'] . '.' . $this->generatePrefixedIndex($info['table'], $key) . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
    ...
           foreach ($schema['indexes'] as $key => $fields) {
    ...
    +        $sql[] = 'CREATE INDEX ' . $info['schema'] . '.' . $this->generatePrefixedIndex($info['table'], $key) . ' ON ' . $info['table'] . ' (' . $this->createKeySql($fields) . ")\n";
    

    We do this for unique keys and indexes, why not for primary keys as well?

  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -536,7 +536,7 @@ protected function introspectSchema($table) {
    -      $index_name = substr($name, strlen($info['table']) + 1);
    +      $index_name = substr($name, strlen($info['table']) + 2);
    

    This change will be problematic for any index/constraint that was created outside of the entity API system, because they are not changed by the current upgrade path.

  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -845,4 +845,20 @@ public function findTables($table_expression) {
    +  protected function generatePrefixedIndex($table, $index) {
    

    As mentioned above, this method is not only used for indexes, but also for unique and primary keys.

    How about renaming it to something more generic like getConstraintName?

    Also, we have the generic \Drupal\Core\Database\Schema::prefixNonTable() method which is pretty much a duplicate of the one introduced here, but it uses a single underscore instead of two.

    We should consider opening a followup issue to deprecate that one, since it's not used anywhere and each DB driver ended up with their own solution for the problem. E.g. the Postgres driver uses \Drupal\Core\Database\Driver\pgsql\Schema::ensureIdentifiersLength() now.

  4. +++ b/core/modules/system/src/Tests/Update/SqliteIndexUpdateTest.php
    @@ -0,0 +1,56 @@
    +      $this->pass('This test can only run on SQLite');
    +      return;
    

    We should use markTestSkipped() here.

  5. +++ b/core/modules/system/system.install
    @@ -2285,3 +2285,31 @@ function system_update_8702() {
    +    $schema = \Drupal::keyValue('entity.storage_schema.sql')->getAll();
    +    foreach ($schema as $item) {
    

    As mentioned above, updating only tables generated by the entity system is a bit problematic because we still have lots of other tables around. But I'm not sure we can do anything about them..

  6. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
    @@ -1248,4 +1248,95 @@ public function testFindTables() {
    +  function testSchemaCornerCases() {
    

    The tests should be extended to cover unique and primary keys too.

    Also, why do we need to use try/catch in this test? Wouldn't it be better to rely on the exception thrown by the database to fail the test?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

daffie’s picture

Issue tags: +Bug Smash Initiative

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.