Do not use a single underscore as table and index separator on PostgreSQL and SQLite

This sounds like a bad bug, but doesn't need to block release. (But I'm happy to commit a fix when it's ready.)

I altered your patch Damien and took in the concept from the patch I made in #1009828: constraintExists doesn't work with multiple schemas.

From what I can tell schemaname isn't available in pg_constraint like it is in pg_index. That is the case in Postgres 9.x. I only altered that part in the constraint check, and do the schema check by joining pg_namespace, which should fix this.

We should make a test case for this by testing adding and deleting indexes, primary keys ect on a database with two identical schemas. I'm not familiar with the db backend, so I'm not sure if this should be done with SQL, or if Drupal can help us to do this.

I'm confused. Why couldn't we just do something like __ instead of _, on the assumption that no sane person would ever have a double-underscore in a table or field name? Wouldn't that resolve the collision? (It wouldn't help with the upgrade path but one thing at a time.)

It sounds like this has been an issue with Postgres before; we just haven't run into it.

And actually it seems this is closely related to #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL, is it not? Wouldn't a proper solution handle both issues?

Why is the new function called getTableObjectName? A string is passed as its only argument and it returns an array. I dont' see why it needs the word "Object" in there.

Also there are a few places where the function is still used inline with a string like " some text " . $this->getTableObjectName($table) . "...". Unless this is one of those horrible objects that allow you to use array syntax in PHP, this will print "Array" in the string.

+++ includes/database/pgsql/schema.inc
@@ -417,7 +418,7 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
+    $this->connection->query('ALTER TABLE {' . $table . '} DROP CONSTRAINT ' . $this->getTableObjectName($table, 'pkey'));

This needs changing as the return value of the method is now an array.

+++ includes/database/schema.inc
@@ -219,11 +219,14 @@ abstract class DatabaseSchema implements QueryPlaceholderInterface {
+  protected function getTableObjectName($table) {

Would this be better named as: getTableOrObjectName?

Also, the documentation for the function needs updating to indicate the return value has changed.

Powered by Dreditor.

Like this.

Yeah, needs work.

Another go at this. Just cleaning up the work of others in this issue, Damien Tournoud's point still holds:

We need to figure out the upgrade path for this... system_update_7070() is just a shot in the dark and is simply wrong (it will change the schema of all the tables before they are properly updated).

I don't have access to a machine that can run all the pgsql tests in a sensible amount of time, but I've run the database tests and there's a single failure in the 'Select tests, subqueries', and I've not run this on sqlite at all either.
Failure is:
Fetched name is correct using EXISTS query.
But I'm not sure if that is because my setup is bad or otherwise :(

I've removed the test for creating a table with zero columns, as that doesn't actually fail on Postgres, i.e. you can create a table with zero columns. I would suggest that if we want to test for that we should throw the exception in the create_table chain rather than just letting PDO do it for us, if we want to enforce such a restriction on Drupal created tables.

getTableOrObjectName is not a good method name. The function only ever returns an array so just cast the array to an object. Why does the name of the function even need to change?

Function names shouldn't normally indicate the return value of the function, the doxygen does.

The method gets the name of a database Table or Object (constraints in the case of pgsql), hence the name. Though I'm not sure it does have much to do with tables, so maybe that can be changed.

So the test was failing because the auto_increment field wasn't involved in an index. So these tests should now pass on MySQL.

This issue is related to #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL.

I wonder if this issue is the better place to solve index length issues in PostgreSQL, instead of that one?

Nice patch.

+ * Recreate all the tables on SQLite, to account for changes in the naming convention of indexes.

Maybe change this so that people don't get worried that they are going to lose data.

Other than that, I'm happy to RTBC it.

I think that functionally the patch is probably ready to go too, but there is no upgrade path at the moment.

I reckon that we need to convert (or drop/create) all indexes that we've created in the old formats into the new one. This is a total pain, and probably makes this issue dependent on #1010188: Add support for database-driver updates.

Subscribe

+++ modules/system/system.install
@@ -2973,6 +2973,21 @@ function system_update_7069() {
 /**
+ * Recreate all the tables on SQLite, to account for changes in the naming convention of indexes.
+ */
+function system_update_7070() {
+  if (Database::getConnection()->databaseType() == 'sqlite') {
+    foreach (drupal_get_schema() as $table_name => $table_specification) {

It "sounds" like this update could run "a while"...? ;)

Powered by Dreditor.

from #21:

I reckon that we need to convert (or drop/create) all indexes that we've created in the old formats into the new one. This is a total pain, and probably makes this issue dependent on #1010188: Add support for database-driver updates.

Actually, I really don't think that issue is the way forward, and yes, I think we need to drop and re-create all indexes for SQLite and PostgreSQL.

While I was checking out the thresholds, I was looking at this because it's a major bug report for 8.x
I'm noticing it's been a long time since this was updated.

This could probably use an issue summary update, so tagging for that. (http://drupal.org/node/1427826)
And to enable people to jump back into this, it could use someone to spell out some next steps for moving this forward and just double checking that some of the assertions are still relevant now.

This update code isn't right. drupal_get_schema() only gets tables for currently enabled modues, and it's recommended to disable modules during major version upgrades.

+  if (Database::getConnection()->databaseType() == 'sqlite') {
+    foreach (drupal_get_schema() as $table_name => $table_specification) {

Looks like this could probably be changed to a db_find_tables() though, although the null operation would have to be changed then.

patch no longer applies.

@martin107 actually I don't think anyone has posted a D8 patch yet.

As mentioned in #8 and #19 though, this issue would better be solved in #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL

Guess we could fix this in #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL at least for D8 where we don't need to care about all that update path stuff and independent of SQLITE?

This had been fixed in #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL

I searched around but didn't find any documentation about the double-underscore "feature" of field API.
Is this documented anywhere?

I happen to be one of the un-sane people using double-underscore in property names, and it's making things a bit haywire.