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.

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

sql: add new check relation to help build insert fast path uniqueness checks #111403

Merged
merged 3 commits into from Oct 4, 2023

Conversation

msirek
Copy link
Contributor

@msirek msirek commented Sep 28, 2023

sql: add new check relation to help build insert fast path uniqueness checks

This creates a FastPathUniqueChecks list as a child of InsertExpr,
similar to UniqueChecks, but consisting of relational expressions
which can be used to build a fast path uniqueness check using a
particular index. Each item in the FastPathUniqueChecks list is a
FastPathUniqueChecksItem whose Check expression, if the unique
constraint is provisionally eligible to use a fast path uniqueness
check, is a filtered scan from the target table of an
INSERT INTO ... VALUES statement. It is only built if the
VALUES clause has a single row. The filter equates each of the unique
constraint key columns with its corresponding value from the VALUES
clause. The values may either be a ValuesExpr or a WithScanExpr
whose source is a ValuesExpr. During exploration,
GenerateConstrainedScans will find all valid constrained index scans
which are equivalent to the original filtered Scan. The built relations
are not executed or displayed in the EXPLAIN, but will be analyzed in a
later commit to specify the index and index key values to use for insert
fast path uniqueness checks.

FastPathUniqueChecksItemPrivate is added with elements which are
designed to communicate details of how to build a fast path uniqueness
check to the execbuilder.

Epic: CRDB-26290
Informs: #58047

Release note: None


memo: optbuild unit tests for fast path unique check exprs

This commit adds the ExprFmtHideFastPathChecks flag, which hides fast
path check expressions from the output of expression formatting. It is
used in all places except TestBuilder. Also, unit tests for
buildFiltersForFastPathCheck are added.

Epic: CRDB-26290
Informs: #58047

Release note: None


xform: opt test for insert fast path checks

This commit adds an insert transformation rules test which
displays optimized fast path checks which are built in the optbuilder
and later explored and optimized. This test is modified in a later
commit to test for rule firing.

Epic: CRDB-26290
Informs: #58047

Release note: None

@msirek msirek requested a review from a team as a code owner September 28, 2023 03:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@msirek msirek force-pushed the insert-fast-path-check-expression branch 2 times, most recently from 04cdf4a to 9129400 Compare September 28, 2023 06:59
Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice work!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @msirek)


pkg/sql/opt/norm/rules/prune_cols.opt line 519 at r1 (raw file):

# Removing ReturnCols will then allow the PruneMutationFetchCols to be more
# conservative with the fetch columns.
# TODO(ridwanmsharif): Mutations shouldn't need to return the primary key

optional nit: Could you turn this into an issue and refer to the issue number here? I don't think Ridwan has worked here in awhile...

@msirek
Copy link
Contributor Author

msirek commented Oct 3, 2023

@mgartner Do you think you might have any comments on this PR beyond those addressed in #102995? If not, I may merge this and open a new PR for subsequent commits.

Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 15 of 15 files at r1, 13 of 13 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @msirek)


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 71 at r2 (raw file):

 └── fast-path-unique-checks
      ├── fast-path-unique-checks-item: &{0 0 [] [] {  record best-effort}}
      │    └── values

nit: This output could be more clear what it represents. Do these represent empty fast-path-unique-checks that could not be constructed? Or are the values simply not being formatted fully? If you want to fix this up in a separate PR, that's fine.


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 427 at r2 (raw file):

 │                        └── k:27 != uniq.k:20
 └── fast-path-unique-checks
      └── fast-path-unique-checks-item: &{0 0 [] [] {  record best-effort}}

nit: It'd be helpful to include a representation of the unique constraint here, like we do for unique-checks-item, e.g., uniq(x,y). Feel free to address in a future PR, if you'd like to do this.


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 2161 at r2 (raw file):

# column use a projection to compute the computed column value. Since it
# is not merged with the ValuesExpr in TestBuilder, UNIQUE WITHOUT INDEX
# constraints involving the computed column are not built as fast path checks.

nit: I see all 4 fast path checks built below. Is this comment still accurate?


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 2460 at r2 (raw file):

----

# Test fast path check building from a WithScanExpr referencing values.

Is this comment still accurate? All the values have been inlined in unique-checks below, so there are no WithScanExprs AFAICT. I agree we should have a test for that though—I don't see any here.


pkg/sql/opt/xform/testdata/rules/insert line 73 at r3 (raw file):

# A computed leading index column allows fast path uniqueness checks.
# Note, the best-cost fast path check relation selected may not be one
# which would enable fast path uniqueness checks.

Is this referring to the first fast-path-check with a non-true index join filter?

Copy link
Contributor Author

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/opt/norm/rules/prune_cols.opt line 519 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

optional nit: Could you turn this into an issue and refer to the issue number here? I don't think Ridwan has worked here in awhile...

Done


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 427 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: It'd be helpful to include a representation of the unique constraint here, like we do for unique-checks-item, e.g., uniq(x,y). Feel free to address in a future PR, if you'd like to do this.

Made the change


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 2161 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: I see all 4 fast path checks built below. Is this comment still accurate?

Fixed up the comment


pkg/sql/opt/xform/testdata/rules/insert line 73 at r3 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is this referring to the first fast-path-check with a non-true index join filter?

Yes, the best-cost relation in this case picked the t_r_c_idx index for UNIQUE WITHOUT INDEX (a). That's why matching has to walk the equivalent relations in the memo group and use one that doesn't have a Select.

@msirek
Copy link
Contributor Author

msirek commented Oct 4, 2023

Is this comment still accurate? All the values have been inlined in unique-checks below, so there are no WithScanExprs AFAICT. I agree we should have a test for that though—I don't see any here.

Updated the comment to indicate this case has a foreign key. A WithScanExpr case is on line 1965. I've updated the comment for that test case.

Copy link
Contributor Author

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)


pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 71 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: This output could be more clear what it represents. Do these represent empty fast-path-unique-checks that could not be constructed? Or are the values simply not being formatted fully? If you want to fix this up in a separate PR, that's fine.

Valid expressions here are only ever things like Select, Scan, IndexJoin.
Values is the dummy statement put in there when we can't build a fast-path expression.
We can't avoid printing values unless we hijack this general-purpose code:

for i, n := 0, scalar.ChildCount(); i < n; i++ {
f.formatExpr(scalar.Child(i), tp)
}

One possibility is, instead of printing something like this:

       └── fast-path-unique-checks-item: t(b)
            └── values

Print this:

       └── fast-path-unique-checks-item: fast path disabled for constraint t(b)
            └── values

What do you think? I might not hold up this PR for this small change. I can add another commit later if we think a change is in order.

@msirek msirek force-pushed the insert-fast-path-check-expression branch 2 times, most recently from 6fac8e4 to f7f875e Compare October 4, 2023 15:20
Mark Sirek added 3 commits October 4, 2023 08:37
… checks

This creates a `FastPathUniqueChecks` list as a child of `InsertExpr`,
similar to `UniqueChecks`, but consisting of relational expressions
which can be used to build a fast path uniqueness check using a
 particular index. Each item in the `FastPathUniqueChecks` list is a
`FastPathUniqueChecksItem` whose `Check` expression, if the unique
constraint is provisionally eligible to use a fast path uniqueness
check, is a filtered scan from the target table of an
`INSERT INTO ... VALUES` statement. It is only built if the
VALUES clause has a single row. The filter equates each of the unique
constraint key columns with its corresponding value from the VALUES
clause. The values may either be a `ValuesExpr` or a `WithScanExpr`
whose source is a `ValuesExpr`. During exploration,
GenerateConstrainedScans will find all valid constrained index scans
which are equivalent to the original filtered Scan. The built relations
are not executed or displayed in the EXPLAIN, but will be analyzed in a
later commit to specify the index and index key values to use for insert
fast path uniqueness checks.

FastPathUniqueChecksItemPrivate is added with elements which are
designed to communicate details of how to build a fast path uniqueness
check to the execbuilder.

Epic: CRDB-26290
Informs: cockroachdb#58047

Release note: None
This commit adds the `ExprFmtHideFastPathChecks` flag, which hides fast
path check expressions from the output of expression formatting. It is
used in all places except TestBuilder. Also, unit tests for
`buildFiltersForFastPathCheck` are added.

Epic: CRDB-26290
Informs: cockroachdb#58047

Release note: None
This commit adds an insert transformation rules test which
displays optimized fast path checks which are built in the optbuilder
and later explored and optimized. This test is modified in a later
commit to test for rule firing.

Epic: CRDB-26290
Informs: cockroachdb#58047

Release note: None
@msirek msirek force-pushed the insert-fast-path-check-expression branch from f7f875e to b7468bb Compare October 4, 2023 15:37
@msirek
Copy link
Contributor Author

msirek commented Oct 4, 2023

TFTRs!
bors r+

@craig
Copy link
Contributor

craig bot commented Oct 4, 2023

Build succeeded:

@craig craig bot merged commit 69f0369 into cockroachdb:master Oct 4, 2023
8 checks passed
@mgartner
Copy link
Collaborator

mgartner commented Oct 5, 2023

pkg/sql/opt/optbuilder/testdata/unique-checks-insert line 71 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

Valid expressions here are only ever things like Select, Scan, IndexJoin.
Values is the dummy statement put in there when we can't build a fast-path expression.
We can't avoid printing values unless we hijack this general-purpose code:

for i, n := 0, scalar.ChildCount(); i < n; i++ {
f.formatExpr(scalar.Child(i), tp)
}

One possibility is, instead of printing something like this:

       └── fast-path-unique-checks-item: t(b)
            └── values

Print this:

       └── fast-path-unique-checks-item: fast path disabled for constraint t(b)
            └── values

What do you think? I might not hold up this PR for this small change. I can add another commit later if we think a change is in order.

Can we avoid creating a FastPathUniqueChecksItem in the FastPathUniqueChecks slice if its not possible? Then we don't need a dummy expression.

@msirek
Copy link
Contributor Author

msirek commented Oct 5, 2023

Can we avoid creating a FastPathUniqueChecksItem in the FastPathUniqueChecks slice if its not possible? Then we don't need a dummy expression.

Per offline discussion, added a 2nd commit in #111822 to build no FastPathUniqueChecks if any of the required checks could not be built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants