DatabaseSchema_pgsql::queryTableInformation() is slow

Moving to PostgreSQL driver, I'm not convinced this is a major bug in Drupal, MySQL used to have very slow information_schema performance which was fixed in more recent releases, so optimizing it in Drupal itself somehow comes under 'normal task' for me.

Also it needs to go into 8.x first then be backported.

Closed #2451723: Database::queryTableInformation optimization for PostgreSQL as a dup and added information here.

Database::queryTableInformation can be optimize, here is an example of the SQL generated by Database::queryTableInformation:

LOG:  statement: SELECT column_name, data_type, column_default FROM information_schema.columns WHERE table_schema = 'public' AND table_name = 'variable' AND (data_type = 'bytea' OR (numeric_precision IS NOT NULL AND column_default::text LIKE '%nextval%'));
LOG:  duration: 14.278 ms

compared with an equivalent (I think) query against PostgreSQL system catalog tables:

LOG:  statement: SELECT pg_attribute.attname, pg_type.typname, pg_attrdef.adsrc
      FROM pg_namespace
      JOIN pg_class ON pg_namespace.oid = pg_class.relnamespace
      JOIN pg_attribute ON pg_attribute.attrelid = pg_class.oid
      JOIN pg_type ON pg_type.oid = pg_attribute.atttypid
      LEFT JOIN pg_attrdef ON pg_attrdef.oid   = pg_attribute.attrelid
                          AND pg_attrdef.adnum = pg_attribute.attnum
    WHERE pg_namespace.nspname = 'public'
      AND pg_class.relname = 'variable'
      AND pg_class.relkind IN ('r', 'v')
      AND (pg_type.typname = 'bytea' OR (pg_attribute.atttypmod <> -1 AND pg_attrdef.adsrc LIKE '%nextval'));
LOG:  duration: 2.249 ms

The problem is originally reported by nathanweeks.

Even so the results in #3 seems a bit outdated, because both queries finish within <1ms on a recent machine, the query against PostgreSQL system catalog tables is still around 8x faster than the current implementation. So seems worth to think about as we call queryTableInformation() very often!

Unfortunately the query against the system catalogue doesn't return anything ;)

While this works:

d8=# SELECT column_name, data_type, column_default FROM information_schema.columns WHERE table_schema = 'public' AND table_name = 'node' AND (data_type = 'bytea' OR (numeric_precision IS NOT NULL AND column_default::text LIKE 'nextval%'));
 column_name | data_type |          column_default           
-------------+-----------+-----------------------------------
 nid         | integer   | nextval('node_nid_seq'::regclass)
(1 row)

The catalogue query doesn't work:

d8=# SELECT pg_attribute.attname, pg_type.typname, pg_attrdef.adsrc
d8-#       FROM pg_namespace
d8-#       JOIN pg_class ON pg_namespace.oid = pg_class.relnamespace
d8-#       JOIN pg_attribute ON pg_attribute.attrelid = pg_class.oid
d8-#       JOIN pg_type ON pg_type.oid = pg_attribute.atttypid
d8-#       LEFT JOIN pg_attrdef ON pg_attrdef.oid   = pg_attribute.attrelid
d8-#                           AND pg_attrdef.adnum = pg_attribute.attnum
d8-#     WHERE pg_namespace.nspname = 'public'
d8-#       AND pg_class.relname = 'node'
d8-#       AND pg_class.relkind IN ('r', 'v')
d8-#       AND (pg_type.typname = 'bytea' OR (pg_attribute.atttypmod <> -1 AND pg_attrdef.adsrc LIKE 'nextval%'));
 attname | typname | adsrc 
---------+---------+-------
(0 rows)

Guess we need to have a second look on this...
BTW Doesn't even work with '%nextval%' and the check for views in AND pg_class.relkind IN ('r', 'v') seems obsolete.

Tried a handful of alternative queries but always failed to gather *all* required information from the catalogues ;)
References:
pg-type
pg-class
pg-attribute
pg-attrdef

Well, at least I got an working alternative here that gives some results. However, sadly this is only 3-4 times faster than the information schema query ;)
Any ideas to further speed up? Guess we should make a patch to actually see what happens :)

d8=# SELECT pg_attribute.attname AS column_name, format_type(pg_attribute.atttypid, pg_attribute.atttypmod) AS data_type ,pg_attrdef.adsrc AS column_default
FROM  pg_attribute                                                            
LEFT JOIN pg_type ON pg_type.typrelid = pg_attribute.atttypid
LEFT JOIN pg_attrdef ON pg_attrdef.adrelid = pg_attribute.attrelid  AND pg_attrdef.adnum = pg_attribute.attnum
WHERE  pg_attribute.attnum > 0
AND NOT pg_attribute.attisdropped
AND pg_attribute.attrelid = 'public.node'::regclass 
AND (pg_type.typname = 'bytea' OR pg_attrdef.adsrc LIKE '%nextval%');
 column_name | data_type |          column_default           
-------------+-----------+-----------------------------------
 nid         | integer   | nextval('node_nid_seq'::regclass)
(1 row)

Putting @bzrudi71's SQL into a patch and see what happens. I thought about if it would be possible to split into 2 distinct queries (bytea columns and sequence) to maybe improve the performance based on #9.

Maybe add an actual patch.

@mradcliffe: You should not remove the quotes around the .:table variable.

@daffie: I don't think I am removing the quotes unless you mean from @bzrudi71's query. The placeholder should add the quotes from the bound parameter, right? If I added quotes around the placeholder it doesn't work.

In any case, typrelid is going to be 0 for any non-composite data types and thus the query is not returning any bytea columns. Maybe exclude pg_type table join? This seems to work locally.

@mradcliffe: If you take a look in the database the value is something like 'public.node'::regclass. And not public.node::regclass.

But when I do this

            $sql .= "AND pg_attribute.attrelid = ':table'::regclass ";

or adding quotes in the parameters array it gives the following error:

2016-10-01 14:30:00 UTC [26976-5] drupal8@drupal8 ERROR:  schema "'public" does not exist at character 374
2016-10-01 14:30:00 UTC [26976-6] drupal8@drupal8 STATEMENT:  SELECT pg_attribute.attname AS column_name, format_type(pg_attribute.atttypid, pg_attribute.atttypmod) AS data_type ,pg_attrdef.adsrc AS column_default FROM pg_attribute LEFT JOIN pg_attrdef ON pg_attrdef.adrelid = pg_attribute.attrelid  AND pg_attrdef.adnum = pg_attribute.attnum WHERE pg_attribute.attnum > 0 AND NOT pg_attribute.attisdropped AND pg_attribute.attrelid = '''public.cache_config'''::regclass AND (format_type(pg_attribute.atttypid, pg_attribute.atttypmod) = 'bytea' OR pg_attrdef.adsrc::text LIKE '%nextval%')

Flipping back to Needs Work.

pg_attribute is not updated yet? Maybe there is a commit/transaction issue? Or a caching issue?

pgattrdef.adsrc is a historical record so won't be updated when the default value for a column is changed. The documentation suggests as such https://www.postgresql.org/docs/9.1/static/catalog-pg-attrdef.html.

Here's a patch that explains the query and makes that change.

Let's write an issue summary too.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -123,11 +123,25 @@ public function queryTableInformation($table) {
+          $sql .= "LEFT JOIN pg_attrdef ON pg_attrdef.adrelid = pg_attribute.attrelid  AND pg_attrdef.adnum = pg_attribute.attnum ";

Extra space here: pg_attribute.attrelid AND pg_attrdef.adnum

The select statement looks great. Good find that you could remove the join to the pg_type table. I have a few small points:

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -123,11 +123,25 @@ public function queryTableInformation($table) {
                ':default' => '%nextval%',
   

   Can we add the constant value direct in the statement.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -123,11 +123,25 @@ public function queryTableInformation($table) {
   +            ':table' => $key,
   

   Can we rename :table to :key. With key being the combined schema.table variable.

+1 for consistency in variables and placeholders.

I think we still need to keep the LIKE, but can search from the end. It's possible a sequence next value could be generated from other functions though I honestly doubt the use of wrapping nextval() in any math functions.

Edit: The reason we might not be able to do exact matching on the nextval is that the sequence/key may need to be hashed based on the length of the string.

@mradcliffe: If you think that it is that important to keep "%nextval%", then lets keep it. You have made the change:

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -134,13 +134,13 @@ public function queryTableInformation($table) {
-            ':default' => '%nextval%',
...
+            ':default' => 'nextval%',

Is this change a mistake? Or not?

No, it is not a mistake. I meant to keep that change as all of Drupal's sequence entries begin with nextval( so we do not need to search for nextval in the middle of a string.

The patch looks great now.
The testbot with this patch is 10 minutes faster then without the patch. So the problem of this issue is addressed.
No extra tests are needed for this issue.

It get a RTBC from me.

Great work @mradcliffe.

Nice research @mradcliffe - well done. I did a short review and all seems good to me, including the change for nextval%.

Thanks a ton and RTBC from my side too :)

Wow the performance improvement due to this patch is impressive.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -123,11 +123,25 @@ public function queryTableInformation($table) {
+          $sql .= "OR pg_attrdef.adsrc LIKE :default)";
...
+            ':default' => 'nextval%',

Why do we bother with value replacement when the the value is hardcoded?

Re #28 Good point let's do that... I also testet at least the query itself on PostgreSQL 9.4 and 9.5 just to make sure it works even with latest versions.

I asked for the same change in comment #22. So, for me this is RTBC again.

It is great to see some effort going into making postgres faster by default since there is no reason it should be this much slower then MySQL. If this is a performance sensitive function then we should do more to avoid unnecessary queries. The patch attached does this - which I think is all in scope because we're doing the change for performance reasons.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -123,11 +123,24 @@ public function queryTableInformation($table) {
+          $sql = "SELECT pg_attribute.attname AS column_name, format_type(pg_attribute.atttypid, pg_attribute.atttypmod) AS data_type, pg_get_expr(pg_attrdef.adbin, pg_attribute.attrelid) AS column_default ";
+          $sql .= "FROM pg_attribute ";
+          $sql .= "LEFT JOIN pg_attrdef ON pg_attrdef.adrelid = pg_attribute.attrelid AND pg_attrdef.adnum = pg_attribute.attnum ";
+          $sql .= "WHERE pg_attribute.attnum > 0 ";
+          $sql .= "AND NOT pg_attribute.attisdropped ";
+          $sql .= "AND pg_attribute.attrelid = :key::regclass ";
+          $sql .= "AND (format_type(pg_attribute.atttypid, pg_attribute.atttypmod) = 'bytea' ";
+          $sql .= "OR pg_attrdef.adsrc LIKE 'nextval%')";

Let's use nowdoc syntax - all this concatenation is unnecessary and ending with the empty space a bit ugly.

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -149,6 +141,13 @@ public function queryTableInformation($table) {
   +      if (empty($result)) {
   +        return $table_information;
   +      }
   

   Interested to see how this will work out on the tests.

   Nice find.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -167,6 +166,19 @@ public function queryTableInformation($table) {
   +    if (!isset($this->tempNamespaceName)) {
   +      $this->tempNamespaceName = $this->connection->query('SELECT nspname FROM pg_namespace WHERE oid = pg_my_temp_schema()')->fetchField();
   +    }
   

   Nice. I think this should work out and should clean up disk usage since we are probably creating lots of temporary schemas each connection.

1 minute off the 5.5 run, and 5 minutes off of the 7.x run. That's fairly significant. That probably means that the PHP changes in the driver are more optimized with the patch in #31 and the temporary schema change in #31 is consistent.

RTBC for me. I'll let someone else confirm and change it to get more eyes on the patch.

Updated issue summary.

Wow! Another 5 min speed improvement for the PHP7 testbot, impressive :) I don't find anything wrong with the patch from alexpott and it's in scope of this issue, so let's get it in - RTBC.

Thanks @alexpott for the nice patch improvements!

+1 for RTBC. Thanks @alexpott for all your help!

Committed/pushed to 8.3.x, thanks!

Here's a back port patch for Drupal 7.

It looks like everything is supported for those old versions of PostgreSQL.

Bad test bot.

@mradcliffe the new backport policy is to open a new issue and related it to this one - see https://www.drupal.org/core/backport-policy

Oh, I didn't realize the policy changed. Thank you, @alexpott.

Follow-up issue created for Drupal 7: #2823488: Backport DatabaseSchema_pgsql::queryTableInformation() improvements.