release-22.2: sql: output RU estimate for EXPLAIN ANALYZE on tenants

[DrewKimball]

Backport:

• 1/1 commits from "sql: output RU estimate for EXPLAIN ANALYZE on tenants" (sql: output RU estimate for EXPLAIN ANALYZE on tenants #89256)
• 1/1 commits from "sql: add cluster setting to disable RU estimation" (sql: add cluster setting to disable RU estimation #92968)

Please see individual PRs for details.

/cc @cockroachdb/release

---

Release justification: low-risk, high-benefit change to existing functionality

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Patches should only be created for serious issues or test-only changes.
• Patches should not break backwards-compatibility.
• Patches should change as little code as possible.
• Patches should not change on-disk formats or node communication protocols.
• Patches should not add new functionality.
• Patches must not add, edit, or otherwise modify cluster versions; or add version gates.

If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

• What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
• Will this work in a cluster of mixed patch versions? Did we test that?
• If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

[cockroach-teamcity]

This change is 

[DrewKimball]

I've left the cluster setting as default-on - should it be off by default for the backport?

Also, while the backport was mostly clean, I had to make a few changes to planNodeToRowSource vs the original PR since there have been changes to context-passing and span collection for that processor after 22.2. Instead of using execstats.ShouldCollectStats when deciding whether to set the ExecStatsForTrace callback, we only check flowCtx.CollectStats, and make the same check when deciding whether to record a span. @yuzefovich does this seem safe to you?

[rytaft]

(although would be good to get sign off from @yuzefovich regarding your question above)

I think it's ok to leave the setting defaulted to on for backporting to 22.2. I think we'll get this PR into 22.2.1, and presumably serverless clusters will start on 22.2.1 or later. If we want to backport to 22.1 it should probably be defaulted to off.

Reviewed 37 of 37 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @yuzefovich)

[andy-kimball]

We don't need to backport this to 22.1.

[yuzefovich]

Reviewed 37 of 37 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

	// We can't use execstats.ShouldCollectStats here because the context isn't
	// passed to InitWithOutput.
	if flowCtx.CollectStats {

This is safe, but we should use execstats.ShouldCollectStats(flowCtx.EvalCtx.Ctx(), flowCtx.CollectStats) for consistency with other places on 22.2 branch.

---

pkg/sql/plan_node_to_row_source.go line 163 at r1 (raw file):

func (p *planNodeToRowSource) Start(ctx context.Context) {
	if p.FlowCtx.CollectStats {
		ctx = p.StartInternal(ctx, nodeName(p.node))

I think we should just always call StartInternal here (we did this in 7291e4d on master). The only thing I would check is whether there are any regressions in BenchmarkSQL/Insert\$\$ in pkg/bench, but I don't expect them to be there.

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @yuzefovich)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This is safe, but we should use execstats.ShouldCollectStats(flowCtx.EvalCtx.Ctx(), flowCtx.CollectStats) for consistency with other places on 22.2 branch.

In the tests I did, flowCtx.EvalCtx.Ctx() didn't necessarily have a span set even when I added the StartInternal call to Start - I guess maybe the difference is because there are more StartInternal calls on master now? Maybe this check should go in a different place to ensure the span is set?

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

In the tests I did, flowCtx.EvalCtx.Ctx() didn't necessarily have a span set even when I added the StartInternal call to Start - I guess maybe the difference is because there are more StartInternal calls on master now? Maybe this check should go in a different place to ensure the span is set?

Hm, this is surprising to me - what was the reproduction that you ran into this with?

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @yuzefovich)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Hm, this is surprising to me - what was the reproduction that you ran into this with?

It wasn't anything special - I just did an insert on a table like this:

CREATE TABLE xy (x INT, y INT);
EXPLAIN ANALYZE INSERT INTO xy (SELECT t, t FROM generate_series(1, 10000) g(t));

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

It wasn't anything special - I just did an insert on a table like this:

CREATE TABLE xy (x INT, y INT);
EXPLAIN ANALYZE INSERT INTO xy (SELECT t, t FROM generate_series(1, 10000) g(t));

Hm, I just tried these two queries in demo --multitenant=true with cockroach binary built on this branch plus

diff --git a/pkg/sql/plan_node_to_row_source.go b/pkg/sql/plan_node_to_row_source.go
index 18401059a5..c7789a88d3 100644
--- a/pkg/sql/plan_node_to_row_source.go
+++ b/pkg/sql/plan_node_to_row_source.go
@@ -123,7 +123,7 @@ func (p *planNodeToRowSource) InitWithOutput(
        }
        // We can't use execstats.ShouldCollectStats here because the context isn't
        // passed to InitWithOutput.
-       if flowCtx.CollectStats {
+       if execstats.ShouldCollectStats(flowCtx.EvalCtx.Ctx(), flowCtx.CollectStats) {
                p.ExecStatsForTrace = p.execStatsForTrace
        }
        return nil

and it worked

root@127.0.0.1:26257/defaultdb> EXPLAIN ANALYZE INSERT INTO xy (SELECT t, t FROM generate_series(1, 10000) g(t));
                 info
--------------------------------------
  planning time: 171µs
  execution time: 325ms
  distribution: local
  vectorized: true
  maximum memory usage: 70 KiB
  network usage: 0 B (0 messages)
  estimated RUs consumed: 10,291
...

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Hm, I just tried these two queries in demo --multitenant=true with cockroach binary built on this branch plus

diff --git a/pkg/sql/plan_node_to_row_source.go b/pkg/sql/plan_node_to_row_source.go
index 18401059a5..c7789a88d3 100644
--- a/pkg/sql/plan_node_to_row_source.go
+++ b/pkg/sql/plan_node_to_row_source.go
@@ -123,7 +123,7 @@ func (p *planNodeToRowSource) InitWithOutput(
        }
        // We can't use execstats.ShouldCollectStats here because the context isn't
        // passed to InitWithOutput.
-       if flowCtx.CollectStats {
+       if execstats.ShouldCollectStats(flowCtx.EvalCtx.Ctx(), flowCtx.CollectStats) {
                p.ExecStatsForTrace = p.execStatsForTrace
        }
        return nil

and it worked

root@127.0.0.1:26257/defaultdb> EXPLAIN ANALYZE INSERT INTO xy (SELECT t, t FROM generate_series(1, 10000) g(t));
                 info
--------------------------------------
  planning time: 171µs
  execution time: 325ms
  distribution: local
  vectorized: true
  maximum memory usage: 70 KiB
  network usage: 0 B (0 messages)
  estimated RUs consumed: 10,291
...

Or was the problem that RUs estimation was wrong?

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @yuzefovich)

---

pkg/sql/plan_node_to_row_source.go line 126 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Or was the problem that RUs estimation was wrong?

Hm, maybe I tested that before changing to use StartInternal or something. Reverted to use ShouldCollectStats. Done.

---

pkg/sql/plan_node_to_row_source.go line 163 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think we should just always call StartInternal here (we did this in 7291e4d on master). The only thing I would check is whether there are any regressions in BenchmarkSQL/Insert\$\$ in pkg/bench, but I don't expect them to be there.

Done.

[yuzefovich]

Reviewed 10 of 10 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball)

[DrewKimball]

TFTRs!

[DrewKimball]

@yuzefovich I had to modify the call to StartInternal for planNodeToRowSource to make TestAlreadyRunningJobsAreHandledProperly test pass.

[yuzefovich]

LGTM, although it probably would have been cleaner to backport the whole 7291e4d without modifying the commit for RU estimation. I'm ok with merging either way.

Reviewed 18 of 18 files at r5, 9 of 9 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/opt/exec/execbuilder/testdata/select line 24 at r5 (raw file):

query ITT
SELECT
  span, split_part(regexp_replace(message, 'pos:[0-9]*', 'pos:?'), E'\n', 1), operation

Why did we need to modify this? Looks like 7291e4d didn't do it.

[DrewKimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @yuzefovich)

---

pkg/sql/opt/exec/execbuilder/testdata/select line 24 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Why did we need to modify this? Looks like 7291e4d didn't do it.

The SPAN START: values lines started including the flow ID, which made the test non-deterministic... it's not clear to me what's changed since 22.2 to cause this

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @DrewKimball)

---

pkg/sql/opt/exec/execbuilder/testdata/select line 24 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

The SPAN START: values lines started including the flow ID, which made the test non-deterministic... it's not clear to me what's changed since 22.2 to cause this

Those were removed on master in ae24dd9. I think we should just backport all three commits from #87317, and then just keep the original two commits here. Will do that now.

[yuzefovich]

Reviewed 21 of 22 files at r7, 16 of 16 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball)

[DrewKimball]

TFTRs!