Cannot query Postgres database that has column names with capital letters

I think this is an issue for the Drupal project, wrong issue queue, re-assigning it.

Randomly setting the last stable, core people will categorize it right.

I tracked this down to /includes/database/database.inc The escapeField() and escapeAlias() are removing the double quotes.

I've attached a patch that stops them from being removed. It probably has SQL injection written all over it, but I'm marking it as needs review.

According to the comments on escapeField():

 * For some database drivers, it may also wrap the field name in
 * database-specific escape characters.

That may be a better solution, force the PDO drive to double quote all field names, but I don't know where that setting would be made.

And your hook_schema would look like:

  $schema['mytable'] = array(
    'description' => 'Custom Table',
    'fields' => array(
      'id' => array(
        'description' => 'Database id.',
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),
      '"firstName"' => array(
        'description' => 'The first name.',
        'type' => 'text',
        'default' => '',
      ),
    ),
    'primary key' => array('id'),
  );

That's single quote ('), double quote ("), column name, double quote ("), single quote ('). No escape slashes.

Attached is a new patch that should address any SQL injection issues. It checks for double quotes at the start and end of the string. It then sanitizes the string the old way and if both quotes were there, it adds them back.

You are going to need to override those methods to the PostgreSQL-specific implementation. The base implementation is sane and should not be touched.

I've moved them to the includes/database/pgsql/database.inc file so they are PostgreSQL-specific.

I know this has to go through 8.x, but could it be backported to D7 if approved?

Should another approach be taken to get support for this?

Thanks for the feedback.

Patch attached.

Oh... and needs review.

Here's a D8 version.

Fixing tags per http://drupal.org/node/1517250

Tagging

IMO it's better to leave field name in schema definition without double quotes

$schema['mytable'] = array(
    'description' => 'Custom Table',
    'fields' => array(
      'id' => array(
        'description' => 'Database id.',
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),
      'firstName' => array(
        'description' => 'The first name.',
        'type' => 'text',
        'default' => '',
      ),
    ),
    'primary key' => array('id'),
  );

but add double quotes in escapeField()
This could make code a little bit safer, and allow the same schema work in both mysql and postgresql

IMO that is a bad approach.

If the double quotes are moved to escapeField() in the PostgreSQL driver, ALL fields would be escaped with double quotes on ALL queries. That is a very aggressive change to make. There is a chance that existing working code would break. If the column name in existing code was 'firstName' => array..., the default behavior has been to change that to 'firstname' when a query is ran. By moving the double quoting to escapeField() that will become '"firstName"' from now on for ALL queries in PostgreSQL. That query will now fail in PostgreSQL on previous installs that upgrade to this feature.

By keeping the double quotes in the schema definition, I'm being explicit that the column name should be double quoted for that column only. The schema definition is the best place to be explicit that a column in a database schema should be double quoted. One could argue that an array property be added and is a better place to store this information:

'double quote' => TRUE

The reason for the double quotes is to add compatibility for querying third party databases that use this convention. That is how you write the queries in PostgreSQL, even using psql to query the database directly from the command line. There is no reason to support both MySQL and PostgreSQL when you are trying to interface with a third party PostgreSQL database that actually requires queries be written like this:

SELECT "firstName" FROM mytable;

If support for MySQL is a concern, my current patch will actually work fine on MySQL. The double quotes will be removed by the default database driver's escapeField(). Also, in MySQL field names are case-insensitive regardless. The following query would be generated for MySQL and run just fine.

SELECT firstName FROM mytable;

First, I don't see how postgres version of escapeField() could affect MySQL. Field name in schema definition is without quotes, and quotes are added by postgresql driver.
Second, while escaping all field names is not a problem at all (and, if I remember correctly is even encouraged by SQL-92), it is possible to escape only "special" field names. Postgres is doing just fine with all-lowercase field names, without double quoting, but requires double quotes if there are other symbols, i.e. uppercase characters. drupal core tables, which do not use upper case, remain the same in both drivers.
Third, since this is a driver-specific problem, I find it more appropriate to solve it in driver, but not in (driver-agnostic) schema.

1. I never said it would affect MySQL. In MySQL, if you make your schema column names double quoted, the current behavior will remove them. MySQL field names are case-insensitive, so a schema like this:
   

   '"firstName"' => array(...
   

   Will create a query like this in MySQL currently and it will run just fine. Double quoting a schema column does not effect MySQL.

   SELECT firstName FROM mytable;
   

2. I'm not concerned with Drupal core compatibility, it should be all lower case. I'm concerned with breaking third party modules where a developer used a capital letter in a schema definition. The current behavior would have created a column with all lower case letters. If we now wrap ALL fields with quotes, queries in that third party module are going to break because what was:

   SELECT firstname FROM mytable;
   

   Will now be:

   SELECT "firstName" FROM mytable;
   

   That query will fail on an existing install because when the table was created the column as added as all lower case letters.

   Yes, in PostgreSQL you can quote all column names and you should be able to. However, you don't make a change like that to a production system without a lot of testing to ensure you do not break your queries. The patch as it is has no potential to break queries. Quoting EVERYTHING does and would require all third party modules to test their code against this change.

3. It's not a driver-specific problem. This is exactly what a schema is supposed to do. Define when creating or interacting with this table that this column should be double quoted. Just like how you define that a column should not be null and it's default should be 0. Maybe adding 'double quote' => TRUE to the schema is a more explicit and cleaner approach. Then database drivers can choose to implement it if needed and no existing code will break.

Changing the default behavior in PostgreSQL to quote ALL fields is dangerous without a massive amount of testing.

Seems this is more or less a duplicate of #1622982: PostgreSQL auto-converts column names into lowercase.

Indeed, this is pretty much the same bug report, both were even created around the same time. Looks like I didn't find this issue back then, because it isn't properly categorized and its title is too vague. Fixing that now + copying over the issue summary.

The shared trigger for both issues is that Drupal code needs to be able to query a Postgres database using table/column identifiers that may be using uppercase letters.

This issue identified the need to query an external Postgres database (not owned by Drupal). My issue identified that a Drupal module cannot use table column names that are simply re-using the letter-casing of response parameters of an external RESTful API.

In order to proceed, I think we should merge the patch from #1622982: PostgreSQL auto-converts column names into lowercase into the patch here.

Merged both patches as suggested by sun. While the patch seems okay for me for Drupal 8 and gives the expected results, I think back port to Drupal 7 needs some discussion because of #17. However let's get things done for D8 at least :-)

Added parent issue

Can someone give me a leg up on where to find the D7 code to implement this patch?

Thanks

@robertwb I don't think there was any yet, try this backport. I also added the extra check mentioned in comment #5 of #1622982: PostgreSQL auto-converts column names into lowercase.

Is there anything we can do to address the concerns in #17? Perhaps a module_invoke_all('schema') in an update hook and renaming the relevant lowercase-only fields to reflect the capitalization in the schema definitions?

Setting back to 8.x now

Great, patch still applies. Did some docker test runs, incl. Database tests and patch doesn't break anything as far as I can see. Anyone doing review, so that we can haz RTBC :-)

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -183,6 +183,49 @@ public function queryTemporary($query, array $args = array(), array $options = a
   +  * @return
   

   Should have "string" as the return type. See API documentation and comment standards.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -183,6 +183,49 @@ public function queryTemporary($query, array $args = array(), array $options = a
   +    $escaped = preg_replace('/[^A-Za-z0-9_.]+/', '', $field);
   

   SQL identifiers and key words must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters in an identifier or key word can be letters, underscores, digits (0-9), or dollar signs ($). Note that dollar signs are not allowed in identifiers according to the letter of the SQL standard, so their use might render applications less portable. The SQL standard will not define a key word that contains digits or starts or ends with an underscore, so identifiers of this form are safe against possible conflict with future extensions of the standard.

   This matches the MySQL documentation regarding identifiers.

   It's probably a rare case that someone would want their localized non-latin characters or latin with diacritics in their column names. And this would technically allow someone to attempt to create a column that begins with a number or underscore.

   Also, this has the "." character in the class, but that's not documented (or supported?).

Neither the sqlite or mysql drivers check the identifier for valid characters.

I guess that means follow-up issues.

Edit: This is not necessarily true. MySQL supports other characters if quoted so it probably doesn't need to implement escapeField.

Since I just ran into the same problem where I need to query an external PostgreSQL DB with CamelCase table (and field) names via Drupal 7, I want to add that the escape function for fields should be extended.

The patch works fine for simple queries (without joins). However, if you need to set a WHERE condition on a field using table aliases, the current patch is not enough since "alias.fieldName" doesn't work. It's either alias."fieldName" or "alias"."fieldName".

So, the line

$escaped = str_replace('.', '"."', $escaped);

should be added to escapeField() in order to allow tableName.fieldName constructs:

public function escapeField($field) {
    $escaped = preg_replace('/[^A-Za-z0-9_.]+/', '', $field);

    // Check for beginning and ending double quote.
    if (preg_match('/^["]/', $field) && preg_match('/["]$/', $field)) {
      $escaped = '"' . $escaped . '"';
      $escaped = str_replace('.', '"."', $escaped);
    }
    return $escaped;
  }

Additionally, I added an escapeTable() function (current D7 patch only has functions for field names and aliases):

  /**
   * Escapes a table name string.
   *
   * Force all table names to be strictly alphanumeric-plus-underscore.
   * For some database drivers, it may also wrap the table name in
   * database-specific escape characters.
   *
   * @return
   *   The sanitized table name string.
   */
  public function escapeTable($table) {
    $escaped = preg_replace('/[^A-Za-z0-9_]+/', '', $table);

  // Check for beginning and ending double quote.
    if (preg_match('/^["]/', $table) && preg_match('/["]$/', $table)) {
      $escaped = '"' . $escaped . '"';
    }
    return $escaped;
  }

This affects revision handling for entities as the "isDefaultRevision" property is set via a Sql When statement (If/Then).

escapeAlias should always escape with quotes in my opinion. We should escape the alias in places such as addExpression (see patch). I incorporated some of my comments from above into this patch as well.

Needs a run on a pgsql testbot - all tests or database + node revision tests.

Edit: I generated interdiff poorly as i modified the prior patch before committing locally :(. Please ignore that file.

Please find attached the output from node and database PG test. Seems we are quoting sometimes to much ;-)

No fair, your machine is faster than mine! :-)

- Patch failed Database tests on db-pgsql-9.1 + web-5.4 test run after I finally got to rebuild my testbot box. (SchemaTest, SelectComplexTest, SelectTest, UpdateComplexTest, AlterTest). (+10 fails, +13 exceptions)
- All node revision tests + NodeTypeRenameConfigImportTest pass now though NodeAccessPagerTest failed (-17 fails, +3 fails)

Still think that escapeAlias should quote aliases for AddExpression and what not cases, and that escapeField should take into account whether it needs to quote or not because it may have already gone through escapeAlias and escapeTable respectively.

Re #35 Yep, mradcliffe I'm with you. We should try to quote all and everything. But since this seems to rise some new fails I just startet a complete (--all) testbot run so we will have a better overview of what breaks and what will be fixed. Will attach output later, and even if my box seems a bit faster, this will still take some time :-)

Here is the full run PostgreSQL log. As I still have bad cold, just posting here ;-)

AlterTest, SelectTest, SelectComplexTest fail because the alias is set as the key. That should be easy.

SchemaTest as we already know fails due to copyTable() not being implemented.

However...
UpdateComplexTest::testSubSelectUpdate fails because the test is bad and asserts that the query string will be the same on all drivers. This is not necessarily the case because of other things in core (the isdefaultrevision alias issue). However the query result is the same despite this failure. I think that requires a follow-up issue of itself to look at if we can even assert identical query strings anymore in core tests.

I could also add the quoting mechanism to the default addExpression() and that'll cause the test to fail on current testbot (mysql) too. :-)

OR we could create an API change in Drupal 8.0.x to change the method signature to only optionally use escapeAlias(). I'm not sure if that would make anyone too happy as we're in beta. I think it would be very confusing to a new developer to wonder why one wouldn't want to escape aliases sent into addExpression() by default.

@alexpott mentions that the test against the query string is necessary in the typo'd comment in #1837118: UPDATE foo SET bar=(SELECT...) is not supported: https://www.drupal.org/node/1837118#comment-6755630

I don't think __toString should get rid of SQL specific escaping or quoting, but that's also an option.

If the assertIdentical for query strings is really required, the only thing I could think of is fixing it within the tests themselves. Given that MySQL will never ever do any quoting within the query string (else we should have failing test on MySQL also) it should be save to hard remove all quotes for the comparison. Guess API change is no option and as you mentioned - why would one not quote?

Let's not add the next workaround. If we could agree with doing the fixes within the tests we should first figure out how many tests use query string comparison ;-)

Just did a quick grep for assertIdentical query strings but just found the one in UpdateComplexTest. So should not something like this be good enough str_replace('"', '', $query_string) to make test pass?

Added quote string for mysql too, but that may come back with a failure. Verified that sqlite uses quote for aliases and columns.

This patch won't fix all the test failures (SelectTest and SelectComplexTest). It looks like adding the quote to expressions is screwing up the orderby logic that we have in pgsql driver.

Yep, there seems to be a problem with orderBy() since the quoting in addExpression(). Especially the comment and search test groups are totally broken since. I already spend some minutes on that but was not able to figure out yet. Anyway thanks for the patch update :-)

Found the issue with orderBy() and select tests. There are some more exact query matches, but those tests use assertEqual().

Additionally I moved the use of escapeAlias() in Select::addExpression().

I only ran this passed the Database test suite with passes except for known copyTable test. Don't have enough time for a Node or full test before my flight.

Edit: could run some tests on my next flight.

Grat work @mradcliffe! Just started complete test run and it looks very promising so far. Just think that we need an issue summary update here before we can haz RTBC?

We are getting closer and closer :-) Patch from #44 does not show any new failures, but fixes a couple of the known ones. All fails in editor and field group tests pass now, as well as some single tests in other test groups! Full test run log attached...

I think my preg_replace() in the test is lazy, and probably should be preg_replace('/[`"][A-Za-z0-9_]+[`"]+/') instead. \S+ may match things that it shouldn't and confuse future developers.

And the revised patch.

Per #45 and #47, marking RTBC.

Just did a review of the patch for syntax but found nothing obviously. RTBC!

+++ b/core/modules/system/src/Tests/Database/SelectTest.php
@@ -39,10 +39,12 @@ function testSimpleComment() {
+    // Remove quote characters.
+    $query_string = preg_replace('/["`]([A-Za-z0-9_]+)["`]/', '$1', $query);

@@ -57,10 +59,12 @@ function testVulnerableComment() {
+    // Remove quote characters.
+    $query_string = preg_replace('/["`]([A-Za-z0-9_]+)["`]/', '$1', $query);

+++ b/core/modules/system/src/Tests/Database/UpdateComplexTest.php
@@ -147,6 +147,8 @@ function testSubSelectUpdate() {
+    // Remove quote characters.
+    $query_string = preg_replace('/["`]([A-Za-z0-9_]+)["`]/', '$1', $query_string);

So I think we should add a new assertion to DatabaseTestBase that does the query string mangling only for postgres rather than all dbs. Something like assertQueryStringEqual()

We also seem to be missing explicit test coverage of the change.

+  public function escapeField($field) {
+    $escaped = preg_replace('/^[^A-Za-z0-9_][^A-Za-z0-9_.]+/', '', $field);
+
+    // Check for beginning and ending double quote.
+    if (preg_match('/^["]/', $field) && preg_match('/["]$/', $field)) {
+      $escaped = '"' . $escaped . '"';
+    }
+
+    return $escaped;
+  }

This doesn't make any sense to me. The field passed to escapeField should not be escaped already, so we should unconditionally escape it here.

As far as the tests are concerned, please just fix them. Tests should not try to do dumb comparisons of query strings, ever.

I think that the reason escapeField needs to check if it's already quoted is that in some cases escapeField is called on a "table.field". I was seeing things like "table"."column" trying to get re-escaped as ""table"."column"".

Also, @Damien Tournoud, do you mean "fix them" as in do as @alexpott suggests or "fix them" as in just leave it as in the latest patch? I'm not sure which is the dumb query string comparison: a) assuming query string will always be the same or b) normalizing the query string before comparison?

I think that the reason escapeField needs to check if it's already quoted is that in some cases escapeField is called on a "table.field". I was seeing things like "table"."column" trying to get re-escaped as ""table"."column"".

That's fine, in that case it should split it and encode it separately.

Also, @Damien Tournoud, do you mean "fix them" as in do as @alexpott suggests or "fix them" as in just leave it as in the latest patch? I'm not sure which is the dumb query string comparison: a) assuming query string will always be the same or b) normalizing the query string before comparison?

Neither; tests really should not do any string comparison. Let's try to change them so that we still assert for the right thing without doing a full string comparison: the two comment tests should really just look for the comment, not compare the whole query string. testSubSelectUpdate() I would prefer would assert on the result set than on the query string.

@mradcliffe any chance to move forward here ;-)

I've been mainly working in contrib land for the past few months, but I was able to look at this today.

I reworked the existing tests to not do full query string comparisons per @Damien Tournoud, but I quite like the simplicity of the current patch, so I think that should remain.

I added a unit test for PostgreSQL connection with one test method for escape field, which can be expanded on later. This would also be useful as a starter example for MySQL, SQLite, and other database driver unit tests.

I think this unit test needs some work with regard to adding more data to test. I am not as familiar with all the uses for these methods that need to be tested.

Flip back to needs work.

+++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
@@ -0,0 +1,111 @@
+      array('""isDefaultRevision""', '"isDefaultRevision"')

As an example, I'm not sure if this is what we should be expecting. That doesn't /feel/ correct.

Here's another try. I expanded the unit tests to capture what I think is an acceptable use case of potential strings to escape beyond the base escape methods. I also no longer escape every field by default and am only focusing on case-sensitivity so that it matches the Drupal functionality provided by the base Connection class.

I need to run this through postgresql test bot, but the phpunit tests should pass on the pifr test bot.

@mradcliffe did you run the patch on PG already? If not, I can do a bot run later. Currently bot is busy with testing my patches...

Not yet, @bzrudi71.

Okay, just verified that all tests in patch pass PG bot!

Awesome patch! just a few nits

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -186,6 +186,69 @@ public function queryTemporary($query, array $args = array(), array $options = a
   +   * Escapes a field name string.
   ...
   +   * @return string
   ...
   +  public function escapeField($field) {
   

   Should use {@inheritdoc} with extended comment.
   Also better to fix parent doc-block, maybe separate issue

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -248,7 +248,8 @@ protected function createTableSql($name, $table) {
   -    $sql = $name . ' ' . $spec['pgsql_type'];
   +    // PostgreSQL converts names into lowercase, unless quoted.
   +    $sql = '"' . $name . '" ' . $spec['pgsql_type'];
   

   comment is not clear, "postgresql" means server or pdo or drupal driver

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php
   @@ -112,6 +112,30 @@ public function orderBy($field, $direction = 'ASC') {
   +    $count = 2;
   +    while (!empty($this->expressions[$alias_candidate])) {
   +      $alias_candidate = $alias . '_' . $count++;
   

   why start from 2? please add inline comment

4. +++ b/core/modules/system/src/Tests/Database/SelectTest.php
   @@ -39,10 +39,10 @@ function testSimpleComment() {
   -    $this->assertEqual($query, $expected, 'The flattened query contains the comment string.');
   +    $this->assertEqual(substr($query, 0, strlen($expected)), $expected, 'The flattened query contains the comment string.');
   
   @@ -57,10 +57,10 @@ function testVulnerableComment() {
   -    $this->assertEqual($query, $expected, 'The flattened query contains the sanitised comment string.');
   +    $this->assertEqual(substr($query, 0, strlen($expected)), $expected, 'The flattened query contains the sanitised comment string.');
   

   contains is assertNotEqual(FALSE, strpos($query, $expocted), ...contains

5. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -0,0 +1,124 @@
   +  }
   +
   +
   +  /**
   +   * Data provider for testEscapeAlias.
   

   extra line--

Thanks for the review.

#1. I re-read the @inheritdoc documentation, and it has to be stand-alone. I rewrote the comments inline instead. I removed the bit about case-sensitivity because we're just supporting what Drupal already expects.

#2. Clarified comment

#3. I looked back to the interdiff that had this change, and this comes from the core driver in multiple places, but without any comment about why it begins with 2. Probably need to go git blame Drupal 7.x code base for the commit. I added an inline comment in the driver regarding the code is from the abstract class.

#4. Fixed test per comment above.

#5. Removed line

I still need to ping @Crell regarding the new unit test.

Fixed tests

Any reason why we hold this one back? Patch looks good and seems RTBC from my side. Is it because of:

I still need to ping @Crell regarding the new unit test.

If so, can't we file a follow up for the unit tests and get this one in?

Yes, that's the reason. I think the coverage is sufficient, but I still have a second-guess fear that I'm missing something security-wise.

What do you think?

@mradcliffe: Can't find anything obvious wrong with the patch, but if you have concerns let's better hold it back until some more feedback.

A few comments on the patch:

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php
   @@ -112,6 +112,31 @@ public function orderBy($field, $direction = 'ASC') {
   +  public function addExpression($expression, $alias = NULL, $arguments = array()) {
   

   Not sure what this new function has to do with capital letters in pgsql identifiers -- can you please explain?

2. +++ b/core/modules/system/src/Tests/Database/SelectTest.php
   @@ -39,10 +39,10 @@ function testSimpleComment() {
   -    $expected = "/* Testing query comments */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
   

   I don't understand this change -- it seems unrelated to the purpose of this patch.

3. +++ b/core/modules/system/src/Tests/Database/SelectTest.php
   @@ -57,10 +57,10 @@ function testVulnerableComment() {
   -    $expected = "/* Testing query comments SELECT nid FROM {node}; -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
   

   Same thing here... what does this have to do with capital letters in pgsql identifiers?

To address the concerns that @mradcliffe expressed about security -- I specifically looked for security concerns in my review, and didn't see anything that jumped out at me. That being said, I think it's good to get as many eyes on this patch as possible before settings it as RTBC.

New patch (#1013034: PostgreSQL constraints do not get renamed by db_rename_table()).

Another review on the latest patch:

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -186,6 +186,64 @@ public function queryTemporary($query, array $args = array(), array $options = a
   +    // Escape any invalid start character.
   

   This comment says that it's escaping any invalid start character, but it's really just removing any invalid characters. Please be precise...

2. +++ b/core/modules/system/src/Tests/Database/SelectTest.php
   @@ -39,10 +39,10 @@ function testSimpleComment() {
   -    $expected = "/* Testing query comments */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
   +    $expected = "/* Testing query comments */";
    
   

   I still don't understand why this test is being changed. It seems unrelated to the purpose of this patch.

3. +++ b/core/modules/system/src/Tests/Database/SelectTest.php
   @@ -57,10 +57,10 @@ function testVulnerableComment() {
   -    $expected = "/* Testing query comments SELECT nid FROM {node}; -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test";
   +    $expected = "/* Testing query comments SELECT nid FROM {node}; -- */";
    
   

   Same thing here...

4. +++ b/core/modules/system/src/Tests/Database/UpdateComplexTest.php
   @@ -139,15 +139,12 @@ function testSubSelectUpdate() {
   -    // Save the query for a __toString test later.
   -    $string_test = $query;
   -    $query->execute();
   +    // Save the number of rows that updated for assertion later.
   +    $num_updated = $query->execute();
   

   This change also seems unrelated to this issue.

Fixed #78.1 comment, suppose RTBC
Can someone test and report the effect of the patch on testing bot?

Answer to #78 2-3-4 in #55

tests really should not do any string comparison. Let's try to change them so that we still assert for the right thing without doing a full string comparison: the two comment tests should really just look for the comment, not compare the whole query string. testSubSelectUpdate() I would prefer would assert on the result set than on the query string.

PG bot seems happy with latest patch, no side effects just some more fixed tests. +1 for RTBC

The patch looks good to me. Good work!

Some PHPUnit test nitpicks:

1. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -0,0 +1,123 @@
   + * Tests the Connection class for PostgreSQL methods.
   

   Can we replace this by @coversDefaultClass \Drupal\Core\Database\Driver\pgsql\Connection

2. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -0,0 +1,123 @@
   +   * Assert that the escapeTable method returns a valid table name that is
   +   * quoted.
   

   Can we replace this by @covers ::escapeTable

3. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -0,0 +1,123 @@
   +   * Assert that the escapeAlias method returns a valid alias name that is
   +   * quoted.
   

   Can we replace this by @covers ::escapeAlias and can we rename the function to testEscapeAlias.

4. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
   @@ -0,0 +1,123 @@
   +   * Assert that the escapeField method returns a valid field value that is
   +   * quoted.
   

   Can we replace this by @covers ::escapeField

Fixed my PHPUnit test nitpicks from comment #81 myself.

This is necessary for postgres to pass tests, so bumping to critical per #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release.

With a hand full of reviews and all nitpicks fixed it seems that this is RTBC now, thanks!

SQLite is not affected by the test changes. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 60f36e8 and pushed to 8.0.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
index cf0ca19..57e38d0 100644
--- a/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
+++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
@@ -21,11 +21,7 @@ class PostgresqlConnectionTest extends UnitTestCase {
    */
   protected function setUp() {
     parent::setUp();
-
     $this->mock_pdo = $this->getMock('Drupal\Tests\Core\Database\Stub\StubPDO');
-    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
-      ->disableOriginalConstructor()
-      ->getMock();
   }
 
   /**

Remove unused mock.