Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support dumping PostgreSQL inheritance & partitioning options to schema.rb #50475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

waymondo
Copy link
Contributor

@waymondo waymondo commented Dec 28, 2023

Motivation / Background & Detail

create_table provides an options: key that allows you to append options to your table definition. One of the things you can do this way with PostgreSQL 10+ is use native partitioning to declare partitioning definitions:

create_table :events, id: false, options: "PARTITION BY LIST (account_id)" do |t|
  t.bigint :account_id, null: false
  t.integer :kind, null: false
  t.datetime :occurred_at, null: false
end

Adding this to a migration and running it will properly create the partitioned table in development, but the dumped schema will not persist the specified partitioning options. Therefore, if you run your test suite which loads from schema.rb, your test database's table won't be properly partitioned.

This PR extends the PostgeSQL adapter's schema statements so that it can dump this partitioning definition as defined in the relevant system tables.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated 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.

@waymondo
Copy link
Contributor Author

waymondo commented Jan 6, 2024

Hi @fatkodima, I've seen that you've done some work lately surrounding updates to the Postgres migrations and schema support in ActiveRecord. Would you mind taking a look at this and let me know if you think of it?

Copy link
Member

@fatkodima fatkodima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some suggestions. Not sure about the feature by itself.

And please keep all the changes in 1 commit.

Copy link
Contributor Author

@waymondo waymondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your review @fatkodima! made some updates and left a couple responses. do you have a recommendation of who might be a good candidate for me to ping about the merit of the feature being included?

@fatkodima
Copy link
Member

No need to ping anyone, just wait some time for some other reviewers (or ask for reviews in discord channel).
And please squash your PR into a single commit.

@waymondo waymondo changed the title support dumping postgres partitioning options to schema support dumping PostgreSQL partitioning options to schema.rb Jan 7, 2024
@waymondo
Copy link
Contributor Author

waymondo commented Jan 7, 2024

@fatkodima pushed up a squashed commit, thanks for your time

@yahonda
Copy link
Member

yahonda commented Jan 9, 2024

options: can be used for some database-specific options like options: 'ENGINE=InnoDB DEFAULT CHARSET=utf8mb4' for MySQL.

PostgreSQL native partition is one of them, not everything. It eventually add the string specified in the option: clause at the end of
create table <table_name> (), so it needs to consider the order of each options to be dumped to meet database specific requirements,
that should be hard for database adapter to consider every order. How about config.active_record.schema_format = :sql and run rails db:schema:dump?

@waymondo
Copy link
Contributor Author

waymondo commented Jan 9, 2024

Hi @yahonda. Yes, this is just one of things that options: can do and supporting all features for all adapters like this would be challenging. My thinking was that starting to add support for specific adapter features through options would be nice to have though. In terms of ordering, for Postgres the create_table option ordering is well documented and there aren't that many, in case other options were to get added in this manner:

https://www.postgresql.org/docs/current/sql-createtable.html

Using the :sql schema format does work, but many teams have a preference to use schema.rb and I'm not sure how one could easily use native partitioning otherwise. It also felt unexpected to me to use options: to correctly create the partitioned parent table (which felt like a valid use case for it) to then have it not work as expected in the test environment when the schema was loaded.

Thanks for your time, let me know what you think.

@waymondo
Copy link
Contributor Author

waymondo commented Jan 10, 2024

As a delayed follow-up, i pushed up support for INHERITS in this PR as well. Note that INHERITS is not compatible with PARTITION so there isn't a need to support combining those options

@waymondo waymondo changed the title support dumping PostgreSQL partitioning options to schema.rb support dumping PostgreSQL inheritance & partitioning options to schema.rb Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants