Problem/Motivation

FieldSqlStorageTest fails currently with PostgreSQL as database backend.

Indexes are quoted because they are used as identifiers, but when an index is used as in a WHERE clause in a query to pg_* table it should not be quoted because it will retain the case sensitivity within the single quotes.

Test Backtrace:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42704]: Undefined object: 7 ERROR: index "drupal_tpicg3wbwpdnvnnhwouxn5_gclwup2aowovvlxjh2ie_idx" does not exist: DROP INDEX drupal_TPIcG3WbwpdNvNNhwOuxN5_gCLwUP2AOWoVvlXjh2IE_idx; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->updateDedicatedTableSchema() (line 1266 of /var/www/drupal8.dev/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).

Drupal\Core\Database\Driver\pgsql\Connection->query('DROP INDEX drupal_TPIcG3WbwpdNvNNhwOuxN5_gCLwUP2AOWoVvlXjh2IE_idx')
Drupal\Core\Database\Driver\pgsql\Schema->dropIndex('entity_test_rev__testfield', 'testfield_value')
Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->updateDedicatedTableSchema(Object, Object)

Proposed resolution

1. Remove quotes when ensureIdentifiersLength() is called on an index name being queried in pg_constraints or pg_index.
2. Clean up quoting of index and constraints by doing this in ensureIdentifiersLength() instead of directly in query method.

Remaining tasks

Write patch.

User interface changes

None.

API changes

None. ensureIdentifiersLength() is an internal driver protected method.

CommentFileSizeAuthor
#5 2443659-5.patch5.98 KBmradcliffe
#5 interdiff-2443659-5.txt1.42 KBmradcliffe
#3 2443659-3.patch5.81 KBdaffie
#3 interdiff-2443659-2-3.txt1.76 KBdaffie
#2 2443659-2.patch5.82 KBdaffie
#2 interdiff-2443659-1-2.txt1.16 KBdaffie
#1 drupal-2443659-escape-constraint-in-ensureidentifierslength-bad-1.patch4.93 KBmradcliffe

Comments

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes
StatusFileSize
new drupal-2443659-escape-constraint-in-ensureidentifierslength-bad-1.patch4.93 KB

I may have been misled by the backtrace. I thought perhaps PostgreSQL returned 0 because the index name was lower case, and so I looked at using escapeTable() inside ensureIdentifiersLength(), and although that cleared the Exception there were several new assertion failures.

I see that this is quoted in the query statement, but not sure if it is relevant.

Here's the work-in-progress patch for this, which may have additional fails in other test groups. In case it helps.

daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new interdiff-2443659-1-2.txt1.16 KB
new 2443659-2.patch5.82 KB

The FieldSqlStorageTest passes with this patch.

daffie’s picture

daffie commented
StatusFileSize
new interdiff-2443659-2-3.txt1.76 KB
new 2443659-3.patch5.81 KB

This patch is a part of the patch from #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest.

bzrudi71’s picture

bzrudi71 commented

Did a patch review and can confirm pass of test. Nothing obvious found, maybe we just need a bit more in terms of comments. Otherwise this seems RTBC to me, leaving this open for mradcliffe for another review.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
StatusFileSize
new interdiff-2443659-5.txt1.42 KB
new 2443659-5.patch5.98 KB

I meant to review this on Saturday, but I got caught up in that other issue all afternoon. 

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -549,6 +549,8 @@ public function fieldSetNoDefault($table, $field) {
+    // Remove leading and trailing quotes.

@@ -562,6 +564,8 @@ public function indexExists($table, $name) {
+    // Remove leading and trailing quotes.

I think this might be good with some clarification.

"Remove leading and trailing quotes because the index name is in a WHERE clause and not used as an identifier."

Other than that +1 for me though I think @bzrudi71's review has the least biased since I authored patches.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes

Updated issue summary.

daffie’s picture

daffie commented

The comment changes are RTBC for me.

bzrudi71’s picture

bzrudi71 commented
Status: Needs review » Reviewed & tested by the community

Looks good to me :-) Thanks all!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed e2d2b24 on 8.0.x 
    Issue #2443659 by daffie, mradcliffe, bzrudi71: PostgreSQL: Fix system\...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.