Database reserved keywords need to be quoted as per the ANSI standard

suggested using [COLUMN_NAME] to match {TABLE_NAME}

This doesn't seem like the worst suggestion given all the other options.

Not really keen on completely deprecating \Drupal\Core\Database\Connection::query(), the query builder already supports where/expression for gnarly syntax stuff and that can include columns just as much (say a REPLACE(foo, 'bar', 'baz') query in MySQL).

Quick review. Generally this looks good and the amount of crap it removes from the postgresql driver shows what we'd be dealing with if we went the dictionary route.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -980,10 +1047,12 @@ public function escapeTable($table) {
   +      $parts = array_map([$this, 'escapeAlias'], $parts);
   

   Why is it using escapeAlias here when the method is escapeField()? If escapeAlias() is doing two different things (actually escaping aliases, and also escaping fields) then it should probably be renamed.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -48,28 +48,6 @@ class Connection extends DatabaseConnection {
   -  protected $postgresqlReservedKeyWords = ['all', 'analyse', 'analyze', 'and',
   

   Nice.

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
   @@ -24,6 +24,11 @@ public function execute() {
        $insert_fields = array_merge($this->defaultFields, $this->insertFields);
   +    // The fields are escaped and quoted when adding to the insert query so this
   +    // is not necessary and actually harmful.
   +    // $insert_fields = array_map(function ($field) {
   +    //   return $this->connection->escapeField($field);
   +    // }, $insert_fields);
    
   

   cruft.

Some review comments:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -143,10 +143,33 @@
   +  /**
   +   * List of escaped table names, keyed by unescaped names.
   +   *
   ...
   +   */
   +  protected $escapedFields = ["" => ""];
   

   This should be "List of escaped field names, keyed..."

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -254,6 +277,11 @@ public function destroy() {
   +   * - allow_square_brackets: By default, queries which contain square brackets
   +   *   will have them replaced with the identifier quote character for the
   +   *   database type. In rare cases, such as creating an SQL function, []
   +   *   characters might be needed and can be allowed by changing this option to
   +   *   TRUE.
       *
       * @return array
       *   An array of default query options.
   

   The default value (FALSE) isn't returned in the defaultOptions() method.
   Since allow_delimiter_in_query also has a default value of FALSE and is returned by the method, this default value also should be add.

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -393,12 +462,21 @@ public function getFullQualifiedTableName($table) {
   +   * @param array $options
   +   *   An associative array of options to control how the query is run. The
   +   *   given options will be merged with self::defaultOptions(). See the
   +   *   documentation for self::defaultOptions() for details.
   +   *   Typically, $options['return'] will be set by a default or by a query
   +   *   builder, and should not be set by a user.
   ...
       */
   -  public function prepareQuery($query) {
   

   Documentation is incorrect: The $options array isn't merged with self::defaultOptions() in this method. I suggest adding $options += $this->defaultOptions();

Is this a duplicate of #1426084: Provide backtick escaping for MySQL in DB abstraction layer?

Thanks for the crediting. I did part of the patching on the backtick escaping for MySQL layer. (#1426084)

Does the patch in this issue also take into account that the prefix replacement patterns also incorrectly replace curly brackets in data, for example if a query is run with JSON data in it's value. In the #1426084 latest patch we added more complex regex's to tackle that problem (it's commented in the code what the regex does).

Ok good to know than. I also saw this patch also doesn't allow a dash (-) in the table and column names, which is allowed in MySQL as long as your names are quoted (backticked). Would you consider that also a followup issue, or something that needs to be added in this patch?

That's good to know, but the thing is, the escapeTable method in the base Connection.php doesn't take the dash into account when escaping the table names, so the - will be filtered out if not overridden. To allow it in the MySQL connection, the extended MySQL Connection should include the '-' then

public function escapeTable($table) {
    if (!isset($this->escapedNames[$table])) {
      $this->escapedNames[$table] = preg_replace('/[^A-Za-z0-9_.-]+/', '', $table);
    }
    return $this->escapedNames[$table];
  }

(untested)

Just nitpicks, nothing functional. Not sure what is holding this issue back.

1. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -256,7 +256,9 @@ protected function getTableIndexes(Connection $connection, $table, &$definition)
   +    // Note that table names are now always quoted.
   

   The now here is good for this patch (because it makes it clear that this is a change that happened here), but I don't think it should end up in core.

   Something like this seems better I think?

   We alway quote table names to make sure that we allow for tables that are reserved keywords.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -306,14 +336,19 @@ protected function setPrefix($prefix) {
   +        // $val can point to another database like 'database.users'. In this
   +        // instance we need to quote the identifiers correctly.
   ...
   +    // $this->prefixes['default'] can point to another database like
   +    // 'prefix.'. In this instance we need to quote the identifiers correctly.
   

   We use another example of another database in these comments, should we align those?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -323,6 +358,20 @@ protected function setPrefix($prefix) {
   +    @trigger_error('In Drupal 9.0 this method will be abstract and contrib and custom drivers will have to implement. See https://www.drupal.org/node/2986894', E_USER_DEPRECATED);
   

   will have to implement it?

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -341,6 +390,27 @@ public function prefixTables($sql) {
   +   *   A string containing a partial or entire SQL query.
   ...
   +   *   The string containing a partial or entire SQL query with all identifiers
   

   Does this work for non-query statements as well? It seems like it does, so we should probably update the language here as well?

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -488,7 +565,11 @@ public function getLogger() {
   +    // @todo oh dear this is not pretty. Also I'm not sure what would happen
   +    //   here if the prefix replacement resulted in a table name with dot in it.
   +    //   I think this might be a problem in HEAD for Postgres and table
   +    //   prefixing that results using different databases.
   

   Should we figure out the postgres thing for HEAD in this issue as well? Or should we postpone this on that? Or can that be a followup?

   In any case, this comment isn't really helpful.

While I understand the intention, {} already makes it incredibly annoying to take queries in and out of Drupal to build and debug them. Adding invalid syntax to support a processed escaping of columns breaks syntax linters and development tools like PhpStorm. [] compounds that, do we _really_ _absolutely_ _positively_ have to degrade site builder and developer experience here and re-invent the wheel? Like this is really bad. :(

Tested and verified patch #63 with our project and seems to work.

For our particular situation to work I also added a followup issue for #50 (dash issue) and #60 (prefix in json data) based on the latest patch (#63) of this issue and provided a patch for it at https://www.drupal.org/project/drupal/issues/3011987

The new reserved keyword 'groups' in MySQL 8.0.2 completely breaks the Group module which uses said keyword as its main table name (because it never was an issue until mySQL 8.0.2). So as far as I'm concerned, this issue is quite critical :o

The test fail seems unrelated to the issue at hand (order of array values).

While I can't speak for the correctness of the patch as a whole - I've not worked with the inner layers of the database API - I can say that the code additions, removals and changes in the patch seem to make sense. They are well documented, the tests seem to reflect what has changed nicely and I simply love the removal of all the reserved keywords in favor of simply specifying an identifier quote.

+1 from me as far as code quality and sanity goes. I'll leave the RTBC up to someone who knows the ins and outs of the database layer more than I do.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -143,10 +143,33 @@
+   * @deprecated in Drupal 8.7.x, to be removed before Drupal 9.0.0. This is no
+   *   longer used. Use \Drupal\Core\Database\Connection::$escapedTables or
+   *   \Drupal\Core\Database\Connection::$escapedFields instead.

should we remove this property and add a __get to trigger a deprecated error?

From slack:

Is there a chance that a sub-class could end up with unescaped stuff if they rely on the old property?
In which case, we could hard throw an exception from __get, which would also prevent leakage

yep, fair enough

A whole bunch of issues about escaping or quoting database identifiers have been closed as duplicates, but either they were closed incorrectly, or the patch here doesn't go far enough.

Migrations into a D8 database that has hyphens in the DB name (e.g. 'drupal-8') fails because of #2845333: can't JOIN to a foreign database whose name contains a hyphen. The patch there fixes the problem, but the patch here does not.

Ok, the patch on this issue is currently definitely NOT handling #2767595: Support "-" in database table names, because dumping the contents of $this->escapedTables in Connection::escapeTable() is showing me this:

> "drupal-8.migrate_map_client" => "drupal8.migrate_map_client"

Where's my hyphen??

Ok so the problem is partly with DB API and partly with Migrate.

Migrate's SqlBase does this:

        $alias = $this->query->leftJoin($this->migration->getIdMap()
          ->getQualifiedMapTableName(), 'map', $map_join);

which boils down to something like this:

        $alias = $this->query->leftJoin('drupal-8.migrate_map_foo', 'map', $map_join);

Note how we are joining BACK to the Drupal 8 site's database, in a query that is being run on a different connection.

Several problems here:

1. If the local Drupal 8 site uses a database table prefix, then the query will be broken, because the {} don't get added to the 'drupal-8.migrate_map_foo' identifier. Presumably, that is because DB API doesn't touch it because it supposes it's an external database. And normally it would be right! So I think DB API needs to have a way of joining to a 2nd database by specifying its connection rather than giving its actual database name. (But that's a whole other issue.)

2. It looks like this patch is breaking Migrate in several ways.

- 2a. When migrate's map query joins to the foreign database, it needs to wrap the 'database.table' in '[]', if I am correctly reading the docs in this patch?

- 2b. With this patch applied, the migrate map query looks like this, which isn't valid SQL:

SELECT "n"."nid" AS "nid", "n"."type" AS "type", "n"."language" AS "language", "n"."status" AS "status", "n"."created" AS "created", "n"."changed" AS "changed", "n"."comment" AS "comment", "n"."promote" AS "promote", "n"."sticky" AS "sticky", "n"."tnid" AS "tnid", "n"."translate" AS "translate", "nr"."vid" AS "vid", "nr"."title" AS "title", "nr"."log" AS "log", "nr"."timestamp" AS "timestamp",....

@alexpott and @joachim if you want hyphens in a table name there is a patch for that in:
https://www.drupal.org/project/drupal/issues/3011987

> @joachim is there any chance you can create a test case that replicates #81.2 - because if this does break migrate map joining then we definitely are missing test coverage.

It looks like there is test coverage of the query that joins to a map table, for example in /core/modules/migrate/tests/src/Kernel/HighWaterTest.php. A dump of $query->__toString() in SqlBase::initializeIterator() gets me:

SELECT m.id AS id, m.title AS title, m.changed AS changed, map.sourceid1 AS migrate_map_sourceid1, map.source_row_status AS migrate_map_source_row_status
FROM
{high_water_node} m
LEFT OUTER JOIN drupalaccounts8.test55734627migrate_map_high_water_test map ON id = map.sourceid1
WHERE (map.sourceid1 IS NULL) OR (map.source_row_status = :db_condition_placeholder_0) OR (changed > :db_condition_placeholder_1)
GROUP BY id, title, changed, map.sourceid1, map.source_row_status
ORDER BY changed ASC; Array
(
    [:db_condition_placeholder_0] => 1
    [:db_condition_placeholder_1] => 3

With the patch from #82 applied, that test fails for me with:

Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupalaccounts8.test77520843migrate_map_high_water_test' doesn't exist: SELECT "m"."id" AS "id", "m"."title" AS "title", "m"."changed" AS "changed", "map"."sourceid1" AS "migrate_map_sourceid1", "map"."source_row_status" AS "migrate_map_source_row_status"
FROM
{high_water_node} "m"
LEFT OUTER JOIN drupalaccounts8.test77520843migrate_map_high_water_test "map" ON id = map.sourceid1
WHERE ("map"."sourceid1" IS NULL) OR ("map"."source_row_status" = :db_condition_placeholder_0) OR ("changed" > :db_condition_placeholder_1)
GROUP BY id, title, changed, map.sourceid1, map.source_row_status
ORDER BY "changed" ASC; Array
(
    [:db_condition_placeholder_0] => 1
    [:db_condition_placeholder_1] => 3
)

but then it's also failing for me locally WITHOUT the patch. Not sure why!

(Incidentally, and off-topic, the tests for migrate look like they could do with some work, as MigrateTestBase for instance has a TODO referencing an issue now closed, and many references to simpletest despite being a phpunit test class.)

queued for all databases

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -64,277 +64,6 @@ class Connection extends DatabaseConnection {
-  private $reservedKeyWords = [

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -48,28 +48,6 @@ class Connection extends DatabaseConnection {
-  protected $postgresqlReservedKeyWords = ['all', 'analyse', 'analyze', 'and',

reserved words are removed, so it makes useless #3020839: Drupal\Core\Database\Driver\mysql\Connection::quoteIdentifier in_array performance improvements

Here's reroll

Fix deprecated "getMock" to pass tests

Fix CS

Fix CS for messages, the second one still needs proper wording

Patch no longer applies against 8.9.x

@alexpott: I think that your 95-97-interdiff.txt is the same as your patch file.

The views module uses a lot of $query->where(), expressions, and joins with sql strings as input. Only in the patch there is nothing about the views module? If you create a view with preserved word as a table name or column the query will not quote the table names and column names.

Maybe query() method (or kind of it) of database layer could use assert to test it properly for the most of queries?

My review of the change record:

1. All the join methods and Connection::queryRange() should be added to the list. db_query() can be removed.
2. With the given examples can we get how it is now and what it should become.
3. Can we add text about what happens when you do not add those square brackets to your code.

My review for the patch:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -341,6 +390,27 @@ public function prefixTables($sql) {
   +  public function quoteIdentifiers($sql) {
   +    return str_replace(['[', ']'], $this->identifierQuote(), $sql);
   +  }
   

   There is no testing for this new method.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -306,14 +336,19 @@ protected function setPrefix($prefix) {
   +    // 'prefix.'. In this instance we need to quote the identifiers correctly.
   

   What is exactly wrong with 'prefix.'? Is it the name prefix or is it the dot or both?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -323,6 +358,20 @@ protected function setPrefix($prefix) {
   +  protected function identifierQuote() {
   +    @trigger_error('In drupal:9.0.0 this method will be abstract and contrib and custom drivers will have to implement it. See https://www.drupal.org/node/2986894', E_USER_DEPRECATED);
   +    return '';
   +  }
   

   Why do we not just create the method and let contrib database driver override this method when they need to? Now the impression is that the default value is an empty string.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -265,6 +293,7 @@ protected function defaultOptions() {
   +      'allow_square_brackets' => FALSE,
   

   I would like to see a lot more testing for this option being TRUE. At least database tests and tests for situations where one might use this option being TRUE.

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -341,6 +390,27 @@ public function prefixTables($sql) {
   +    return str_replace(['[', ']'], $this->identifierQuote(), $sql);
   

   Can we add a comment what this code actually does.

6. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -387,18 +457,25 @@ public function getFullQualifiedTableName($table) {
   +   * @param bool $quote_identifiers
   +   *   (optional) Quote any identifiers enclosed in square brackets. Defaults to
   +   *   TRUE.
   ...
   +  public function prepareQuery($query, $quote_identifiers = TRUE) {
   

   Can we add testing for when the $quote_identifiers = FALSE.

7. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -494,7 +571,9 @@ public function getLogger() {
   +    return str_replace($this->identifierQuote(), '', $this->prefixTables('{' . $table . '}_' . $field . '_seq'));
   

   Can we do in 2 lines of code to increase readability.

8. The amount of testing for the method Connection::setPrefix() is very limited. See: Drupal\Tests\Core\Database\ConnectionTest::testPrefixRoundTrip(). Can we do more?

9. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -958,30 +1037,32 @@ public function schema() {
       *   The sanitized database name.
   ...
      public function escapeDatabase($database) {
   ...
   +    return $this->identifierQuote() . $database . $this->identifierQuote();
   

   Now we are not only sanitizing the database name, but also adding quotes to it. This is a public method. This is a BC break!

10. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -998,10 +1079,14 @@ public function escapeTable($table) {
       public function escapeField($field) {
    ...
    +      $this->escapedFields[$field] = $identifier_quote . str_replace('.', $identifier_quote . '.' . $identifier_quote, $escaped) . $identifier_quote;
    

    Now we are not only sanitizing the field name, but also adding quotes to it. This is a public method. This is a BC break!

11. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1022,7 +1107,7 @@ public function escapeAlias($field) {
         if (!isset($this->escapedAliases[$field])) {
    ...
    +    return $this->identifierQuote() . $this->escapedAliases[$field] . $this->identifierQuote();
    

    Now we are not only sanitizing the alias name, but also adding quotes to it. This is a public method. This is a BC break!

12. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -553,7 +553,7 @@ public function renameTable($table, $new_name) {
    -    $old_full_name = $this->connection->prefixTables('{' . $table . '}');
    +    $old_full_name = str_replace('"', '', $this->connection->prefixTables('{' . $table . '}'));
    

    Why this change. Is it because method Connection::prefixTables() has changed. If so, then that would be a BC break!

13. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
    @@ -123,7 +123,6 @@ public function where($snippet, $args = []) {
    -      'operator' => NULL,
    

    Why this change? And why in this patch?

14. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
    @@ -820,7 +820,7 @@ public function __toString() {
    +      $fields[] = $this->connection->escapeField((isset($field['table']) ? $field['table'] . '.' : '') . $field['field']) . ' AS ' . $this->connection->escapeAlias($field['alias']);
    

    Can we change this from a single line to multi line to improve readability.

15. +++ b/core/modules/system/tests/modules/database_test/database_test.install
    @@ -313,20 +313,18 @@ function database_test_schema() {
    -      'offset' => [
    -        'description' => 'A column with preserved name.',
    -        'type' => 'text',
    -      ],
    -      'function' => [
    -        'description' => 'A column with reserved name in MySQL 8.',
    

    This removes the testing with the special column names "offset" and "function". Please do not change this test, just add another test.

16. +++ b/core/modules/workspaces/src/EntityQuery/Tables.php
    @@ -93,7 +93,7 @@ protected function addJoin($type, $table, $join_condition, $langcode, $delta = N
    +      list($base_table, $id_field) = explode('.', str_replace(['[', ']'], '', $condition_parts[1]));
    

    Can we change this from a single line to multi line to improve readability.

17. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
    @@ -297,4 +263,98 @@ public function testFilterComments($expected, $comment) {
    +    $mock_pdo = $this->createMock(StubPDO::class);
    +    $connection = new StubConnection($mock_pdo, [], $identifier_quote);
    

    Why not use a real connection?

18. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
    @@ -297,4 +263,98 @@ public function testFilterComments($expected, $comment) {
    +  public function providerEscapeTables() {
    ...
    +  public function providerEscapeAlias() {
    ...
    +  public function providerEscapeFields() {
    ...
    +  public function testEscapeTable($expected, $name, $identifier_quote = '"') {
    ...
    +  public function testEscapeAlias($expected, $name, $identifier_quote = '"') {
    ...
    +  public function testEscapeField($expected, $name, $identifier_quote = '"') {
    

    Nit: Can we put the provider methods next to their test methods. Improves readability.

19. $saved_value = $this->connection->query('SELECT [update] FROM {select} WHERE id = :id', [':id' => 2])->fetchField();
    Solutions like the above piece of code, with sometimes the square brackets and sometimes the curly brackets, looks like to me as a ugly drupalism.
    Other developers are not going to like this.

What happens to the current code that has not been updated to use the square brackets? Can we add testing for that so we a sure that contrib/custom code does not break!
In the patch are a lot of little changes that can break existing contrib/custom code. I also think they will be very nasty little bugs to debug.
I know that the alternatives like do nothing or deprecate all methods that take raw sql strings as a parameter are not very attractive.
I am not sure what the best solution is.

The method SelectInterface::having() is not mentioned in the CR and properly some work needs to be done on the patch.

+++ b/core/modules/workspaces/src/EntityQuery/Tables.php
+++ b/core/modules/workspaces/src/EntityQuery/Tables.php
@@ -93,7 +93,8 @@ protected function addJoin($type, $table, $join_condition, $langcode, $delta = N

@@ -93,7 +93,8 @@ protected function addJoin($type, $table, $join_condition, $langcode, $delta = N
       // If those two conditions are met, we have to update the join condition
       // to also look for a possible workspace-specific revision using COALESCE.
       $condition_parts = explode(' = ', $join_condition);
-      list($base_table, $id_field) = explode('.', $condition_parts[1]);
+      $condition_parts_1 = str_replace(['[', ']'], '', $condition_parts[1]);
+      list($base_table, $id_field) = explode('.', $condition_parts_1);
 
       if (isset($this->baseTablesEntityType[$base_table])) {
         $entity_type_id = $this->baseTablesEntityType[$base_table];

What would happen if workspaces didn't make this change? Query failure? Error?

Trying to figure out what the impact on contrib would be - not that many contrib modules do anything as evil as workspaces has to.

Thank you @alexpott on working on this issue. The patch looks better. I did some more reviewing:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -144,9 +144,32 @@ abstract class Connection {
   +   * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. This is no
   

   This is going into 9.0 first, it therefor cannot be removed in 9.0.

2. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -264,7 +264,9 @@ protected function getTableIndexes(Connection $connection, $table, &$definition)
   +    $table = trim($connection->prefixTables('{' . $table . '}'), '"');
   

   Why removing only the double qouotes and not others like backticks?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -306,14 +336,19 @@ protected function setPrefix($prefix) {
   +        // $val can point to another database like 'database.users'. In this
   +        // instance we need to quote the identifiers correctly.
   ...
   +    // $this->prefixes['default'] can point to another database like
   +    // 'other_db.'. In this instance we need to quote the identifiers correctly.
   

   And what is then the correct way of quoting those? I know what it is, but can we add an example to the comments.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -323,6 +358,20 @@ protected function setPrefix($prefix) {
   +    @trigger_error('In drupal:9.0.0 this method will be abstract and contrib and custom drivers will have to implement it. See https://www.drupal.org/node/2986894', E_USER_DEPRECATED);
   

   In Drupal 10.0.0 instead of 9.0.0.

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -956,30 +1036,32 @@ public function schema() {
   +    $database = preg_replace('/[^A-Za-z0-9_]+/', '', $database);
   +    return $this->identifierQuote() . $database . $this->identifierQuote();
   

   Escaped tables, fields and aliases are stored in an array for fast retrieval. Should we not do the same for database names?

6. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -996,10 +1078,14 @@ public function escapeTable($table) {
   +      $this->escapedFields[$field] = $identifier_quote . str_replace('.', $identifier_quote . '.' . $identifier_quote, $escaped) . $identifier_quote;
   
   @@ -1020,7 +1106,7 @@ public function escapeAlias($field) {
   +    return $this->identifierQuote() . $this->escapedAliases[$field] . $this->identifierQuote();
   

   Why are with escaped fields storing the value with the identifier quotes and aliases without? Please be consistent.

7. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -550,7 +550,7 @@ public function renameTable($table, $new_name) {
   +    $old_full_name = str_replace('"', '', $this->connection->prefixTables('{' . $table . '}'));
   
   @@ -866,8 +866,10 @@ protected function introspectIndexSchema($table) {
   +    $full_name = str_replace('"', '', $this->connection->prefixTables('{' . $table . '}'));
   

   Can we use the method identifierQuote() instead of a hard coded double quote.

8. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php
   @@ -50,6 +50,13 @@ public function compile($conditionContainer) {
   +        if ($field === 0) {
   +          // Only add a condition if necessary.
   +          if ($condition['value'] != 0) {
   +            $conditionContainer->alwaysFalse();
   +          }
   +          continue;
   +        }
   

   Why is this change added, it looks like it does not belong in this patch.

9. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
   @@ -1252,4 +1252,27 @@ public function testFindTables() {
   +  public function testUpperCaseTableName() {
   

   Can we add a provider method and add different kinds of allowed table names. Mixing it up shall we say. Should we also do the same for other schema stuff like fields and indexes. Or should we create a followup for that.

10. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
    @@ -93,49 +96,12 @@ public function providerTestPrefixTables() {
         $mock_pdo = $this->createMock('Drupal\Tests\Core\Database\Stub\StubPDO');
    

    If we are touching this test then can we change it to StubPDP::class

11. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
    @@ -297,4 +263,98 @@ public function testFilterComments($expected, $comment) {
    +  public function providerEscapeTables() {
    ...
    +  public function providerEscapeAlias() {
    ...
    +  public function providerEscapeFields() {
    

    Can we testing with backticks.

12. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
    index 8325079986..fb0ead458d 100644
    --- a/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
    

    I am missing testing for the method escapeDatabase().

@alexpott said in comment #109.12 the following:

They all should be @internal

Ok. Lets document them like that.

@alexpott: The changes in #118 look good to me.

For 118.9: Mixing it up in the sense of testing with table names that have both lower and upper case characters. We have only testing for lower case table names. Can we add testing for uppercase table names and for table names with both lower and upper case table names.

Also still open #106.13 and #108.

I shall keep reviewing this patch because we need it for #2746541: Migrate D6 and D7 node revision translations to D8.

All my points are addressed. Thank you @alexpott for taking the time to explain everything to me.
The patch looks good.
I am still a bit worried about sites out there that are programmed by people that are still learning Drupal/PHP shall we say. To me the need to fix the problems that are stated in the IS are more important then poorly programmed that might/will fail.
The change record is also in order.
For me the patch is RTBC.
Thanks everybody for working on this issue!

The original issue where this was brought up, is nearly old enough to drink and vote in the UK. https://www.drupal.org/project/drupal/issues/371

As discussed above, there is a risk of disruption to contributed modules with this change, however as core shows, it should only be contrib modules doing complex SQL string manipulation (workspaces is the only module doing this in core, even views isn't touched here), or ones not using the database API properly at all.

Since both of those cases are impossible for us to detect, that makes this a major-only change - although it's technically allowable in the same sense that we might change queries that other modules are trying to alter in minor releases.

+++ b/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php
@@ -150,4 +150,14 @@ public function testNumericExpressionSubstitution() {
 
+  /**
+   * Tests quoting identifiers in queries.
+   */
+  public function testQuotingIdentifiers() {
+    // Use the tabled named an ANSI SQL reserved word with a column that is as
+    // well.
+    $result = $this->connection->query('SELECT [update] FROM {select}')->fetchObject();
+    $this->assertEquals('Update value 1', $result->update);
+  }
+

s/tabled/table/ - fixed on commit.

Committed 85202b4 and pushed to 9.0.x. Thanks!

#371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements is from 2002. Almost the same as 2020. Just 2 numbers reversed. :)
Just be patient. Your issue will be fixed in the end.

Is it going to be backported to 8.9?
CR needs update accordingly

I don't think we should backport this to 8.9, due to the potential disruption to query rewriting.