sql: remove restriction of foreign key on multiple outbound columns #43417
Conversation
|
This change is |
|
Does this have any interaction with my pr here -- https://github.com/cockroachdb/cockroach/pull/43332/files#diff-e463e2682e19105ef8cffe2fd72b7241R975? I was worried about not being able to uniquely find fk information given an index. |
it would appear that way... |
|
Is that what it means? The issue is trying to take an fk back reference on an index, that only has a table id and an index ID, and trying to find the fk ref that this corresponds to. |
|
i'm kind of wondering if this is an issue already - i think this change makes it so. by the looks of that code, it assumes one index maps to only one foreign key -- which is true if we have only 1 foreign key per outbound column. after this change, one index can now map to multiple foreign keys -- meaning the check is not updated correctly on any subsequent FK. does that seems right to you? |
otan
force-pushed
the
multi_track_drifting
branch
from
0993b33 to
3f84de4
Compare
|
That assumption only comes into play for 19.1 descriptors -- if this code comes into affect then it doesn't affect the upgrade path |
otan
force-pushed
the
multi_track_drifting
branch
from
3f84de4 to
6f922ed
Compare
There was a problem hiding this comment.
I just realized that I don't know what is supposed to happen if we have two FK constraints on an overlapping set of columns with apparently conflicting ON DELETE/ON UPDATE behavior. What if one of them has ON DELETE SET DEFAULT and the other has ON DELETE SET NULL?
In Postgres, this happens:
lucy=# create table t1 (a int primary key);
CREATE TABLE
lucy=# create table t2 (a int default 1);
CREATE TABLE
lucy=# alter table t2 add constraint fk1 foreign key (a) references t1 on delete set null;
ALTER TABLE
lucy=# alter table t2 add constraint fk2 foreign key (a) references t1 on delete set default;
ALTER TABLE
lucy=# insert into t1 values (123);
INSERT 0 1
lucy=# insert into t2 values (123);
INSERT 0 1
lucy=# delete from t1 where a=123;
DELETE 1
lucy=# select * from t2;
a
---
(1 row)
So it seems that ON DELETE SET NULL takes precedence in some sense, but I have no idea what the general principle is here. I also glanced at the Postgres documentation but couldn't find any information.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan)
pkg/sql/create_table.go, line 516 at r1 (raw file):
// If the name already exists, we try append a digit at the end and see if that exists. // We do this until we find one that is available. _, nameExists := constraintInfo[constraintName]
Should we try to pull out the logic here and in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L349-L352 and https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L401-L404 into a separate function, now that we have 3 usages of it?
pkg/sql/create_table.go, line 519 at r1 (raw file):
nextNum := 1 for nameExists { constraintName = fmt.Sprintf("fk_%s_ref_%s%d", string(d.FromCols[0]), target.Name, nextNum)
What if we started including all of the column names in the auto-generated name instead of just the first column? I think if there's one FK from (a) and one from (a, b) then fk_a_ref_t and fk_a_b_ref_t would be more informative than fk_a_ref_t and fk_a_ref_t1. (Although we'd still need the numbers, since I guess now there's nothing preventing someone from creating two indexes on the same list of columns.)
pkg/sql/create_table.go, line 637 at r1 (raw file):
for _, id := range fk.OriginColumnIDs { if _, err := tbl.FindColumnByID(id); err != nil { return errors.AssertionFailedf("trying to add foreign key for column %d that doesn't exist", id)
Do we even still need this check at all?
pkg/sql/sqlbase/structured.go, line 968 at r1 (raw file):
// Found a match. // TODO(otan after PTO): write tests. desc.InboundFKs = append(
I'm not following this part. Would you mind clarifying?
There was a problem hiding this comment.
So it seems that ON DELETE SET NULL takes precedence in some sense, but I have no idea what the general principle is here. I also glanced at the Postgres documentation but couldn't find any information.
Looks like the first one wins - try switching the order of set default and set null.
You'll get:
ERROR: insert or update on table "t2" violates foreign key constraint "fk2"
DETAIL: Key (a)=(1) is not present in table "t1".
This seems to work in cockroach - but I'm curious where that is coded :)
Added a test!
Reviewable status:
pkg/sql/create_table.go, line 516 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
Should we try to pull out the logic here and in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L349-L352 and https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/alter_table.go#L401-L404 into a separate function, now that we have 3 usages of it?
Slightly different but I think the extra _ in the other implementation doesn't matter!
Done!
pkg/sql/create_table.go, line 519 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
What if we started including all of the column names in the auto-generated name instead of just the first column? I think if there's one FK from
(a)and one from(a, b)thenfk_a_ref_tandfk_a_b_ref_twould be more informative thanfk_a_ref_tandfk_a_ref_t1. (Although we'd still need the numbers, since I guess now there's nothing preventing someone from creating two indexes on the same list of columns.)
I think this is a good change that also that addresses #42208.
I think this seems like a bit of a bigger change than this PR intends - so if we want to approach this maybe in a separate PR?
pkg/sql/create_table.go, line 637 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
Do we even still need this check at all?
appears not (it's checked elsewhere!) deleting
pkg/sql/sqlbase/structured.go, line 968 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I'm not following this part. Would you mind clarifying?
i think we don't need this because this should only affect 19.1 -> 19.2 (as @rohany has said), and this is available 20.1 onwards
so I will just remove this.... :D
otan
force-pushed
the
multi_track_drifting
branch
from
6f922ed to
a4d98ba
Compare
There was a problem hiding this comment.
LGTM, I think it's just a matter of adding more tests now.
Reviewed 5 of 5 files at r2.
Reviewable status:
pkg/sql/create_table.go, line 519 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
I think this is a good change that also that addresses #42208.
I think this seems like a bit of a bigger change than this PR intends - so if we want to approach this maybe in a separate PR?
Yeah, makes sense.
pkg/sql/create_table.go, line 639 at r2 (raw file):
name := prefix for i := 1; nameExistsFunc(name); i++ { name = fmt.Sprintf("%s%d", prefix, i)
I might prefer %s_%d, to match the current (arbitrary) implementation for primary keys, but it doesn't really matter.
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file):
DROP TABLE messages # Also test ordering of `ON DELETE` with conflicts.
(Per offline discussion) I think we should be testing both ON DELETE and ON UPDATE with all the possible behaviors (including NO ACTION/RESTRICT and especially CASCADE), and also the case where e.g. there are "conflicting" FK constraints on (a, b) and on (b, c) referencing the same table with columns (a, b, c), though we don't have to test every single combination in the logic tests.
Our understanding is that Postgres implements ON DELETE/UPDATE behavior as a series of consecutive triggers, and this is also what our current implementation does, even though its behavior for FKs from the same column hasn't really been exercised until now.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go, line 519 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
Yeah, makes sense.
Done.
pkg/sql/create_table.go, line 639 at r2 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I might prefer
%s_%d, to match the current (arbitrary) implementation for primary keys, but it doesn't really matter.
Done.
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
(Per offline discussion) I think we should be testing both
ON DELETEandON UPDATEwith all the possible behaviors (includingNO ACTION/RESTRICTand especiallyCASCADE), and also the case where e.g. there are "conflicting" FK constraints on(a, b)and on(b, c)referencing the same table with columns(a, b, c), though we don't have to test every single combination in the logic tests.Our understanding is that Postgres implements
ON DELETE/UPDATEbehavior as a series of consecutive triggers, and this is also what our current implementation does, even though its behavior for FKs from the same column hasn't really been exercised until now.
Cockroach actually does SET DEFAULT/NULL/CASCADE in order, and evaluates RESTRICT/NO ACTION after all row updates have occurred at the end of the statement. Tests reflect this.
otan
force-pushed
the
multi_track_drifting
branch
from
a4d98ba to
2df2a69
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
Cockroach actually does SET DEFAULT/NULL/CASCADE in order, and evaluates RESTRICT/NO ACTION after all row updates have occurred at the end of the statement. Tests reflect this.
Hmm, what I originally vaguely had in mind was that we'd be able to write an end-to-end test that essentially does what the script does, as opposed to putting the output in the logic tests themselves. But I don't even know if that's possible.
Anyway, I am somewhat conflicted about adding ~900 lines of exhaustive, programmatically generated logic tests. On the one hand, there is some precedent for covering "every case" of something (e.g., earlier in the fk file we have every combination of inserts involving NULLs and composite FKs with different matching behavior, which is why the file is already almost 3000 lines long), and there is some sense in which every case tested is genuinely different from the others.
On the other hand, I don't like the idea of either the script and the logic test getting out of sync or having to update the script to update the logic tests. Also, the fk test file is already one of the longest-running logic test files, so I'm reluctant to make it even longer.
I think the logic tests are more useful for hand-generated edge cases and regression tests than for exhaustively covering a space of possibilities. (Arguably we've already violated this with the current logic tests.) I think we probably agree that some kind of randomized testing where we can verify results would be ideal, but given that we don't have that....I'm not sure whether this is a good stopgap measure.
Is there any way to pare this down to a more manageable set of cases that's more human-readable/editable? For instance, I think some of the UPDATE vs. DELETE cases are in some sense equivalent, and SET NULL vs. SET DEFAULT are also similar in some cases.
|
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file): Previously, lucy-zhang (Lucy Zhang) wrote…
I couldn't really find a nice way of doing a end2end test way - if there is would be happy to change it. Perhaps worth investing in a framework of programatic logic tests... I've simplified to ten permutations of DELETE and UPDATE, and updated it accordingly :) |
otan
force-pushed
the
multi_track_drifting
branch
from
2df2a69 to
3ad7a9f
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
I couldn't really find a nice way of doing a end2end test way - if there is would be happy to change it. Perhaps worth investing in a framework of programatic logic tests...
I've simplified to ten permutations of DELETE and UPDATE, and updated it accordingly :)
SGTM
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r4 (raw file):
DROP TABLE messages # Test conflicting foreign keys ON DELETE and ON UPDATE - some known corners cases.
nit: corner
pkg/sql/logictest/testdata/logic_test/fk, line 2849 at r4 (raw file):
# Test conflicting foreign keys ON DELETE and ON UPDATE - some known corners cases. # Tests were generated from https://github.com/otan-cockroach/otan-scripts/blob/master/38850.
I think if we leave the URL in here we may also want to mention that the tests aren't expected to be updated using the script.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r2 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
SGTM
Done.
pkg/sql/logictest/testdata/logic_test/fk, line 2848 at r4 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
nit:
corner
Done.
pkg/sql/logictest/testdata/logic_test/fk, line 2849 at r4 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I think if we leave the URL in here we may also want to mention that the tests aren't expected to be updated using the script.
I'll remove it, as it's no longer really needed to be known.
otan
force-pushed
the
multi_track_drifting
branch
from
3ad7a9f to
00635ee
Compare
Removing the restriction of foreign keys on multiple outbound columns. There is one minor issue we need to address which is done here -- if the constraint name already exists, append a number next to it as postgres does as well. We can make this fancier if we wish later. This PR comes with a test suite of corner cases that we suspect may be hinted at being wrong. It uses both legacy and the new optimizer settings for foreign key checks. Release note (sql change): We previously had a restriction that foreign keys can only reference one outbound column at any given point in time, e.g. in `CREATE TABLE test(a int)`, we cannot have two foreign keys on column a. This PR removes this restriction.
otan
force-pushed
the
multi_track_drifting
branch
from
00635ee to
9dc7e27
Compare
|
bors r=lucy-zhang tftr |
43417: sql: remove restriction of foreign key on multiple outbound columns r=lucy-zhang a=otan Resolves #38850. Removing the restriction of foreign keys on multiple outbound columns. There is one minor issue we need to address which is done here -- if the constraint name already exists, append a number next to it as postgres does as well. We can make this fancier if we wish later. This PR comes with a test suite of corner cases that we suspect may be hinted at being wrong. It uses both legacy and the new optimizer settings for foreign key checks. Release note (sql change): We previously had a restriction that foreign keys can only reference one outbound column at any given point in time, e.g. in `CREATE TABLE test(a int)`, we cannot have two foreign keys on column a. This PR removes this restriction. 43889: server: declare node ready while decommissioning r=andreimatei a=andreimatei Before this patch, a decommissioning or draining, but otherwise healthy, node would return an error from the /health?ready=1 endpoint, thus declaring itself unready and signaling load balancers to send traffic away. This patch makes it so that the decommissioning status no longer matters for the readyness determination. Draining nodes continue to declare themselves unready. Note that decommissioning nodes typically go through draining at the end of the process. The justification is that a node can be decommissioning for an arbitrary amount of time. During that time it can continue to hold leases, etc. So trying to avoid traffic is not particularly desirable. In fact, some people even want to keep a node in a decommissioning state indefinitely (by restarting a decommissioning node without recommissioning it). Also, the server.shutdown.drain_wait cluster setting is there to give load balancers ample time to find out about a draining node. Also, tactically, the code is simplified. Release note (general change): A node no longer declares itself to not be ready through the /health/ready=1 endpoint while it's in the process of decommissioning. It continues to declare itself unready while draining. Co-authored-by: Oliver Tan <[email protected]> Co-authored-by: Andrei Matei <[email protected]>
Build succeeded |
Resolves #38850.
Removing the restriction of foreign keys on multiple outbound columns.
There is one minor issue we need to address which is done here -- if the
constraint name already exists, append a number next to it as postgres
does as well. We can make this fancier if we wish later.
This PR comes with a test suite of corner cases that we suspect may be
hinted at being wrong. It uses both legacy and the new optimizer
settings for foreign key checks.
Release note (sql change): We previously had a restriction that foreign
keys can only reference one outbound column at any given point in time,
e.g. in
CREATE TABLE test(a int), we cannot have two foreign keyson column a. This PR removes this restriction.