Skip to content

rowcontainer: support concurrent reads and some cleanup#96836

Closed
yuzefovich wants to merge 1 commit intocockroachdb:masterfrom
yuzefovich:rowcontainer
Closed

rowcontainer: support concurrent reads and some cleanup#96836
yuzefovich wants to merge 1 commit intocockroachdb:masterfrom
yuzefovich:rowcontainer

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

@yuzefovich yuzefovich commented Feb 9, 2023

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 EncDatumRows) 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.

Informs: #95184.

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

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
@yuzefovich yuzefovich marked this pull request as ready for review February 13, 2023 22:23
@yuzefovich yuzefovich requested review from a team as code owners February 13, 2023 22:23
@yuzefovich yuzefovich requested review from cucaroach, mgartner, msirek, rharding6373 and sumeerbhola and removed request for mgartner and sumeerbhola February 13, 2023 22:23
@yuzefovich
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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 @cucaroach and @msirek)

Copy link
Copy Markdown
Contributor

@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.

:lgtm:

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @cucaroach)

@yuzefovich
Copy link
Copy Markdown
Member Author

TFTRs!

I already included this commit in #96123.

@yuzefovich yuzefovich closed this Feb 13, 2023
@yuzefovich yuzefovich deleted the rowcontainer branch February 13, 2023 23:56
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.

4 participants