Skip to content

Commit

Permalink
Merge #139256 #140167
Browse files Browse the repository at this point in the history 
139256: sql/rowenc: reduce index key prefix calls r=annrpom a=annrpom

### sql/rowenc: reduce index key prefix calls
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

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

Release note: None

---

### sql/rowexec: run BenchmarkIndexBackfill on-disk
Epic: none
Release note: None


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.

Co-authored-by: Annie Pompa <[email protected]>
  • Loading branch information
@annrpom
craig[bot] and annrpom committed Jan 31, 2025
3 parents 4493b25 + afda62e + c308da0 commit 311ed57
Show file tree
Hide file tree
Showing 8 changed files with 185 additions and 54 deletions.
23 changes: 23 additions & 0 deletions pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/testdata/partitions
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions pkg/sql/backfill/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ go_test(
],
embed = [":backfill"],
deps = [
"//pkg/keys",
"//pkg/kv",
"//pkg/roachpb",
"//pkg/sql/catalog",
Expand Down
34 changes: 27 additions & 7 deletions pkg/sql/backfill/backfill.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"time"
"unsafe"

"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings"
Expand Down Expand Up @@ -477,6 +478,10 @@ type IndexBackfiller struct {
// backfilled.
indexesToEncode []catalog.Index

// keyPrefixes is a slice of key prefixes for each index in indexesToEncode.
// indexesToEncode and keyPrefixes should both have the same ordering.
keyPrefixes [][]byte

alloc tree.DatumAlloc

// mon is a memory monitor linked with the IndexBackfiller on creation.
Expand Down Expand Up @@ -506,7 +511,7 @@ func (ib *IndexBackfiller) InitForLocalUse(
) error {

// Initialize ib.added.
ib.initIndexes(desc, nil /* allowList */)
ib.initIndexes(evalCtx.Codec, desc, nil /* allowList */)

// Initialize ib.cols and ib.colIdxMap.
if err := ib.initCols(desc); err != nil {
Expand Down Expand Up @@ -644,18 +649,18 @@ func (ib *IndexBackfiller) InitForDistributedUse(
allowList []catid.IndexID,
mon *mon.BytesMonitor,
) error {
// We'll be modifying the eval.Context in BuildIndexEntriesChunk, so we need
// to make a copy.
evalCtx := flowCtx.NewEvalCtx()

// Initialize ib.added.
ib.initIndexes(desc, allowList)
ib.initIndexes(evalCtx.Codec, desc, allowList)

// Initialize ib.indexBackfillerCols.
if err := ib.initCols(desc); err != nil {
return err
}

// We'll be modifying the eval.Context in BuildIndexEntriesChunk, so we need
// to make a copy.
evalCtx := flowCtx.NewEvalCtx()
var predicates map[descpb.IndexID]tree.TypedExpr
var colExprs map[descpb.ColumnID]tree.TypedExpr
var referencedColumns catalog.TableColSet
Expand Down Expand Up @@ -733,17 +738,22 @@ func (ib *IndexBackfiller) initCols(desc catalog.TableDescriptor) (err error) {
}

// initIndexes is a helper to populate index metadata of an IndexBackfiller. It
// populates the added field to be all adding index mutations.
// populates the added field to be all adding index mutations, along with the
// keyPrefixes field to be the respective keyPrefixes (these slices should
// maintain the same ordering).
// If `allowList` is non-nil, we only add those in this list.
// If `allowList` is nil, we add all adding index mutations.
func (ib *IndexBackfiller) initIndexes(desc catalog.TableDescriptor, allowList []catid.IndexID) {
func (ib *IndexBackfiller) initIndexes(
codec keys.SQLCodec, desc catalog.TableDescriptor, allowList []catid.IndexID,
) {
var allowListAsSet catid.IndexSet
if len(allowList) > 0 {
allowListAsSet = catid.MakeIndexIDSet(allowList...)
}

mutations := desc.AllMutations()
mutationID := mutations[0].MutationID()
ib.keyPrefixes = make([][]byte, 0, len(ib.added))
// Mutations in the same transaction have the same ID. Loop through the
// mutations and collect all index mutations.
for _, m := range mutations {
Expand All @@ -754,6 +764,8 @@ func (ib *IndexBackfiller) initIndexes(desc catalog.TableDescriptor, allowList [
(allowListAsSet.Empty() || allowListAsSet.Contains(m.AsIndex().GetID())) {
idx := m.AsIndex()
ib.added = append(ib.added, idx)
keyPrefix := rowenc.MakeIndexKeyPrefix(codec, desc.GetID(), idx.GetID())
ib.keyPrefixes = append(ib.keyPrefixes, keyPrefix)
}
}
}
Expand All @@ -776,6 +788,7 @@ func (ib *IndexBackfiller) init(
ib.indexesToEncode = ib.added
if len(ib.predicates) > 0 {
ib.indexesToEncode = make([]catalog.Index, 0, len(ib.added))
ib.keyPrefixes = make([][]byte, 0, len(ib.added))
}

ib.types = make([]*types.T, len(ib.cols))
Expand Down Expand Up @@ -918,6 +931,7 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
}
return nil
}

for i := int64(0); i < chunkSize; i++ {
ok, _, err := fetcher.NextRowDecodedInto(ctx, ib.rowVals, ib.colIdxMap)
if err != nil {
Expand All @@ -943,11 +957,14 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
// indexes that the current row should be added to.
if len(ib.predicates) > 0 {
ib.indexesToEncode = ib.indexesToEncode[:0]
ib.keyPrefixes = ib.keyPrefixes[:0]
for _, idx := range ib.added {
if !idx.IsPartial() {
// If the index is not a partial index, all rows should have
// an entry.
ib.indexesToEncode = append(ib.indexesToEncode, idx)
keyPrefix := rowenc.MakeIndexKeyPrefix(ib.evalCtx.Codec, tableDesc.GetID(), idx.GetID())
ib.keyPrefixes = append(ib.keyPrefixes, keyPrefix)
continue
}

Expand All @@ -962,6 +979,8 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(

if val == tree.DBoolTrue {
ib.indexesToEncode = append(ib.indexesToEncode, idx)
keyPrefix := rowenc.MakeIndexKeyPrefix(ib.evalCtx.Codec, tableDesc.GetID(), idx.GetID())
ib.keyPrefixes = append(ib.keyPrefixes, keyPrefix)
}
}
}
Expand All @@ -983,6 +1002,7 @@ func (ib *IndexBackfiller) BuildIndexEntriesChunk(
ib.evalCtx.Codec,
tableDesc,
ib.indexesToEncode,
ib.keyPrefixes,
ib.colIdxMap,
ib.rowVals,
buffer,
Expand Down
5 changes: 3 additions & 2 deletions pkg/sql/backfill/index_backfiller_cols_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package backfill
import (
"testing"

"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
Expand Down Expand Up @@ -411,15 +412,15 @@ func TestInitIndexesAllowList(t *testing.T) {
t.Run("nil allowList", func(t *testing.T) {
// A nil allowList means no filtering.
ib := &IndexBackfiller{}
ib.initIndexes(desc, nil /* allowList */)
ib.initIndexes(keys.SystemSQLCodec, desc, nil /* allowList */)
require.Equal(t, 2, len(ib.added))
require.Equal(t, catid.IndexID(2), ib.added[0].GetID())
require.Equal(t, catid.IndexID(3), ib.added[1].GetID())
})

t.Run("non-nil allowList", func(t *testing.T) {
ib := &IndexBackfiller{}
ib.initIndexes(desc, []catid.IndexID{3} /* allowList */)
ib.initIndexes(keys.SystemSQLCodec, desc, []catid.IndexID{3} /* allowList */)
require.Equal(t, 1, len(ib.added))
require.Equal(t, catid.IndexID(3), ib.added[0].GetID())
})
Expand Down
29 changes: 20 additions & 9 deletions pkg/sql/partition_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)]
}
Expand All @@ -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 {
Expand All @@ -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...)
Expand All @@ -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(
Expand All @@ -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
Expand Down
Loading

0 comments on commit 311ed57

Please sign in to comment.