sql/opt: propagate row-level locking mode to index, lookup, inverted, and zigzag joins#60719
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
The opt part looks good, except that we should add the locking info in expr_format.go and add some xform tests.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/sql/opt/ops/relational.opt, line 291 at r1 (raw file):
# retrieval of DELETE and UPDATE statements. The locking item's Targets list # will always be empty when part of a IndexJoinPrivate. Locking LockingItem
It seems that we only want the Strength and Wait policy here. I'd consider splitting those into a separate struct and using that.
This commit separates an unresolved row-level locking target from a new row-level physical property struct. The former is used to represent a row-level locking clause that is then "resolved" to specific relational operators. Once attached to a specific operator, the locking mode no longer needs to include a target, so it makes sense to represent as a separate struct. This addresses Radu's comment from cockroachdb#60719. I think it's correct to call row-level locking a "physical property", but I could be wrong about that.
This commit separates an unresolved row-level locking target from a new row-level locking properties struct. The former is used to represent a row-level locking clause that is then "resolved" to specific relational operators. Once attached to a specific operator, the locking mode no longer needs to include a target, so it makes sense to represent as a separate struct. This addresses Radu's comment from cockroachdb#60719.
This commit separates an unresolved row-level locking target from a new row-level locking properties struct. The former is used to represent a row-level locking clause that is then "resolved" to specific relational operators. Once attached to a specific operator, the locking mode no longer needs to include a target, so it makes sense to represent as a separate struct. This addresses Radu's comment from cockroachdb#60719.
79679: sql/opt: split out row-level locking properties struct r=mgartner a=nvanbenschoten This commit separates an unresolved row-level locking target from a new row-level physical property struct. The former is used to represent a row-level locking clause that is then "resolved" to specific relational operators. Once attached to a specific operator, the locking mode no longer needs to include a target, so it makes sense to represent it as a separate struct. This addresses `@RaduBerinde's` comment from #60719. I think it's correct to call row-level locking a "physical property", but I could be wrong about that. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
e23c168 to
69dcf35
Compare
nvb
left a comment
There was a problem hiding this comment.
Thanks for the review Radu. Apologies for letting this sit for so long. I've revived it and addressed your two comments.
The opt part looks good, except that we should add the locking info in expr_format.go and add some xform tests.
Done.
Reviewable status:
pkg/sql/opt/ops/relational.opt, line 291 at r1 (raw file):
Previously, RaduBerinde wrote…
It seems that we only want the Strength and Wait policy here. I'd consider splitting those into a separate struct and using that.
Done in #79679.
69dcf35 to
0616bf9
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
I think the only remaining operator that talks to KV that doesn't yet support locking is zigzag join - do we want to add the plumbing there too?
Reviewed 18 of 18 files at r2.
Reviewable status:
0616bf9 to
3351795
Compare
3351795 to
b6253d6
Compare
Done. I added support for SFU propagation into zigzag joins in a second commit. |
yuzefovich
left a comment
There was a problem hiding this comment.
Thanks!
Reviewed 11 of 11 files at r3, 15 of 15 files at r4.
Reviewable status:
pkg/sql/opt/xform/rules/select.opt, line 62 at r4 (raw file):
# planned. # # Zigzag joins are prohibited when the source Scan operator has been configured
nit: the comment needs an adjustment.
pkg/sql/opt/xform/rules/select.opt, line 79 at r4 (raw file):
# at any two of the JSON paths we are querying for. # # Zigzag joins are prohibited when the source Scan operator has been configured
nit: ditto.
mgartner
left a comment
There was a problem hiding this comment.
Very nice!
Reviewed 8 of 18 files at r2, 11 of 11 files at r3, 15 of 15 files at r4.
Reviewable status:
pkg/sql/opt/ops/relational.opt, line 648 at r4 (raw file):
# conflicting locks. LeftLocking Locking RightLocking Locking
A zigzag join always operates on two indexes of the same table, so these locking modes should always be the same. Can we make it a single field instead of two, then?
pkg/sql/opt/optbuilder/testdata/select_for_update, line 1005 at r3 (raw file):
# ------------------------------------------------------------------------------ # Tests with lookup joins.
I think a better place for these would be pkg/sql/opt/xform/testdata/rules/join and .../xform/testdata/rules/select, in the sections with the associated rule tests, e.g. GeneratePartialIndexScans, GenerateConstrainedScans, GenerateInvertedIndexScans, GenerateLookupJoins, GenerateInvertedJoins. Then, it'll be more nature to use the opt directive instead of the build directive so the test will actually show the join and the correct locking. Alternatively, you could make a new test file like xform/testdata/rules/for_update_locking for these tests if you think they're better kept together.
Also, it be good to add test cases when GenerateLookupJoins and GenerateInvertedJoins build a lookup/inverted join and an index join (because the lookup index is not covering).
…ted joins Fixes cockroachdb#56941. This commit updates the execbuilder to propagate row-level locking modes through transformations from standard Scan and Join operations to specialized IndexJoin, LookupJoin, and InvertedJoin operations. Release note (sql change): table scans performed as a part of index joins, lookup joins, and inverted joins now respect the row-level locking strength and wait policy specified by the optional FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
This commit updates the execbuilder to propagate row-level locking modes through transformations from standard Scan and Join operations to ZigZagJoins, and allows for the use of zigzag joins when explicit row-level locking modes are in use. Release note (sql change): table scans performed as a part of zigzag joins now respect the row-level locking strength and wait policy specified by the optional FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
b6253d6 to
884af54
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/sql/opt/ops/relational.opt, line 648 at r4 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
A zigzag join always operates on two indexes of the same table, so these locking modes should always be the same. Can we make it a single field instead of two, then?
Do we want to restrict ourselves to this limited form of zigzag joins indefinitely? My reading of the rest of this struct is that we'd like to keep it as general as possible (with the caveat that it still assumes only 2 join sides) and minimize the places where we assume the tables are the same. The best example of that is the LeftTable and RightTable fields in ZigzagJoinPrivate, which are always identical for the current use of zigzag joins.
pkg/sql/opt/xform/rules/select.opt, line 62 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: the comment needs an adjustment.
Done.
pkg/sql/opt/xform/rules/select.opt, line 79 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: ditto.
Done.
pkg/sql/opt/optbuilder/testdata/select_for_update, line 1005 at r3 (raw file):
Alternatively, you could make a new test file like xform/testdata/rules/for_update_locking for these tests if you think they're better kept together.
Done in a separate commit. I think it's nice to keep the for_update tests together because we already have a lot of coverage for join rules and the primary thing we want to verify in all of these tests is that row-level locking properties propagate down into scans.
Also, it be good to add test cases when GenerateLookupJoins and GenerateInvertedJoins build a lookup/inverted join and an index join (because the lookup index is not covering).
Good idea. This was already being tested in the inverted join case, but not in the lookup join case. Done.
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 17 files at r5, 13 of 15 files at r6, 1 of 1 files at r7.
Reviewable status:
pkg/sql/opt/ops/relational.opt, line 648 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we want to restrict ourselves to this limited form of zigzag joins indefinitely? My reading of the rest of this struct is that we'd like to keep it as general as possible (with the caveat that it still assumes only 2 join sides) and minimize the places where we assume the tables are the same. The best example of that is the
LeftTableandRightTablefields inZigzagJoinPrivate, which are always identical for the current use of zigzag joins.
Good point, I wasn't thinking about the possibility of multi-table zig zag joins. Let's leave as-is!
pkg/sql/opt/optbuilder/testdata/select_for_update, line 1005 at r3 (raw file):
the primary thing we want to verify in all of these tests is that row-level locking properties propagate down into scans.
Ahh I misunderstood. There's two things we're testing:
- That modes propagate down into scans during optbuilding. These belong in optbuilder tests, and is covered by these tests you originally added. I think the titles like "Tests with lookup joins" confused me — something like "Tests with joins" might be better?
- That lookup, index, and inverted joins inherit the locking modes of the scans they are derived from. These are the cases that I thought should be in xform tests since xform generates theses operators. It sounds like you were originally covering this in the execbuilder tests. In that case, apologies for the extra work I created for you. There's some overlap in the xform tests you added, but I think that's ok.
pkg/sql/opt/xform/testdata/rules/select_for_update, line 14 at r7 (raw file):
# ------------------------------------------------------------------------------ # Basic tests.
To me it'd make the most sense for this file to only include cases related to exploration — tests that ensure that lookup, index, and inverted joins have the correct locking modes. All other tests sound like they are verifying that locking modes are pushed down correctly during the optbuild phase, so I think they should go there.
This commit adds a new `pkg/sql/opt/xform/testdata/rules/select_for_update` test to `pkg/sql/opt/xform`'s TestRules test suite. This allows the select for update tests to use the `opt` directive to show joins, assert the use of specific rules, and assert the propagation of row-level locking modes into joins.
884af54 to
75ac30c
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/testdata/select_for_update, line 1005 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
the primary thing we want to verify in all of these tests is that row-level locking properties propagate down into scans.
Ahh I misunderstood. There's two things we're testing:
- That modes propagate down into scans during optbuilding. These belong in optbuilder tests, and is covered by these tests you originally added. I think the titles like "Tests with lookup joins" confused me — something like "Tests with joins" might be better?
- That lookup, index, and inverted joins inherit the locking modes of the scans they are derived from. These are the cases that I thought should be in xform tests since xform generates theses operators. It sounds like you were originally covering this in the execbuilder tests. In that case, apologies for the extra work I created for you. There's some overlap in the xform tests you added, but I think that's ok.
That's well put. These xform tests are specifically interested in #2, because that can't be tested in the optbuilder tests. They were already exercised to some degree in the execbuider tests, but that was a little less direct so I'm glad you encouraged me to add this test.
pkg/sql/opt/xform/testdata/rules/select_for_update, line 14 at r7 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
To me it'd make the most sense for this file to only include cases related to exploration — tests that ensure that lookup, index, and inverted joins have the correct locking modes. All other tests sound like they are verifying that locking modes are pushed down correctly during the optbuild phase, so I think they should go there.
Good point. I've stripped this down to just the set of tests related to exploration rules.
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 17 files at r5, 2 of 15 files at r6, 1 of 1 files at r8.
Reviewable status:
pkg/sql/opt/optbuilder/testdata/select_for_update line 1005 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
That's well put. These xform tests are specifically interested in #2, because that can't be tested in the optbuilder tests. They were already exercised to some degree in the execbuider tests, but that was a little less direct so I'm glad you encouraged me to add this test.
👍
|
TFTRs! bors r+ |
|
Build succeeded: |
|
Any chance we could backport this? At least to 22.1? |
This commit separates an unresolved row-level locking target from a new row-level locking properties struct. The former is used to represent a row-level locking clause that is then "resolved" to specific relational operators. Once attached to a specific operator, the locking mode no longer needs to include a target, so it makes sense to represent as a separate struct. This addresses Radu's comment from cockroachdb#60719.
Fixes #56941.
The first commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.
The second commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
ZigZagJoins, and allows for the use of zigzag joins when explicit
row-level locking modes are in use.
Release note (sql change): table scans performed as a part of index
joins, lookup joins, and inverted joins now respect the row-level
locking strength and wait policy specified by the optional
FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
Release note (sql change): table scans performed as a part of zigzag
joins now respect the row-level locking strength and wait policy
specified by the optional FOR SHARE/UPDATE [NOWAIT] clause on SELECT
statements.