From 242758a8058e0ea5009a302dcef8f56b6d802748 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 1 Nov 2022 16:44:35 -0700 Subject: [PATCH 1/3] colexecargs: fix the monitor names In 649113dbbc567efa0198c903cf07407ef00d0b7b 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. Release note: None --- pkg/sql/colexec/colexecargs/monitor_registry.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/sql/colexec/colexecargs/monitor_registry.go b/pkg/sql/colexec/colexecargs/monitor_registry.go index 14533c52aeaf..e6c5038af70a 100644 --- a/pkg/sql/colexec/colexecargs/monitor_registry.go +++ b/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 From 030204cd7054944d95ad30a8893dccb38595bf6f Mon Sep 17 00:00:00 2001 From: Mark Sirek Date: Fri, 7 Oct 2022 15:08:25 -0700 Subject: [PATCH 2/3] opt: avoid looking up MultiregionConfig for non-multiregion tables This avoids expensive lookups of the regions in a table's parent database for non-multiregion tables. Release note: None --- pkg/sql/opt/cat/table.go | 4 ++++ pkg/sql/opt/exec/explain/plan_gist_factory.go | 5 +++++ pkg/sql/opt/metadata_test.go | 1 + pkg/sql/opt/table_meta.go | 19 ++++++++++++++----- pkg/sql/opt/testutils/testcat/test_catalog.go | 14 ++++++++++++++ pkg/sql/opt_catalog.go | 11 +++++++++++ 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/pkg/sql/opt/cat/table.go b/pkg/sql/opt/cat/table.go index 0b427b76c7b8..346963dab37f 100644 --- a/pkg/sql/opt/cat/table.go +++ b/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. diff --git a/pkg/sql/opt/exec/explain/plan_gist_factory.go b/pkg/sql/opt/exec/explain/plan_gist_factory.go index 59df67ee0d30..ff50b111becb 100644 --- a/pkg/sql/opt/exec/explain/plan_gist_factory.go +++ b/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 diff --git a/pkg/sql/opt/metadata_test.go b/pkg/sql/opt/metadata_test.go index 0d26eb58be54..fdf25e2afd21 100644 --- a/pkg/sql/opt/metadata_test.go +++ b/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. diff --git a/pkg/sql/opt/table_meta.go b/pkg/sql/opt/table_meta.go index 1ae3156beb0e..44a8d47687d3 100644 --- a/pkg/sql/opt/table_meta.go +++ b/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) diff --git a/pkg/sql/opt/testutils/testcat/test_catalog.go b/pkg/sql/opt/testutils/testcat/test_catalog.go index f6b318b7705d..5a3a8cd99d6d 100644 --- a/pkg/sql/opt/testutils/testcat/test_catalog.go +++ b/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 diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index 3f2c01c1b24a..a03cb86b9d59 100644 --- a/pkg/sql/opt_catalog.go +++ b/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 From fc5a662ce8d6680ac8ad83098b1b7a7f2f67b9e6 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 8 Nov 2022 10:09:10 -0800 Subject: [PATCH 3/3] Revert "build/nightlies: enable crdb_test on sqllite logic tests" This reverts commit a4e8bd232611a71c1fc96f92cd0b1e1d9ae52b9e. Release note: None --- build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh b/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh index a31ac65ba5b2..0b825bce5656 100755 --- a/build/teamcity/cockroach/nightlies/sqlite_logic_test_impl.sh +++ b/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=$?