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.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2625664-38--fix_sqlite_index_names.patch | 10.18 KB | drunken monkey |
Comments
Comment #2
mollux CreditAttribution: mollux commentedThis 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.
Comment #3
borisson_Updated IS and added the Search API issue as a related issue.
Comment #4
Nick_vhComment #5
Nick_vhUpgrade 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.
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWouldn'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 :)
We usually put
$this->pass()
and$this->fail()
messages inside the try / catch blocks instead of asserting that there is no exception.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 :)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?
Comment #7
mollux CreditAttribution: mollux commented#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()
.A PDO exception exception is thrown, and can lead to a broken state (e.g. when creating a unique index fails).
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat! Now we just need the upgrade path and tests for it :)
We should specify that this is an unprefixed table name.
Comment #9
alexpottDiscussed 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.
Comment #12
daffie CreditAttribution: daffie commentedRerolled 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.
Comment #15
daffie CreditAttribution: daffie commentedThe 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.
Comment #16
drunken monkeyLooks 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.).
Comment #17
alexpott@drunken monkey I think we still need an upgrade path.
Comment #18
daffie CreditAttribution: daffie commentedAdded the upgrade path.
Comment #19
daffie CreditAttribution: daffie commentedThere needs to be an UpdatePathTest.
Comment #20
daffie CreditAttribution: daffie commentedAdded the UpdatePathTest.
Comment #22
daffie CreditAttribution: daffie commentedPlease forget the patch from comment #20.
Comment #23
daffie CreditAttribution: daffie commentedRemoved the table prefix from the index name.
Comment #24
daffie CreditAttribution: daffie commentedIf I run the test on my local machine with a SQLite database they pass. Any help is much appreciated.
Comment #25
daffie CreditAttribution: daffie commentedRerolled
Comment #28
claudiu.cristea#2867672: Table name conflicts with table index is a duplicate of this.
Comment #33
andypostComment #34
andypostReroll for 8.8
Comment #35
drunken monkeySome minor code clean-up, otherwise I’d have set it to RTBC. Still works perfectly.
Comment #36
borisson_The code cleanup that @drunken monkey did was what I was going to mention on this issue as well, this now looks very good.
Comment #37
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIf someone does a reroll of this patch at some point, we should remove the leading slash here :)
Comment #38
drunken monkeyFixed.
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?
Comment #39
borisson_Yes, looks good and the only issue that @amateescu mentioned is now fixed as well.
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'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:
We do this for unique keys and indexes, why not for primary keys as well?
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.
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.We should use
markTestSkipped()
here.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..
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?
Comment #42
andypostComment #44
daffie CreditAttribution: daffie commented