Add foreign key support #53
Conversation
| if !con.IsInheritable { | ||
| sb.WriteString(" NO INHERIT") | ||
| } | ||
|
|
||
| if !con.IsValid { | ||
| sb.WriteString(" NOT VALID") | ||
| } else if !csg.isNewTable { | ||
| hazards = append(hazards, MigrationHazard{ |
There was a problem hiding this comment.
We were missing this hazard previously
bplunkett-stripe
force-pushed
the
bplunkett/foreign-key-support
branch
from
a1242f2 to
1475640
Compare
| WHERE | ||
| constraint_namespace.nspname = 'public' | ||
| AND pg_constraint.contype = 'f' | ||
| AND pg_constraint.conislocal; |
There was a problem hiding this comment.
Does this imply that fk constraints for partitioned tables will be handled only at the parent table level?
There was a problem hiding this comment.
Yes. The foreign key will trickle down automatically for us. We don't need to do any complicated stuff like we do for indexes where we attach the foreign key
| @@ -42,150 +42,6 @@ var ( | |||
| } | |||
|
|
|||
| schemaMigrationPlanTestCases = []schemaMigrationPlanTestCase{ | |||
| { | |||
| name: "No-op", | |||
There was a problem hiding this comment.
What's the purpose of removing these test cases?
There was a problem hiding this comment.
The schema migration plan cases test "how" the migration is done, not just the migration's corectness. The latter are tested by the acceptance tests.
Since there are many "no-op" acceptance tests that assert the generated plan is empty, we don't need this test case. I'm trying to minimize the number of these test cases, since it's tricky to maintain the before and after schemas.
| deletedTablesByName: deletedTablesByName, | ||
| addedTablesByName: addedTablesByName, |
There was a problem hiding this comment.
double checking: Will this include recreated tables?
There was a problem hiding this comment.
Yes! We validate this works in the test cases that change a table from partitioned to unpartitioned and vice-versa.
| if !strings.HasSuffix(diff.old.ConstraintDef, " NOT VALID") { | ||
| return nil, fmt.Errorf("expected the old constraint def to be suffixed with NOT VALID: %q", diff.old.ConstraintDef) | ||
| } | ||
| diff.old.ConstraintDef = strings.TrimSuffix(diff.old.ConstraintDef, " NOT VALID") |
There was a problem hiding this comment.
nit: Is this just so the cmp.Equal on 1482 works? Might make sense to just do that inline on 1482
There was a problem hiding this comment.
This is a really good question. This is a pattern of the Alter functions that I'm switching too.
The Alter functions need to keep track of what they can successfully alter and what they can't. If they can't successfully resolve a diff, then ideally they return an error to the user.
I found the easiest way to do this is:
For each alterable difference,
- generate a statement to alter the difference
- update a copy of old to include the difference
Finally, compare old to new. If they are equal, then there were no unresolvable diffs.
With the case of invalid to valid, we need both update the old to be valid and the constraint def in order for the equality check to pass. The strings.HasSuffix is more of a confidence check that the constraint def follows the expected format.
There was a problem hiding this comment.
SGTM if you want to do this for consistency with that pattern 👍
Co-authored-by: alexaub-stripe <130488060+alexaub-stripe@users.noreply.github.com>
Description
Add foreign key support
Motivation
#49
Testing
Tested with acceptance tests