Remove N+1 due to column name resolution in PG adapter methods

[owst]

Motivation / Background

I spotted in a test (that uses issued-DB-query tracking) that calling indices makes N+1 queries based on the number of indexes on a table. After investigation, it seems that there are a couple of other methods that can be simplified to avoid multiple trips to the DB when resolving column ids to names.

Detail

For a table with N indices, rather than issuing one query to retrieve the indices, and then N queries to resolve the indexed column ids to their names, we can instead issue one query that retrieves the indices along with their column names.

Similarly, we can avoid an N+1 when retrieving column names for multi-column foreign_keys and unique_constraints.

N.b. the sub-queries are quite verbose due to maintaining PG 9.3 compatability. If using >= 9.4 then unnest(...) WITH ORDINALITY would remove the inner sub-queries that generate the array indices used for ordering, saving ~12 lines. 9.4 was first released nearly 10 years ago, and has been unsupported for nearly 5, so perhaps it is time to raise the minimum version of PG required by Rails?

Additional information

With the following test in a file in this repo, we can see the change in behaviour before and after this PR.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "pry-byebug"

  # gem "rails", path: "."
  gem "rails"

  gem "pg"
end

require "active_record/railtie"
require "minitest/autorun"

class TestApp < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f
  config.eager_load = false
  config.logger = Logger.new($stdout)
end
Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :things, force: :cascade do |t|
    t.integer :a
    t.integer :b
    t.integer :c, index: :unique
    t.integer :d, index: :unique
    t.integer :e
    t.integer :f
    t.integer :g

    t.index '(a*2), b', name: "index_things_on_double_a_and_b"

    t.index %i[a b], unique: true
  end

  execute("ALTER TABLE things ADD CONSTRAINT unique_ef UNIQUE (e, f);")
  execute("ALTER TABLE things ADD CONSTRAINT unique_g UNIQUE (g);")

  create_table :other_things, force: :cascade do |t|
    t.references :things, foreign_key: true
  end

  create_table :products, primary_key: [:store_id, :sku], force: :cascade do |t|
    t.integer :store_id
    t.string :sku
    t.text :description
  end

  create_table :product_orders, force: true do |t|
    t.integer :product_store_id
    t.string :product_sku
  end

  add_foreign_key :product_orders, :products, primary_key: [:store_id, :sku]
end

class Thing < ActiveRecord::Base
end

class Product < ActiveRecord::Base
end

class ProductOrder < ActiveRecord::Base
end

module LogQuery
  def raw_execute(sql, ...)
    puts ">>>>>> raw_execute:\n#{sql}"
    super
  end
end

ActiveRecord::ConnectionAdapters::DatabaseStatements.prepend(LogQuery)


class BugTest < ActiveSupport::TestCase
  def test_association_stuff
    conn = ActiveRecord::Base.connection
    assert_equal(
      [["a", "b"], ["c"], ["d"], "((a * 2)), b", ["e", "f"], ["g"]],
      conn.indexes(Thing.table_name).map(&:columns)
    )
    assert_equal(
      [["e", "f"], ["g"]],
      conn.unique_constraints(Thing.table_name).map(&:column)
    )
    assert_equal(
      [["product_store_id", "product_sku"]],
      conn.foreign_keys(ProductOrder.table_name).map { _1.options[:column] }
    )
  end
end

If the above is saved in a file called test.rb then execute it with DATABASE_URL=postgresql://... ruby test.rb.

Before this PR the logged queries are:

>>>>>> raw_execute:
SELECT distinct i.relname, d.indisunique, d.indkey, pg_get_indexdef(d.indexrelid), t.oid,
                pg_catalog.obj_description(i.oid, 'pg_class') AS comment, d.indisvalid
FROM pg_class t
INNER JOIN pg_index d ON t.oid = d.indrelid
INNER JOIN pg_class i ON d.indexrelid = i.oid
LEFT JOIN pg_namespace n ON n.oid = t.relnamespace
WHERE i.relkind IN ('i', 'I')
  AND d.indisprimary = 'f'
  AND t.relname = 'things'
  AND n.nspname = ANY (current_schemas(false))
ORDER BY i.relname
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (2, 3)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (4)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (5)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (6, 7)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (8)
>>>>>> raw_execute:
SELECT c.conname, c.conrelid, c.conkey, c.condeferrable, c.condeferred, pg_get_constraintdef(c.oid) AS constraintdef
FROM pg_constraint c
JOIN pg_class t ON c.conrelid = t.oid
JOIN pg_namespace n ON n.oid = c.connamespace
WHERE c.contype = 'u'
  AND t.relname = 'things'
  AND n.nspname = ANY (current_schemas(false))
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (6, 7)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226032
AND a.attnum IN (8)
>>>>>> raw_execute:
SELECT t2.oid::regclass::text AS to_table, a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, c.conkey, c.confkey, c.conrelid, c.confrelid
FROM pg_constraint c
JOIN pg_class t1 ON c.conrelid = t1.oid
JOIN pg_class t2 ON c.confrelid = t2.oid
JOIN pg_attribute a1 ON a1.attnum = c.conkey[1] AND a1.attrelid = t1.oid
JOIN pg_attribute a2 ON a2.attnum = c.confkey[1] AND a2.attrelid = t2.oid
JOIN pg_namespace t3 ON c.connamespace = t3.oid
WHERE c.contype = 'f'
  AND t1.relname = 'product_orders'
  AND t3.nspname = ANY (current_schemas(false))
ORDER BY c.conname
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226067
AND a.attnum IN (2, 3)
>>>>>> raw_execute:
SELECT a.attnum, a.attname
FROM pg_attribute a
WHERE a.attrelid = 226059
AND a.attnum IN (1, 2)

and after this PR (after swapping the gem "rails" when in the Rails repo with this PR's branch checked out):

>>>>>> raw_execute:
SELECT distinct i.relname, d.indisunique,
                (
                  SELECT array_agg(a.attname ORDER BY idx)
                  FROM (
                    SELECT idx, d.indkey[idx] AS indkey_elem
                    FROM generate_subscripts(d.indkey, 1) AS idx
                  ) indexed_indkeys
                  LEFT JOIN pg_attribute a ON a.attrelid = t.oid
                  AND a.attnum = NULLIF(indexed_indkeys.indkey_elem, 0)
                ) AS indkey_names,
                pg_get_indexdef(d.indexrelid), t.oid,
                pg_catalog.obj_description(i.oid, 'pg_class') AS comment, d.indisvalid
FROM pg_class t
INNER JOIN pg_index d ON t.oid = d.indrelid
INNER JOIN pg_class i ON d.indexrelid = i.oid
LEFT JOIN pg_namespace n ON n.oid = t.relnamespace
WHERE i.relkind IN ('i', 'I')
  AND d.indisprimary = 'f'
  AND t.relname = 'things'
  AND n.nspname = ANY (current_schemas(false))
ORDER BY i.relname
>>>>>> raw_execute:
SELECT c.conname, c.conrelid, c.condeferrable, c.condeferred, pg_get_constraintdef(c.oid) AS constraintdef,
(
  SELECT array_agg(a.attname ORDER BY idx)
  FROM (
    SELECT idx, c.conkey[idx] AS conkey_elem
    FROM generate_subscripts(c.conkey, 1) AS idx
  ) indexed_conkeys
  JOIN pg_attribute a ON a.attrelid = t.oid
  AND a.attnum = indexed_conkeys.conkey_elem
) AS conkey_names
FROM pg_constraint c
JOIN pg_class t ON c.conrelid = t.oid
JOIN pg_namespace n ON n.oid = c.connamespace
WHERE c.contype = 'u'
  AND t.relname = 'things'
  AND n.nspname = ANY (current_schemas(false))
>>>>>> raw_execute:
SELECT t2.oid::regclass::text AS to_table, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred, c.conrelid, c.confrelid,
  (
    SELECT array_agg(a.attname ORDER BY idx)
    FROM (
      SELECT idx, c.conkey[idx] AS conkey_elem
      FROM generate_subscripts(c.conkey, 1) AS idx
    ) indexed_conkeys
    JOIN pg_attribute a ON a.attrelid = t1.oid
    AND a.attnum = indexed_conkeys.conkey_elem
  ) AS conkey_names,
  (
    SELECT array_agg(a.attname ORDER BY idx)
    FROM (
      SELECT idx, c.confkey[idx] AS confkey_elem
      FROM generate_subscripts(c.confkey, 1) AS idx
    ) indexed_confkeys
    JOIN pg_attribute a ON a.attrelid = t2.oid
    AND a.attnum = indexed_confkeys.confkey_elem
  ) AS confkey_names
FROM pg_constraint c
JOIN pg_class t1 ON c.conrelid = t1.oid
JOIN pg_class t2 ON c.confrelid = t2.oid
JOIN pg_namespace n ON c.connamespace = n.oid
WHERE c.contype = 'f'
  AND t1.relname = 'product_orders'
  AND n.nspname = ANY (current_schemas(false))
ORDER BY c.conname

we can see only 3 top-level SELECTs instead of 1+5 + 1+2 + 1+2 = 12

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[fatkodima]

I had a similar PR for indexes #45381, but wasn't able to understand the review comments and why it was not merged 😆

[byroot]

Hum, actually, I think I misunderstood, this one does a bit more than #45381 right?

If so you'll have to rebase it.

[owst]

Hum, actually, I think I misunderstood, this one does a bit more than #45381 right?

If so you'll have to rebase it.

Thanks - rebased!

And yes, it does do a bit more - it applies a similar approach to #45381 to foreign_keys and unique_constraints (with multiple columns) to make those methods one query, too.

[owst]

This deletes column and primary_key columns, as they can be recovered from conkey_names and confkey_names for a single column key (the first element of each array), and conkey/confkey as we no longer need those as we've resolved the column ids to names

[owst]

Removes conkey, which we've now resolved to its name(s) in conkey_names

[byroot]

Since there's a schema cache, we only really need this early, I don't think this memoization is worth it.

Might as well keep it as a local var in the method that need it like before even if it means one extra allocation.

[byroot]

cc @matthewd

[fatkodima]

Unique constraints is a very rarely used feature (compared to indexes and foreign keys) and the table usually have at most one. This query is now much more complicated and I think not worth it, so I would avoid doing it.

[owst]

I agree that unique constraints are used less-frequently than indexes/FKs. As one data point, at work we do at least make use of them in ~10 cases out of ~150 tables, and of those cases, it turns out most are composite constraints. In terms of additional complication, I personally think it's ok, but it's not my call 😄 A few points:

1. It's the same sub-query pattern as in foreign_keys, so there's a single additional "concept" to understand. (It would have been the same pattern in indexes too, before your PR showed us pg_get_indexdef for one indkey position - it's a shame there isn't something similar for FKs/constraints!)
2. It allows us to remove column_names_from_column_numbers (and avoid the associated trip to/from the DB), which prevents it being used when inappapropriate in the future
3. In the future, if/when the minimum supported version of PG is increased, we can reduce some of the incidental complexity:

(
  SELECT array_agg(a.attname ORDER BY idx)
  FROM (
    SELECT idx, c.conkey[idx] AS conkey_elem
    FROM generate_subscripts(c.conkey, 1) AS idx
  ) indexed_conkeys
  JOIN pg_attribute a ON a.attrelid = t.oid
  AND a.attnum = indexed_conkeys.conkey_elem
) AS conkey_names

can become

(
  SELECT array_agg(a.attname ORDER BY idx)
  FROM unnest(c.conkey) WITH ORDINALITY i(conkey_elem, idx)
  JOIN pg_attribute a ON a.attrelid = t.oid 
  AND a.attnum = i.conkey_elem
) AS conkey_names

The main point of my original PR was indexes, which your PR improved. Therefore I feel less strongly about the remainder in this PR, but I thought it was good to make them consistently avoid separate queries. If you or others feel strongly, I'm fine with removing this change for unique_constraints.

[fatkodima]

This lambda is like avoiding creating a private method. I think, better to do an inline if in the code (preferred) or extract a private method.

[owst]

Yes, agreed an inline if is better 👍

[owst]

Thanks for the review @fatkodima 👍 I've pushed another commit and replied to your comments - let me know what you think

[fatkodima]

Thanks, lgtm. 👍