Merge #89590 #91108 #91531

89590: opt: avoid looking up MultiregionConfig for non-multiregion tables r=rytaft a=msirek

This avoids expensive lookups of the regions in a table's parent database for non-multiregion tables.

Epic: CRDB-18645

Release note: None

91108: colexecargs: fix the monitor names r=yuzefovich a=yuzefovich

In 649113d we removed the dashes between different parts of the monitor names by mistake, so we now have something like `sort-all0limited0` instead of `sort-all-0-limited-0`. This is now fixed.

Epic: None

Release note: None

91531: Revert "build/nightlies: enable crdb_test on sqllite logic tests" r=yuzefovich a=yuzefovich

This reverts commit a4e8bd2.

Informs: #91486

Epic: None

Release note: None

Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh

@@ -7,7 +7,7 @@ dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))"
 bazel build //pkg/cmd/bazci --config=ci
 BAZEL_BIN=$(bazel info bazel-bin --config=ci)
 exit_status=0
-$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci -- test --config=ci \
+$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci -- test --config=ci --config=crdb_test_off \
     //pkg/sql/sqlitelogictest/tests/... \
     --test_arg -bigtest --test_arg -flex-types --test_timeout 86400 \
     || exit_status=$?

pkg/sql/colexec/colexecargs/monitor_registry.go

@@ -46,8 +46,8 @@ func (r *MonitorRegistry) NewStreamingMemAccount(flowCtx *execinfra.FlowCtx) *mo
 func (r *MonitorRegistry) getMemMonitorName(
 	opName redact.RedactableString, processorID int32, suffix redact.RedactableString,
 ) redact.RedactableString {
-	return opName + redact.RedactableString(strconv.Itoa(int(processorID))) + suffix +
-		redact.RedactableString(strconv.Itoa(len(r.monitors)))
+	return opName + "-" + redact.RedactableString(strconv.Itoa(int(processorID))) + "-" +
+		suffix + "-" + redact.RedactableString(strconv.Itoa(len(r.monitors)))
 }
 
 // CreateMemAccountForSpillStrategy instantiates a memory monitor and a memory

pkg/sql/opt/cat/table.go

@@ -162,6 +162,10 @@ type Table interface {
 	// BY ROW.
 	IsRegionalByRow() bool
 
+	// IsMultiregion returns true if the table is a Multiregion table, defined
+	// with one of the LOCALITY clauses.
+	IsMultiregion() bool
+
 	// HomeRegionColName returns the name of the crdb_internal_region column name
 	// specifying the home region of each row in the table, if this table is a
 	// REGIONAL BY ROW TABLE, otherwise "", false is returned.

pkg/sql/opt/exec/explain/plan_gist_factory.go

@@ -596,6 +596,11 @@ func (u *unknownTable) IsRegionalByRow() bool {
 	return false
 }
 
+// IsMultiregion is part of the cat.Table interface.
+func (u *unknownTable) IsMultiregion() bool {
+	return false
+}
+
 // HomeRegionColName is part of the cat.Table interface.
 func (u *unknownTable) HomeRegionColName() (colName string, ok bool) {
 	return "", false

pkg/sql/opt/metadata_test.go

@@ -427,6 +427,7 @@ func TestTableMeta_GetRegionsInDatabase(t *testing.T) {
 	tab.DatabaseID = 1 // must be non-zero to trigger the region lookup
 	a := md.AddTable(tab, tn)
 	tabMeta := md.TableMeta(a)
+	tab.SetMultiRegion(true)
 
 	p := &fakeGetMultiregionConfigPlanner{}
 	// Call the function once, make sure our planner method gets invoked.

pkg/sql/opt/table_meta.go

@@ -470,8 +470,7 @@ func (tm *TableMeta) GetRegionsInDatabase(
 			)
 		}
 	}()
-
-	if dbID == 0 {
+	if dbID == 0 || !tm.Table.IsMultiregion() {
 		return nil /* regionNames */, false
 	}
 	regionConfig, ok := planner.GetMultiregionConfig(ctx, dbID)
@@ -502,11 +501,21 @@ func (tm *TableMeta) GetDatabaseSurvivalGoal(
 		return multiregionConfig.SurvivalGoal(), true
 	}
 	dbID := tm.Table.GetDatabaseID()
+	defer func() {
+		if !ok {
+			tm.SetTableAnnotation(
+				regionConfigAnnID,
+				// Use a nil pointer to a RegionConfig, which is distinct from the
+				// untyped nil and will be detected in the type assertion above.
+				(*multiregion.RegionConfig)(nil),
+			)
+		}
+	}()
+	if dbID == 0 || !tm.Table.IsMultiregion() {
+		return descpb.SurvivalGoal_ZONE_FAILURE /* regionNames */, false
+	}
 	regionConfig, ok := planner.GetMultiregionConfig(ctx, dbID)
 	if !ok {
-		// Use a nil pointer to a RegionConfig, which is distinct from the
-		// untyped nil and will be detected in the type assertion above.
-		tm.SetTableAnnotation(regionConfigAnnID, (*multiregion.RegionConfig)(nil))
 		return descpb.SurvivalGoal_ZONE_FAILURE /* survivalGoal */, false
 	}
 	multiregionConfig, _ = regionConfig.(*multiregion.RegionConfig)

pkg/sql/opt/testutils/testcat/test_catalog.go

@@ -693,6 +693,8 @@ type Table struct {
 	// partitionBy is the partitioning clause that corresponds to the primary
 	// index. Used to initialize the partitioning for the primary index.
 	partitionBy *tree.PartitionBy
+
+	multiRegion bool
 }
 
 var _ cat.Table = &Table{}
@@ -703,6 +705,13 @@ func (tt *Table) String() string {
 	return tp.String()
 }
 
+// SetMultiRegion can make a table in the test catalog appear to be a
+// multiregion table, in that it can cause cat.Table.IsMultiregion() to return
+// true after SetMultiRegion(true) has been called.
+func (tt *Table) SetMultiRegion(val bool) {
+	tt.multiRegion = val
+}
+
 // ID is part of the cat.DataSource interface.
 func (tt *Table) ID() cat.StableID {
 	return tt.TabID
@@ -863,6 +872,11 @@ func (tt *Table) IsRegionalByRow() bool {
 	return false
 }
 
+// IsMultiregion is part of the cat.Table interface.
+func (tt *Table) IsMultiregion() bool {
+	return tt.multiRegion
+}
+
 // HomeRegionColName is part of the cat.Table interface.
 func (tt *Table) HomeRegionColName() (colName string, ok bool) {
 	return "", false

pkg/sql/opt_catalog.go

@@ -1306,6 +1306,12 @@ func (ot *optTable) IsRegionalByRow() bool {
 	return localityConfig.GetRegionalByRow() != nil
 }
 
+// IsMultiregion is part of the cat.Table interface.
+func (ot *optTable) IsMultiregion() bool {
+	localityConfig := ot.desc.GetLocalityConfig()
+	return localityConfig != nil
+}
+
 // HomeRegionColName is part of the cat.Table interface.
 func (ot *optTable) HomeRegionColName() (colName string, ok bool) {
 	localityConfig := ot.desc.GetLocalityConfig()
@@ -2285,6 +2291,11 @@ func (ot *optVirtualTable) IsRegionalByRow() bool {
 	return false
 }
 
+// IsMultiregion is part of the cat.Table interface.
+func (ot *optVirtualTable) IsMultiregion() bool {
+	return false
+}
+
 // HomeRegionColName is part of the cat.Table interface.
 func (ot *optVirtualTable) HomeRegionColName() (colName string, ok bool) {
 	return "", false