sql: add nodes for each EXPLAIN ANALYZE operator #60550

RaduBerinde · 2021-02-12T23:42:07Z

Show the cluster nodes involved in the execution of each operator.

Note that this info does not show up in the non-analyze EXPLAIN. It is
technically much more challenging to do that because of the indirect
way we do distsql planning. Once we have the new DistSQL exec factory,
we will be able to add it.

Note on the implementation: I started by trying to make execution set
ComponentID.NodeID in all cases, but I got stuck in ProcessorBase
where we only have a SQLIDContainer available. I don't fully
understand the new abstraction and whether the distsql components and
flows should really use SQLIDs instead of NodeIDs.

Unfortunately, there is not much we can do to test this currently
(other than manual testing). I will investigate making the
"deterministic" flag more fine-grained, so that we can hide truly
non-deterministic values (like timings) separately from those that
just vary with the configuration (query distribution).

Example:

movr> EXPLAIN ANALYZE SELECT * FROM rides JOIN vehicles ON rides.vehicle_id = vehicles.id;
                    info
--------------------------------------------
  planning time: 158µs
  execution time: 7ms
  distribution: full
  vectorized: true

   hash join
  │ cluster nodes: n1, n2, n3
  │ actual row count: 500
  │ equality: (vehicle_id) = (id)
  │
  ├──  scan
  │     cluster nodes: n1, n2, n3
  │     actual row count: 500
  │     KV rows read: 500
  │     KV bytes read: 86 KiB
  │     missing stats
  │     table: rides@primary
  │     spans: FULL SCAN
  │
  └──  scan
        cluster nodes: n1, n2, n3
        actual row count: 15
        KV rows read: 15
        KV bytes read: 2.3 KiB
        missing stats
        table: vehicles@primary
        spans: FULL SCAN

Release note (sql change): EXPLAIN ANALYZE now includes the nodes
which were involved in the execution of each operator in the tree.

Informs #59860.

RaduBerinde · 2021-02-12T23:42:14Z

CC @awoods187

cockroach-teamcity · 2021-02-12T23:42:14Z

This change is

asubiotto

Note on the implementation: I started by trying to make execution set
ComponentID.NodeID in all cases, but I got stuck in ProcessorBase
where we only have a SQLIDContainer available. I don't fully
understand the new abstraction and whether the distsql components and
flows should really use SQLIDs instead of NodeIDs.

SQLIDContainer is just the node ID and you can treat it as such (by casting). The end goal is to remove all notion of NodeIDs and just have sql instance IDs (#49596).

Reviewed 10 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

RaduBerinde · 2021-02-16T17:21:06Z

I see. I tried working on it some more but the change is getting pretty big, we need to plumb the node ID in other places (e.g. diagram data). I think I'd rather merge this as-is for now.

awoods187 · 2021-02-16T17:31:18Z

This looks great and I'll take it in explain analyze for now. We can add to explain in the future as you said.

asubiotto · 2021-02-16T17:45:32Z

LGTM

RaduBerinde · 2021-02-16T18:13:29Z

Actually, I think I figured out the NodeID stuff, PTAL.

asubiotto

Reviewed 14 of 14 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

pkg/sql/execinfra/flow_context.go, line 120 at r2 (raw file):

// The stream must originate from the node associated with this FlowCtx.
func (ctx *FlowCtx) StreamComponentID(streamID execinfrapb.StreamID) execinfrapb.ComponentID {
	// TODO(radu): the component stats should store SQLInstanceID instead.

nit: Could we do the inverse and create a sql instance ID from places where we have a nodeID?

pkg/sql/flowinfra/outbox_test.go, line 570 at r2 (raw file):

					NodeDialer: nodedialer.New(clientRPC, staticAddressResolver(addr)),
				},
				NodeID: base.NewSQLIDContainer(1, nil /* nodeID */),

nit: maybe make a TestingSQLIDContainer function with no args that returns this to avoid repeating these calls

Show the cluster nodes involved in the execution of each operator. Note that this info does not show up in the non-analyze EXPLAIN. It is technically much more challenging to do that because of the indirect way we do distsql planning. Once we have the new DistSQL exec factory, we will be able to add it. To implement this, we make execution set `ComponentID.NodeID` in all cases. Unfortunately, there is not much we can do to test this currently (other than manual testing). I will investigate making the "deterministic" flag more fine-grained, so that we can hide truly non-deterministic values (like timings) separately from those that just vary with the configuration (query distribution). Example: ``` movr> EXPLAIN ANALYZE SELECT * FROM rides JOIN vehicles ON rides.vehicle_id = vehicles.id; info -------------------------------------------- planning time: 158µs execution time: 7ms distribution: full vectorized: true hash join │ cluster nodes: n1, n2, n3 │ actual row count: 500 │ equality: (vehicle_id) = (id) │ ├── scan │ cluster nodes: n1, n2, n3 │ actual row count: 500 │ KV rows read: 500 │ KV bytes read: 86 KiB │ missing stats │ table: rides@primary │ spans: FULL SCAN │ └── scan cluster nodes: n1, n2, n3 actual row count: 15 KV rows read: 15 KV bytes read: 2.3 KiB missing stats table: vehicles@primary spans: FULL SCAN ``` Release note (sql change): EXPLAIN ANALYZE now includes the nodes which were involved in the execution of each operator in the tree.

RaduBerinde

TFTR!

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

pkg/sql/execinfra/flow_context.go, line 120 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: Could we do the inverse and create a sql instance ID from places where we have a nodeID?

Sorry, I'm not sure I follow.. The TODO is saying that ComponentID should store SQLInstanceID, so that we would need to obtain or pass that instead of NodeID; this does mean that in some places, at least until all the SQLInstanceID plumbing is done, we'd need to create the sql instance ID from the node ID.

pkg/sql/flowinfra/outbox_test.go, line 570 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: maybe make a TestingSQLIDContainer function with no args that returns this to avoid repeating these calls

Done, apparently there was already one defined :)

asubiotto

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto and @RaduBerinde)

pkg/sql/execinfra/flow_context.go, line 120 at r2 (raw file):

Previously, RaduBerinde wrote…

Sorry, I'm not sure I follow.. The TODO is saying that ComponentID should store SQLInstanceID, so that we would need to obtain or pass that instead of NodeID; this does mean that in some places, at least until all the SQLInstanceID plumbing is done, we'd need to create the sql instance ID from the node ID.

Sorry the thought wasn't complete. What I meant is that we could already make ComponentID store SQLInstanceID and instead of converting instance ids to node ids like we do here, convert node ids to instance ids in the other sites. Just a thought to make the future plumbing easier.

RaduBerinde

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

pkg/sql/execinfra/flow_context.go, line 120 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Sorry the thought wasn't complete. What I meant is that we could already make ComponentID store SQLInstanceID and instead of converting instance ids to node ids like we do here, convert node ids to instance ids in the other sites. Just a thought to make the future plumbing easier.

Oh, that's exactly what the TODO said. Done (as a separate commit).

Switch to using SQLInstanceID instead of NodeID in ComponentStats. Eventually, the same change should happen in the exec protos (e.g. StreamEndpointSpec). Release note: None

RaduBerinde · 2021-02-19T18:46:06Z

bors r+

craig · 2021-02-19T20:05:48Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-02-19T20:38:12Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde requested a review from asubiotto February 12, 2021 23:42

RaduBerinde requested a review from a team as a code owner February 12, 2021 23:42

RaduBerinde force-pushed the explain-nodes branch from e82b712 to 6cdaeda Compare February 13, 2021 19:10

asubiotto reviewed Feb 16, 2021

View reviewed changes

RaduBerinde force-pushed the explain-nodes branch from 6cdaeda to e0fa95c Compare February 16, 2021 18:13

RaduBerinde force-pushed the explain-nodes branch from e0fa95c to 1a74e20 Compare February 16, 2021 20:21

asubiotto approved these changes Feb 17, 2021

View reviewed changes

RaduBerinde force-pushed the explain-nodes branch from 1a74e20 to a24fa86 Compare February 17, 2021 19:35

RaduBerinde commented Feb 17, 2021

View reviewed changes

asubiotto reviewed Feb 18, 2021

View reviewed changes

RaduBerinde commented Feb 18, 2021

View reviewed changes

RaduBerinde force-pushed the explain-nodes branch from a39695d to 607a6ef Compare February 18, 2021 18:35

sql: use SQLInstanceID in ComponentStats

878bf45

Switch to using SQLInstanceID instead of NodeID in ComponentStats. Eventually, the same change should happen in the exec protos (e.g. StreamEndpointSpec). Release note: None

RaduBerinde force-pushed the explain-nodes branch from 607a6ef to 878bf45 Compare February 18, 2021 19:34

craig bot merged commit 0e7cfdb into cockroachdb:master Feb 19, 2021

RaduBerinde mentioned this pull request Feb 23, 2021

opt: add node and region information to explain #59860

Closed

RaduBerinde deleted the explain-nodes branch February 23, 2021 17:42

jseldess mentioned this pull request Mar 23, 2021

sql: add nodes for each EXPLAIN ANALYZE operator cockroachdb/docs#10102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add nodes for each EXPLAIN ANALYZE operator #60550

sql: add nodes for each EXPLAIN ANALYZE operator #60550

RaduBerinde commented Feb 12, 2021 •

edited

Loading

RaduBerinde commented Feb 12, 2021

cockroach-teamcity commented Feb 12, 2021

asubiotto left a comment

RaduBerinde commented Feb 16, 2021

awoods187 commented Feb 16, 2021

asubiotto commented Feb 16, 2021

RaduBerinde commented Feb 16, 2021

asubiotto left a comment

RaduBerinde left a comment

asubiotto left a comment

RaduBerinde left a comment

RaduBerinde commented Feb 19, 2021

craig bot commented Feb 19, 2021

craig bot commented Feb 19, 2021

sql: add nodes for each EXPLAIN ANALYZE operator #60550

sql: add nodes for each EXPLAIN ANALYZE operator #60550

Conversation

RaduBerinde commented Feb 12, 2021 • edited Loading

RaduBerinde commented Feb 12, 2021

cockroach-teamcity commented Feb 12, 2021

asubiotto left a comment

Choose a reason for hiding this comment

RaduBerinde commented Feb 16, 2021

awoods187 commented Feb 16, 2021

asubiotto commented Feb 16, 2021

RaduBerinde commented Feb 16, 2021

asubiotto left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde commented Feb 19, 2021

craig bot commented Feb 19, 2021

craig bot commented Feb 19, 2021

RaduBerinde commented Feb 12, 2021 •

edited

Loading