sql: add support for foreign key cascades in udfs #110396
Conversation
|
This change is |
rharding6373
force-pushed
the
20230607_udf_fk_cascade_87289
branch
3 times, most recently
from
658fed9 to
41d81f0
Compare
There was a problem hiding this comment.
Nice! I only have a few nits.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rharding6373)
pkg/sql/logictest/testdata/logic_test/udf_fk line 258 at r1 (raw file):
statement ok SELECT f_fk_p_cascade(1, 2);
nit: since we use RETURNING * in our UDFs might as well as double-check what's being returned in all cases, and not only when multiple UDFs are invoked in a single stmt.
pkg/sql/logictest/testdata/logic_test/udf_fk line 424 at r1 (raw file):
query IT rowsort SELECT (SELECT * FROM (VALUES ((SELECT x FROM (VALUES (1)) AS s (x)) + y))),
nit: there are tabs here (which I think we generally try to avoid (?)).
pkg/sql/logictest/testdata/logic_test/udf_fk line 446 at r1 (raw file):
12 13
nit: it might be interesting to a test case where the first invocation of mutating UDF succeeds but the second one fails, so the stmt errors out and ensure that the tables aren't modified.
It also might be good to have a test case where we have a cascade and a non-cascade FK check (e.g. we have parent_cascade and child_cascade linked by CASCADE, but also we have grandchild_fk which has FK with child_cascade but without CASCADE (or something along those lines)). WDYT?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf_fk line 446 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it might be interesting to a test case where the first invocation of mutating UDF succeeds but the second one fails, so the stmt errors out and ensure that the tables aren't modified.
It also might be good to have a test case where we have a cascade and a non-cascade FK check (e.g. we have
parent_cascadeandchild_cascadelinked by CASCADE, but also we havegrandchild_fkwhich has FK withchild_cascadebut without CASCADE (or something along those lines)). WDYT?
To add to those suggestions, these tests might be interesting:
- A UDF with multiple statements that cascade to the same table.
- An update/delete that cascades to multiple rows in the same table (can add this to one of the setups above pretty easily I think)
- A self-referencing FK cascade (a table with a foreign key on the same table)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/apply_join.go line 341 at r1 (raw file):
// when executing the outer query. defer func() { plan.subqueryPlans = nil
It's not obvious where subqueryPlans becomes non-nil since we check len(plan.subqueryPlans) != 0. Can you clarify that?
rharding6373
force-pushed
the
20230607_udf_fk_cascade_87289
branch
from
41d81f0 to
7f4c738
Compare
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf_fk line 258 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: since we use RETURNING * in our UDFs might as well as double-check what's being returned in all cases, and not only when multiple UDFs are invoked in a single stmt.
Done.
pkg/sql/logictest/testdata/logic_test/udf_fk line 424 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: there are tabs here (which I think we generally try to avoid (?)).
Fixed.
pkg/sql/logictest/testdata/logic_test/udf_fk line 446 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
To add to those suggestions, these tests might be interesting:
- A UDF with multiple statements that cascade to the same table.
- An update/delete that cascades to multiple rows in the same table (can add this to one of the setups above pretty easily I think)
- A self-referencing FK cascade (a table with a foreign key on the same table)
Thanks for the suggestions! Added these tests. Want to note (since Marcus and I were discussing this offline) that the self-referencing FK cascade test is correct -- I thought it wasn't compared to postgres, but I was accidentally omitting the cascades.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf_fk line 309 at r2 (raw file):
statement ok DROP TABLE child_cascade;
nit: looks like we're recreating child_cascade table with the same schema, why not use TRUNCATE?
This commit adds testing and makes some fixes to support foreign key cascades in UDFs. Epic: CRDB-25388 Informs: cockroachdb#87289 Release note: none
rharding6373
force-pushed
the
20230607_udf_fk_cascade_87289
branch
from
7f4c738 to
eb09145
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf_fk line 309 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: looks like we're recreating
child_cascadetable with the same schema, why not use TRUNCATE?
They aren't actually the same. The first version doesn't have UNIQUE, allowing us to test cascading updates to multiple rows. We need UNIQUE when we cascade to the grandchild_cascade table. I added some comments to the test to help explain what each series of statements is designed to test in the hopes it will clarify this in the future.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/udf_fk line 309 at r2 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
They aren't actually the same. The first version doesn't have
UNIQUE, allowing us to test cascading updates to multiple rows. We needUNIQUEwhen we cascade to thegrandchild_cascadetable. I added some comments to the test to help explain what each series of statements is designed to test in the hopes it will clarify this in the future.
I see, thanks!
|
TFTRs! bors r+ |
|
Build succeeded: |
This commit adds testing and makes some fixes to support foreign key cascades in UDFs.
Epic: CRDB-25388
Informs: #87289
Release note: none