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

feat(pg): Adds support for using partitioned table parents #801

Merged
merged 7 commits into from Aug 21, 2023

Conversation

msjonker
Copy link
Contributor

Description

This adds the ability to toggle which tables are exposed for partitioned tables.
By default, partitions are exposed and parents are hidden, as they were previously, to prevent a breaking change.
To expose parents and hide the partitions, opt in with:

const options = {
  graphileBuildOptions: {
    pgUsePartitionParents: true,
  },
};

See graphile/crystal#1160

I'm not sure the process for submitting a PR to the postgraphile repo to add the option exposed in this PR, so that PR has not yet been created.

Performance impact

The is ultimately just a change to the introspection query, so I don't see there being an impact on performance.

Security impact

Unknown

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

From a quick scan this looks good - thanks! (Will review more thoroughly soon.)

@benjie
Copy link
Member

benjie commented Aug 2, 2022

(Sorry; this is on my radar, but I'm completely ran off my feet until September.)

@blahbleepew
Copy link

this feature would be very useful, I hope it gets looked at soon!

@robertcnix
Copy link

👋 Any chance this can be revived?

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Excellent; really simple addition - nice and clean. Please add another partition or two to ensure no conflicts occur (they won't with the code you have, but a future version might work in a different way) and then write a query and mutation test against it and then we're good to merge! 🙌

To write a query test:

  1. Create a file like this:
  2. Edit the metadata at the top to pass the relevant options, and edit the query
  3. Run the tests - snapshots should be generated automatically.

Use the prefix you created to prevent the tests running pre PG11. If you need any advice, hit up the Discord https://discord.gg/graphile

And sorry this fell off my radar, it got buried under a large number of notifications and the significant effort I'm putting into PostGraphile V5.

) partition by range (log_date);

create table partitioned.temperature_log_y2022m07 partition OF partitioned.temperature_log
FOR VALUES FROM ('2022-07-01') TO ('2022-07-31');
Copy link
Member

Choose a reason for hiding this comment

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

Please add at least one more partition.

@msjonker
Copy link
Contributor Author

msjonker commented Jun 1, 2023

Sounds good! I'll try to get to this in the next couple of weeks.

@robertcnix
Copy link

FYI, we've run into a small problem with this patch. It seems like the introspection query is not able to determine which table to use as foreign key references.

That is, if there are 2 partitioned tables (t1 and t2) with 2 partitions each, and there is a foreign key between them - there will be 5 records for the foreign keys in the pg_constraint table:

  • t1 -> t2
  • t1 -> t2_1
  • t1 -> t2_2
  • t1_1 -> t2
  • t1_2 -> t2

The query needs to return just the t1 -> t2 constraint.

@benjie
Copy link
Member

benjie commented Jun 15, 2023

Please ensure that the query test you write encompasses:

  1. querying a partitioned table directly
  2. querying from a partitioned table to a non-partitioned table
  3. querying from a non-partitioned table to a partitioned table
  4. querying from one partitioned table to another

@msjonker
Copy link
Contributor Author

FYI, we've run into a small problem with this patch. It seems like the introspection query is not able to determine which table to use as foreign key references.

That is, if there are 2 partitioned tables (t1 and t2) with 2 partitions each, and there is a foreign key between them - there will be 5 records for the foreign keys in the pg_constraint table:

  • t1 -> t2
  • t1 -> t2_1
  • t1 -> t2_2
  • t1_1 -> t2
  • t1_2 -> t2

The query needs to return just the t1 -> t2 constraint.

I'm not sure I understand how this is occurring. Yes, this is how the raw pg_constraint catalog is, but it would seem that the querying takes care of this. The constraints query inner joins and filters on the results of class query, which either includes children tables or the parent table, but not both.

@benjie
Copy link
Member

benjie commented Jun 26, 2023

You've missed the schema snapshots; I tried to add them myself but you haven't granted me permission to push to this branch.

$ git commit -m"Add missing schema snapshots"
[partitioned-tables 3e91f237] Add missing schema snapshots
 2 files changed, 3903 insertions(+)
 create mode 100644 packages/postgraphile-core/__tests__/integration/schema/partitioned.1.graphql
 create mode 100644 packages/postgraphile-core/__tests__/integration/schema/partitioned.2.graphql
$ git push -u
ERROR: Permission to ECO-Parking-Technologies/graphile-engine.git denied to benjie.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looking really good! Couple minor issues including the missing files mentioned above. Very close now!

Please do not edit history now

(Note to self: fully reviewed including data comparisons in tests. Also the graphileBuildOptions correctly uses pg prefix even though the PostGraphile option does not - good 👍 )

packages/postgraphile-core/scripts/test Outdated Show resolved Hide resolved
packages/postgraphile-core/scripts/test Outdated Show resolved Hide resolved
@msjonker
Copy link
Contributor Author

Sorry for the late reply; I was out on vacation.

I have checked in the partitioned schema snapshots and committed your change. Thanks!

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Excellent; thanks!

@benjie benjie merged commit 4deb984 into graphile:v4 Aug 21, 2023
@benjie
Copy link
Member

benjie commented Oct 5, 2023

Finally released in v4.14.0 - sorry for the delay, I've been a bit overwhelmed with work as of late 😅 (Plus I wanted to tie up a number of V4 loose ends before this release.)

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.

None yet

4 participants