sql: fall back to raw SQL for statement diagnostics bundle, and add errors.txt

[michae2]

sql: fall back to raw SQL for statement diagnostics bundle

When gathering the statement for a statement diagnostics bundle, fall
back to using the raw SQL when we cannot pretty-print the statement AST,
which happens for statements that fail with certain errors.

(This also fixes a crash in addTrace, which was not handling these
error cases. It looks like this crash was partially fixed in #56768 but
not completely.)

Fixes: #94416 #56705

Epic: None

Release note (bug fix): Fix a crash that occurs on the gateway node when
collecting a statement diagnostics bundle (e.g. EXPLAIN ANALYZE (DEBUG)) on a statement that fails with certain errors. This crash has
existed in various forms since the introduction of statement bundles in
v20.1.0.

sql: add errors.txt to statement diagnostics bundles

Add an errors.txt file to statement diagnostics bundles containing the
printed messages of various error objects.

Informs: #94416

Release note: None

[cockroach-teamcity]

This change is 

[michae2]

Now for that bad AOST query we get this statement.sql (notice the EXPLAIN ANALYZE (DEBUG) hasn't been stripped off):

EXPLAIN ANALYZE (DEBUG) SELECT * FROM t AS OF SYSTEM TIME '-0.0000001s'

and this errors.txt:

res error:
<nil>

payload error:
AS OF SYSTEM TIME: interval value '-0.0000001s' too small, absolute value must be >= 1µs

ret error:
<nil>

[mgartner]

Thanks for fixing this!

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

---

-- commits line 17 at r1:
Can you make it explicit that this would crash the gateway node? Also, do we know how far back this was present?

[yuzefovich]

Nice!

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

---

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

}

// buildStatement saves the pretty-printed statement, without arguments.

nit: /s/buildStatement/buildPrettyStatement/.

---

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

}

// addStatement adds the pretty-printed statement as file statement.txt.

nit: maybe mention that the pretty-printed statement is stored in b.stmt.

---

pkg/sql/explain_bundle.go line 134 at r2 (raw file):

	trace tracingpb.Recording,
	placeholders *tree.PlaceholderInfo,
	resErr, payloadErr, retErr error,

nit: let's rename resErr to queryErr (it's an error encountered during the query optimization / execution) and retErr to commErr (since it should be about an error communicating with the client).

---

pkg/sql/explain_bundle.go line 450 at r2 (raw file):

		resErr, payloadErr, retErr,
	)
	b.z.AddFile("errors.txt", output)

It'd be good to add a simple unit test for this.

[michae2]

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

---

-- commits line 17 at r1:

Previously, mgartner (Marcus Gartner) wrote…

Can you make it explicit that this would crash the gateway node? Also, do we know how far back this was present?

Done. Looks like this existed back in v20.1.0 (when statement bundles were first introduced), and then was partially fixed in #56768 but remained an issue until now.

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: /s/buildStatement/buildPrettyStatement/.

Done.

---

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

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe mention that the pretty-printed statement is stored in b.stmt.

Done.

---

pkg/sql/explain_bundle.go line 134 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: let's rename resErr to queryErr (it's an error encountered during the query optimization / execution) and retErr to commErr (since it should be about an error communicating with the client).

Ah, thank you! Done.

---

pkg/sql/explain_bundle.go line 450 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

It'd be good to add a simple unit test for this.

Now checked in TestExplainAnalyzeDebug/error.

[michae2]

TFYRs!

bors r=mgartner,yuzefovich

[craig]

Build succeeded:

• Bazel Essential 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 4f66dcc to blathers/backport-release-22.1-94440: 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 22.1.x failed. See errors above.

---

error creating merge commit from 4f66dcc to blathers/backport-release-22.2-94440: 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 22.2.x failed. See errors above.

---

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