sql: only fetch specific columns needed to validate check constraints for UPDATES #30707
Conversation
|
This change is |
There was a problem hiding this comment.
Thanks for this change.
It looks to me like the new logic test would work before your PR, because the code was requesting all columns. Did it not?
If it did indeed work, then I think your PR is missing a test that demonstrates the before/after change. Perhaps a report of a logical plan where the requested columns are detailed?
If it did not work previously, that means there was a bug. In that case I'd like to see a release note.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
TFTR!
Yes, it would. The new logic test is the only test in our code base that would fail if we didn't specifically check the to
Good point. I was hoping to merge this after #30744 so that we could see the difference in the requested spans, but I'll look into other ways to test this. Hopefully the requested columns are detailed in the logical plan. |
There was a problem hiding this comment.
Good point. I was hoping to merge this after #30744 so that we could see the difference in the requested spans, but I'll look into other ways to test this. Hopefully the requested columns are detailed in the logical plan.
Thanks. Let me know if you need help.
Reviewable status:
Release note: None
This was immediately being replaced in `planner.initTargets` Release note: None
Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/upsertCleanup
branch
from
47133e3 to
576f83c
Compare
There was a problem hiding this comment.
#30744 made this much easier to test. Please see the new logic test. @solongordon might be interested in this as well.
Reviewable status:
There was a problem hiding this comment.
Thanks for the extra tests. LGTM modulo resolution of my question below.
Reviewed 1 of 1 files at r6, 1 of 1 files at r7, 2 of 2 files at r8.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/check_constraints, line 305 at r8 (raw file):
UPDATE t9 SET b = 6 WHERE a = 5 # Only column families that are needed to validate check constraints are fetched.
I'm not sure this test tells us which columns are fetched (or not) -- I think only the list of columns produced by explain(eerbose) can tell us that.
… for UPDATES This addresses a longstanding TODO to only request the columns used in check constraints when performing an UPDATE instead of blindly requesting all columns when a check constraint is present. 8d37935 did all of the heavy lifting for this. This change just plugs it in. The new logic tests fail if we don't correctly request the columns we need for the check constraint. Interestingly, this isn't because we don't fetch the other columns from KV. Instead, it's because we don't decode them and pass them up the stack. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/upsertCleanup
branch
from
576f83c to
82a1d58
Compare
There was a problem hiding this comment.
TFTR.
bors r+
Reviewable status:
pkg/sql/logictest/testdata/logic_test/check_constraints, line 305 at r8 (raw file):
Previously, knz (kena) wrote…
I'm not sure this test tells us which columns are fetched (or not) -- I think only the list of columns produced by
explain(eerbose)can tell us that.
Done.
30707: sql: only fetch specific columns needed to validate check constraints for UPDATES r=nvanbenschoten a=nvanbenschoten This change begins with some cleanup I stumbled upon when looking into addressing #30618. The tweaks are minor, although the second commit might be backport worthy. It then addresses a related issue that was slightly easier to fix. The change addresses a longstanding TODO that's related to #30618 but was a little easier to start with. The goal is to only request the columns used in check constraints when performing an UPDATE instead of blindly requesting all columns when a check constraint is present. 8d37935 did all of the heavy lifting for this. This change just plugs it in. The new logic tests fail if we don't correctly request the columns we need for the check constraint. Interestingly, this isn't because we don't fetch the other columns from KV. Instead, it's because we don't decode them and pass them up the stack. Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
This change begins with some cleanup I stumbled upon when looking into addressing #30618. The tweaks are minor, although the second commit might be backport worthy. It then addresses a related issue that was slightly easier to fix.
The change addresses a longstanding TODO that's related to #30618 but was a little easier to start with. The goal is to only request the columns used in check constraints when performing an UPDATE instead of blindly requesting all columns when a check constraint is present. 8d37935 did all of the heavy lifting for this. This change just plugs it in.
The new logic tests fail if we don't correctly request the columns we need for the check constraint. Interestingly, this isn't because we don't fetch the other columns from KV. Instead, it's because we
don't decode them and pass them up the stack.