sql: validate against array type usages when dropping enum values

[arulajmani]

Previously, when dropping an enum value, we weren't validating if the
enum value was used in a column of array type. This patch fixes the bug.

Fixes #60004

Release justification: bug fix to new functionality
Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @otan)

---

pkg/sql/type_change.go, line 849 at r1 (raw file):

		// Construct a query of the form:
		// SELECT * FROM [

I couldn't find an easier/less expensive way of doing this validation.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/sql/type_change.go, line 849 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I couldn't find an easier/less expensive way of doing this validation.

why square brackets on the outer one? can't this just be subqueries? Also, can we just do a UNION and put the where on the outside?

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @otan)

---

pkg/sql/type_change.go, line 849 at r1 (raw file):

Previously, ajwerner wrote…

why square brackets on the outer one? can't this just be subqueries? Also, can we just do a UNION and put the where on the outside?

Something like this?

[ajwerner]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/sql/type_change.go, line 849 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Something like this?

indeed

---

pkg/sql/type_change.go, line 733 at r2 (raw file):

Quoted 8 lines of code…

	var byteRep strings.Builder
	byteRep.WriteString("b'")
	for _, b := range bytes {
		byteRep.WriteByte(b)
	}
	byteRep.WriteString("'")
	return byteRep.String()
}

How tested is this? I'm scared that this is extremely wrong. https://play.golang.org/p/vG5lTcG3O4o

0, I believe, is absolutely a valid member for these things. FWIW that string is not empty but it does contain invisible 0-byte charcters. I'm way out of my element here, I have no idea what a b'' string is in cockroach and how exactly we treat it but I am scared about some bad behavior trying to round-trip this. Can we get a domain area expert to wave me off?

---

pkg/sql/type_change.go, line 851 at r2 (raw file):

		//  		SELECT unnest(c1) FROM [SELECT %d AS t]
		// 		UNION
		//			SELECT unnest(c2) FROM [SELECT %d AS t]

nit: spacing

---

pkg/sql/type_change.go, line 895 at r2 (raw file):

		}
		if len(rows) > 0 {
			return errors.Newf("could not remove enum value %q as it is being used by table %q",

nit: grab a fully qualified name.

Should we put a condition failure pgcode in here?

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/sql/type_change.go, line 733 at r2 (raw file):

Previously, ajwerner wrote…

	var byteRep strings.Builder
	byteRep.WriteString("b'")
	for _, b := range bytes {
		byteRep.WriteByte(b)
	}
	byteRep.WriteString("'")
	return byteRep.String()
}

How tested is this? I'm scared that this is extremely wrong. https://play.golang.org/p/vG5lTcG3O4o

0, I believe, is absolutely a valid member for these things. FWIW that string is not empty but it does contain invisible 0-byte charcters. I'm way out of my element here, I have no idea what a b'' string is in cockroach and how exactly we treat it but I am scared about some bad behavior trying to round-trip this. Can we get a domain area expert to wave me off?

I think we may be fine because the way we generate these physical representations, specifically

cockroach/pkg/sql/enum/enum.go

Line 88 in e050d6a

if mid == p {

, ensures that we never end up with a bunch of zeros as the bytes representation.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

---

pkg/sql/type_change.go, line 733 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I think we may be fine because the way we generate these physical representations, specifically

cockroach/pkg/sql/enum/enum.go

Line 88 in e050d6a

if mid == p {

, ensures that we never end up with a bunch of zeros as the bytes representation.

But wait, if I keep saying BEFORE won't it decrement to 1? I guess what I'm saying is less that 0 is itself specifically the problem but rather that these strings don't seem obviously to carry a valid encoding. What is b and can somebody confirm it works correctly in binary strings? What about across the wire

[arulajmani]

Fixed the byte string stuff as per what we discussed on Slack. TFTR!

bors r=ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @otan)

[craig]

Build succeeded:

• GitHub CI (Cockroach)