sql: allow booleans in SET tracing and friendlier error message

[yuzefovich]

Previously, we would fail an assertion if a datum of the wrong type is
provided in SET tracing query (only strings were allowed). This resulted
in an internal error and printing out of the stack trace which can be
scary to users. This commit removes the assertion and makes it a regular
query error.

Also, booleans are now allowed as argument to SET tracing, and true
is mapped to on mode and false to off.

Fixes: #44244.

Release note (sql change, bug fix): Previously, CockroachDB would return
an internal error when using SET tracing with any type other than
string. Now it will return a regular query error. Additionally, boolean
arguments are now supported in SET tracing, and true is mapped to
on mode of tracing whereas false is mapped to off.

[cockroach-teamcity]

This change is 

[knz]

Thanks for spotting it. Just a minor comment.

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/conn_executor_exec.go, line 1184 at r1 (raw file):

		strVal, ok := v.(*tree.StrVal)
		if !ok {
			res.SetError(errors.Newf("expected string for set tracing argument, not %T", v))

I agree we need to fix this but this is not the way to go about it.
The main problem is that it's not good style to show something internal (%T) in a user-facing message.
The second problem is that it could be nice to allow booleans.

There are two ways to go:

• either the way of (*planner) SetVar
• or that of (*planner)EvalAsOfTimestamp (really tree.EvalAsOfTimestamp)

It's possible you can't use analyzeExpr /Eval like the first option, that's why I am mentioning the second.
(The reason why SET tracing is not an actual SetVar is that we want, I think, to be able to run it even when the txn is in error; in which case scalar evaluation is unavailable. But I'm not 100% sure. For the record, I prefer the SET (SetVar) semantics more.)

Long story short, get the expression, type switch on the result, and print a friendly error message.

Also the pg code for something that doesn't have the right type is pgcode.Syntax.

---

pkg/sql/logictest/testdata/logic_test/set, line 305 at r1 (raw file):

SET ssl_renegotiation_limit = 123

statement error pq: expected string for set tracing argument, not \*tree.DBool

when you have a code, do statement pgerror XXXX ....

[yuzefovich]

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

---

pkg/sql/conn_executor_exec.go, line 1184 at r1 (raw file):

Previously, knz (kena) wrote…

I agree we need to fix this but this is not the way to go about it.
The main problem is that it's not good style to show something internal (%T) in a user-facing message.
The second problem is that it could be nice to allow booleans.

There are two ways to go:

• either the way of (*planner) SetVar
• or that of (*planner)EvalAsOfTimestamp (really tree.EvalAsOfTimestamp)

It's possible you can't use analyzeExpr /Eval like the first option, that's why I am mentioning the second.
(The reason why SET tracing is not an actual SetVar is that we want, I think, to be able to run it even when the txn is in error; in which case scalar evaluation is unavailable. But I'm not 100% sure. For the record, I prefer the SET (SetVar) semantics more.)

Long story short, get the expression, type switch on the result, and print a friendly error message.

Also the pg code for something that doesn't have the right type is pgcode.Syntax.

Thanks for the comment! I added the support for boolean arguments and changed the error to a regular query error.

In terms of SetVar semantics, I'm also not sure about the behavior there, so I'd rather stick with this least invasive approach.

[knz]

LGTM modulo resolution of my comment below

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/logictest/testdata/logic_test/set, line 305 at r1 (raw file):

Previously, knz (kena) wrote…

when you have a code, do statement pgerror XXXX ....

can ou add an additional test that shows the syntax error

[yuzefovich]

TFTR!

bors r+

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

---

pkg/sql/logictest/testdata/logic_test/set, line 305 at r1 (raw file):

Previously, knz (kena) wrote…

can ou add an additional test that shows the syntax error

Done.

[craig]

Build succeeded

• GitHub CI (Cockroach)

[yuzefovich]

Raphael, do you think we should backport it?

[knz]

Yes because we don't want this to continue spamming Sentry -- Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.