sql: fix TestRandomSyntaxFunctions related bugs/flakes

[fqazi]

Previously TestRandomSyntaxFunctions could flake due to the following bugs:

1. crdb_internal.revalidate_unique_constraints_in_all_tables did not handle cancelled contexts properly which could lead to timeouts
2. crdb_internal.pretty_value could fail on random values could panic decoding, so a recover should be used inside this builtin.
3. TestRandomSyntaxFunctions was testing crdb_internal.gen_rand_ident, which can take a long time based on the input parameters (specifically when a large number of identifiers is requested)

Fixes: #107410
Fixes: #107411

[cockroach-teamcity]

This change is 

[rafiss]

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

---

pkg/sql/sem/builtins/builtins.go line 5768 at r3 (raw file):

			Fn: func(_ context.Context, _ *eval.Context, args tree.Datums) (d tree.Datum, err error) {
				defer func() {
					if r := recover(); r != nil {

i'm curious about this one - i think it's unexpected that the PrettyPrint function would ever panic. how does that happen?

[fqazi]

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

---

pkg/sql/sem/builtins/builtins.go line 5768 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i'm curious about this one - i think it's unexpected that the PrettyPrint function would ever panic. how does that happen?

@rafiss DecodeUntaggedUUIDValue and DecodeUntaggedIPAddrValue do not validate the input, so that they will hit out-of-bounds errors. The other approach is to update those code paths to generate errors. Not sure if there are performance considerations for those code paths

[rafiss]

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

---

pkg/sql/check.go line 439 at r1 (raw file):

		// If the context is cancelled, then we should bail out, since
		// the actual revalidate operation might not check anything.
		if err := ctx.Err(); err != nil {

i think sql-queries added this builtin so let's get them to sign off

---

pkg/sql/sem/builtins/builtins.go line 5768 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

@rafiss DecodeUntaggedUUIDValue and DecodeUntaggedIPAddrValue do not validate the input, so that they will hit out-of-bounds errors. The other approach is to update those code paths to generate errors. Not sure if there are performance considerations for those code paths

ah i see. i think we should actually do the recover inside of the PrettyPrint function, that way the error reporting is consistent with other kinds of errors that can happen in that function.

[fqazi]

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

---

pkg/sql/sem/builtins/builtins.go line 5768 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah i see. i think we should actually do the recover inside of the PrettyPrint function, that way the error reporting is consistent with other kinds of errors that can happen in that function.

Done.

[rafiss]

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

[fqazi]

bors r+

[fqazi]

bors r-

[fqazi]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[fqazi]

blathers backport 23.1

[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 cf8fb9c to blathers/backport-release-23.1-107491: 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 23.1 failed. See errors above.

---

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