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

colbuilder: introduce testing framework for meta objects tracking #64256

Open
yuzefovich opened this issue Apr 27, 2021 · 1 comment
Open

colbuilder: introduce testing framework for meta objects tracking #64256

yuzefovich opened this issue Apr 27, 2021 · 1 comment
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-queries SQL Queries Team

Comments

@yuzefovich
Copy link
Member

yuzefovich commented Apr 27, 2021

With the merge of #62221 we attempt to precisely attribute "meta" objects to their corresponding tree and try to hand off the responsibility of handling them once a component that is able to take over is created. However, we currently don't have any testing in place that would verify that this tracking is correct - we should add something.

One idea on how to add the testing of this is to introduce EXPLAIN (VEC, DEBUG) (or something similar) that would create the operator tree and would print out each "meta" object associated with the corresponding execinfra.OpNode.


For example, while working on another issue I noticed that we currently have at least one bug in this tracking. Consider the following scenario:

CREATE TABLE t (_timestamptz TIMESTAMPTZ, _interval INTERVAL, _bool BOOL);

EXPLAIN (VEC, VERBOSE) SELECT _timestamptz - _interval FROM t WHERE _bool;
                      info
-------------------------------------------------
  │
  └ Node 1
    └ *colexec.Materializer (1)
      └ *colexec.Columnarizer
        └ *rowexec.noopProcessor
          └ *colexec.Materializer (2)
            └ *colexecutils.BoolVecToSelOp
              └ *colexecutils.selBoolOp
                └ *colexecbase.simpleProjectOp
                  └ *colexecutils.CancelChecker
                    └ *colfetcher.ColBatchScan

Note that this plan contains two materializers (components that we try to give the responsibility over meta objects once the materializers are created). In this example ColBatchScan is at least one metadata source. Ideally, we would give the ownership over it to the materializer (2), but we currently give it to the root materializer (1). What's happening is that we have a Filterer core that we're able to vectorize which is followed by a PostProcessSpec with a render expression that we don't support. The latter is handled by planning via wrapPostProcessSpec which creates an artificial (and incomplete colexecargs.InputWithMetaInfo), so the materializer (2) doesn't get any meta objects to take over.


Here is an example why this imprecise tracking is problematic. Consider the same example as above but with the execution statistics being collected. At the moment of writing, the root materializer (1) will "own" two stats collectors (around the columnarizer and the around the colbatchscan). Now imagine that for some reason selBoolOp encounters a panic in its Init method. The panic will be caught by the materializer (2) which will transition into the draining state right away.

However, the panic is not propagated any further, so from the perspective of the root materializer (1), the initialization of its input Columnarizer succeeded, so the materializer (1) will happily first attempt to g roetws from the columnarizer (there will be none) and then will attempt to retrieve execution stats from both of "its owned" stats collectors. Retrieving the stats from the colbatchscan will fail because it wasn't properly initialized.

Once the tracking is fixed, the materializer (2) will own the stats collector around the colbatchscan and will not attempt to retrieve the stats because the materializer (2) knows that the initialization wasn't proper.

Jira issue: CRDB-6969

@yuzefovich yuzefovich added the C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. label Apr 27, 2021
@yuzefovich yuzefovich self-assigned this Apr 27, 2021
@yuzefovich yuzefovich changed the title colbuilder: introduce testing framework for meta object handling colbuilder: introduce testing framework for meta objects tracking Apr 27, 2021
craig bot pushed a commit that referenced this issue Apr 27, 2021
64006: Updated ALTER SEQUENCE, ALTER VIEW syntax diagrams r=ericharmeling a=ericharmeling

Release note: None

64258: colbuilder,colflow: fixes around stats collection of uninitialized components r=yuzefovich a=yuzefovich

**colbuilder: fix logging message when planning a filter expression**

Originally, a filter expression was handled by a wrapped row-execution
processor only when planning ON expression filter for inner merge and
hash joins, so the logging message mentioned the ON expr explicitly.
However, since then we reused the same code in a couple of other places,
so the message no longer made sense.

Release note: None

**colbuilder: fix passing of the responsibility over meta objects**

Previously, in some edge cases we didn't pass the responsibility over
the meta objects to the materializers that are created in order to wrap
row-execution processors into the vectorized flows. For example, if we
had a Filterer processor core that is supported by the vectorized
engine, but we had some render expressions that weren't supported, the
materializer created to wrap a noop processor (that would handle the
render expressions) wouldn't take over the responsibility of the meta
objects in the input tree to the filterer core. This is now fixed.

This commit is missing any regression testing since verifying that the
internal state is as expected is hard. I imagine that we might want to
introduce something like `EXPLAIN (VEC, DEBUG)` where we would annotate
the operator chains with some information about the meta objects
"assigned" to each operator. This seems non-trivial, however, so it is
left as a TODO.

Informs: #64256.

Release note: None

**colflow: handle uninitialized op gracefully during stats collection**

Previously, the vectorized stats collector wrapper would always attempt
to get stats from the wrapped operator. However, in some edge cases that
operator might not be properly initialized, and we could encounter a NPE
during the stats retrieval. Now the wrapper tracks whether Init was
successful and will get stats only if so. This strategy was chosen over
teaching each operator to handle uninitialized state itself since it is
cleaner this way and more extensible.

This commit additionally clarifies the VectorizedStatsCollector interface
a bit.

Fixes: #64180.
Fixes: #64192.
Fixes: #64193.
Fixes: #64232.
Fixes: #64236.

Release note: None

64284: docs/generated: remove unused bnfs r=ianjevans a=RichardJCai

Some bnfs were generated that no longer have corresponding
statements / entries in diagrams.go.

Release note: None

64287: diagrams: fix diagram for show_grants_stmt r=ianjevans a=RichardJCai

Previously the diagram was incorrect, specifically the replace
caused the bnf grammar to be invalid causing docgen svg to fail.

Release note: None

Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: richardjcai <[email protected]>
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@yuzefovich yuzefovich removed their assignment May 31, 2022
@mgartner mgartner moved this to New Backlog in SQL Queries Jul 24, 2023
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-queries SQL Queries Team
Projects
Status: Backlog
Development

No branches or pull requests

2 participants