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

sql/postgres: Avoid using referential_constraints view #2025

Closed
wants to merge 2 commits into from

Conversation

remen
Copy link
Contributor

@remen remen commented Aug 26, 2023

Replace usage of referential_constraints with manual switch/case when introspecting foreign key constraints in postgresql. The switch/case logic was copied from the referential_constraints view DDL.

Rationale:

  1. The referential_constraints view does not contain a reference to the table containing the constraint, which leads to duplicate entries. Fixes foreign keys in different tables with the same name couple on update and on delete behavior #1449
  2. The referential_constraints view has some ownership clauses that prevents lookups from being successful if a user only has SELECT access. Fixes Postgres: missing FK constraints for non-owner #1940

@rotemtam
Copy link
Member

Sorry didn’t mean to unmark draft. Tried to click “run tests” with my chubby fingers

@remen remen changed the title Fix cascade behavior when foreign keys with same name sql/postgres: Avoid using referential_constraints view Aug 26, 2023
@remen
Copy link
Contributor Author

remen commented Aug 26, 2023

Sorry didn’t mean to unmark draft. Tried to click “run tests” with my chubby fingers

No worries.

I updated the title & description with what I think this solves. I'm a bit unsure on how to take it from here though. Should I add a test case? Should I try to find out why it breaks for cockroach db?

Happy to continue the conversation on discord if that's better for you.

@remen
Copy link
Contributor Author

remen commented Aug 26, 2023

Latest commit should make it compatible with cockroachdb. Feel free to run tests again.

Comment on lines +1285 to +1298
CASE con.confupdtype
WHEN 'c' THEN 'CASCADE'
WHEN 'n' THEN 'SET NULL'
WHEN 'd' THEN 'SET DEFAULT'
WHEN 'r' THEN 'RESTRICT'
WHEN 'a' THEN 'NO ACTION'
END AS update_rule,
CASE con.confdeltype
WHEN 'c' THEN 'CASCADE'
WHEN 'n' THEN 'SET NULL'
WHEN 'd' THEN 'SET DEFAULT'
WHEN 'r' THEN 'RESTRICT'
WHEN 'a' THEN 'NO ACTION'
END AS delete_rule
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to append the con.confupdtype, con.confdeltype to the query and do this mapping in Go.

There are small test cases to change then in inspect_test.go and maybe add another one in internal/integration/testdata/postgres.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @a8m! I had a look at doing it this way, but the problem is that both MySQL and Postgres go through the same code path in sqlx.SchemaFKs, and I couldn't find a way to resolve this without more refactoring than I'm comfortable with.

I think handing it over to you is better at this point.

Copy link
Member

@a8m a8m 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 the contribution, @remen. See my comment and let me know if that requires too much time from your side and I can take it.

@a8m
Copy link
Member

a8m commented Aug 28, 2023

Thanks for your contribution, @remen. Your commits were added to #2029.

@a8m a8m closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants