From 024c297877c1297e2ab83652873fbe86f08cedec Mon Sep 17 00:00:00 2001 From: Andrew Kimball Date: Wed, 10 Apr 2019 12:30:43 -0700 Subject: [PATCH] opt: Fix and improve zone-based index selection This PR fixes a couple of related issues with zone-based index selection. First, placeholder zones were incorrectly discarded. Placeholder zones are used when there are indexes with Constraints/Leaseholder Preferences on a table that doesn't have either of those things. The fix is to merge the indexes placeholder into the table zone. Second, the optimizer required that a prefix of the locality tiers match constraints. That is no longer required after this commit. For example, if locality=region=us;dc=east, then it now matches any of these constraint sets equally well: [+locality=region=us;+dc=east] [+dc=east] [+dc=east,+dc=west] A missing constraint match is OK (i.e. region=us), as long as a more specific locality tier matches (i.e. dc=east). Fixes #36642 Fixes #36644 Release note: None --- pkg/ccl/logictestccl/testdata/logic_test/zone | 82 ++++++- pkg/config/system.go | 41 +++- pkg/config/zone.go | 2 +- pkg/sql/opt/xform/coster.go | 227 +++++++++++------- pkg/sql/opt/xform/coster_test.go | 102 ++++++++ pkg/sql/opt/xform/testdata/coster/zone | 95 +++++--- pkg/sql/opt_catalog.go | 4 +- pkg/sql/zone_config.go | 12 +- 8 files changed, 431 insertions(+), 134 deletions(-) create mode 100644 pkg/sql/opt/xform/coster_test.go diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone index 94a8951f4252..5233bc3db32d 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/zone +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone @@ -209,6 +209,86 @@ scan · · statement ok DEALLOCATE p -# test for #36348; place this at the bottom of this file. + +# ------------------------------------------------------------------------------ +# Regression for issue #36642. Optimizer picked wrong index when the index had +# constraints / lease preferences, but the table had no zone config. +# ------------------------------------------------------------------------------ + +statement ok +CREATE TABLE t36642 ( + k INT PRIMARY KEY, + v STRING, + INDEX secondary (k) STORING (v), + INDEX tertiary (k) STORING (v) +); + +statement ok +ALTER INDEX t36642@secondary CONFIGURE ZONE USING lease_preferences='[[+region=test,+dc=dc1]]' + +query TTT retry +EXPLAIN SELECT * FROM t36642 WHERE k=10 +---- +scan · · +· table t36642@secondary +· spans /10-/11 + +statement ok +ALTER INDEX t36642@tertiary CONFIGURE ZONE USING lease_preferences='[[+region=test,+dc=dc1]]' + +statement ok +ALTER INDEX t36642@secondary CONFIGURE ZONE USING lease_preferences='[[+region=test,+dc=dc2]]' + +query TTT retry +EXPLAIN SELECT * FROM t36642 WHERE k=10 +---- +scan · · +· table t36642@tertiary +· spans /10-/11 + + +# ------------------------------------------------------------------------------ +# Regression for issue #36644. Allow matching constraints for leading locality +# tiers to be omitted. +# ------------------------------------------------------------------------------ + +statement ok +CREATE TABLE t36644 ( + k INT PRIMARY KEY, + v STRING, + INDEX secondary (k) STORING (v), + INDEX tertiary (k) STORING (v) +); + +statement ok +ALTER INDEX t36644@secondary +CONFIGURE ZONE USING constraints='[+region=test]', lease_preferences='[[+dc=dc1]]' + +query TTT retry +EXPLAIN SELECT * FROM t36644 WHERE k=10 +---- +scan · · +· table t36644@secondary +· spans /10-/11 + +statement ok +ALTER INDEX t36644@secondary CONFIGURE ZONE USING lease_preferences='[[+dc=dc3]]' + +statement ok +ALTER INDEX t36644@tertiary +CONFIGURE ZONE USING constraints='[+region=test]', lease_preferences='[[+dc=dc1]]' + +query TTT retry +EXPLAIN SELECT * FROM t36644 WHERE k=10 +---- +scan · · +· table t36644@tertiary +· spans /10-/11 + + +# ------------------------------------------------------------------------------ +# Regression test for #36348; place this at the bottom of this file. +# ------------------------------------------------------------------------------ + statement ok DROP INDEX t@secondary diff --git a/pkg/config/system.go b/pkg/config/system.go index 8abc234e5828..0f225e2dd260 100644 --- a/pkg/config/system.go +++ b/pkg/config/system.go @@ -48,6 +48,17 @@ var ( type zoneEntry struct { zone *ZoneConfig placeholder *ZoneConfig + + // combined merges the zone and placeholder configs into a combined config. + // If both have subzone information, the placeholder information is preferred. + // This may never happen, but while the existing code gives preference to the + // placeholder, there appear to be no guarantees that there can be no overlap. + // + // TODO(andyk): Use the combined value everywhere early in 19.2, so there's + // enough bake time to ensure this is OK to do. Until then, only use the + // combined value in GetZoneConfigForObject, which is only used by the + // optimizer. + combined *ZoneConfig } // SystemConfig embeds a SystemConfigEntries message which contains an @@ -270,15 +281,22 @@ func (s *SystemConfig) GetZoneConfigForKey(key roachpb.RKey) (*ZoneConfig, error return s.getZoneConfigForKey(objectID, keySuffix) } -// GetZoneConfigForObject returns the zone ID for a given object ID. +// GetZoneConfigForObject returns the combined zone config for the given object +// identifier. +// NOTE: any subzones from the zone placeholder will be automatically merged +// into the cached zone so the caller doesn't need special-case handling code. func (s *SystemConfig) GetZoneConfigForObject(id uint32) (*ZoneConfig, error) { entry, err := s.getZoneEntry(id) if err != nil { return nil, err } - return entry.zone, nil + return entry.combined, nil } +// getZoneEntry returns the zone entry for the given object ID. In the fast +// path, the zone is already in the cache, and is directly returned. Otherwise, +// getZoneEntry will hydrate new ZoneConfig(s) from the SystemConfig and install +// them as an entry in the cache. func (s *SystemConfig) getZoneEntry(id uint32) (zoneEntry, error) { s.mu.RLock() entry, ok := s.mu.zoneCache[id] @@ -294,7 +312,16 @@ func (s *SystemConfig) getZoneEntry(id uint32) (zoneEntry, error) { return zoneEntry{}, err } if zone != nil { - entry := zoneEntry{zone: zone, placeholder: placeholder} + entry := zoneEntry{zone: zone, placeholder: placeholder, combined: zone} + if placeholder != nil { + // Merge placeholder with zone by copying over subzone information. + // Placeholders should only define the Subzones and SubzoneSpans fields. + combined := *zone + combined.Subzones = placeholder.Subzones + combined.SubzoneSpans = placeholder.SubzoneSpans + entry.combined = &combined + } + if cache { s.mu.Lock() s.mu.zoneCache[id] = entry @@ -314,16 +341,16 @@ func (s *SystemConfig) getZoneConfigForKey(id uint32, keySuffix []byte) (*ZoneCo if entry.placeholder != nil { if subzone := entry.placeholder.GetSubzoneForKeySuffix(keySuffix); subzone != nil { if indexSubzone := entry.placeholder.GetSubzone(subzone.IndexID, ""); indexSubzone != nil { - subzone.Config.InheritFromParent(indexSubzone.Config) + subzone.Config.InheritFromParent(&indexSubzone.Config) } - subzone.Config.InheritFromParent(*entry.zone) + subzone.Config.InheritFromParent(entry.zone) return &subzone.Config, nil } } else if subzone := entry.zone.GetSubzoneForKeySuffix(keySuffix); subzone != nil { if indexSubzone := entry.zone.GetSubzone(subzone.IndexID, ""); indexSubzone != nil { - subzone.Config.InheritFromParent(indexSubzone.Config) + subzone.Config.InheritFromParent(&indexSubzone.Config) } - subzone.Config.InheritFromParent(*entry.zone) + subzone.Config.InheritFromParent(entry.zone) return &subzone.Config, nil } return entry.zone, nil diff --git a/pkg/config/zone.go b/pkg/config/zone.go index dc6d9e404e2d..69fb14156278 100644 --- a/pkg/config/zone.go +++ b/pkg/config/zone.go @@ -505,7 +505,7 @@ func (z *ZoneConfig) Validate() error { } // InheritFromParent hydrates a zones missing fields from its parent. -func (z *ZoneConfig) InheritFromParent(parent ZoneConfig) { +func (z *ZoneConfig) InheritFromParent(parent *ZoneConfig) { if z.NumReplicas == nil { if parent.NumReplicas != nil { z.NumReplicas = proto.Int32(*parent.NumReplicas) diff --git a/pkg/sql/opt/xform/coster.go b/pkg/sql/opt/xform/coster.go index 87826d83554e..c981d8e5cf91 100644 --- a/pkg/sql/opt/xform/coster.go +++ b/pkg/sql/opt/xform/coster.go @@ -27,6 +27,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/util/log" + "golang.org/x/tools/container/intsets" ) // Coster is used by the optimizer to assign a cost to a candidate expression @@ -520,79 +521,170 @@ func (c *coster) rowSortCost(numKeyCols int) memo.Cost { return memo.Cost(cost) } +// rowScanCost is the CPU cost to scan one row, which depends on the number of +// columns in the index and (to a lesser extent) on the number of columns we are +// scanning. +func (c *coster) rowScanCost(tabID opt.TableID, idxOrd int, numScannedCols int) memo.Cost { + md := c.mem.Metadata() + tab := md.Table(tabID) + idx := tab.Index(idxOrd) + numCols := idx.ColumnCount() + + // Adjust cost based on how well the current locality matches the index's + // zone constraints. + var costFactor memo.Cost = cpuCostFactor + if len(c.locality.Tiers) != 0 { + // If 0% of locality tiers have matching constraints, then add additional + // cost. If 100% of locality tiers have matching constraints, then add no + // additional cost. Anything in between is proportional to the number of + // matches. + adjustment := 1.0 - localityMatchScore(idx.Zone(), c.locality) + costFactor += latencyCostFactor * memo.Cost(adjustment) + } + + // The number of the columns in the index matter because more columns means + // more data to scan. The number of columns we actually return also matters + // because that is the amount of data that we could potentially transfer over + // the network. + return memo.Cost(numCols+numScannedCols) * costFactor +} + // localityMatchScore returns a number from 0.0 to 1.0 that describes how well // the current node's locality matches the given zone constraints and -// leaseholder preferences, with 0.0 indicating 0% and 1.0 indicating 100%. In -// order to match, each successive locality tier must match at least one -// REQUIRED constraint and not match any PROHIBITED constraints. Locality tiers -// are hierarchical, so if a locality tier does not match, then tiers after it -// do not match either. For example: +// leaseholder preferences, with 0.0 indicating 0% and 1.0 indicating 100%. This +// is the basic algorithm: +// +// t = total # of locality tiers +// +// Match each locality tier against the constraint set, and compute a value +// for each tier: +// +// 0 = key not present in constraint set or key matches prohibited +// constraint, but value doesn't match +// +1 = key matches required constraint, and value does match +// -1 = otherwise +// +// m = length of longest locality prefix that ends in a +1 value and doesn't +// contain a -1 value. // -// Locality = [region=us,dc=east] -// 0.0 = [] -// 0.0 = [+region=eu,+dc=uk] -// 0.0 = [-region=us] -// 0.0 = [+region=eu,+dc=east] -// 0.5 = [+region=us,+dc=west] -// 0.5 = [+region=us,-dc=east] -// 1.0 = [+region=us,+dc=east] -// 1.0 = [+region=us,+dc=east,+rack=1,-ssd] +// Compute "m" for both the ReplicaConstraints constraints set, as well as for +// the LeasePreferences constraints set: // -// Note that constraints need not be specified in any particular order, so scan -// all constraints when matching each locality tier. +// constraint-score = m / t +// lease-pref-score = m / t // -// Similarly, matching leaseholder preferences are considered in the final -// score. However, since leaseholder preferences are not guaranteed, its score -// is weighted at half of the replica constraint score, in order to reflect the -// possibility that the leaseholder has moved from the preferred location. -func (c *coster) localityMatchScore(zone cat.Zone) float64 { +// if there are no lease preferences, then final-score = lease-pref-score +// else final-score = (constraint-score * 2 + lease-pref-score) / 3 +// +// Here are some scoring examples: +// +// Locality = region=us,dc=east +// 0.0 = [] // No constraints to match +// 0.0 = [+region=eu,+dc=uk] // None of the tiers match +// 0.0 = [+region=eu,+dc=east] // 2nd tier matches, but 1st tier doesn't +// 0.0 = [-region=us,+dc=east] // 1st tier matches PROHIBITED constraint +// 0.0 = [-region=eu] // 1st tier PROHIBITED and non-matching +// 0.5 = [+region=us] // 1st tier matches +// 0.5 = [+region=us,-dc=east] // 1st tier matches, 2nd tier PROHIBITED +// 0.5 = [+region=us,+dc=west] // 1st tier matches, but 2nd tier doesn't +// 1.0 = [+region=us,+dc=east] // Both tiers match +// 1.0 = [+dc=east] // 2nd tier matches, no constraints for 1st +// 1.0 = [+region=us,+dc=east,+rack=1,-ssd] // Extra constraints ignored +// +// Note that constraints need not be specified in any particular order, so all +// constraints are scanned when matching each locality tier. In cases where +// there are multiple replica constraint groups (i.e. where a subset of replicas +// can have different constraints than another subset), the minimum constraint +// score among the groups is used. +// +// While matching leaseholder preferences are considered in the final score, +// leaseholder preferences are not guaranteed, so its score is weighted at half +// of the replica constraint score, in order to reflect the possibility that the +// leaseholder has moved from the preferred location. +func localityMatchScore(zone cat.Zone, locality roachpb.Locality) float64 { // Fast path: if there are no constraints or leaseholder preferences, then // locality can't match. if zone.ReplicaConstraintsCount() == 0 && zone.LeasePreferenceCount() == 0 { return 0.0 } - // matchConstraints returns true if it can locate a required constraint that - // matches the given tier. - matchConstraints := func(set cat.ConstraintSet, tier *roachpb.Tier) bool { - for i, n := 0, set.ConstraintCount(); i < n; i++ { - con := set.Constraint(i) - if tier.Key == con.GetKey() && tier.Value == con.GetValue() { - // If this is a required constraint, then it matches, and no need to - // iterate further. If it's prohibited, then it cannot match, so no - // need to go further. - return con.IsRequired() + // matchTier matches a tier to a set of constraints and returns: + // + // 0 = key not present in constraint set or key only matches prohibited + // constraints where value doesn't match + // +1 = key matches any required constraint key + value + // -1 = otherwise + // + matchTier := func(tier roachpb.Tier, set cat.ConstraintSet) int { + foundNoMatch := false + for j, n := 0, set.ConstraintCount(); j < n; j++ { + con := set.Constraint(j) + if con.GetKey() != tier.Key { + // Ignore constraints that don't have matching key. + continue + } + + if con.GetValue() == tier.Value { + if !con.IsRequired() { + // Matching prohibited constraint, so result is -1. + return -1 + } + + // Matching required constraint, so result is +1. + return +1 + } + + if con.IsRequired() { + // Remember that non-matching required constraint was found. + foundNoMatch = true } } - return false + + if foundNoMatch { + // At least one non-matching required constraint was found, and no + // matching constraints. + return -1 + } + + // Key not present in constraint set, or key only matches prohibited + // constraints where value doesn't match. + return 0 } - // matchReplConstraints returns true if all replica constraints match the - // given tier. - matchReplConstraints := func(zone cat.Zone, tier *roachpb.Tier) bool { - for i, n := 0, zone.ReplicaConstraintsCount(); i < n; i++ { - replCon := zone.ReplicaConstraints(i) - if !matchConstraints(replCon, tier) { - return false + // matchConstraints returns the number of tiers that match the given + // constraint set ("m" in algorithm described above). + matchConstraints := func(set cat.ConstraintSet) int { + matchCount := 0 + for i, tier := range locality.Tiers { + switch matchTier(tier, set) { + case +1: + matchCount = i + 1 + case -1: + return matchCount } } - return true + return matchCount } // Score any replica constraints. var constraintScore float64 if zone.ReplicaConstraintsCount() != 0 { - // Keep iterating until non-matching tier is found, or all tiers are found - // to match. - matchCount := 0 - for i := range c.locality.Tiers { - if !matchReplConstraints(zone, &c.locality.Tiers[i]) { - break + // Iterate over the replica constraints and determine the minimum value + // returned by matchConstraints for any replica. For example: + // + // 3: [+region=us,+dc=east] + // 2: [+region=us] + // + // For the [region=us,dc=east] locality, the result is min(2, 1). + minCount := intsets.MaxInt + for i := 0; i < zone.ReplicaConstraintsCount(); i++ { + matchCount := matchConstraints(zone.ReplicaConstraints(i)) + if matchCount < minCount { + minCount = matchCount } - matchCount++ } - constraintScore = float64(matchCount) / float64(len(c.locality.Tiers)) + constraintScore = float64(minCount) / float64(len(locality.Tiers)) } // If there are no lease preferences, then use replica constraint score. @@ -602,44 +694,9 @@ func (c *coster) localityMatchScore(zone cat.Zone) float64 { // Score the first lease preference, if one is available. Ignore subsequent // lease preferences, since they only apply in edge cases. - var leaseScore float64 - matchCount := 0 - for i := range c.locality.Tiers { - if !matchConstraints(zone.LeasePreference(0), &c.locality.Tiers[i]) { - break - } - matchCount++ - } - - leaseScore = float64(matchCount) / float64(len(c.locality.Tiers)) + matchCount := matchConstraints(zone.LeasePreference(0)) + leaseScore := float64(matchCount) / float64(len(locality.Tiers)) // Weight the constraintScore twice as much as the lease score. return (constraintScore*2 + leaseScore) / 3 } - -// rowScanCost is the CPU cost to scan one row, which depends on the number of -// columns in the index and (to a lesser extent) on the number of columns we are -// scanning. -func (c *coster) rowScanCost(tabID opt.TableID, idxOrd int, numScannedCols int) memo.Cost { - md := c.mem.Metadata() - tab := md.Table(tabID) - idx := tab.Index(idxOrd) - numCols := idx.ColumnCount() - - // Adjust cost based on how well the current locality matches the index's - // zone constraints. - var costFactor memo.Cost = cpuCostFactor - if len(c.locality.Tiers) != 0 { - // If 0% of locality tiers have matching constraints, then add additional - // cost. If 100% of locality tiers have matching constraints, then add no - // additional cost. Anything in between is proportional to the number of - // matches. - costFactor += latencyCostFactor * memo.Cost(1.0-c.localityMatchScore(idx.Zone())) - } - - // The number of the columns in the index matter because more columns means - // more data to scan. The number of columns we actually return also matters - // because that is the amount of data that we could potentially transfer over - // the network. - return memo.Cost(numCols+numScannedCols) * costFactor -} diff --git a/pkg/sql/opt/xform/coster_test.go b/pkg/sql/opt/xform/coster_test.go new file mode 100644 index 000000000000..79c32823e995 --- /dev/null +++ b/pkg/sql/opt/xform/coster_test.go @@ -0,0 +1,102 @@ +// Copyright 2019 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +package xform + +import ( + "math" + "testing" + + "github.com/cockroachdb/cockroach/pkg/config" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "gopkg.in/yaml.v2" +) + +func TestLocalityMatchScore(t *testing.T) { + defer leaktest.AfterTest(t)() + + testCases := []struct { + locality string + constraints string + leasePrefs string + expected float64 + }{ + {locality: "region=us,dc=east", constraints: "[]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[+region=eu,+dc=uk,+dc=de]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[-region=us,+dc=east]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[+region=eu,+dc=east]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[-region=eu]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[+region=us]", expected: 0.5}, + {locality: "region=us,dc=east", constraints: "[+region=us,+region=eu]", expected: 0.5}, + {locality: "region=us,dc=east", constraints: "[+region=eu,+region=ap,+region=us]", expected: 0.5}, + {locality: "region=us,dc=east", constraints: "[+region=us,-dc=east]", expected: 0.5}, + {locality: "region=us,dc=east", constraints: "[+region=us,+dc=west]", expected: 0.5}, + {locality: "region=us,dc=east", constraints: "[+region=us,+dc=east]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[+dc=east]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[+dc=west,+dc=east]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[-region=eu,+dc=east]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[+region=eu,+dc=east,+region=us,+dc=west]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[+region=us,+dc=east,+rack=1,-ssd]", expected: 1.0}, + + {locality: "region=us,dc=east", constraints: `{"+region=us,+dc=east":3,"-dc=east":2}`, expected: 0.0}, + {locality: "region=us,dc=east", constraints: `{"+region=us,+dc=east":3,"+region=eu,+dc=east":2}`, expected: 0.0}, + {locality: "region=us,dc=east", constraints: `{"+region=us,+dc=east":3,"+region=us,+region=eu":2}`, expected: 0.5}, + {locality: "region=us,dc=east", constraints: `{"+region=us,+dc=east":3,"+dc=east,+dc=west":2}`, expected: 1.0}, + + {locality: "region=us,dc=east", leasePrefs: "[[]]", expected: 0.0}, + {locality: "region=us,dc=east", leasePrefs: "[[+dc=west]]", expected: 0.0}, + {locality: "region=us,dc=east", leasePrefs: "[[+region=us]]", expected: 0.17}, + {locality: "region=us,dc=east", leasePrefs: "[[+region=us,+dc=east]]", expected: 0.33}, + + {locality: "region=us,dc=east", constraints: "[+region=eu]", leasePrefs: "[[+dc=west]]", expected: 0.0}, + {locality: "region=us,dc=east", constraints: "[+region=eu]", leasePrefs: "[[+region=us]]", expected: 0.17}, + {locality: "region=us,dc=east", constraints: "[+region=eu]", leasePrefs: "[[+dc=east]]", expected: 0.33}, + {locality: "region=us,dc=east", constraints: "[+region=us]", leasePrefs: "[[+dc=west]]", expected: 0.33}, + {locality: "region=us,dc=east", constraints: "[+region=us]", leasePrefs: "[[+region=us]]", expected: 0.50}, + {locality: "region=us,dc=east", constraints: "[+region=us]", leasePrefs: "[[+dc=east]]", expected: 0.67}, + {locality: "region=us,dc=east", constraints: "[+dc=east]", leasePrefs: "[[+region=us]]", expected: 0.83}, + {locality: "region=us,dc=east", constraints: "[+dc=east]", leasePrefs: "[[+dc=east]]", expected: 1.0}, + {locality: "region=us,dc=east", constraints: "[+region=us,+dc=east]", leasePrefs: "[[+region=us,+dc=east]]", expected: 1.0}, + } + + for _, tc := range testCases { + zone := &config.ZoneConfig{} + + var locality roachpb.Locality + if err := locality.Set(tc.locality); err != nil { + t.Fatal(err) + } + + if tc.constraints != "" { + constraintsList := &config.ConstraintsList{} + if err := yaml.UnmarshalStrict([]byte(tc.constraints), constraintsList); err != nil { + t.Fatal(err) + } + zone.Constraints = constraintsList.Constraints + } + + if tc.leasePrefs != "" { + if err := yaml.UnmarshalStrict([]byte(tc.leasePrefs), &zone.LeasePreferences); err != nil { + t.Fatal(err) + } + } + + actual := math.Round(localityMatchScore(zone, locality)*100) / 100 + if actual != tc.expected { + t.Errorf("locality=%v, constraints=%v, leasePrefs=%v: expected %v, got %v", + tc.locality, tc.constraints, tc.leasePrefs, tc.expected, actual) + } + } +} diff --git a/pkg/sql/opt/xform/testdata/coster/zone b/pkg/sql/opt/xform/testdata/coster/zone index 45700dd58d56..f7ced2d2ee81 100644 --- a/pkg/sql/opt/xform/testdata/coster/zone +++ b/pkg/sql/opt/xform/testdata/coster/zone @@ -163,11 +163,12 @@ ALTER INDEX abc@bc1 CONFIGURE ZONE USING constraints='[+region=us,+dc=east,+rack ZONE └── constraints: [+region=us,+dc=east,+rack=1] +# Do not specify region constraint. exec-ddl -ALTER INDEX abc@bc2 CONFIGURE ZONE USING constraints='[+region=us,+dc=west,+rack=1]' +ALTER INDEX abc@bc2 CONFIGURE ZONE USING constraints='[+dc=west]' ---- ZONE - └── constraints: [+region=us,+dc=west,+rack=1] + └── constraints: [+dc=west] # With locality in us + central, use primary index. opt format=show-all locality=(region=us,dc=central) @@ -210,7 +211,8 @@ scan t.public.abc@bc1 ├── prune: (3) └── interesting orderings: (+2,+3) -# With locality in us + west, use bc2 index. +# With locality in us + west, use bc2 index, even though region does not match +# any constraint on the index. opt format=show-all locality=(region=us,dc=west) SELECT b, c FROM abc WHERE b=10 ---- @@ -224,20 +226,6 @@ scan t.public.abc@bc2 ├── prune: (3) └── interesting orderings: (+2,+3) -# Ignore "dc=west,rack=1" match if "region" does not match. -opt format=show-all locality=(region=eu,dc=west,rack=1) -SELECT b, c FROM abc WHERE b=10 ----- -scan t.public.abc@bc1 - ├── columns: b:2(int!null) c:3(string) - ├── constraint: /2/3: [/10 - /10] - ├── stats: [rows=9.9, distinct(2)=1, null(2)=0] - ├── cost: 10.9 - ├── lax-key: (3) - ├── fd: ()-->(2) - ├── prune: (3) - └── interesting orderings: (+2,+3) - # -------------------------------------------------- # Multiple replica constraints. # -------------------------------------------------- @@ -251,10 +239,10 @@ ZONE └── 1 replicas: [+region=us,+dc=west] exec-ddl -ALTER INDEX abc@bc2 CONFIGURE ZONE USING constraints='[+region=us,+dc=east]' +ALTER INDEX abc@bc2 CONFIGURE ZONE USING constraints='[+dc=east]' ---- ZONE - └── constraints: [+region=us,+dc=east] + └── constraints: [+dc=east] # With locality in us, use bc1 index, since only one tier matches in case of # both indexes. @@ -345,6 +333,61 @@ scan t.public.abc@bc2 ├── prune: (3) └── interesting orderings: (+2,+3) +# With locality in ap + east, use bc1, since ap is not in list of regions for +# bc2, even though dc=east matches. +opt format=show-all locality=(region=ap,dc=east) +SELECT b, c FROM abc WHERE b=10 +---- +scan t.public.abc@bc1 + ├── columns: b:2(int!null) c:3(string) + ├── constraint: /2/3: [/10 - /10] + ├── stats: [rows=9.9, distinct(2)=1, null(2)=0] + ├── cost: 10.6525 + ├── lax-key: (3) + ├── fd: ()-->(2) + ├── prune: (3) + └── interesting orderings: (+2,+3) + +exec-ddl +ALTER INDEX abc@bc1 CONFIGURE ZONE USING constraints='[-region=eu,+dc=east]' +---- +ZONE + └── constraints: [-region=eu,+dc=east] + +exec-ddl +ALTER INDEX abc@bc2 CONFIGURE ZONE USING constraints='[+dc=east]' +---- +ZONE + └── constraints: [+dc=east] + +# With locality in us + east, use bc1, since it's first in order. +opt format=show-all locality=(region=us,dc=east) +SELECT b, c FROM abc WHERE b=10 +---- +scan t.public.abc@bc1 + ├── columns: b:2(int!null) c:3(string) + ├── constraint: /2/3: [/10 - /10] + ├── stats: [rows=9.9, distinct(2)=1, null(2)=0] + ├── cost: 10.405 + ├── lax-key: (3) + ├── fd: ()-->(2) + ├── prune: (3) + └── interesting orderings: (+2,+3) + +# With locality in eu + east, use bc2, since eu is prohibited for bc1. +opt format=show-all locality=(region=eu,dc=east) +SELECT b, c FROM abc WHERE b=10 +---- +scan t.public.abc@bc2 + ├── columns: b:2(int!null) c:3(string) + ├── constraint: /2/3: [/10 - /10] + ├── stats: [rows=9.9, distinct(2)=1, null(2)=0] + ├── cost: 10.405 + ├── lax-key: (3) + ├── fd: ()-->(2) + ├── prune: (3) + └── interesting orderings: (+2,+3) + # -------------------------------------------------- # Lookup join. # -------------------------------------------------- @@ -598,20 +641,6 @@ scan t.public.abc@bc2 ├── prune: (3) └── interesting orderings: (+2,+3) -# Ignore "dc=west,rack=1" match if "region" does not match. -opt format=show-all locality=(region=eu,dc=west,rack=1) -SELECT b, c FROM abc WHERE b=10 ----- -scan t.public.abc@bc1 - ├── columns: b:2(int!null) c:3(string) - ├── constraint: /2/3: [/10 - /10] - ├── stats: [rows=9.9, distinct(2)=1, null(2)=0] - ├── cost: 10.9 - ├── lax-key: (3) - ├── fd: ()-->(2) - ├── prune: (3) - └── interesting orderings: (+2,+3) - # -------------------------------------------------- # Zone constraint + leaseholder preference. # -------------------------------------------------- diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index cbdcba1aa47d..7ad68c2287f6 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -506,7 +506,9 @@ func newOptTable( for j := range tblZone.Subzones { subzone := &tblZone.Subzones[j] if subzone.IndexID == uint32(idxDesc.ID) && subzone.PartitionName == "" { - idxZone = &subzone.Config + copyZone := subzone.Config + copyZone.InheritFromParent(tblZone) + idxZone = ©Zone } } diff --git a/pkg/sql/zone_config.go b/pkg/sql/zone_config.go index ccc4d3132c0a..bbf0c09bd0ac 100644 --- a/pkg/sql/zone_config.go +++ b/pkg/sql/zone_config.go @@ -131,7 +131,7 @@ func completeZoneConfig( if err != nil { return err } - cfg.InheritFromParent(*dbzone) + cfg.InheritFromParent(dbzone) } } @@ -143,7 +143,7 @@ func completeZoneConfig( if err != nil { return err } - cfg.InheritFromParent(*defaultZone) + cfg.InheritFromParent(defaultZone) return nil } @@ -200,17 +200,17 @@ func GetZoneConfigInTxn( if placeholder != nil { if subzone = placeholder.GetSubzone(uint32(index.ID), partition); subzone != nil { if indexSubzone := placeholder.GetSubzone(uint32(index.ID), ""); indexSubzone != nil { - subzone.Config.InheritFromParent(indexSubzone.Config) + subzone.Config.InheritFromParent(&indexSubzone.Config) } - subzone.Config.InheritFromParent(*zone) + subzone.Config.InheritFromParent(zone) return placeholderID, placeholder, subzone, nil } } else { if subzone = zone.GetSubzone(uint32(index.ID), partition); subzone != nil { if indexSubzone := zone.GetSubzone(uint32(index.ID), ""); indexSubzone != nil { - subzone.Config.InheritFromParent(indexSubzone.Config) + subzone.Config.InheritFromParent(&indexSubzone.Config) } - subzone.Config.InheritFromParent(*zone) + subzone.Config.InheritFromParent(zone) } } }