PostgreSQL: Add support for reserved field/column names

Note that you will also have to update core/modules/migrate_drupal/src/Tests/d6.gz following the procedure detailed in core/scripts/migrate-dump-d6.sh, see #2454669-10: SQLite: Fix tests in migrate_drupal test group and #2452709-4: migrate_drupal test dump generation documentation.

Yep, it's quite cumbersome and time consuming :/ But at least there's a plan to improve it: #2469623: Process for creating migration source DBs for automated tests

Visibility.

@bzrudi is the d6 dump file still needed?

Is renaming the column the right approach? Just been having a look at the event contrib module, which defines the event_timezones table. It does contain a column called offset, which is allowed by PostgreSQL but requires surrounding with quotes.

See event.install:

  switch ($GLOBALS['db_type']) {
    case 'mysql':
    case 'mysqli':
      db_query("CREATE TABLE {event_timezones} (
                timezone int NOT NULL default '0',
                name varchar(255) NOT NULL default '',
                offset TIME NOT NULL default '0',
                offset_dst TIME NOT NULL default '0',
                dst_region int NOT NULL default '0',
                is_dst int NOT NULL default '0',
                PRIMARY KEY (timezone)
                ) /*!40100 DEFAULT CHARACTER SET utf8 */;");
      break;
    case 'pgsql':
      db_query("CREATE TABLE {event_timezones} (
                timezone integer NOT NULL default '0',
                name varchar(255) NOT NULL default '',
                \"offset\" interval NOT NULL default '0 seconds',
                offset_dst interval NOT NULL default '0 seconds',
                dst_region integer NOT NULL default '0',
                is_dst integer NOT NULL default '0',
                PRIMARY KEY (timezone)
                ) ");
      break;
  }

The tests aren't failing when the table is created, because the createTableSql function in Drupal\Core\Database\Driver\pgsql\Schema escapes the column name with "". However the Drupal\Core\Database\Driver\pgsql\Insert class does not do this, and perhaps it should. If reserved column names are supported then the PG driver seems like the right place to escape them.

Attached patch illustrates what I mean (and fixes various test failures), but I imagine needs some more work as there may well be other instances of this happening (DELETE, UPDATE etc).

Please note it also only works with #2477845: PostgreSQL: Fix broken migrate table creation applied too.

Just noticed a silly mistake in #9, where the escape function isn't actually doing anything. Uploading a new patch.

Flip to needs review.

Thanks for the extra test coverage.

#1600670: Cannot query Postgres database that has column names with capital letters addressed a similar concern, making case identifiers case-sensitive (again, technically allowed by PostgreSQL, but you need to escape them). It added some functions in the Drupal\Core\Database\Driver\pgsql\Connection class to escape table/column names if they have an uppercase character. Their use seems inconsistent across create table/insert/update etc though.

Is it best to always escape all identifiers? Are there any implications with doing that, other than an unnecessarily verbose query string?

I've attached a new patch that changes the various escape... functions to add the quotes regardless of case. Also calls the escaping for UPDATE statements.

Changed the name of the patch file from comment #18 to have it tested by the testbot.
The patch looks good to me.
Does anybody know which tests this patch should fix?

@bzrudi71: Can you test the patch with a postgresql database.

Thanks for the feedback @bzrudi, and I now have some more time to work on this over the next few days.

Here is a new patch based on #18 but without the changes to the escape functions. This doesn't stop MigrateDrupal6Test from failing though, because the column name offset is no longer quoted within the INSERT statement.

Specifically, this code:

    $this->database->insert("event_timezones")->fields(array(
      'timezone',
      'name',
      'offset',
      'offset_dst',
      'dst_region',
      'is_dst',
    ))
    ->values(...);

... results in this SQL statement:

INSERT INTO simpletest1050020event_timezones (timezone, name, offset, offset_dst, dst_region, is_dst) VALUES ...

What I'm a bit uncertain of is why we are always quoting identifiers in the createTable method, but not insert/update. So the table is created ok despite the reseved work, but the tests fail once we try to populate it. Shouldn't the behaviour be consistent?

Please see the comments in the mentioned issue #1600670: Cannot query Postgres database that has column names with capital letters, why the identifiers are not all quoted by default.

Please can you clarify which comments you are referring to as I read through them but am still a bit confused.

For the testbot

I have changed the patch so that only PostgreSQL reserved key words are quoted.
The following tests passes for me locally with a PostgreSQL backend:

• Drupal\Tests\Core\Database\Driver\pgsql\PostgresqlConnectionTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserPictureFileTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserProfileEntityDisplayTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserProfileEntityFormDisplayTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserProfileFieldTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserProfileFieldInstanceTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserProfileValuesTest
• Drupal\migrate_drupal\Tests\d6\MigrateUserTest
• Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test

@bzrudi71: Thank you for testing the patch on your testbot. ;)

The Drupal\system\Tests\Installer\SingleVisibleProfileTest passes with the patch from #26 on my local machine.

The approach looks good to me, and the migrate_drupal tests that were failing before are all passing.

One small thing:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -32,6 +32,26 @@ class Connection extends DatabaseConnection {
 
   /**
+   * The list of PostgreSQL reserved key words.
+   * See http://www.postgresql.org/docs/9.1/static/sql-keywords-appendix.html.
+   */

The list should probably be based on PostgreSQL 9.4, and include collation and lateral which have become reserved keywords since 9.1.

@ingaro: Thanks for the review and you made a very good point!

Added collation and lateral to the list.
Accidentally added over to the list. Now removed.

Updated issue summary, added beta evaluation template, and beta evaluation after reviewing the issue and patch.

Firefox crashed on me the first time so I had to rewrite it.

1. Doing the following query in postgres
   select * from pg_get_keywords() where catcode in ('R', 'T');
   reveals that there are 99 reserved words - the list in the method appears to be 98 long. Not sure what is missing. Also the query looks useful for a kernel test :)

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -32,6 +32,26 @@ class Connection extends DatabaseConnection {
   +   * See http://www.postgresql.org/docs/9.4/static/sql-keywords-appendix.html.
   

   Should be @see and no fullstop and a blank line above.

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -32,6 +32,26 @@ class Connection extends DatabaseConnection {
   +  protected $postgresqlReservedKeyWords = ['all', 'analyse', 'analyze', 'and',
   +  'any', 'array', 'as', 'asc', 'asymmetric', 'authorization', 'binary', 'both',
   +  'case', 'cast', 'check', 'collate', 'collation', 'column', 'concurrently',
   +  'constraint', 'create', 'cross', 'current_catalog', 'current_date',
   +  'current_role', 'current_schema', 'current_time', 'current_timestamp',
   +  'current_user', 'default', 'deferrable', 'desc', 'distinct', 'do', 'else',
   +  'end', 'except', 'false', 'fetch', 'for', 'foreign', 'from', 'full', 'grant',
   +  'group', 'having', 'ilike', 'in', 'initially', 'inner', 'intersect', 'into',
   +  'is', 'isnull', 'join', 'lateral', 'leading', 'left', 'like', 'limit',
   +  'localtime', 'localtimestamp', 'natural', 'not', 'notnull', 'null', 'offset',
   +  'on', 'only', 'or', 'order', 'outer', 'overlaps', 'placing', 'primary',
   +  'references', 'returning', 'right', 'select', 'session_user', 'similar',
   +  'some', 'symmetric', 'table', 'then', 'to', 'trailing', 'true', 'union',
   +  'unique', 'user', 'using', 'variadic', 'verbose', 'when', 'where', 'window',
   +  'with'];
   
   @@ -167,6 +187,10 @@ public function escapeField($field) {
   +    elseif (in_array(strtolower($escaped), $this->postgresqlReservedKeyWords)) {
   +      // Quote the field name for PostgreSQL reserved key words.
   +      $escaped = '"' . $escaped . '"';
   +    }
   
   @@ -181,6 +205,10 @@ public function escapeAlias($field) {
   +    elseif (in_array(strtolower($escaped), $this->postgresqlReservedKeyWords)) {
   +      // Quote the alias name for PostgreSQL reserved key words.
   +      $escaped = '"' . $escaped . '"';
   +    }
   

   This looks very testable. It'd be great to see a test that ensure all postgres reserved words are quoted as expected for both escapeField, escapeAlias, and escapeTable. Given that the reserved word list is queryable perhaps a kernel testbase that skips if the db is not pgsql would be good.

4. +++ b/core/modules/system/src/Tests/Database/DeleteTruncateTest.php
   @@ -70,4 +70,20 @@ function testTruncate() {
   +   * Confirms that we can delete a single special column name record
   +   * successfully.
   

   Two line method summary - should be one line.

My list from psql -c "select * from pg_get_keywords() where catcode in ('R', 'T');"

       word        | catcode |                 catdesc                 
-------------------+---------+-----------------------------------------
 all               | R       | reserved
 analyse           | R       | reserved
 analyze           | R       | reserved
 and               | R       | reserved
 any               | R       | reserved
 array             | R       | reserved
 as                | R       | reserved
 asc               | R       | reserved
 asymmetric        | R       | reserved
 authorization     | T       | reserved (can be function or type name)
 binary            | T       | reserved (can be function or type name)
 both              | R       | reserved
 case              | R       | reserved
 cast              | R       | reserved
 check             | R       | reserved
 collate           | R       | reserved
 collation         | T       | reserved (can be function or type name)
 column            | R       | reserved
 concurrently      | T       | reserved (can be function or type name)
 constraint        | R       | reserved
 create            | R       | reserved
 cross             | T       | reserved (can be function or type name)
 current_catalog   | R       | reserved
 current_date      | R       | reserved
 current_role      | R       | reserved
 current_schema    | T       | reserved (can be function or type name)
 current_time      | R       | reserved
 current_timestamp | R       | reserved
 current_user      | R       | reserved
 default           | R       | reserved
 deferrable        | R       | reserved
 desc              | R       | reserved
 distinct          | R       | reserved
 do                | R       | reserved
 else              | R       | reserved
 end               | R       | reserved
 except            | R       | reserved
 false             | R       | reserved
 fetch             | R       | reserved
 for               | R       | reserved
 foreign           | R       | reserved
 freeze            | T       | reserved (can be function or type name)
 from              | R       | reserved
 full              | T       | reserved (can be function or type name)
 grant             | R       | reserved
 group             | R       | reserved
 having            | R       | reserved
 ilike             | T       | reserved (can be function or type name)
 in                | R       | reserved
 initially         | R       | reserved
 inner             | T       | reserved (can be function or type name)
 intersect         | R       | reserved
 into              | R       | reserved
 is                | T       | reserved (can be function or type name)
 isnull            | T       | reserved (can be function or type name)
 join              | T       | reserved (can be function or type name)
 lateral           | R       | reserved
 leading           | R       | reserved
 left              | T       | reserved (can be function or type name)
 like              | T       | reserved (can be function or type name)
 limit             | R       | reserved
 localtime         | R       | reserved
 localtimestamp    | R       | reserved
 natural           | T       | reserved (can be function or type name)
 not               | R       | reserved
 notnull           | T       | reserved (can be function or type name)
 null              | R       | reserved
 offset            | R       | reserved
 on                | R       | reserved
 only              | R       | reserved
 or                | R       | reserved
 order             | R       | reserved
 outer             | T       | reserved (can be function or type name)
 overlaps          | T       | reserved (can be function or type name)
 placing           | R       | reserved
 primary           | R       | reserved
 references        | R       | reserved
 returning         | R       | reserved
 right             | T       | reserved (can be function or type name)
 select            | R       | reserved
 session_user      | R       | reserved
 similar           | T       | reserved (can be function or type name)
 some              | R       | reserved
 symmetric         | R       | reserved
 table             | R       | reserved
 then              | R       | reserved
 to                | R       | reserved
 trailing          | R       | reserved
 true              | R       | reserved
 union             | R       | reserved
 unique            | R       | reserved
 user              | R       | reserved
 using             | R       | reserved
 variadic          | R       | reserved
 verbose           | T       | reserved (can be function or type name)
 when              | R       | reserved
 where             | R       | reserved
 window            | R       | reserved
 with              | R       | reserved
(99 rows)

The missing one is freeze. I've added this and also addressed points 2 and 4 in #38.

Added a new test in Drupal\system\Tests\Database\ConnectionTest that does the query @alexpott mentioned, and tests the various escapeXXX methods. Is this the right place to do that?

By having this query in the test we should also catch any new reserved words that might appear in future versions of PostgreSQL.

Interesting. It looks like over was a reserved word up to and including 9.3, but is no longer. I had a look through other versions since 9.1 and as far as I can tell that is the only one.

I've added it to the list.

The added test looks good to me. Thanks ingaro.

I think that Drupal\system\Tests\Database\ConnectionTest is the right place to add the new test for the escapeTable(), escapeField() and escapeAlias() methods.

By having this query in the test we should also catch any new reserved words that might appear in future versions of PostgreSQL.

We only test the keywords from the PostgreSQL database version(s) we use in our testbot.

The keyword "over" was in my patch from comment #26. I removed it later because in PostgreSQL 9.4 it is no longer a keyword. I thought that I made an error by adding it in the first place. :(

I made one minor documentation change to the patch:

+++ b/core/modules/system/src/Tests/Database/ConnectionTest.php
@@ -138,4 +138,24 @@ public function testMultipleStatementsForNewPhp() {
+   * Test the escapeTable(), escapeField() and escapeAlias() methods
+   * with all possible reserved words in PostgreSQL.

alexpott stated in comment #38.4 that a summary line should be on one line.

All points from alexpott are addressed.
The newly added Drupal\system\Tests\Database\ConnectionTest passes for me locally.
It all looks good to me.
The last patch from me was only a documentation change.
So for me it is back to RTBC.

Good point bzrudi71. I missed that one.

Committed 40335e9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

For the backport.

I only just became aware of this issue via #2560965: Reserved column names are broken on MySQL, the test lies.

Per comments 5 and 7 in that issue, and the very-old issue linked from there as well, this fix is contrary to the design of the DB layer and is also incomplete and buggy. It should be reverted.

From the summary: "Some migrate tables use reserved keywords as column name like OFFSET". Well, then it shouldn't be using reserved keywords as columns. That's a deliberate design decision and changing that design should not be done in an ad hoc fashion like this.

Well this has not been reverted and I think the whole

this fix is contrary to the design of the DB layer and is also incomplete and buggy.

has been blown out-of-water by MySQL 8 and the new reserved words that's introduced. The decisions in #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements are no longer tenable - db drivers need to be able to quote field (and table) names as they see fit. We are never going to be able to come up with a list of reserved words because Drupal just is not in charge of what different databases considered a reserved keyword.

This issue appears to be preventing us from using aegir hostmaster with Postgresql