util/pretty: mitigate stack overflows of Pretty

[mgartner]

errorutil: moved kv-specific errors out of errorutil

Errors for missing store and node descriptors have been moved from the
errorutil package to the kvpb package, because the errorutil package is
a low-level package to aid in working with errors in general. It should
not contain facilities for creating specific errors as this muddles the
package and can lead to import cycles.

Release note: None

errorutil: move SendReport to new sentryutil package

The SendReport function has been moved out of the errorutil package
and into a new sentryutil package. This avoids muddling the errorutil
package with a Sentry-specific function, and it breaks errorutil's
dependence on pkg/settings and pkg/util/log/logcrash.

Release note: None

util/pretty: mitigate stack overflows of Pretty

This commit reduces the chance of a stack overflow from recursive calls
of *beExec.be. The Pretty function will now return an internal error
if the recursive depth of be surpasses 10,000.

Informs #91197

Release note: None

[cockroach-teamcity]

This change is 

[knz]

The approach taken here works if we choose to error out when the SQL is sufficiently complex.
But is that the right UX wrt user expectations?

We are looking here at a formatter. If the SQL is sufficiently complex, we still have an option to render the SQL -- using the simple formatter (the one that's not pretty).
What of avoiding an error return in Pretty() and instead have it call .AST.String() if memory was exceeded?

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

pkg/util/pretty/pretty.go line 119 at r1 (raw file):

}

const maxBeDepth = 100_000

arguably i could see us use 10.000 here (anything above 1000 looks suspicious to me).

[michae2]

Thanks! I agree with @knz that we should make an effort to try to render the SQL even if it is slightly ugly.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)

---

pkg/cmd/reduce/reduce/reducesql/reducesql.go line 443 at r1 (raw file):

		p, err := cfg.Pretty(stmt)
		if err != nil {
			panic(err)

Maybe instead of panic we should revert to using stmt?

---

pkg/internal/sqlsmith/sqlsmith.go line 195 at r1 (raw file):

		p, err := prettyCfg.Pretty(stmt)
		if err != nil {
			panic(err)

Same comment here.

---

pkg/sql/explain_bundle.go line 266 at r1 (raw file):

		b.stmt, err = cfg.Pretty(b.plan.stmt.AST)
		if err != nil {
			return err

If we hit this error, instead of giving up on saving the statement, I propose we copy this logic from the b.plan.stmt.AST == nil case above so that the bundle still has something useful:

		b.stmt = stmtRawSQL
		// If we're collecting a redacted bundle, redact the raw SQL completely.
		if b.flags.RedactValues && b.stmt != "" {
			b.stmt = string(redact.RedactedMarker())
		}

If you want to save the formatting error, could add something like "\n-- failed to format SQL: %v" to the b.stmt.

---

pkg/util/pretty/pretty.go line 119 at r1 (raw file):

}

const maxBeDepth = 100_000

Personally I would go even smaller (maybe 10k) but this is also fine.

---

pkg/util/pretty/pretty.go line 125 at r1 (raw file):

	defer func() { b.beDepth-- }()
	if b.beDepth > maxBeDepth {
		return nil, errors.AssertionFailedf("max call stack depth of be exceeded")

Another idea, if @knz doesn't like this, is to panic instead of adding error handling to every stack frame.

[knz]

Another idea, if @knz doesn't like this, is to panic instead of adding error handling to every stack frame.

lol, yes I am known to advertise for this type of error handling, which would be super justified here.

[mgartner]

I agree that the UX could improve, but I'd argue this is already a vast improvement. An internal error is strictly better than a crashed node.

IMO it should be up to the caller of Pretty to decide what to do if there is an error, but if you both feel strongly that it should just spit out a "non-pretty" string I can make that change.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @michae2)

---

pkg/util/pretty/pretty.go line 119 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Personally I would go even smaller (maybe 10k) but this is also fine.

Heh, you two are on the same page today, huh? 😉 Done.

---

pkg/util/pretty/pretty.go line 119 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

arguably i could see us use 10.000 here (anything above 1000 looks suspicious to me).

Done.

---

pkg/util/pretty/pretty.go line 125 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Another idea, if @knz doesn't like this, is to panic instead of adding error handling to every stack frame.

A panic here won't be guaranteed to be caught by a recover. I originally wanted to do this but from what I could tell the code path for the explain bundle would not recover from the panic, so it'd end up crashing the node.

[mgartner]

A panic here won't be guaranteed to be caught by a recover. I originally wanted to do this but from what I could tell the code path for the explain bundle would not recover from the panic, so it'd end up crashing the node.

Or are you suggesting to add recover to the Pretty function?

[knz]

Or are you suggesting to add recover to the Pretty function?

yes, that. panic-based error handling should never cross package boundaries.

[mgartner]

yes, that. panic-based error handling should never cross package boundaries.

👍 Done.

I added two more commits to avoid some import cycles. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @michae2)

---

pkg/cmd/reduce/reduce/reducesql/reducesql.go line 443 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Maybe instead of panic we should revert to using stmt?

Done.

---

pkg/internal/sqlsmith/sqlsmith.go line 195 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Same comment here.

Done.

---

pkg/sql/explain_bundle.go line 266 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

If we hit this error, instead of giving up on saving the statement, I propose we copy this logic from the b.plan.stmt.AST == nil case above so that the bundle still has something useful:

		b.stmt = stmtRawSQL
		// If we're collecting a redacted bundle, redact the raw SQL completely.
		if b.flags.RedactValues && b.stmt != "" {
			b.stmt = string(redact.RedactedMarker())
		}

If you want to save the formatting error, could add something like "\n-- failed to format SQL: %v" to the b.stmt.

Done.

[knz]

Reviewed 28 of 28 files at r3, 10 of 10 files at r4, 14 of 14 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

-- commits line 18 at r4:
💯 nice

---

pkg/kv/kvpb/errors.go line 1631 at r3 (raw file):

// StoreDescNotFoundError is reported when a stored descriptor is missing.
type StoreDescNotFoundError struct {

For the sake of error encode/decode, I recommend you keep a single data type (maybe with a boolean var to distinguish node/store); then you will want to add this to the init function:

errorutilpath := reflect.TypeOf(errorutil.XXX_TBD{}).PkgPath()
errors.RegisterTypeMigration(errorutilpath, "*errorutil.descriptorNotFound", &DescNotFoundError)

where XXX_TBD is a sentinel type remaining behind in errorutil to enable retrieving the PkgPath via reflect.

---

pkg/sql/explain_bundle.go line 261 at r5 (raw file):

		b.stmt, err = cfg.Pretty(b.plan.stmt.AST)
		if err != nil {
			// Use the raw statement string if pretty-printing fails.

I'm not keen on having a blanket ignore of the object in err. This feels like a candidate for errors.Is. If there's any other error later in the Pretty interface that is more "Serious" we won't want that to get ignored.

---

pkg/sql/show_create_clauses.go line 173 at r5 (raw file):

		}
		query, err = cfg.Pretty(parsed.AST)
		if err != nil {

ditto my previous comment about errors.Is

---

pkg/sql/show_create_clauses.go line 174 at r5 (raw file):

		query, err = cfg.Pretty(parsed.AST)
		if err != nil {
			log.Warningf(ctx, "error pretty-printing query for view %s (%v): %+v",

What is this warning going to help with? What should an operator do if they see it?

---

pkg/sql/sem/builtins/builtins.go line 11162 at r5 (raw file):

		p, err := p.Pretty(stmts[idx].AST)
		if err != nil {
			return "", err

I think this is a case where we probably want an alternate representation if this one fails. This code is used when displaying queries in DB Console.

---

pkg/util/pretty/pretty.go line 141 at r5 (raw file):

	defer func() { b.beDepth-- }()
	if b.beDepth > maxBeDepth {
		panic(errors.AssertionFailedf("max call stack depth of be exceeded"))

possibly a custom error marker here to enable errors.Is.

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @michae2)

---

-- commits line 18 at r4:

Previously, knz (Raphael 'kena' Poss) wrote…

💯 nice

👍

---

pkg/kv/kvpb/errors.go line 1631 at r3 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

For the sake of error encode/decode, I recommend you keep a single data type (maybe with a boolean var to distinguish node/store); then you will want to add this to the init function:

errorutilpath := reflect.TypeOf(errorutil.XXX_TBD{}).PkgPath()
errors.RegisterTypeMigration(errorutilpath, "*errorutil.descriptorNotFound", &DescNotFoundError)

where XXX_TBD is a sentinel type remaining behind in errorutil to enable retrieving the PkgPath via reflect.

Done.

---

pkg/sql/explain_bundle.go line 261 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I'm not keen on having a blanket ignore of the object in err. This feels like a candidate for errors.Is. If there's any other error later in the Pretty interface that is more "Serious" we won't want that to get ignored.

Great catch. Done.

---

pkg/sql/show_create_clauses.go line 173 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

ditto my previous comment about errors.Is

Done.

---

pkg/sql/show_create_clauses.go line 174 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

What is this warning going to help with? What should an operator do if they see it?

It wasn't intended for an operator. I thought it might aid us in debugging in the future, but I'm not sure exactly how. I've removed it.

---

pkg/sql/sem/builtins/builtins.go line 11162 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I think this is a case where we probably want an alternate representation if this one fails. This code is used when displaying queries in DB Console.

Makes sense, done.

---

pkg/util/pretty/pretty.go line 141 at r5 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

possibly a custom error marker here to enable errors.Is.

Done.

[knz]

Reviewed 7 of 31 files at r6, 9 of 10 files at r7, 15 of 15 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

pkg/kv/kvpb/errors.go line 1662 at r8 (raw file):

func (e *DescNotFoundError) SafeFormatError(p errors.Printer) (next error) {
	if e.isStore {
		p.Printf("store descriptor with store ID %d was not found", e.storeID)

nit: maybe

s := redact.SafeString("node")
id := uint64(e.NodeID)
if e.isStore {
  s = "store"
  id = uint64(e.StoreID)
}
p.Printf("%s descriptor with %s ID %d was not found", s, s, id)

(you could even collapse the two ID members into a single integer field)

---

pkg/sql/sem/builtins/builtins.go line 11161 at r8 (raw file):

	for idx := range stmts {
		p, err := p.Pretty(stmts[idx].AST)
		if err != nil {

errors.Is

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @michae2)

---

pkg/kv/kvpb/errors.go line 1662 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: maybe

s := redact.SafeString("node")
id := uint64(e.NodeID)
if e.isStore {
  s = "store"
  id = uint64(e.StoreID)
}
p.Printf("%s descriptor with %s ID %d was not found", s, s, id)

(you could even collapse the two ID members into a single integer field)

👍 Done.

---

pkg/sql/sem/builtins/builtins.go line 11161 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

errors.Is

Done.

---

pkg/util/pretty/pretty.go line 135 at r8 (raw file):

// maxBeDepth is the maximum allowed recursive call depth of be. If the depth
// exceeds this value, be will panic.
const maxBeDepth = 10_000

I raised this back to 100,000 because this test was failing to pretty-round-trip.

[knz]

LGTM modulo settling on the max size constant.

Reviewed 1 of 19 files at r11.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

pkg/util/pretty/pretty.go line 135 at r8 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I raised this back to 100,000 because this test was failing to pretty-round-trip.

I would rather change the test than the constant here. These tests were not principled.

[mgartner]

pkg/util/pretty/pretty.go line 135 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I would rather change the test than the constant here. These tests were not principled.

The test looks like a regression test and it's not a terribly obscure case—it's an insert of ~400 rows. Shouldn't we be able to handle such a statement?

[knz]

Shouldn't we be able to handle such a statement?

I would expect so, but not with a recursion depth of 100000. What is the effective recursion depth used for it?

[mgartner]

The depth for that test grows to 19,361.

[michae2]

Thank you for fixing this!

Reviewed 8 of 28 files at r3, 6 of 31 files at r6, 25 of 25 files at r9, 11 of 11 files at r10, 15 of 19 files at r11, 1 of 1 files at r12, 3 of 3 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @mgartner)

---

pkg/util/pretty/pretty.go line 135 at r8 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

The test looks like a regression test and it's not a terribly obscure case—it's an insert of ~400 rows. Shouldn't we be able to handle such a statement?

🤷 I'm ok either way.

(Personally I think Go's default stack limit of 1 GiB is giant but I'm coming from a previous codebase where we could only use 20 KiB stacks, so I'm biased.)

[mgartner]

pkg/util/pretty/pretty.go line 135 at r8 (raw file):

Previously, michae2 (Michael Erickson) wrote…

🤷 I'm ok either way.

(Personally I think Go's default stack limit of 1 GiB is giant but I'm coming from a previous codebase where we could only use 20 KiB stacks, so I'm biased.)

I updated it to 50,000, which allows the test to pass with plenty of buffer and also catches the stack overflow. Let me know what y'all think.

[knz]

i'm ok with it.

[mgartner]

TFTRs!

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[mgartner]

Flaked.

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)