Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: when RETURNING expression is present, only fetch the specific columns used #30618

Closed
nvanbenschoten opened this issue Sep 25, 2018 · 3 comments · Fixed by #38594
Closed

sql: when RETURNING expression is present, only fetch the specific columns used #30618

nvanbenschoten opened this issue Sep 25, 2018 · 3 comments · Fixed by #38594
Assignees
@ridwanmsharif
Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. A-sql-optimizer SQL logical planning and optimizations. C-performance Perf of queries or internals. Solution not expected to change functional behavior. E-starter Might be suitable for a starter project for new employees or team members.
Milestone
19.2

Comments

@nvanbenschoten
Copy link
Member

See the following TODO:

cockroach/pkg/sql/update.go

Lines 191 to 192 in b2bd8e8

// TODO(dan): This could be made tighter, just the rows needed for RETURNING
// exprs.

We currently fetch all columns when an INSERT, UPDATE, or DELETE statement contains a RETURNING <exprs> clause. This is an issue because it forces us to retreive unnecessary data and it can create exrta contention. We should determine which columns the RETURNING expression needs and only request those exact columns.

This should be solved in conjunction with #18168 for maximum benefit.
@nvanbenschoten nvanbenschoten added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-sql-optimizer SQL logical planning and optimizations. labels Sep 25, 2018
@knz knz added the A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. label Sep 25, 2018
@knz knz changed the title sql: when RETUNING expression is present, only fetch the specific columns used sql: when RETURNING expression is present, only fetch the specific columns used Sep 25, 2018
@nvanbenschoten nvanbenschoten mentioned this issue Sep 25, 2018
Merged
@nvanbenschoten nvanbenschoten self-assigned this Sep 26, 2018
@nvanbenschoten nvanbenschoten mentioned this issue Sep 27, 2018
Merged
craig bot pushed a commit that referenced this issue Nov 12, 2018
@nvanbenschoten
Merge #30707
b2d892a 
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]>
@nvanbenschoten
Copy link
Member Author

@andy-kimball I've heard you're looking into changes in this area. Mind describing/pointing me at the status of that work? This is currently the only blocker for #30624.

@andy-kimball
Copy link
Contributor

I've got a PR out for incorporating parts of the Insert operation into the optimizer:
#32423

I'll soon post a similar PR for Update. I'll then work on Upsert, Delete, and CreateTable.

As for your question, I think my PR's set the stage for the time when we can get more specific with the fetched columns. But the time is not yet, as we'd still need to incorporate analysis of check columns, foreign key checks, and maybe other usages of columns before we could figure out a minimal set of fetch columns. The current focus of the optimizer work is to enable decorrelated subqueries in mutation statements, as well as to incorporate latency into the optimizer's coster:

#31955

That focus doesn't require us to figure out the minimal set of fetch columns, so I was not planning on doing it as part of my current work-stream. That said, @knz has been working on revamping SQL mutations, so between the two teams there may be a possibility of doing the necessary work this release.

@nvanbenschoten nvanbenschoten removed their assignment Nov 29, 2018
@jordanlewis
Copy link
Member

@RaduBerinde, I'm going to move this issue to planning, since I think determining the minimal set of fetch columns is something that only the optimizer can do.

@RaduBerinde RaduBerinde added this to the 19.2 milestone Jun 19, 2019
@ridwanmsharif ridwanmsharif self-assigned this Jun 26, 2019
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 1, 2019
opt: fetch minimal set of columns on returning mutations
6fd7498 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
@ridwanmsharif ridwanmsharif mentioned this issue Jul 1, 2019
Merged
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 3, 2019
opt: fetch minimal set of columns on returning mutations
e300764 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
@RaduBerinde RaduBerinde added the E-starter Might be suitable for a starter project for new employees or team members. label Jul 8, 2019
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 9, 2019
opt: fetch minimal set of columns on returning mutations
f995bf4 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 9, 2019
opt: fetch minimal set of columns on returning mutations
5bf8375 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 11, 2019
opt: fetch minimal set of columns on returning mutations
fea7598 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 14, 2019
opt: fetch minimal set of columns on returning mutations
c4a93b6 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

This change passes down the minimal required return cols
to SQL so it knows to only return columns that's requested.
This fixes the output column calculation done by opt and
makes sure the execPlan's output columns are the same as
output cols of the opt plan.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 16, 2019
opt: fetch minimal set of columns on returning mutations
01059a7 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

This change passes down the minimal required return cols
to SQL so it knows to only return columns that's requested.
This fixes the output column calculation done by opt and
makes sure the execPlan's output columns are the same as
output cols of the opt plan.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
ridwanmsharif pushed a commit to ridwanmsharif/cockroach that referenced this issue Jul 16, 2019
opt: fetch minimal set of columns on returning mutations
5a44334 
Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

This change passes down the minimal required return cols
to SQL so it knows to only return columns that's requested.
This fixes the output column calculation done by opt and
makes sure the execPlan's output columns are the same as
output cols of the opt plan.

Fixes cockroachdb#30618.
Unblocks cockroachdb#30624.

Release note: None
craig bot pushed a commit that referenced this issue Jul 16, 2019
Merge #38594
8ee12cb 
38594: opt: fetch minimal set of columns on returning mutations r=ridwanmsharif a=ridwanmsharif

Previously, we used to fetch all columns when a mutation contained
a `RETURNING` clause. This is an issue because it forces us to
retrieve unnecessary data and creates extra contention.
This change adds logic to compute the minimal set of required columns
and fetches only those.

Fixes #30618.
Unblocks #30624.

Release note: None

Co-authored-by: Ridwan Sharif <[email protected]>
@craig craig bot closed this as completed in #38594 Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ridwanmsharif ridwanmsharif

Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. A-sql-optimizer SQL logical planning and optimizations. C-performance Perf of queries or internals. Solution not expected to change functional behavior. E-starter Might be suitable for a starter project for new employees or team members.
Projects
None yet
Milestone
19.2
Development

Successfully merging a pull request may close this issue.

opt: fetch minimal set of columns on returning mutations ridwanmsharif/cockroach
6 participants
@jordanlewis @knz @nvanbenschoten @RaduBerinde @ridwanmsharif @andy-kimball