Merge #140167 #140218 #140244

140167: sql: account for duplicate partition names in subzone span gen r=annrpom a=annrpom

This patch fixes a bug during GenerateSubzoneSpans that does not
correctly distinguish between partitions of the same name across
indexes. The issue was introduced when we relaxed restrictions on
partition name reuse across indexes of a table (#39332).

Epic: [CRDB-43310](https://cockroachlabs.atlassian.net/browse/CRDB-43310)
Fixes: #128692

Release note (bug fix): Configuring replication controls on a
partition name of an index that is not unique across all indexes
will correctly impact only that partition.

140218: sql: fix quoting of session settings in stmt bundle env.sql r=mgartner a=mgartner

The values of session setting in the `env.sql` file of a statement
bundle are now wrapped in single quotes if they contain whitespace. The
statement bundle session setting tests have been updated to ensure that
`env.sql` is parsable.

Epic: None

Release note: None


140244: storage: remove parameter from RegisterEngines r=RaduBerinde a=andrewbaptist

Previously both the StoreSpec and the Engines were passed into RegisterEngines. With this change we get the directory directly from the Engine.

Epic: none

Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>

Author: 4 people

pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/partitions

@@ -253,3 +253,26 @@ translate database=db table=part
 /Table/111/2/{51-60}                       range default
 /Table/111/2/6{0-1}                        num_replicas=11
 /Table/11{1/2/61-2}                        range default
+
+exec-sql
+CREATE TABLE db.foo(i INT PRIMARY KEY, j INT) PARTITION BY LIST (i) (
+  PARTITION p VALUES IN (1, 5)
+);
+CREATE INDEX idx ON db.foo(j) PARTITION BY LIST (j) (
+  PARTITION p VALUES IN (2, 4)
+);
+ALTER PARTITION p OF INDEX db.foo@foo_pkey CONFIGURE ZONE USING gc.ttlseconds = 2;
+ALTER PARTITION p OF INDEX db.foo@idx CONFIGURE ZONE USING gc.ttlseconds = 5;
+----
+
+translate database=db table=foo
+----
+/Table/112{-/1/1}                          range default
+/Table/112/1/{1-2}                         ttl_seconds=2
+/Table/112/1/{2-5}                         range default
+/Table/112/1/{5-6}                         ttl_seconds=2
+/Table/112/{1/6-2/2}                       range default
+/Table/112/2/{2-3}                         ttl_seconds=5
+/Table/112/2/{3-4}                         range default
+/Table/112/2/{4-5}                         ttl_seconds=5
+/Table/11{2/2/5-3}                         range default

pkg/server/debug/BUILD.bazel

@@ -12,7 +12,6 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/server/debug",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/base",
         "//pkg/base/serverident",
         "//pkg/kv/kvserver",
         "//pkg/kv/kvserver/closedts/sidetransport",

pkg/server/debug/server.go

@@ -14,7 +14,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/base/serverident"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/closedts/sidetransport"
@@ -251,12 +250,7 @@ func FormatLSMStats(stats map[roachpb.StoreID]string) string {
 }
 
 // RegisterEngines setups up debug engine endpoints for the known storage engines.
-func (ds *Server) RegisterEngines(specs []base.StoreSpec, engines []storage.Engine) error {
-	if len(specs) != len(engines) {
-		// TODO(yevgeniy): Consider adding accessors to storage.Engine to get their path.
-		return errors.New("number of store specs must match number of engines")
-	}
-
+func (ds *Server) RegisterEngines(engines []storage.Engine) error {
 	ds.mux.HandleFunc("/debug/lsm", func(w http.ResponseWriter, req *http.Request) {
 		stats, err := GetLSMStats(engines)
 		if err != nil {
@@ -265,18 +259,18 @@ func (ds *Server) RegisterEngines(specs []base.StoreSpec, engines []storage.Engi
 		fmt.Fprint(w, FormatLSMStats(stats))
 	})
 
-	for i := 0; i < len(specs); i++ {
-		if specs[i].InMemory {
+	for _, eng := range engines {
+		dir := eng.Env().Dir
+		if dir == "" {
 			// TODO(yevgeniy): Add plumbing to support LSM visualization for in memory engines.
 			continue
 		}
 
-		storeID, err := engines[i].GetStoreID()
+		storeID, err := eng.GetStoreID()
 		if err != nil {
 			return err
 		}
 
-		dir := specs[i].Path
 		ds.mux.HandleFunc(fmt.Sprintf("/debug/lsm-viz/%d", storeID),
 			func(w http.ResponseWriter, req *http.Request) {
 				if err := analyzeLSM(dir, w); err != nil {

pkg/server/server.go

@@ -2103,7 +2103,7 @@ func (s *topLevelServer) PreStart(ctx context.Context) error {
 	}
 
 	// Register the engines debug endpoints.
-	if err := s.debug.RegisterEngines(s.cfg.Stores.Specs, s.engines); err != nil {
+	if err := s.debug.RegisterEngines(s.engines); err != nil {
 		return errors.Wrapf(err, "failed to register engines with debug server")
 	}
 

pkg/sql/explain_bundle.go

@@ -822,6 +822,8 @@ func init() {
 	binarySVForBundle = &st.SV
 }
 
+var anyWhitespace = regexp.MustCompile(`\s+`)
+
 // PrintSessionSettings appends information about all session variables that
 // differ from their defaults.
 //
@@ -900,7 +902,7 @@ func (c *stmtEnvCollector) PrintSessionSettings(w io.Writer, sv *settings.Values
 		if skip && !all {
 			continue
 		}
-		if _, ok := sessionVarNeedsEscaping[varName]; ok {
+		if _, ok := sessionVarNeedsEscaping[varName]; ok || anyWhitespace.MatchString(value) {
 			value = lexbase.EscapeSQLString(value)
 		}
 		if value == "" {

pkg/sql/explain_bundle_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/pgtest"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -222,6 +223,7 @@ CREATE TABLE users(id UUID DEFAULT gen_random_uuid() PRIMARY KEY, promo_id INT R
 			{"testing_optimizer_random_seed", "123"},
 			{"timezone", "+8"},
 			{"unconstrained_non_covering_index_scan_enabled", "on"},
+			{"default_transaction_isolation", "'read committed'"},
 		}
 		for _, tc := range testcases {
 			t.Run(tc.sessionVar, func(t *testing.T) {
@@ -235,6 +237,9 @@ CREATE TABLE users(id UUID DEFAULT gen_random_uuid() PRIMARY KEY, promo_id INT R
 							if reg.FindString(contents) == "" {
 								return errors.Errorf("could not find 'SET %s' in env.sql", tc.sessionVar)
 							}
+							if _, err := parser.Parse(contents); err != nil {
+								return errors.Wrap(err, "could not parse env.sql")
+							}
 						}
 						return nil
 					}, false, /* expectErrors */

pkg/sql/partition_utils.go

@@ -18,6 +18,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 )
 
+// partitionKey is used to group a partition's name and its index ID for
+// indexing into a map.
+type partitionKey struct {
+	indexID descpb.IndexID
+	name    string
+}
+
 // GenerateSubzoneSpans constructs from a TableDescriptor the entries mapping
 // zone config spans to subzones for use in the SubzoneSpans field of
 // zonepb.ZoneConfig. SubzoneSpans controls which splits are created, so only
@@ -76,10 +83,11 @@ func GenerateSubzoneSpans(
 	a := &tree.DatumAlloc{}
 
 	subzoneIndexByIndexID := make(map[descpb.IndexID]int32)
-	subzoneIndexByPartition := make(map[string]int32)
+	subzoneIndexByPartition := make(map[partitionKey]int32)
 	for i, subzone := range subzones {
 		if len(subzone.PartitionName) > 0 {
-			subzoneIndexByPartition[subzone.PartitionName] = int32(i)
+			partKey := partitionKey{indexID: descpb.IndexID(subzone.IndexID), name: subzone.PartitionName}
+			subzoneIndexByPartition[partKey] = int32(i)
 		} else {
 			subzoneIndexByIndexID[descpb.IndexID(subzone.IndexID)] = int32(i)
 		}
@@ -140,7 +148,8 @@ func GenerateSubzoneSpans(
 		}
 		var ok bool
 		if subzone := payloads[0].(zonepb.Subzone); len(subzone.PartitionName) > 0 {
-			subzoneSpan.SubzoneIndex, ok = subzoneIndexByPartition[subzone.PartitionName]
+			partKey := partitionKey{indexID: descpb.IndexID(subzone.IndexID), name: subzone.PartitionName}
+			subzoneSpan.SubzoneIndex, ok = subzoneIndexByPartition[partKey]
 		} else {
 			subzoneSpan.SubzoneIndex, ok = subzoneIndexByIndexID[descpb.IndexID(subzone.IndexID)]
 		}
@@ -165,7 +174,7 @@ func indexCoveringsForPartitioning(
 	tableDesc catalog.TableDescriptor,
 	idx catalog.Index,
 	part catalog.Partitioning,
-	relevantPartitions map[string]int32,
+	relevantPartitions map[partitionKey]int32,
 	prefixDatums []tree.Datum,
 ) ([]covering.Covering, error) {
 	if part.NumColumns() == 0 {
@@ -191,10 +200,11 @@ func indexCoveringsForPartitioning(
 				if err != nil {
 					return err
 				}
-				if _, ok := relevantPartitions[name]; ok {
+				partKey := partitionKey{indexID: idx.GetID(), name: name}
+				if _, ok := relevantPartitions[partKey]; ok {
 					listCoverings[len(t.Datums)] = append(listCoverings[len(t.Datums)], covering.Range{
 						Start: keyPrefix, End: roachpb.Key(keyPrefix).PrefixEnd(),
-						Payload: zonepb.Subzone{PartitionName: name},
+						Payload: zonepb.Subzone{IndexID: uint32(idx.GetID()), PartitionName: name},
 					})
 				}
 				newPrefixDatums := append(prefixDatums, t.Datums...)
@@ -219,7 +229,8 @@ func indexCoveringsForPartitioning(
 
 	if part.NumRanges() > 0 {
 		err := part.ForEachRange(func(name string, from, to []byte) error {
-			if _, ok := relevantPartitions[name]; !ok {
+			partKey := partitionKey{indexID: idx.GetID(), name: name}
+			if _, ok := relevantPartitions[partKey]; !ok {
 				return nil
 			}
 			_, fromKey, err := rowenc.DecodePartitionTuple(
@@ -232,10 +243,10 @@ func indexCoveringsForPartitioning(
 			if err != nil {
 				return err
 			}
-			if _, ok := relevantPartitions[name]; ok {
+			if _, ok := relevantPartitions[partKey]; ok {
 				coverings = append(coverings, covering.Covering{{
 					Start: fromKey, End: toKey,
-					Payload: zonepb.Subzone{PartitionName: name},
+					Payload: zonepb.Subzone{IndexID: uint32(idx.GetID()), PartitionName: name},
 				}})
 			}
 			return nil

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go

@@ -710,6 +710,13 @@ func accumulateNewUniqueConstraints(currentZone, newZone *zonepb.ZoneConfig) []z
 	return retConstraints
 }
 
+// partitionKey is used to group a partition's name and its index ID for
+// indexing into a map.
+type partitionKey struct {
+	indexID descpb.IndexID
+	name    string
+}
+
 // generateSubzoneSpans constructs from a TableID the entries mapping
 // zone config spans to subzones for use in the SubzoneSpans field of
 // zonepb.ZoneConfig. SubzoneSpans controls which splits are created, so only
@@ -764,10 +771,11 @@ func generateSubzoneSpans(
 	}
 
 	subzoneIndexByIndexID := make(map[descpb.IndexID]int32)
-	subzoneIndexByPartition := make(map[string]int32)
+	subzoneIndexByPartition := make(map[partitionKey]int32)
 	for i, subzone := range subzones {
 		if len(subzone.PartitionName) > 0 {
-			subzoneIndexByPartition[subzone.PartitionName] = int32(i)
+			partKey := partitionKey{indexID: descpb.IndexID(subzone.IndexID), name: subzone.PartitionName}
+			subzoneIndexByPartition[partKey] = int32(i)
 		} else {
 			subzoneIndexByIndexID[descpb.IndexID(subzone.IndexID)] = int32(i)
 		}
@@ -827,7 +835,8 @@ func generateSubzoneSpans(
 		}
 		var ok bool
 		if subzone := payloads[0].(zonepb.Subzone); len(subzone.PartitionName) > 0 {
-			subzoneSpan.SubzoneIndex, ok = subzoneIndexByPartition[subzone.PartitionName]
+			partKey := partitionKey{indexID: descpb.IndexID(subzone.IndexID), name: subzone.PartitionName}
+			subzoneSpan.SubzoneIndex, ok = subzoneIndexByPartition[partKey]
 		} else {
 			subzoneSpan.SubzoneIndex, ok = subzoneIndexByIndexID[descpb.IndexID(subzone.IndexID)]
 		}
@@ -853,7 +862,7 @@ func indexCoveringsForPartitioning(
 	indexID catid.IndexID,
 	index []*scpb.IndexColumn,
 	part catalog.Partitioning,
-	relevantPartitions map[string]int32,
+	relevantPartitions map[partitionKey]int32,
 	prefixDatums []tree.Datum,
 ) ([]covering.Covering, error) {
 	if part.NumColumns() == 0 {
@@ -879,10 +888,11 @@ func indexCoveringsForPartitioning(
 				if err != nil {
 					return err
 				}
-				if _, ok := relevantPartitions[name]; ok {
+				partKey := partitionKey{indexID: indexID, name: name}
+				if _, ok := relevantPartitions[partKey]; ok {
 					listCoverings[len(t.Datums)] = append(listCoverings[len(t.Datums)], covering.Range{
 						Start: keyPrefix, End: roachpb.Key(keyPrefix).PrefixEnd(),
-						Payload: zonepb.Subzone{PartitionName: name},
+						Payload: zonepb.Subzone{IndexID: uint32(indexID), PartitionName: name},
 					})
 				}
 				newPrefixDatums := append(prefixDatums, t.Datums...)
@@ -907,7 +917,8 @@ func indexCoveringsForPartitioning(
 
 	if part.NumRanges() > 0 {
 		err := part.ForEachRange(func(name string, from, to []byte) error {
-			if _, ok := relevantPartitions[name]; !ok {
+			partKey := partitionKey{indexID: indexID, name: name}
+			if _, ok := relevantPartitions[partKey]; !ok {
 				return nil
 			}
 			_, fromKey, err := decodePartitionTuple(
@@ -920,10 +931,10 @@ func indexCoveringsForPartitioning(
 			if err != nil {
 				return err
 			}
-			if _, ok := relevantPartitions[name]; ok {
+			if _, ok := relevantPartitions[partKey]; ok {
 				coverings = append(coverings, covering.Covering{{
 					Start: fromKey, End: toKey,
-					Payload: zonepb.Subzone{PartitionName: name},
+					Payload: zonepb.Subzone{IndexID: uint32(indexID), PartitionName: name},
 				}})
 			}
 			return nil