Add support for partitioned indexes in PostgreSQL 11+ #37585

sebastian-palma · 2019-10-29T12:19:45Z

This PR adds support to retrieve partitioned indexes when asking for indexes in a table.

Currently, the pg_class catalog is filtered out to retrieve the indexes in a
table by its relkind value. Which in versions lower than 11 of PostgreSQL
is always i (lower case). But since version 11, PostgreSQL
supports partitioned indexes referenced with a relkind value of I
(upper case). This makes any feature within the current code base to exclude those
partitioned indexes.

The solution proposed is to make use of the IN clause to filter those
relkind values of i and/or I when retrieving table indexes.

This adds the needed changes to cover the issue when performing an upsert_all with a partitioned table and a unique index in the parent table.

As index_name_exists? also performs the relkind = 'i' filter when asking for the indexes, it's been updated to reflect these changes.

simi · 2019-10-29T13:57:37Z

activerecord/test/schema/schema.rb

@@ -503,6 +503,18 @@
    t.column :weight, :integer
  end

+  create_table(:measurements, id: false, force: true, options: "PARTITION BY LIST (city_id)") do |t|


This (and following) measurements table definitions in schema are valid only for postgres adapter.

CI can't create those tables on mysql and sqlite.

https://buildkite.com/rails/rails/builds/64610#b993a2df-ea4f-45f4-a6f2-0dd8487e2237
https://buildkite.com/rails/rails/builds/64610#8112f1a1-8b9e-4cf0-9b48-49f8caf5fb24

Any chance to make this table conditional and load coresponding model only when currently used adapter supports_partitioned_indexes?

Hey @simi, thanks for the feedback. And yes, I'm gonna take a look at that, didn't check before if they were adapter constrained.

simi · 2019-10-29T14:18:59Z

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -93,7 +93,7 @@ def indexes(table_name) # :nodoc:
            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 = i.relnamespace
-            WHERE i.relkind = 'i'


🤔 We can hardcode IN ('i', 'I') for all versions here since it will not break older versions AFAIK.

I'm gonna try hardcoding the IN clause and running the test suite with older PG versions. I used PostgreSQL 11 to test this.

simi · 2019-10-29T14:23:13Z

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -774,6 +774,11 @@ def quoted_scope(name = nil, type: nil)
            scope
          end

+          def relkind


🤔 There's already space before interpolation at WHERE i.relkind #{relkind}. There's no need to return string starting with space.

If we hardcode the IN clause, we can get rid of this method 😉

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

simi · 2019-10-31T09:25:38Z

activerecord/test/cases/insert_all_test.rb

@@ -3,6 +3,7 @@
 require "cases/helper"
 require "models/book"
 require "models/speedometer"
+require "models/measurement"


🤔 Maybe we can require this only in test_upsert_all_works_with_partitioned_indexes after the skip part since it will be used only there and for all other cases it will get loaded model having no real db table (since it is created conditionally).

That's a nice catch! And I think is a good idea, since there's only one test in the whole file making use of it.

BTW I tested the WHERE i.relkind IN ('i', 'I') changes in PostgreSQL 9.4, 9.6 and 10 (plus 11) and the test suite passes without problems.

sebastian-palma · 2019-12-15T19:08:02Z

Hey @kamipo, could you take a look at this PR, please? I'm wondering if it can be merged in order to solve the problem stated in the description.

kamipo · 2019-12-16T03:00:39Z

index_name_exists? also makes a query to pg_class asking for those rows relkind i only, but that's hasn't been changed.

Why?

sebastian-palma · 2019-12-16T11:53:46Z

index_name_exists? also makes a query to pg_class asking for those rows relkind i only, but that's hasn't been changed.

Why?

I thought it'd make the PR "non-atomic" by trying to solve two things at once. Should I go with those changes?

simi · 2019-12-16T12:03:48Z

@vnhnhm if you would like to add support for partitioned indexes of PostgreSQL adapter in general, you should fix all places needed. Your current change does fix only upsert_all part.

simi · 2019-12-16T12:11:33Z

activerecord/test/schema/schema.rb

@@ -503,6 +503,20 @@
    t.column :weight, :integer
  end

+  if supports_partitioned_indexes?


🤔 I think you can move this to test/schema/postgresql_specific_schema.rb.

sebastian-palma · 2019-12-20T23:20:30Z

@kamipo I've updated the code for index_name_exists? and added some test examples for that. @simi thanks for the clarification (;

simi · 2019-12-27T17:52:58Z

@vnhnhm I think you should update the changelog entry title as well.

sebastian-palma · 2019-12-27T21:28:28Z

It seems I did something wrong while trying to update the changelog, no @simi?

simi · 2019-12-27T21:41:38Z

Try to rebase against latest master @vnhnhm.

sebastian-palma · 2019-12-27T23:00:31Z

It looks better now, thanks @simi

simi · 2019-12-27T23:14:01Z

Looks ok now. @kamipo any chance to take another look at this?

eugeneius · 2019-12-27T23:34:32Z

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -124,6 +124,10 @@ def supports_partial_index?
        true
      end

+      def supports_partitioned_indexes?
+        false
+      end


This shouldn't be necessary, since it's identical to the version in AbstractAdapter.

eugeneius · 2019-12-27T23:37:03Z

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Add support for PostgreSQL 11+ partitioned indexes when using `upsert-all`.


Suggested change

* Add support for PostgreSQL 11+ partitioned indexes when using `upsert-all`.

* Add support for PostgreSQL 11+ partitioned indexes when using `upsert_all`.

sebastian-palma · 2019-12-28T15:41:58Z

Hey @eugeneius, just removed the changes from the SQLite3Adapter and updated the changelog as suggested, thanks!

simi

looks good to me

PS: rebase would be nice

sebastian-palma · 2020-01-29T07:15:31Z

looks good to me

PS: rebase would be nice

Yes, also to me :-). Thanks @simi

kamipo · 2020-02-04T23:35:22Z

Can you squash your commits into one?

indexes in a table. Currently the pg_class catalog is filtered out to retrieve the indexes in a table by its relkind value. Which in versions lower than 11 of PostgreSQL is always `i` (lower case). But since version 11, PostgreSQL supports partitioned indexes referenced with a relkind value of `I` (upper case). This makes any feature within the current code base to exclude those partitioned indexes. The solution proposed is to make use of the `IN` clause to filter those relkind values of `i` and/or `I` when retrieving a table indexes.

sebastian-palma · 2020-02-06T06:11:25Z

Sure, done @kamipo

simi · 2020-02-06T11:46:53Z

🎉 thanks everone involved!

…-indexes Add support for partitioned indexes in PostgreSQL 11+

rails-bot bot added the activerecord label Oct 29, 2019

sebastian-palma mentioned this pull request Oct 29, 2019

Model.upsert_all not playing nicely with partitioned tables #37426

Closed

simi reviewed Oct 29, 2019

View reviewed changes

bogdan reviewed Oct 29, 2019

View reviewed changes

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb Outdated Show resolved Hide resolved

simi reviewed Oct 31, 2019

View reviewed changes

simi reviewed Dec 16, 2019

View reviewed changes

rails-bot bot added actioncable actionmailbox actionmailer actionpack actiontext labels Dec 27, 2019

eugeneius reviewed Dec 27, 2019

View reviewed changes

eugeneius removed actioncable actionmailbox actionmailer actionpack labels Dec 28, 2019

eugeneius removed the actiontext label Dec 28, 2019

simi approved these changes Jan 27, 2020

View reviewed changes

kamipo merged commit ee65181 into rails:master Feb 6, 2020

sebastian-palma deleted the upsert-all-partitioned-indexes branch February 6, 2020 09:07

raivil mentioned this pull request Mar 30, 2020

"indexes(:table)" is not working with partitioned tables in postgresql 11 #38847

Closed

rafaelfranca pushed a commit that referenced this pull request Mar 30, 2020

Merge pull request #37585 from sebastian-palma/upsert-all-partitioned…

1db2ae1

…-indexes Add support for partitioned indexes in PostgreSQL 11+

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for partitioned indexes in PostgreSQL 11+ #37585

Add support for partitioned indexes in PostgreSQL 11+ #37585

sebastian-palma commented Oct 29, 2019 •

edited

simi Oct 29, 2019

sebastian-palma Oct 29, 2019

simi Oct 29, 2019

sebastian-palma Oct 29, 2019

simi Oct 29, 2019

sebastian-palma Oct 29, 2019

simi Oct 31, 2019

sebastian-palma Oct 31, 2019

sebastian-palma commented Dec 15, 2019

kamipo commented Dec 16, 2019

sebastian-palma commented Dec 16, 2019

simi commented Dec 16, 2019

simi Dec 16, 2019

sebastian-palma commented Dec 20, 2019

simi commented Dec 27, 2019

sebastian-palma commented Dec 27, 2019

simi commented Dec 27, 2019

sebastian-palma commented Dec 27, 2019

simi commented Dec 27, 2019

eugeneius Dec 27, 2019

eugeneius Dec 27, 2019

sebastian-palma commented Dec 28, 2019

simi left a comment •

edited

sebastian-palma commented Jan 29, 2020

kamipo commented Feb 4, 2020

sebastian-palma commented Feb 6, 2020

simi commented Feb 6, 2020

		@@ -1,3 +1,7 @@
		* Add support for PostgreSQL 11+ partitioned indexes when using `upsert-all`.

Add support for partitioned indexes in PostgreSQL 11+ #37585

Add support for partitioned indexes in PostgreSQL 11+ #37585

Conversation

sebastian-palma commented Oct 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-palma commented Dec 15, 2019

kamipo commented Dec 16, 2019

sebastian-palma commented Dec 16, 2019

simi commented Dec 16, 2019

Choose a reason for hiding this comment

sebastian-palma commented Dec 20, 2019

simi commented Dec 27, 2019

sebastian-palma commented Dec 27, 2019

simi commented Dec 27, 2019

sebastian-palma commented Dec 27, 2019

simi commented Dec 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-palma commented Dec 28, 2019

simi left a comment • edited

Choose a reason for hiding this comment

sebastian-palma commented Jan 29, 2020

kamipo commented Feb 4, 2020

sebastian-palma commented Feb 6, 2020

simi commented Feb 6, 2020

sebastian-palma commented Oct 29, 2019 •

edited

simi left a comment •

edited