rowcontainer: support concurrent reads and some cleanup#96836
Closed
yuzefovich wants to merge 1 commit intocockroachdb:masterfrom
Closed
rowcontainer: support concurrent reads and some cleanup#96836yuzefovich wants to merge 1 commit intocockroachdb:masterfrom
yuzefovich wants to merge 1 commit intocockroachdb:masterfrom
Conversation
Member
75b70d1 to
3fd604a
Compare
This commit refactors the row container infrastructure in order to make it possible to read from a single container concurrently (via separate iterators). It required moving some of the scratch space from the container directly into the iterators (so that the iterators don't share anything between each other) as well as making a similar adjustment to the `pebbleMapIterator`. As a result, it is now possible to safely concurrently iterate over all types of row containers; however, the iterators need to be created and closed under mutex protection (which seems like an acceptable limitation). This commit also makes a minor change to make the container a named parameter inside of the iterator (which makes it easier to see when an iterator is touching the container). The ability to read from the same row container (after all rows have been added to it) concurrently will be utilized by the follow-up commit in order to support validation of multiple FK and UNIQUE checks in parallel. Additionally, this commit improves `RowIterator` interface a bit by renaming `Row` to `EncRow` (since the method returns `EncDatumRow`s) and introducing a new `Row` implementation which returns `tree.Datums`. The new method is now utilized in the `sql.rowContainerIterator` which removes some of the unnecessary conversions between different row types. Release note: None
3fd604a to
8b4d676
Compare
Member
Author
|
This is RFAL. I intend to include this commit as the first one in #96123 and merge all commits together to be backported to 22.2. |
rharding6373
approved these changes
Feb 13, 2023
Collaborator
rharding6373
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @cucaroach and @msirek)
msirek
approved these changes
Feb 13, 2023
Contributor
msirek
left a comment
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status:complete! 2 of 0 LGTMs obtained (waiting on @cucaroach)
Member
Author
|
TFTRs! I already included this commit in #96123. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit refactors the row container infrastructure in order to make
it possible to read from a single container concurrently (via separate
iterators). It required moving some of the scratch space from the
container directly into the iterators (so that the iterators don't share
anything between each other) as well as making a similar adjustment to
the
pebbleMapIterator. As a result, it is now possible to safelyconcurrently iterate over all types of row containers; however, the
iterators need to be created and closed under mutex protection (which
seems like an acceptable limitation). This commit also makes a minor
change to make the container a named parameter inside of the iterator
(which makes it easier to see when an iterator is touching the
container).
The ability to read from the same row container (after all rows have
been added to it) concurrently will be utilized by the follow-up commit
in order to support validation of multiple FK and UNIQUE checks in
parallel.
Additionally, this commit improves
RowIteratorinterface a bit byrenaming
RowtoEncRow(since the method returnsEncDatumRows) andintroducing a new
Rowimplementation which returnstree.Datums. Thenew method is now utilized in the
sql.rowContainerIteratorwhichremoves some of the unnecessary conversions between different row types.
Informs: #95184.
Release note: None