opt: inline constant values for FK and uniqueness checks#65093
opt: inline constant values for FK and uniqueness checks#65093mgartner wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Ok, this was more of a doozy than I thought it would be. Please review carefully. I used "Informs" instead of "Fixes" in the commit because I think there's probably more work to be done here. But I think this is a good start. For example, can we do something similar for DELETES on parent tables? What about FK cascades? |
a8d258b to
79293dc
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)
pkg/sql/opt/xform/testdata/external/tpcc-later-stats, line 165 at r2 (raw file):
└── f-k-checks └── f-k-checks-item: order(o_w_id,o_d_id,o_c_id) -> customer(c_w_id,c_d_id,c_id) └── anti-join (cross)
I think this will prevent the use of "insert fast path", which is an important optimization. We should probably add some execbuilder tests for these. I'm not quite sure how to integrate these two things.
6851027 to
ebe1373
Compare
|
This is ready for a look now. Radu and I discussed how this breaks the insert fast path for some cases, but we'll address that in a future PR. The fix is not yet obvious. I also made a slight tweak so that DEFAULT and computed insert values will be inlined, assuming they are normalized to constant values in |
rytaft
left a comment
There was a problem hiding this comment.
Nice! I'm just a bit worried about merging this before we have a fix to the fast path in case it causes a TPC-C regression...
Reviewed 7 of 11 files at r1, 10 of 11 files at r2, 7 of 7 files at r3.
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit, line 749 at r2 (raw file):
└── • error if rows │ └── • norows
nice simplification!
pkg/sql/opt/optbuilder/mutation_builder.go, line 1350 at r3 (raw file):
// checking for FK and uniqueness violations. It returns either a WithScan that // iterates over the input to the mutation operator, or a Values expression with // constant mutation values inlined. produces constant values
nit: "produces constant values" looks it was left over
pkg/sql/opt/xform/testdata/external/tpcc, line 162 at r3 (raw file):
└── f-k-checks └── f-k-checks-item: order(o_w_id,o_d_id,o_c_id) -> customer(c_w_id,c_d_id,c_id) └── anti-join (cross)
do you think we'll see a regression in TPC-C if we merge this without fixing the fast path issue?
In FK and uniqueness checks, WithScans that buffer the mutation's input are replaced with Values expression when inserted values are constant. This is especially beneficial for `REGIONAL BY ROW` tables where the `crdb_region` column is a computed column dependent on a FK. Because the constant values are inlined, the optimizer is not able to reduce a FK check that scans multiple regions with a FK check that checks only a single region. Informs cockroachdb#63882 Release note (performance improvement): Previously, foreign key checks performed for inserts into `REGIONAL BY ROW` tables always searched the parent table in all regions to check for FK violations. Now, only a single region is searched if constant values are inserted and the `crdb_region` column is a computed column dependent on the FK column.
ebe1373 to
286eb2c
Compare
mgartner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit, line 749 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nice simplification!
🎉
pkg/sql/opt/optbuilder/mutation_builder.go, line 1350 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
nit: "produces constant values" looks it was left over
Done.
pkg/sql/opt/xform/testdata/external/tpcc, line 162 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
do you think we'll see a regression in TPC-C if we merge this without fixing the fast path issue?
I would assume we would see a regression. I'm fine with leaving this PR unmerged until we can come up with a fix for the fast path issue.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:
pkg/sql/opt/xform/testdata/external/tpcc, line 162 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I would assume we would see a regression. I'm fine with leaving this PR unmerged until we can come up with a fix for the fast path issue.
Sounds good -- I think it would be good to hold off merging in that case.
|
Closing in favor of #77943. |
In FK and uniqueness checks, WithScans that buffer the mutation's input
are replaced with Values expression when inserted values are constant.
This is especially beneficial for
REGIONAL BY ROWtables where thecrdb_regioncolumn is a computed column dependent on a FK. Because theconstant values are inlined, the optimizer is not able to reduce a FK
check that scans multiple regions with a FK check that checks only a
single region.
Informs #63882
Release note (performance improvement): Previously, foreign key checks
performed for inserts into
REGIONAL BY ROWtables always searched theparent table in all regions to check for FK violations. Now, only a
single region is searched if constant values are inserted and the
crdb_regioncolumn is a computed column dependent on the FK column.