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

Partitioning: Initial suggestion #485

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

m0rt3nlund
Copy link

Hi!

This is my initial suggestion to include "partitioning" support.
Feedback is appreciated!

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

@m0rt3nlund m0rt3nlund changed the title Initial suggestion Partitioning: Initial suggestion Feb 14, 2025
partition_name = table <> "_" <> "#{key}"

if partition_exists?(repo, resource, partition_name) do
{:error, :allready_exists}
Copy link
Contributor

Choose a reason for hiding this comment

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

ty

Suggested change
{:error, :allready_exists}
{:error, :already_exists}

@zachdaniel
Copy link
Contributor

The general pattern looks good, although there will be some details to talk through and some docs about how to manage partitions, maybe some builtin changes to do it automatically etc. The migration generator is going to have to have some kind of handling for changing the partition strategy as well.

I noticed some unfinished bits as well, i.e for creating range partitions. We should definitely avoid the string interpolation in the SQL there as well.

@m0rt3nlund m0rt3nlund marked this pull request as draft February 17, 2025 12:24
@m0rt3nlund
Copy link
Author

I made some updates before reading your comments.
I will try to make a new draft considering your comments :)

@m0rt3nlund
Copy link
Author

Also found some problems when updating the primary key on multitenancy resource.
Not sure it this is how you would prefer to fix it.

@zachdaniel
Copy link
Contributor

Could we separate out that other fix that you mentioned into its own PR so that I can review in isolation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants