Merge #83249 #83455 #83462

83249: authors: add Jason Chen to authors r=jason-crl a=jason-crl

Release note: None

83455: sql/schemachanger/rel: increase ordinalSet size to 32-bit, test excee… r=ajwerner a=ajwerner

…ding it

Before this change, we were butting up against the 16-attribute limit for a
schema. When it was exceeded, nothing happend except that ordinal values in
the ordinalSet were dropped silently. We now validate that a schema does not
have too many attributes when constructing the schema.

Along the way, we also increase the limit from 14 to 30.

Release note: None

83462: multi-tenant: Fix MT testing probability r=knz a=ajstorm

The changes in 76582 introduced probabilistic testing within tenants. That PR
should have enabled testing in tenants with probability 0.5. A late change was
made in that PR which accidentally merged probability 1.0 - meaning that _all_
testing is now occurring in tenants. This PR reverts that change, and applies
the intended 0.5 probability so that half of the time we're testing outside of
tenants.

Release note: None

Co-authored-by: Jason Chen <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Adam Storm <[email protected]>

AUTHORS

@@ -200,6 +200,7 @@ Jan Owsiany <[email protected]>
 Jane Xing <[email protected]> <[email protected]>
 Jason E. Aten <[email protected]>
 Jason Chan <[email protected]> <[email protected]>
+Jason Chen <[email protected]>
 Jason Young <[email protected]>
 Jay Kominek <[email protected]>
 Jay Lim <[email protected]> <[email protected]> <[email protected]>

pkg/sql/schemachanger/rel/BUILD.bazel

@@ -48,6 +48,7 @@ go_test(
     size = "small",
     srcs = [
         "bench_test.go",
+        "ordinal_set_test.go",
         "rel_internal_test.go",
         "rel_test.go",
     ],

pkg/sql/schemachanger/rel/ordinal_set.go

@@ -10,15 +10,21 @@
 
 package rel
 
-import "math/bits"
+import (
+	"math/bits"
+	"unsafe"
+)
 
 // ordinal is used to correlate attributes in a schema.
 // It enables use of the ordinalSet.
 type ordinal uint8
 
 // ordinalSet represents A bitmask over ordinals.
-// Note that it cannot contain attributes with ordinals greater than 15.
-type ordinalSet uint16
+// Note that it cannot contain attributes with ordinals greater than 30.
+type ordinalSet uint32
+
+const ordinalSetCapacity = (unsafe.Sizeof(ordinalSet(0)) * 8)
+const ordinalSetMaxOrdinal = ordinal(ordinalSetCapacity - 1)
 
 // ForEach iterates the set of attributes.
 func (m ordinalSet) forEach(f func(a ordinal) (wantMore bool)) {
@@ -64,12 +70,12 @@ func (m ordinalSet) union(other ordinalSet) ordinalSet {
 
 // len returns the number of ordinals in the set.
 func (m ordinalSet) len() int {
-	return bits.OnesCount16(uint16(m))
+	return bits.OnesCount32(uint32(m))
 }
 
 // rank returns the rank in the set of the passed ordinal.
 func (m ordinalSet) rank(a ordinal) int {
-	return bits.OnesCount16(uint16(m & ((1 << a) - 1)))
+	return bits.OnesCount32(uint32(m & ((1 << a) - 1)))
 }
 
 // isContainedIn returns true of m contains every element of other.

pkg/sql/schemachanger/rel/ordinal_set_test.go

@@ -0,0 +1,30 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package rel
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestOrdinalSetCapacity ensures that ordinalSet operations
+func TestOrdinalSetCapacity(t *testing.T) {
+	var os ordinalSet
+	for o := ordinal(0); o <= ordinalSetMaxOrdinal; o++ {
+		added := os.add(o)
+		require.NotEqual(t, os, added)
+		added = os
+	}
+
+	// Adding a value which is out-of-range should be a no-op.
+	require.Equal(t, os, os.add(ordinalSetMaxOrdinal+1))
+}

pkg/sql/schemachanger/rel/rel_test.go

@@ -173,14 +173,74 @@ func TestInvalidData(t *testing.T) {
 	})
 }
 
+// TestTooManyAttributesInSchema tests that a schema with too many attributes
+// causes an error.
+func TestTooManyAttributesInSchema(t *testing.T) {
+	// At time of writing, there are two system attributes. That leaves 30
+	// bits of the 32-bit ordinal set for use by the user.
+	type tooManyAttrs struct {
+		F0, F1, F2, F3, F4, F5, F6, F7, F8, F9           *uint32
+		F10, F11, F12, F13, F14, F15, F16, F17, F18, F19 *uint32
+		F20, F21, F22, F23, F24, F25, F26, F27, F28, F29 *uint32
+		F30                                              *uint32
+	}
+	justEnoughMappings := []rel.EntityMappingOption{
+		rel.EntityAttr(stringAttr("A0"), "F0"),
+		rel.EntityAttr(stringAttr("A1"), "F1"),
+		rel.EntityAttr(stringAttr("A2"), "F2"),
+		rel.EntityAttr(stringAttr("A3"), "F3"),
+		rel.EntityAttr(stringAttr("A4"), "F4"),
+		rel.EntityAttr(stringAttr("A5"), "F5"),
+		rel.EntityAttr(stringAttr("A6"), "F6"),
+		rel.EntityAttr(stringAttr("A7"), "F7"),
+		rel.EntityAttr(stringAttr("A8"), "F8"),
+		rel.EntityAttr(stringAttr("A9"), "F9"),
+		rel.EntityAttr(stringAttr("A10"), "F10"),
+		rel.EntityAttr(stringAttr("A11"), "F11"),
+		rel.EntityAttr(stringAttr("A12"), "F12"),
+		rel.EntityAttr(stringAttr("A13"), "F13"),
+		rel.EntityAttr(stringAttr("A14"), "F14"),
+		rel.EntityAttr(stringAttr("A15"), "F15"),
+		rel.EntityAttr(stringAttr("A16"), "F16"),
+		rel.EntityAttr(stringAttr("A17"), "F17"),
+		rel.EntityAttr(stringAttr("A18"), "F18"),
+		rel.EntityAttr(stringAttr("A19"), "F19"),
+		rel.EntityAttr(stringAttr("A20"), "F20"),
+		rel.EntityAttr(stringAttr("A21"), "F21"),
+		rel.EntityAttr(stringAttr("A22"), "F22"),
+		rel.EntityAttr(stringAttr("A23"), "F23"),
+		rel.EntityAttr(stringAttr("A24"), "F24"),
+		rel.EntityAttr(stringAttr("A25"), "F25"),
+		rel.EntityAttr(stringAttr("A26"), "F26"),
+		rel.EntityAttr(stringAttr("A27"), "F27"),
+		rel.EntityAttr(stringAttr("A28"), "F28"),
+		rel.EntityAttr(stringAttr("A29"), "F29"),
+	}
+	{
+		_, err := rel.NewSchema("just_enough",
+			rel.EntityMapping(reflect.TypeOf((*tooManyAttrs)(nil)), justEnoughMappings...),
+		)
+		require.NoError(t, err)
+	}
+	{
+		_, err := rel.NewSchema("too_many",
+			rel.EntityMapping(reflect.TypeOf((*tooManyAttrs)(nil)),
+				append(justEnoughMappings, rel.EntityAttr(stringAttr("A30"), "F30"))...,
+			),
+		)
+		require.Regexp(t, "too many attributes", err)
+	}
+}
+
 type stringAttr string
 
 func (sa stringAttr) String() string { return string(sa) }
 
-// TestTooManyAttributes tests that errors are returned whenever a predicate
-// is constructed with too many attributes. One could imagine disallowing this
-// altogether in the schema, but it's somewhat onerous to do so.
-func TestTooManyAttributes(t *testing.T) {
+// TestTooManyAttributesInValues tests that errors are returned whenever a
+// predicate is constructed with too many attributes. One could imagine
+// disallowing this altogether in the schema, but it's somewhat onerous to do
+// so.
+func TestTooManyAttributesInValues(t *testing.T) {
 	type tooManyAttrs struct {
 		F1, F2, F3, F4, F5, F6, F7, F8 *uint32
 	}

pkg/sql/schemachanger/rel/schema.go

@@ -151,7 +151,7 @@ func (sb *schemaBuilder) maybeAddAttribute(a Attr, typ reflect.Type) ordinal {
 	ord, exists := sb.attrToOrdinal[a]
 	if !exists {
 		ord = ordinal(len(sb.attrs))
-		if ord >= maxUserAttribute {
+		if ord > maxOrdinal {
 			panic(errors.Errorf("too many attributes"))
 		}
 		sb.attrs = append(sb.attrs, a)

pkg/sql/schemachanger/rel/system_attributes.go

@@ -23,15 +23,19 @@ type systemAttribute int8
 //go:generate stringer -type systemAttribute
 
 const (
-	_ systemAttribute = 64 - iota
+	// Note that the value of the system attribute here is not relevant because
+	// inside a given schema, each attribute is mapped internally to an ordinal
+	// independently.
+	_ systemAttribute = iota
 
 	// Type is an attribute which stores a value's type.
 	Type
 
 	// Self is an attribute which stores the variable itself.
 	Self
-
-	maxUserAttribute ordinal = 64 - iota
 )
 
 var _ Attr = systemAttribute(0)
+
+// maxOrdinal is the maximum ordinal value an attribute in a schema may use.
+const maxOrdinal = ordinalSetMaxOrdinal

pkg/testutils/serverutils/test_server_shim.go

@@ -78,7 +78,7 @@ const (
 // If both the environment variable and the test flag are set, the environment
 // variable wins out.
 func ShouldStartDefaultTestTenant(t testing.TB) bool {
-	const defaultProbabilityOfStartingTestTenant = 1.0
+	const defaultProbabilityOfStartingTestTenant = 0.5
 	var probabilityOfStartingDefaultTestTenant float64
 
 	tenantModeTestString, envSet := envutil.EnvString("COCKROACH_TEST_TENANT_MODE", 0)