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

colexec: wrap DrainMeta with panic-catcher and protect columnarizer #63108

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Apr 5, 2021

colexec: wrap DrainMeta with panic-catcher and protect columnarizer

Previously, in some edge cases (like when a panic is encountered during
Operator.Init) the metadata sources could have been uninitialized, so
when we tried to drain them, we'd encounter a crash. In order to avoid
that in the future, now all root components will wrap the draining with
the panic-catcher. Additionally, we now protect the columnarizer in this
case explicitly - if it wasn't initialized, it won't drain the wrapped
processor in DrainMeta.

Fixes: #62514.

Release note: None

rowexec: remove redundant implementations of MetadataSource interface

Previously, some row-by-row processors implemented
execinfra.MetadataSource interface. The idea behind that originally
was to allow for wrapped processors to return their metadata in the
vectorized flow, but nothing explicit is actually needed because every
wrapped processor has a columnarizer after it which will drain the
processor according to row-by-row model (by moving into draining state
and exhausting the trailing meta). This commit removes those redundant
implementations.

This allows us to move the interface into colexecop package where it
belongs.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich requested review from rytaft, michae2 and a team April 6, 2021 00:44
@yuzefovich
Copy link
Member Author

Note that the first commit fixes a GA-blocker (which I'm not sure whether it deserves such status), so I intend to backport the first commit to 21.1 branch (possibly for 21.1.0).

Copy link
Collaborator

@rytaft rytaft 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 5 of 5 files at r1, 30 of 30 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)


pkg/sql/colflow/routers.go, line 640 at r1 (raw file):

	if inputInitialized {
		// If we encountered a panic when initializing the input, the metadata
		// sources might be uninitialized, so we drain them only otherwise.

nit: "so we drain them only otherwise" -> "so we only drain them if initialized"


pkg/sql/colflow/routers.go, line 643 at r1 (raw file):

		// Although the DrainMeta call contains a panic-catcher, we would rather
		// not emit errors (which are likely to occur) in this case.
		r.bufferedMeta = append(r.bufferedMeta, r.metadataSources.DrainMeta(ctx)...)

is there no way to tell from the metadataSources directly whether they are initialized? Seems like this check could go inside the DrainMeta function (maybe before the panic catcher?)

@yuzefovich yuzefovich changed the title colexec: wrap DrainMeta with panic-catcher colexec: wrap DrainMeta with panic-catcher and protect columnarizer Apr 7, 2021
Previously, in some edge cases (like when a panic is encountered during
`Operator.Init`) the metadata sources could have been uninitialized, so
when we tried to drain them, we'd encounter a crash. In order to avoid
that in the future, now all root components will wrap the draining with
the panic-catcher. Additionally, we now protect the columnarizer in this
case explicitly - if it wasn't initialized, it won't drain the wrapped
processor in `DrainMeta`.

Release note: None
Previously, some row-by-row processors implemented
`execinfra.MetadataSource` interface. The idea behind that originally
was to allow for wrapped processors to return their metadata in the
vectorized flow, but nothing explicit is actually needed because every
wrapped processor has a columnarizer after it which will drain the
processor according to row-by-row model (by moving into draining state
and exhausting the trailing meta). This commit removes those redundant
implementations.

This allows us to move the interface into `colexecop` package where it
belongs.

Release note: None
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2 and @rytaft)


pkg/sql/colflow/routers.go, line 643 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

is there no way to tell from the metadataSources directly whether they are initialized? Seems like this check could go inside the DrainMeta function (maybe before the panic catcher?)

Good point. We already had protection in most places, and I updated the code to add the check to the columnarizer (the only place that I found to be missing the initialization check), so I removed this "input initialized" business.

Actually, doing just that should be sufficient to fix the issue, but I think that it is beneficial to keep the panic-catcher around draining for stability.

craig bot pushed a commit that referenced this pull request Apr 7, 2021
63108: colexec: wrap DrainMeta with panic-catcher and protect columnarizer r=yuzefovich a=yuzefovich

**colexec: wrap DrainMeta with panic-catcher and protect columnarizer**

Previously, in some edge cases (like when a panic is encountered during
`Operator.Init`) the metadata sources could have been uninitialized, so
when we tried to drain them, we'd encounter a crash. In order to avoid
that in the future, now all root components will wrap the draining with
the panic-catcher. Additionally, we now protect the columnarizer in this
case explicitly - if it wasn't initialized, it won't drain the wrapped
processor in `DrainMeta`.

Fixes: #62514.

Release note: None

**rowexec: remove redundant implementations of MetadataSource interface**

Previously, some row-by-row processors implemented
`execinfra.MetadataSource` interface. The idea behind that originally
was to allow for wrapped processors to return their metadata in the
vectorized flow, but nothing explicit is actually needed because every
wrapped processor has a columnarizer after it which will drain the
processor according to row-by-row model (by moving into draining state
and exhausting the trailing meta). This commit removes those redundant
implementations.

This allows us to move the interface into `colexecop` package where it
belongs.

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig
Copy link
Contributor

craig bot commented Apr 7, 2021

Build failed:

@yuzefovich
Copy link
Member Author

A flake.

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 7, 2021

Build succeeded:

@craig craig bot merged commit fe1327d into cockroachdb:master Apr 7, 2021
@yuzefovich yuzefovich deleted the drain-meta-catch branch April 27, 2021 01:35
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.

roachtest: sqlsmith: nil ctx when draining the vectorized flow
3 participants