sql, import: add metrics for max_row_size guardrails

[michae2]

sql: rename max_row_size guardrails to match transaction row limits

Addresses: #67400

Rename sql.mutations.max_row_size.{log|err} to
sql.guardrails.max_row_size_{log|err} for consistency with
transaction_rows_{read|written}_{log|err} and upcoming metrics.

Release justification: Low-risk update to new functionality.

Release note (ops change): New variables
sql.mutations.max_row_size.{log|err} were renamed to
sql.guardrails.max_row_size_{log|err} for consistency with other
variables and metrics.

sql/row: remove dependency on sql/execinfra

The next commit needs to make sql/row a dependency of sql/execinfra, so
remove two small references to sql/execinfra.

(I will squash this into the next commit before merging.)

Release justification: Low-risk update to new functionality.

Release note: None

sql, import: plumb RowMetrics from ExecutorConfig to row.Helper

Addresses: #67400

Add metrics for sql/row and pass them down from ExecutorConfig and
FlowCtx all the way to row.Helper. Like sql.Metrics, there are two
copies, one for user queries and one for internal queries. (These were
originally part of sql.Metrics, but there are several users of sql/row
that do not operate under a sql.Server or connExecutor so we are forced
to add these to the ExecutorConfig and FlowCtx instead.)

I ran into difficulty passing these through import, as FlowCtx itself
is not plumbed through.

There are only two metrics at first, corresponding to
sql.guardrails.max_row_size_{log|err}.

Release justification: Low-risk update to new functionality.

Release note (ops): Added four new metrics,
sql.guardrails.max_row_size_{log|err}.count{.internal} which are
incremented whenever a large row violates
sql.guardrails.max_row_size_{log|err}.

[cockroach-teamcity]

This change is 

[rytaft]

Nice!

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 24 of 24 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)

---

pkg/sql/exec_util.go, line 2975 at r3 (raw file):

}

func NewRowMetrics(internal bool) row.RowMetrics {

I'd put this in a different package other than sql.

---

pkg/sql/row/fetcher.go, line 343 at r2 (raw file):

	if memMonitor != nil {
		rf.mon = mon.NewMonitorInheritWithLimit("fetcher-mem", 0 /* limit */, memMonitor)

why are you using a limit of 0 here?

[michae2]

Thank you for looking! 🙂

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @rytaft, and @yuzefovich)

---

pkg/sql/exec_util.go, line 2975 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I'd put this in a different package other than sql.

Unfortunately it has to be in sql to use getMetricMeta without creating a circular dependency.

---

pkg/sql/row/fetcher.go, line 343 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why are you using a limit of 0 here?

This is a paste of the body of execinfra.NewMonitor from processorbase.go which uses 0 internally. I think the de facto limit comes from the parent monitor (or an ancestor of the parent), in this case memMonitor.

[michae2]

@adityamaru I think you were tagged because of the third commit. It makes some changes to the sql/row package which then required plumbing some information through various parts of bulk IO. I was struggling with this when I saw your comment on plumbing FlowCtx and I definitely agree as this would make it much easier to pass these metrics down through import! 😄 Maybe later today I will take a shot at it, if that's alright with you.

[rytaft]

although it would be good to get approval from at least one other person who knows this code better than I do.

Reviewed 2 of 27 files at r4, 1 of 2 files at r5, 24 of 24 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @yuzefovich)

---

pkg/sql/exec_util.go, line 2975 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Unfortunately it has to be in sql to use getMetricMeta without creating a circular dependency.

Ah ok, makes sense.

[yuzefovich]

Reviewed 2 of 3 files at r1, 2 of 27 files at r4, 1 of 2 files at r5, 24 of 24 files at r6, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @adityamaru and @michae2)

---

pkg/sql/execinfra/server_config.go, line 293 at r6 (raw file):

// GetRowMetrics returns the proper RowMetrics for either internal or user
// queries.
func (flowCtx *FlowCtx) GetRowMetrics(internal bool) *row.Metrics {

nit: I thinkFlowCtx has EvalContext inside, so maybe we could peek into the session data and remove the need for an argument here?

---

pkg/sql/row/fetcher.go, line 343 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

This is a paste of the body of execinfra.NewMonitor from processorbase.go which uses 0 internally. I think the de facto limit comes from the parent monitor (or an ancestor of the parent), in this case memMonitor.

Yep. Just to expand on Michael's comment: passing 0 as the limit we're only making the new monitor "locally unlimited" - any reservations still need to be approved by the parent (which might also not have a local limit and will ask its parent, etc, eventually the ask will be against rootSQLMemoryMonitor).

---

pkg/sql/row/metrics.go, line 16 at r6 (raw file):

var (
	// Metadata for sql.guardrails.max_row_size_log.count{.internal}

nit: missing period.

[michae2]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @adityamaru, @rytaft, and @yuzefovich)

---

pkg/sql/execinfra/server_config.go, line 293 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I thinkFlowCtx has EvalContext inside, so maybe we could peek into the session data and remove the need for an argument here?

Oh! Good point! Done!

(It looks like the EvalContext inside FlowCtx is often copied by operators using NewEvalCtx but that's fine, we can still use FlowCtx.EvalContext because SessionData().Internal will always be the same in both copies.)

---

pkg/sql/row/metrics.go, line 16 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: missing period.

Done.

[michae2]

TFTRs! Failure looks like an unrelated flake. I'd like to merge this before the 21.2 branch is cut (and before I go on vacation 😎) so I think I'll give it a shot.

bors r=rytaft,yuzefovich

[michae2]

Looks like bors got stuck.

bors r-

[craig]

Canceled.

[michae2]

Let's try this again! 🎲

bors r=rytaft,yuzefovich

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed:

• GitHub CI (Cockroach)

[michae2]

bors r-

Hmm.

[maryliag]

@michae2 there were some tests failing that we fixed and got merged this morning. I suggest rebase your branch and trying again, maybe it helps :)

[michae2]

Third time's the charm!

bors r=rytaft,yuzefovich

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 38c0a3c to blathers/backport-release-21.1-69457: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}