sql: Fix bug in zone constraint validation for attributes.

[rohany]

Fixes #39800.

Release note: None

[cockroach-teamcity]

This change is 

[solongordon]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 535 at r1 (raw file):

			// Don't perform any validation for pairs with an empty key.
			// These attributes are not real key value pairs, and instead
			// are node attributes, such as +ssd.

Would it still make sense to check for conflicting attributes, like +ssd,-ssd?

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @solongordon)

---

pkg/sql/set_zone_config.go, line 535 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Would it still make sense to check for conflicting attributes, like +ssd,-ssd?

you're right. PTAL

[solongordon]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 537 at r2 (raw file):

				// are attributes, due to the keys being the same for attributes.
				if curr.Key == "" && other.Key == "" {
					if curr.Value == other.Value {

Nit: You can combine these into a single if. Also maybe we should also check that curr.Type != other.Type? ["+ssd", "+ssd"] is redundant but not really incompatible.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @solongordon)

---

pkg/sql/set_zone_config.go, line 537 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Nit: You can combine these into a single if. Also maybe we should also check that curr.Type != other.Type? ["+ssd", "+ssd"] is redundant but not really incompatible.

I had that originally, but we don't want that -- if the values are not equal (and the keys are both empty), then we fall into the other case and perform an incorrect validation sequence on the attributes. I can update the comment if that isn't clear.

I think we want to keep that +ssd and +ssd is invalid -- to me it looks like its a user error, and they should be notified (for example forgot to copy the correct node attributes in or something).

[solongordon]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 537 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I had that originally, but we don't want that -- if the values are not equal (and the keys are both empty), then we fall into the other case and perform an incorrect validation sequence on the attributes. I can update the comment if that isn't clear.

I think we want to keep that +ssd and +ssd is invalid -- to me it looks like its a user error, and they should be notified (for example forgot to copy the correct node attributes in or something).

Ah right, ok works for me.

[rohany]

bors r=solongordon

[craig]

Build succeeded

• GitHub CI (Cockroach)