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(schema-engine): CHECK and EXCLUDE constraints stopgap for Postgres #3860

Merged
merged 28 commits into from
Apr 19, 2023

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Apr 10, 2023

Notion Details:

Note: the stopgap implementation for Check constraints is exactly the same as the one for Exclusion constraints for Postgres. Hence why they were implemented at the same time.

In terms of introspection warning codes:

  • CHECK: 33
  • EXCLUDE: 34

TODOs

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 10, 2023

CodSpeed Performance Report

Merging #3860 feat/postgres-check-constraints-stopgap (ea9510a) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 6 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

@jkomyno jkomyno changed the title feat(schema-engine): CHECK constraints stopgap for Postgres feat(schema-engine): CHECK and EXCLUDE constraints stopgap for Postgres Apr 10, 2023
@jkomyno jkomyno added this to the 4.13.0 milestone Apr 10, 2023
Copy link
Contributor

@eviefp eviefp left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I wonder if @tomhoule or anyone else could confirm that they had at least a quick look as well.

There's one nitpick comment below.

schema-engine/sql-schema-describer/src/postgres.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

Nice! A few minor things but this is looking good, with the other PR that addresses the extra roundtrip (will have a look there next).

@prisma prisma deleted a comment from sonatype-lift bot Apr 16, 2023
jkomyno and others added 2 commits April 17, 2023 16:12
* Exclude constraints are PG only, lighten up the api

* Split constraint tests into its own file

* Fix describer tests
schema-engine/sql-schema-describer/src/lib.rs Outdated Show resolved Hide resolved
schema-engine/sql-schema-describer/src/lib.rs Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ WITH rawindex AS (
WHERE
indpred IS NULL -- filter out partial indexes
AND array_position(indkey::int2[], 0::int2) IS NULL -- filter out expression indexes
AND NOT indisexclusion -- filter out exclusion constraints
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we actually do exclusions here instead?

@pimeys
Copy link
Contributor

pimeys commented Apr 17, 2023

you also want to rebase with main to get rid of flaky vitess

@janpio janpio modified the milestones: 4.13.0, 4.14.0 Apr 18, 2023
janpio
janpio previously requested changes Apr 18, 2023
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Ensure this does not get merged in 4.13.0 accidentally

@tomhoule
Copy link
Contributor

yep lots of changes since yesterday too, I'll do another review before we can merge

@jkomyno
Copy link
Contributor Author

jkomyno commented Apr 18, 2023

you also want to rebase with main to get rid of flaky vitess

Fyi, I have pulled main in here, but Vitess seems still flaky

@jkomyno jkomyno requested review from tomhoule and pimeys April 18, 2023 15:57
@janpio janpio self-requested a review April 18, 2023 19:02
Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@jkomyno
Copy link
Contributor Author

jkomyno commented Apr 19, 2023

Ensure this does not get merged in 4.13.0 accidentally

Could you please remove "requested changes" now? It's blocking the merge :)

@jkomyno jkomyno merged commit 42f41a6 into main Apr 19, 2023
49 checks passed
@jkomyno jkomyno deleted the feat/postgres-check-constraints-stopgap branch April 19, 2023 14:08
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

6 participants