PostgreSQL 10 new relkind for partitioned tables

[ys]

Starting with PostgreSQL 10, we can now have partitioned tables natively

Summary

Because of the new partition type for tables in PostgreSQL 10, table_exists? is not finding the table for natively partition tables.

A new p type has been introduced.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[eileencodes]

Added one comment and otherwise I think this looks good. Are there any tests around this code that would be helpful to update?

[eileencodes]

Can you update the comment to include p?

[ys]

Thanks @eileencodes
For the tests, they would need to be postgres 10 specific. I haven't found much tests around postgres native functions...
Maybe I haven't looked at them correctly.

[matthewd]

This is a bit of a drive-by comment, because I haven't actually read through to check this is where you need to look, but I think you want https://github.com/rails/rails/tree/master/activerecord/test/cases/adapters/postgresql and https://github.com/rails/rails/blob/master/activerecord/test/cases/view_test.rb.

The former is where all the PG-specific tests go; the latter is, I believe, basically testing the 'v' (and 'm') part of this expression. So I think we want a partitioned_table_test.rb, in the style of view_test.rb, and in the postgresql/ directory.

I don't know of a specific one off the top of my head, but I expect you'll find a number of version gates on tests inside postgresql/, too.

[ys]

Added a test for table_exists? but Rails has no part running postgreSQL 10, should I just update the postgreSQL version in .travis.yml or duplicate one of the entries?

[rafbm]

Wouldn’t it also make sense to handle 'p' along with 'r' here?

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

Lines 763 to 773 in 889eb91

	def quoted_scope(name = nil, type: nil)
	schema, name = extract_schema_qualified_name(name)
	type = \
	case type
	when "BASE TABLE"
	"'r'"
	when "VIEW"
	"'v','m'"
	when "FOREIGN TABLE"
	"'f'"
	end

[ys]

@rafbm Probably! I am not familiar with a lot of this :D

[simi]

This test should be run only when this feature is supported. What about to add method to PostgreSQLTestCase checking if feature is available? You can get current version via ActiveRecord::Base.connection.postgresql_version.

def postgresql_version     
  ActiveRecord::Base.connection.postgresql_version
end

def supports_partitioned_tables?
  postgresql_version >= 100000
end

in test:

def test_partitions_table_exists
  skip unless supports_partitioned_tables?
  ...
end

This is similar how adapter supported features are detected, but it is probably not worth it to introduce this feature into adapter, since it is used only for tests now.

Also direct version check is possible as used in

rails/activerecord/test/cases/adapters/postgresql/transaction_test.rb

Line 97 in 4f0eb1c

skip unless ActiveRecord::Base.connection.postgresql_version >= 90300

[ys]

I forgot about this PR and feel a bit overwhelmed by the things to do to test this and what is really impacted by that change.

Would love to get some more help on this.

[ys]

Looking at all the code linked is not really helping me as I have no idea of the impact beyond this

[yahonda]

Hi, I'm interested in this pull request then I am attempting to update a test for this.
yahonda@ee6dd9d

Somehow, it looks like this table has not created. It is likely this transaction has not committed.
Will update when I have some more information but it would be appreciated if someone gave us some advice about DDL transaction differences between partitioned tables and non-partitioned tables.

Thank you.

[yahonda]

I have updated this pull request to also select 'p' for "BASE TABLE" and add a test case for this change. yahonda@717d5a3
Now all unit tests are green.

@ys Would you consider to cherry-pick my commit to complete this pull request?

[ys]

@eileencodes Just added the tests that @yahonda wrote!

Btw, Yahonda, thanks so much for this!

[yahonda]

Thanks for the cherry-pick and push. I have found my commit needs fixed. Pushed two additional commits to make CI green.

• Address RuboCop offense
  yahonda@67ef017
• Addressed incorrect postgresql_version
  yahonda@127dc68

Thanks

[yahonda]

Thanks. It looks good now. one CI failure https://travis-ci.org/rails/rails/jobs/392120838 is not related to this pull request, I think.