PostgreSQL constraints do not get renamed by db_rename_table()

Nice. Looks like the ideal thing would be fore db_rename_table() in the PostgreSQL driver will need to figure out all of these and rename them, have no idea if that's doable but moving this into that queue.

What version of PostgreSQL? I was able to upgrade a fresh install of Drupal 6.20 to Drupal 7 on PostgreSQL 9 PHP 5.3.2 fine.

I think this is a won't fix. The {date_format} table does not exist in D6.

could this be a table from a contrib module? There is a table named date_formats in the D6 version of date_api (from the Date project), which might be the cause. The table no longer exists in the D7 version afaics, but there is no update function to remove the old table. Perhaps core should provide an upgrade path (since we pulled support for date formats into core now), keeping the custom defined date formats from the date_api module.

In that case, its not a PostgreSQL issue.

Tried to upgrade my MySQL site from D6.20 to D7. Got a similar fatal error:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'mydatabase_t7.date_formats' doesn't exist: SELECT df.dfid, df.format, df.type, df.locked, dfl.language FROM {date_formats} df LEFT JOIN {date_format_type} dft ON df.type = dft.type LEFT JOIN {date_format_locale} dfl ON df.format = dfl.format AND df.type = dfl.type ORDER BY df.type, df.format; Array ( ) in _system_date_formats_build() (line 3744 of /home1/mydatabase/public_html/test7/modules/system/system.module).

Editing title, since same thing happened on a site with mysql database.

I'm sure this is a PostgreSQL issue.

The D6 contrib table is renamed here:

    // Sites that have the Drupal 6 Date module installed already have the
    // following tables.
    if (db_table_exists('date_formats')) {
      db_rename_table('date_formats', 'd6_date_formats');
    }

The original bug report shows that when the date formats table is renamed, the indexes on that table do not get renamed alongside it. This means that when you try to create the same index name on the new table, it fails.

What confuses me though is that this is already supposed to be fixed:

#718016: DatabaseSchema_pgsql::renameTable() needs to rename the indexes

The reports on MySQL look unrelated to me.

So I'm moving back to the postgres driver, but bumping down to 'normal' and someone please try to reproduce this again.

I'm able to reproduce while upgrading from 6.22 to 7.4. Got same error first time and then again after starting from scratch (ouch ;):

PDOException: SQLSTATE[42P07]: Duplicate table: 7 ERROR: relation "date_formats_formats_key" already exists: CREATE TABLE {date_formats} ( dfid serial CHECK (dfid >= 0), format varchar(100) NOT NULL, type varchar(64) NOT NULL, locked smallint NOT NULL default 0, PRIMARY KEY (dfid), CONSTRAINT date_formats_formats_key UNIQUE (format, type) ); Array ( ) in db_create_table() (line 2657 of _snip_/includes/database/database.inc).

Info:
- FreeBSD 8.0-RELEASE-p2
- PostgreSQL 8.4.3
- Nginx 1.0.5
- PHP 5.3.5 via FastCGI
- Date module 6.x-2.7 installed in Drupal.

And sure enough the indexes did not get renamed. Connecting to the database after failed upgrade:

# \d d6_date_formats 
                                Table "public.d6_date_formats"
 Column |          Type          |                          Modifiers                          
--------+------------------------+-------------------------------------------------------------
 dfid   | integer                | not null default nextval('date_formats_dfid_seq'::regclass)
 format | character varying(100) | not null
 type   | character varying(200) | not null
 locked | smallint               | not null default 0
Indexes:
    "date_formats_pkey" PRIMARY KEY, btree (dfid)
    "date_formats_formats_key" UNIQUE, btree (format, type)
Check constraints:
    "date_formats_dfid_check" CHECK (dfid >= 0)

Confirmed. The problem is that we are renaming the indexes, but not the constraints.

Erghem... Are you? Look above. Neither indexes nor constraints got renamed in my case. I think the problem is in renameTable() in includes/database/pgsql/schema.inc:

<?php
    // Index names and constraint names are global in PostgreSQL, so we need to
    // rename them when renaming the table.
    $indexes = $this->connection->query('SELECT indexname FROM pg_indexes WHERE schemaname = :schema AND tablename = :table', array(':schema' => $old_schema, ':table' => $old_table_name));
    foreach ($indexes as $index) {
      if (preg_match('/^' . preg_quote($old_full_name) . '_(.*)_idx$/', $index->indexname, $matches)) {
        $index_name = $matches[1];
        $this->connection->query('ALTER INDEX ' . $index->indexname . ' RENAME TO {' . $new_name . '}_' . $index_name . '_idx');
      }
    }
?>

I tried the SELECT on my database and it produces this:

# select indexname from pg_indexes where schemaname = 'public' and tablename = 'd6_date_formats';
        indexname
--------------------------
 date_formats_formats_key
 date_formats_pkey
(2 rows)

So it does at least get the index names correct. It would seem that if(pgrep_match()) test fails here. What is it in there for anyway?

What we call indexes and what PostgreSQL call indexes is slightly different: indexes are things that end in _idx and are used for query performance only; constraints are things that end in _key or _pkey and are used for relational integrity.

We do rename the indexes but not the constraints. We need to extend the regexp to add _key and maybe (has to be confirmed properly) _pkey.

I see. I have no idea if this is the right way but...

Proof of concept patch. Rename all keys in renameTable(). Update completed with no errors. Successfully installed date-7.x-2.0-alpha3 afterwards.

There's a patch here

Tagging.

I think we want to only rename stuff that end in '_key' and '_idx'. We should not touch the indexes/constraints we are not sure we have created. Otherwise, on visual inspection the patch does the trick.

Update of patch for D8 /core dir.

Would be great if someone with PostgreSQL could test this patch.

Is there any reason to apply this patch other than D6 -> D7 upgrades?

It makes sense not only for D6=>D7 upgrades, but for module updates as well
Tested on Debian testing, PHP5.3.8, Postgres 8.3/8.4 -

Thanks for testing valthebald (per the request in #21). Committed to 8.x. Moving to 7.x.

Hm. This looks a tad bit risky, so I'd like to hold it until after the release this Wednesday. But it'll be part of the release after that!

Committed and pushed to 7.x. Thanks!

I don't think this was fixed.

I just ran db_rename_table() on a table with a unique key, SELECT * FROM information_schema looks like:

                List of relations
 Schema |         Name         |   Type   | Owner 
--------+----------------------+----------+-------
 public | old_name_id_seq | sequence | incap
 public | new_name            | table    | incap

(3 rows)

In other words the id_seq was not renamed along with the table.

Confirm, there's 2 issues:
1) sequence needs to be renamed with alter sequence {name} rename to {new name} for pgsql >= 8.3
2) queryTableInformation() function works with non-updatable data so each schema change should be reflected here, or be need to quiery this data on each request (it seems not so much expensive because schema changed not so frequently)

Patch adds reset for stored information for each schema changing functions

Also extends a schema test to check for primary keys, unique keys and psql sequences (I'm testing under pgsql 8.4 debian)

Changed table definition - all keys are defined

I'm downgrading this to major since it's driver-specific (and not the MySQL driver). When we can support different databases with qa.drupal.org then bugs like this would cause actual test failures (and hence be critical), but until then it doesn't make sense to hold up everything else.

#32: 1013034-pgsql-32.patch queued for re-testing.

re-roll, SchemaTest.php now lives in core/modules/system/lib/Drupal/system/Tests/Database

#39: PostgreSQL-constraints-do-not-get-renamed-by-db_rename_table.101303-1.patch queued for re-testing.

#39: PostgreSQL-constraints-do-not-get-renamed-by-db_rename_table.101303-1.patch queued for re-testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SchemaTest.phpundefined
@@ -123,6 +123,78 @@ function testSchema() {
+    try {
+      db_add_primary_key('test_table2', array('id'));
+      $constraint_exists = FALSE;
+    }
+    catch (Exception $e) {
+      $constraint_exists = TRUE;
+    }
+    $this->assertIdentical($constraint_exists, TRUE, 'Primary key was renamed.');
+
+    // Unique key should be renamed.
+    try {
+      db_add_unique_key('test_table2', 'test_field', array('test_field'));
+      $constraint_exists = FALSE;
+    }
+    catch (Exception $e) {
+      $constraint_exists = TRUE;
+    }
+    $this->assertIdentical($constraint_exists, TRUE, 'Unique key was renamed.');

this needs cleaned up (which might fix the error)

Something like this should work:

<?php
try {
  do_illegal_task();
  $this->fail('No exception was caught');
}
catch (Exception $e) {
  $this->assertEqual("exception message here", $e->getMessage());
}
?>

I'm stumped right at the beginning…

Tried to install D8 with Postgres and discovered that there isn't currently an option to try to use PostgreSQL as the database. Am I missing something obvious?

Yeah, pgsql needs to be compiled into PHP for postgresql installation to work. If your using a mac, try something like brew install php54 --with-pgsql --with-mysql.

Thanks! I thought I'd done that… But all evidence suggests that I did not :-)

Re-added tag removed from last comment.

it has been a while. un assigning. please make a comment to pick it up again if you like.

does this need tests also? I think so. See comment #45

Adding a related issue, #2061879: Remove Schema::copyTable. Because of the single long test, when the copy table exception is thrown, none of the subsequent tests, including the ones added here, get called.

I've also attached a reroll of #39, and am removing the 'needs tests' tag since it adds tests.

@jhedstrom thanks for picking up this one :-) Will do a PostgreSQL bot run tomorrow and post the results here. Also adding parent issue #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release

Well, guess there is actually not much to test here as long as #2061879: Remove Schema::copyTable is not done ;-)

This is critical unless we do #2160433: [policy, no patch] Move PostgreSQL driver support into contrib.

I think it's OK for the initial beta-to-beta upgrade path to be MySQL only while test bot support is getting sorted out but this issue would need to be resolved before an RC.

Okay, let's move forward here to finally make SchemaTest pass, rerolled patch attached (doesn't apply any more). I did local testing of the patch and it shows 1 fail and 1 exception. The fail is because of Index renaming and this is because ensureIdentifiersLength() does stupid double imploding of '__' and adding the idx suffix again, so the new index results in something like this:

simpletest133555test_table2___test_field__idx__idx

And then there is an new exception we need to take care of, because of:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42P16]: Invalid table definition: 7 ERROR: multiple primary keys for table "simpletest133555test_table" are not allowed: ALTER TABLE {test_table} ADD PRIMARY KEY ("id"); Array ( ) in db_add_primary_key() (line 824 of /opt/local/apache2/htdocs/drupal8/core/includes/database.inc).

This one should fix the the failing index rename test by cleaning up the index name before calling ensureIdentifiersLength() to prevent wrong index names.

Let preg_match() do the index name clean up...

Let's test patches

I just spend an hour to make index renaming in renameTable() a bit smarter, by something like this:

    // Index names and constraint names are global in PostgreSQL, so we need to
    // rename them when renaming the table.
    $indexes = $this->connection->query('SELECT indexname FROM pg_indexes WHERE schemaname = :schema AND tablename = :table', array(':schema' => $old_schema, ':table' => $old_table_name));
    foreach ($indexes as $index) {
      // Get the index type by suffix, e.g. idx/key/pkey
      $index_type = substr($index->indexname, strrpos($index->indexname, '_') + 1);

      // If the index is already rewritten by ensureIdentifiersLength() to not
      // exeed the 63 chars limit of PostgreSQL, we need to take care of that.
      // (drupal_Gk7Su_T1jcBHVuvSPeP22_I3Ni4GrVEgTYlIYnBJkro_idx)
      if(strpos($index->indexname,'drupal_') !== false) {
        preg_match('/^drupal_(.*)_' . $index_type . '/', $index->indexname, $matches);
        $index_name = $matches[1];
      } else {
        // Make sure to remove the suffix from index names, because
        // $this->ensureIdentifiersLength() will add the suffix again and thus
        // would result in a wrong index name.
        preg_match('/^' . preg_quote($old_full_name) . '__(.*)__' . $index_type . '/', $index->indexname, $matches);
        $index_name = $matches[1];
      }
      $this->connection->query('ALTER INDEX ' . $index->indexname . ' RENAME TO ' . $this->ensureIdentifiersLength($new_name, $index_name, $index_type));
    }

But this still fails (tested with the file->FileFieldValidateTest()). I also discovered while working on this, that we create pkeys not through ensureIdentifiersLength(). Usually this is not a problem, because pkeys usually do not exceed the 63chars limit, but that leads to more problems here, because they are named table_pkey, but it must read table__pkey. Please note the double underscore instead of the single one! Would be happy if anyone could pick up this issue, as I'm busy over the next week ;-)

Great, I can get file->FileFieldValidateTest() and others pass now, but currently only due some hacks around pkey (please see #63 for details). So to move forward,

• First of all make pkey names compatible with all other index names, e.g. double underscore instead of a single one.
• Second make ensureIdentifiersLength() smarter. All indexes are build from three parameters ensureIdentifiersLength($table, $index_name, $index_type). Only pkey is build from only two ensureIdentifiersLength($table, 'pkey'). Thus results in undefined offset in ensureIdentifiersLength() if no index name is given. Also the 63chars rewrite will fail for to long pkey identifiers currently.
• Make sure to quote identifiers in $this->connection->query('ALTER INDEX "' . $index->indexname . '" RENAME TO "' . $new_index_name .'"') to get hit. This may become obsolete by #1600670: Cannot query Postgres database that has column names with capital letters (untested).
• And last the schema tests here will need some love, we can make constraintExists() public and use that for testing.

Feel free to pick up, I'm busy these days ;-)

Okay, spend another hour the last night on this. This patch passes all schema test on PostgreSQL now, and seems to address the issues discovered in #2425127: Prevent PostgreSQL from creating duplicated index names within schema. There is still an issue for now because pkeys are currently prefixed by four underscores (instead of two), but this could be done in a follow up #2426579: Change method name for pgsql driver's ensureIdentifiersLength() to accurately describe what the method is doing. Now I'm interested if we have schema test pass for MySQL too, so setting to needs review.

This should pass now on MySQL also, let's see...

I have reviewed this patch, and it seems straightforward to me. It certainly helps PostgreSQL pass more of the failing simpletests, and doesn't seem to trigger any additional failures.

I'm almost ready to mark it RTBC, but I want to make sure we've come to a decision about whether to fix the underscores prefixing the pkey, or to make that part of another issue. Either way, I don't want it to fall through the cracks.

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -420,17 +434,43 @@ function renameTable($table, $new_name) {
   +      if(strpos($index->indexname,'drupal_') !== false) {
   

   Small coding standard fix here to put a space between control functions and opening parentheses.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -420,17 +434,43 @@ function renameTable($table, $new_name) {
   -        $this->connection->query('ALTER INDEX ' . $index->indexname . ' RENAME TO ' . $this->ensureIdentifiersLength($new_name, $index_name, 'idx'));
          }
   +      $this->connection->query('ALTER INDEX "' . $index->indexname . '" RENAME TO "' . $this->ensureIdentifiersLength($new_name, $index_name, $index_type) .'"');
   

   Is it possible that $new_name can be passed straight into the ALTER query here? It's not a vulnerability since it's in code. I'm not sure if there's a module that provides an interface for renaming tables. I don't think it's a big deal, but something to think about. db_rename_table('node', '"; delete from users') ?

I agree with @mradcliffe here -- the use of parameterized queries (placeholders) would make me a lot more comfortable here than just concatenating values together to form a SQL statement, and help us avoid SQL injection.

See the "Placeholders" section of https://www.drupal.org/node/310072 for some great examples.

Thanks for the review! The patch attached address the syntax fix from #70. Regarding the placeholders, I already tried this but with no luck. This is because all placeholder values get escaped by a single quote only, e.g. 'value'. All my attempt to add double quotes fail, they are always removed. While this is not a problem for usual values in queries, it is a problem in our case here:

d8=> CREATE TABLE test_table (id int null, data varchar(64) null);
CREATE TABLE
d8=> CREATE INDEX testindex ON test_table (id);
CREATE INDEX
d8=> CREATE INDEX 'testindex1' ON test_table (id);
ERROR:  syntax error at or near "'testindex1'"
LINE 1: CREATE INDEX 'testindex1' ON test_table (id);
                     ^
d8=> CREATE INDEX "testindex1" ON test_table (id);
CREATE INDEX
d8=> CREATE INDEX "'testindex2'" ON test_table (id);
CREATE INDEX
d8=>

Open for suggestions :-)

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -139,6 +139,20 @@ public function queryTableInformation($table) {
   +    if (!strpos($key, '.')) {
   

   We should be doing strict checking here - strpos($key, '.') === FALSE

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -420,17 +434,43 @@ function renameTable($table, $new_name) {
   +        preg_match('/^drupal_(.*)_' . $index_type . '/', $index->indexname, $matches);
   

   do we need to preg_quote $index_type?

Update with suggestions from @larowlan, thanks!

Issue summary update.

There's only one module that could possibly cause data loss, and that's (ironically) data module via data_ui, but the module does not allow table renaming so it's a non-issue. I think looking into adding escaping could be done later after the escape methods are built out in the other issue.

Patch looks good to me too.

I have added some additional safety checks around table and schema names, to help avoid the problem of unescaped table and schema names being simply concatenated to form SQL queries. After further reading, it looks like placeholders should *not* be used for table and schema names, and instead we should use db_escape_table() before using a table or schema name in a query.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -110,8 +110,8 @@ public function queryTableInformation($table) {
+          ':schema' => db_escape_table($schema),
+          ':table' => db_escape_table($table_name),

If that's the route to take, then there's a method on the Connection class to escape tables: escapeTable.

I don't see this being done in any other Core driver for alter queries. I'm trying to figure out why to see if this is necessary or not. So far this has me digging into PDO C code. :(

If that's the route to take, then there's a method on the Connection class to escape tables: escapeTable.

Right... db_escape_table calls that same method.

I don't see this being done in any other Core driver for alter queries. I'm trying to figure out why to see if this is necessary or not. So far this has me digging into PDO C code. :(

As mentioned on another issue (which I can't seem to find at the moment), the risk seems to me to be limited to code, but there's a risk that someone passes "foo; drop table system" as the table name, or something similarly nefarious. I'd rather make sure that something like that can't happen. My patch might have gone a bit overboard (as it seems to be causing additional test failures, booooo....), but I'd rather err on the side of caution, personally. I'd love to hear from other developers on whether they think I'm being too paranoid.

+1 for improved security, but this smells like a follow up to me, especially if we open new fails with that approach. Based on the comments from @mradcliffe in #78 no other driver for now takes care of this additional security improvement. Would be happy if we can agree on a follow up based on the interdiff in #77 and get this one in as is for now?

OK, fair enough... I'm re-uploading the patch from comment 74 and marking it as RTBC, as I've re-reviewed it thoroughly and think it's ready for commit. I'll then go back and double-check if we really need to escape the table names (as I had attempted to do in comment 76), and create a follow-up issue if that's really necessary.

The changes to the test fail on SQLite. SQLite does not create an index for primary keys. Please try to run all changed db tests on SQLite too - for info on how and what currently fails read #2318191: [meta] Database tests fail on SQLite.

Awesome work, PostgreSQL Brigade! :)

Committed and pushed to 8.0.x. Thanks!

Moving for backport. I believe PostgreSQL issues are not critical in D7, so downgrading accordingly.

Great to see this one committed right after my vacation, thanks! So I think #2425127: Prevent PostgreSQL from creating duplicated index names within schema and #2426581: Fix schema generators to prevent creating duplicate table, indexes, and other constraints so that database drivers do not need to implement their own solutions can now be set closed/fixed?

Note that the patch that was committed in #83 failed to consider the case when the table name combined with the primary key column name exceeds 63 chars. Opened a followup issue to address this: #3026290: PostgreSQL constraints are still not renamed properly on table renames.

Also, I think it's time to mark this issue "fixed" in 8.x. If anyone wants to backport this patch and the one from the issue mentioned above to 7.x, please open a new issue :)