sql, kv: add sql.mutations.max_row_size guardrails

[michae2]

kv: set Batch.pErr during Batch.prepare

If we fail to construct a Batch (e.g. fail to marshal a key or value)
then an error will be placed in the resultsBuf and the batch will not
actually be sent to the layers below. In this case we still need to set
Batch.pErr, so that Batch.MustPErr is able to return a roachpb.Error to
higher layers without panicking.

I imagine in practice we never fail to marshal the key or value, so we
have never seen this panic in the wild.

Release note: None

Release justification: Bug fix.

sql: add sql.mutations.max_row_size.log guardrail (large row logging)

Addresses: #67400

Add sql.mutations.max_row_size.log, a new cluster setting which
controls large row logging. Rows larger than this size will have their
primary keys logged to the SQL_PERF or SQL_INTERNAL_PERF channels
whenever the SQL layer puts them into the KV layer.

This logging takes place in rowHelper, which is used by both
row.Inserter and row.Updater. Most of the work is plumbing
settings.Values and SessionData into rowHelper, and adding a new
structured event type.

Release note (ops change): A new cluster setting,
sql.mutations.max_row_size.log, was added, which controls large row
logging. Whenever a row larger than this size is written (or a single
column family if multiple column families are in use) a LargeRow event
is logged to the SQL_PERF channel (or a LargeRowInternal event is logged
to SQL_INTERNAL_PERF if the row was added by an internal query). This
could occur for INSERT, UPSERT, UPDATE, CREATE TABLE AS, CREATE INDEX,
ALTER TABLE, ALTER INDEX, IMPORT, or RESTORE statements. SELECT, DELETE,
TRUNCATE, and DROP are not affected by this setting.

Release justification: Low risk, high benefit change to existing
functionality. This adds logging whenever a large row is written to the
database. Default is 64 MiB, which is also the default for
kv.raft.command.max_size, meaning on a cluster with default settings
statements writing these rows will fail with an error anyway.

sql: add sql.mutations.max_row_size.err guardrail (large row errors)

Addresses: #67400

Add sql.mutations.max_row_size.err, a new cluster setting similar to
sql.mutations.max_row_size.log, which limits the size of rows written to
the database. Statements trying to write a row larger than this will
fail with an error. (Internal queries will not fail with an error, but
will log a LargeRowInternal event to the SQL_INTERNAL_PERF channel.)

We're reusing eventpb.CommonLargeRowDetails as the error type, out of
convenience.

Release note (ops change): A new cluster setting,
sql.mutations.max_row_size.err, was added, which limits the size of rows
written to the database (or individual column families, if multiple
column families are in use). Statements trying to write a row larger
than this will fail with a code 54000 (program_limit_exceeded) error.
(Internal queries writing a row larger than this will not fail, but will
log a LargeRowInternal event to the SQL_INTERNAL_PERF channel.) This
limit is enforced for INSERT, UPSERT, and UPDATE statements. CREATE
TABLE AS, CREATE INDEX, ALTER TABLE, ALTER INDEX, IMPORT, and RESTORE
will not fail with an error, but will log LargeRowInternal events to the
SQL_INTERNAL_PERF channel. SELECT, DELETE, TRUNCATE, and DROP are not
affected by this limit. Note that existing rows violating the limit
cannot be updated, unless the update shrinks the size of the row
below the limit, but can be selected, deleted, altered, backed-up, and
restored. For this reason we recommend using the accompanying setting
sql.mutations.max_row_size.log in conjunction with
SELECT pg_column_size() queries to detect and fix any existing large
rows before lowering sql.mutations.max_row_size.err.

Release justification: Low risk, high benefit change to existing
functionality. This causes statements adding large rows to fail with an
error. Default is 512 MiB, which was the maximum KV size in 20.2 as of
#61818 and also the default
range_max_bytes in 21.1, meaning rows larger than this were not possible
in 20.2 and are not going to perform well in 21.1.

[cockroach-teamcity]

This change is 

[michae2]

Obviously this still needs testing, but I'm hoping to solicit feedback about whether this is a good idea in the first place.

[jordanlewis]

cc @andreimatei as well.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @michae2, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/batch.go, line 13 at r2 (raw file):

package kv

import (

It seems weird to me that the new limit is enforced inside the Batch. Isn't there a higher layer that can just as easily enforce it - like the tableWriter? If there's not too many distinct callers that want this, I'd rather keep it in the caller(s).

---

pkg/sql/tablewriter.go, line 38 at r2 (raw file):

var maxKeyValueSize = settings.RegisterByteSizeSetting(
	"sql.mutations.kv_max_size",

is it feasible to talk in terms of some SQL concept - like a col fam / index entry - instead of "key-value entry".

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @michae2, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/batch.go, line 13 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

It seems weird to me that the new limit is enforced inside the Batch. Isn't there a higher layer that can just as easily enforce it - like the tableWriter? If there's not too many distinct callers that want this, I'd rather keep it in the caller(s).

After reading though how the errors make it out of the batch in a deferred way, I want this logic to live out of the batch even more.

---

pkg/kv/batch.go, line 385 at r2 (raw file):

func (b *Batch) checkKVSize(k roachpb.Key, v roachpb.Value) error {
	if size := len(k) + len(v.RawBytes); b.MaxKVBytes != 0 && size > b.MaxKVBytes {
		return fmt.Errorf(

useWithCandidateCode. And generally prefer errors.Errorf to fmt.Errorf except in cases where you're sure you'll never want a stack.

[bdarnell]

How exactly does this relate to kv.raft.command.max_size? It looks like this is meant to be a slightly "softer" limit but it's not clear what exactly is allowed that would be blocked by the existing limit. In particular the fact that updates are not allowed unless they shrink the row back below the limit appears to be the same behavior as the existing limit, and this is a problem for the customer who motivated this PR.

If we do have both a SQL and KV-level setting for batch size limits, I think it would make sense for the SQL one to have its default no higher than the KV-level default (probably by setting the SQL-level default to the same as the current KV limit and bumping the KV limit up a bit).

[andreimatei]

How exactly does this relate to kv.raft.command.max_size?

This one operates at the level of single column families (right?).

If we do have both a SQL and KV-level setting for batch size limits, I think it would make sense for the SQL one to have its default no higher than the KV-level default (probably by setting the SQL-level default to the same as the current KV limit and bumping the KV limit up a bit).

What I'd like clarified in comments and perhaps in the validator is the relationship with the new sql.mutations.mutation_batch_byte_size

[nvanbenschoten]

This one operates at the level of single column families (right?).

Right, the limit here operates at the level of a single key-value. kv.raft.command.max_size operates at the level of an entire Raft proposal, which may include multiple KVs. kv.raft.command.max_size may also block writes that look smaller than they actually end up being, due to the size of their corresponding intent. @dt recently found that it's quite difficult to predict the size of a KV write ahead of time, because we include the transaction key in intents, so even a 10 byte Put may be 5MB large if it's transaction has a large archor key.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @michae2, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/batch.go, line 13 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

After reading though how the errors make it out of the batch in a deferred way, I want this logic to live out of the batch even more.

After reading through this, I was left with the same thought. I don't think this needs to live in the KV batch. And so if there is only a few callers, we may be better off lifting this up a level.

[michae2]

Thanks for the feedback, everyone. Yes, you have it right about the relationship with kv.raft.command.max_size. I agree about lifting this out of the KV batch and shall do so.

In particular the fact that updates are not allowed unless they shrink the row back below the limit appears to be the same behavior as the existing limit, and this is a problem for the customer who motivated this PR.

We are discussing this with the customer. The trouble is that, when not using multiple column families, at this level it is difficult to distinguish between something like:

-- before new limit
INSERT INTO table (key, shortpayload, longpayload) VALUES (42, 99, "giant text ... ");
-- after new limit
UPDATE table SET shortpayload = 100 WHERE key = 42;

and:

-- before new limit
INSERT INTO table VALUES (42, 99, NULL);
-- after new limit
UPDATE table SET longpayload = "giant text ..." WHERE key = 42;

If multiple column families are in use, as we recommend for this type of schema, then only the column family with the giant text column will be affected by the size limit, and the first update will succeed.

If we do have both a SQL and KV-level setting for batch size limits, I think it would make sense for the SQL one to have its default no higher than the KV-level default (probably by setting the SQL-level default to the same as the current KV limit and bumping the KV limit up a bit).

Makes sense to me.

[sumeerbhola]

Is this PR meant to supersede #67537 (that PR came up in https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1626287781175700)?

[michae2]

Is this PR meant to supersede #67537 (that PR came up in https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1626287781175700)?

This PR is similar to, but distinct in purpose from the batch_byte_size setting. The batch_byte_size setting controls the number of put / del operations sent in one batch, using the size of operations as a guide, but does not limit the size of any one operation. The setting introduced in this PR limits the size of any one operation.

More changes to come today, so feel free to hold off reading for a while.

[michae2]

• Check has been moved up into SQL rowHelper.
• Single setting has been split into two, warn and error. The latest push only contains the warn setting, which causes an new LargeRow event to be logged. error setting is in-progress. I'm hoping to backport the warn setting.
• Default now matches kv.raft.command.max_size default.
• Added a test.

The error setting is still in-progress, but this is RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @michae2, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/batch.go, line 13 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

After reading through this, I was left with the same thought. I don't think this needs to live in the KV batch. And so if there is only a few callers, we may be better off lifting this up a level.

Check has been lifted up into row.rowHelper, entirely within SQL layer. I kept the small fix to Batch in the first commit.

---

pkg/sql/tablewriter.go, line 38 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is it feasible to talk in terms of some SQL concept - like a col fam / index entry - instead of "key-value entry".

I decided to simply use "row".

[rytaft]

This is looking great! I'll let others approve since I'm not so familiar with all these code paths.

Is there a reason you don't think you'll be able to backport the error setting (assuming the default is that the error is disabled)?

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 5 of 5 files at r4, 24 of 24 files at r5, 2 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @michae2, and @sumeerbhola)

---

pkg/sql/slow_query_log_test.go, line 25 at r6 (raw file):

	errRe    string
	logRe    string
	expected int

what does expected mean here? (add a comment)

---

pkg/sql/slow_query_log_test.go, line 213 at r6 (raw file):

}

func TestSlowQueryLog(t *testing.T) {

nit: I'd give this test (and filename) a different name since it's confusing that large row events are also tested here. Maybe you could also just add this test to event_log_test.go?

---

pkg/sql/row/helper.go, line 43 at r5 (raw file):

	"sql.mutations.max_row_size.warn",
	"maximum size of row (or column family if multiple column families are in use) that SQL can "+
		"write to the KV store, above which an event is logged to SQL_PERF or SQL_INTERNAL_PERF "+

why would it be written to one or the other? Maybe add "depending on whether or not the transaction is internal"?

---

pkg/sql/sessiondatapb/session_data.proto, line 77 at r4 (raw file):

                                              (gogoproto.stdduration) = true];
  // Internal indicates whether this query came from InternalExecutor or an
  // internal planner.

nit: sounds like these are the only two options. Perhaps you could say:

  // Internal is true if this query came from InternalExecutor or an
  // internal planner.

Is that correct?

[michae2]

Latest push should fix the CI errors, as well as address comments (thank you!). Also adds sql.mutations.max_row_size.err. Testcases for that are coming.

Is there a reason you don't think you'll be able to backport the error setting (assuming the default is that the error is disabled)?

No particular reason, it's just more consequential, so I'm more cautious. I made the default 512 MiB.

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

---

pkg/kv/batch.go, line 385 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

useWithCandidateCode. And generally prefer errors.Errorf to fmt.Errorf except in cases where you're sure you'll never want a stack.

Thanks, done. Hopefully I've managed this leaf error thing correctly. 🙂

---

pkg/sql/row/helper.go, line 43 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why would it be written to one or the other? Maybe add "depending on whether or not the transaction is internal"?

Done.

---

pkg/sql/sessiondatapb/session_data.proto, line 77 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: sounds like these are the only two options. Perhaps you could say:

  // Internal is true if this query came from InternalExecutor or an
  // internal planner.

Is that correct?

Oh, good point. Done.

---

pkg/sql/slow_query_log_test.go, line 25 at r6 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

what does expected mean here? (add a comment)

Done.

---

pkg/sql/slow_query_log_test.go, line 213 at r6 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: I'd give this test (and filename) a different name since it's confusing that large row events are also tested here. Maybe you could also just add this test to event_log_test.go?

Ah, I didn't know about event_log_test, thank you!

[michae2]

(Plan for backport is to completely disable both settings.)

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @michae2, and @rytaft)

---

pkg/sql/row/helper.go, line 50 at r21 (raw file):

	func(size int64) error {
		if size != 0 && size < maxRowSizeFloor {
			return fmt.Errorf(

In #68707 Raphael switched all fmt.Errorf into errors.Newf.

[michae2]

• Switched fmt.Errorf to errors.Newf.
• Added 0 setting to sql.mutations.max_row_size.err to disable.
• Cleaned up internal / shouldErr logic in checkRowSize (thanks @yuzefovich!).
• Squashed into 3 commits and added release justifications.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @dt, @rytaft, and @yuzefovich)

---

pkg/sql/row/helper.go, line 50 at r21 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

In #68707 Raphael switched all fmt.Errorf into errors.Newf.

Oh, thanks! Switched.

[rytaft]

Reviewed 23 of 31 files at r23, 10 of 10 files at r24.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @rytaft, and @yuzefovich)

[michae2]

Added a short test for backup and restore of existing large rows.

[rytaft]

Reviewed 1 of 1 files at r25.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @rytaft, and @yuzefovich)

[andreimatei]

First commit modulo removing an if b.pErr == nil {

Release note (bug fix): Fix an extremely rare panic in Batch.MustPErr.

This message won't make sense to a user. Scrap it.

Out of curiosity, how did run into the need for the first commit?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @andreimatei, @dt, @michae2, @rytaft, and @yuzefovich)

---

pkg/kv/batch.go, line 97 at r22 (raw file):

}

func (b *Batch) prepare() error {

Please comment that this method checks for marshalling errors; it took me a while to convince that this method is necessary at all.
And consider renaming it to validate or something. It hardly does any "preparing".

---

pkg/kv/batch.go, line 97 at r22 (raw file):

}

func (b *Batch) prepare() error {

consider adding a test for this method, given that you're fixing something

---

pkg/kv/batch.go, line 99 at r22 (raw file):

func (b *Batch) prepare() error {
	if err := b.resultErr(); err != nil {
		if b.pErr == nil {

Can b.pErr be set at this point? I don't think it can, so I think we don't need this condition. Having it is confusing.
Perhaps you've copied this guard from sendAndFill, but there it's used to differentiate (I think) between an error result and an RPC error (which is not present in the results).

---

pkg/kv/batch.go, line 100 at r22 (raw file):

	if err := b.resultErr(); err != nil {
		if b.pErr == nil {
			b.pErr = roachpb.NewError(err)

since this pattern of both saving an error and returning it is unusual, consider adding a comment that we're doing this analogously to sendAndFill.

---

pkg/kv/batch.go, line 104 at r22 (raw file):

		return err
	}
	return nil

nit: if you make err above not be scoped to the if, you can always return err.

---

pkg/kv/batch.go, line 302 at r22 (raw file):

// if one exists.
func (b *Batch) resultErr() error {
	for _, r := range b.Results {

I think this change is not worth taking the blame on this stanza.

[michae2]

Thanks for the reviews!

Out of curiosity, how did run into the need for the first commit?

The prototype of these guardrails was implemented at the kv.Batch level, and used this path to return errors. So I noticed when the errors caused a panic. 🙂

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

---

pkg/kv/batch.go, line 97 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Please comment that this method checks for marshalling errors; it took me a while to convince that this method is necessary at all.
And consider renaming it to validate or something. It hardly does any "preparing".

Done.

---

pkg/kv/batch.go, line 97 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

consider adding a test for this method, given that you're fixing something

Done.

---

pkg/kv/batch.go, line 99 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Can b.pErr be set at this point? I don't think it can, so I think we don't need this condition. Having it is confusing.
Perhaps you've copied this guard from sendAndFill, but there it's used to differentiate (I think) between an error result and an RPC error (which is not present in the results).

Agreed (yes I did copy it from sendAndFill). Done.

---

pkg/kv/batch.go, line 100 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

since this pattern of both saving an error and returning it is unusual, consider adding a comment that we're doing this analogously to sendAndFill.

Done.

---

pkg/kv/batch.go, line 104 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: if you make err above not be scoped to the if, you can always return err.

Done.

---

pkg/kv/batch.go, line 302 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think this change is not worth taking the blame on this stanza.

Good point, done.

[yuzefovich]

nit: the PR description should be updated according to the latest commit message (I'm not sure where we're taking the release note from).

[michae2]

TFTRs!

bors r=rytaft,andreimatei

[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 711d42e to blathers/backport-release-21.1-67953: 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.}

[michae2]

Note that when this is backported, #69457 will also need to be backported.