Problem/Motivation

Improve pgsql driver performance with optimizations from #1079762: DatabaseSchema_pgsql::queryTableInformation() is slow. See the parent issue for more details.

Proposed resolution

Bring in the improvements and changes from Drupal 8 which include:

  • Temporary table fix
  • Change from information_schema to custom query

Remaining tasks

  • Write patch - DONE
  • Review patch

User interface changes

None

API changes

A protected method is added, but this is backwards-compatible with the driver class.

Data model changes

None.

CommentFileSizeAuthor
#7 interdiff_1-7.txt872 bytespoker10
#7 2823488-7.patch3.75 KBpoker10
1079762-40.patch3.83 KBmradcliffe

Comments

mradcliffe created an issue. See original summary.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I added a test run for PostgreSQL, but if I recall correctly the tests don't all pass in Drupal 7 anyway with a CI error.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: +#1031122: postgres changeField() is unable to convert to bytea column type correctly

@mradcliffe, excellent initiative!

Just FYI, here's the related issue for resolving the PostgreSQL CI Error (unrelated to your patch)
#1031122: postgres changeField() is unable to convert to bytea column type correctly

poker10’s picture

poker10 commented

D7/PostgreSQL testbot should be fixed now and this patch still passes the tests. I have added this to the list of potential candidates to the 06/2022 D7 release. Hopefully it will get in.

mcdruid’s picture

mcdruid commented
Status: Needs review » Needs work 
+++ b/includes/database/pgsql/schema.inc
@@ -13,6 +13,13 @@
+  protected $temNamespaceName;

Typo?

Should be $tempNamespaceName.

mcdruid’s picture

mcdruid commented 
+++ b/includes/database/pgsql/schema.inc
@@ -39,23 +46,47 @@ class DatabaseSchema_pgsql extends DatabaseSchema {
+    // ::generateTemporaryTableName() sets 'db_temporary_' prefix to all
...
       $key = 'public.' . $key;

Bit of a nit but ::generateTemporaryTableName() suggests this is a method within the same class, but it's not.

\DatabaseConnection::generateTemporaryTableName would be the complete reference AFAICS.

poker10’s picture

poker10 commented
Status: Needs work » Needs review
StatusFileSize
new 2823488-7.patch3.75 KB
new interdiff_1-7.txt872 bytes

Yes, it seems like a typo. Attaching updated patch (also with updated comment as mentioned in #6).

mcdruid’s picture

mcdruid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Thanks, I've compared the patch with what's in D9 today:

https://git.drupalcode.org/project/drupal/-/blob/9.4.0-alpha1/core/modul...

...and I think this looks good now.

fabianx’s picture

fabianx commented

RTBC + 1, approved for Merge! Thanks all!

fabianx’s picture

fabianx commented
Assigned: Unassigned » mcdruid

  • mcdruid committed 0a7ef86 on 7.x 
    Issue #2823488 by poker10, mradcliffe, mcdruid: Backport...
mcdruid’s picture

mcdruid commented
Assigned: mcdruid » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks everybody!

Status: Fixed » Closed (fixed)

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