Skip to content

Commit

Permalink
Merge cockroachdb#85442 cockroachdb#85762 cockroachdb#86114
Browse files Browse the repository at this point in the history 
85442: sql/colexec: specialize RangeStats operator to achieve parallelism r=ajwerner a=ajwerner

Release justification: This is needed to make new observability features reasonably performant, also sneaking it in under the deadline. 

Release note (performance improvement): The crdb_internal.range_statistics
function now uses a vectorized implementation which allows the lookup of
range metadata to occur in parallel.

85762: partitionccl: remove unused functions r=ajwerner a=yuzefovich

This commit removes a couple of functions that are only used in tests.

Release note: None

86114: sql: deflake TestSchemaChangeRetry r=ajwerner a=ajwerner

Now that GC happens eagerly in the GC job, we need to hold off the GC job to
make sure that key counts match the test's expectations.

Fixes flakes like [this one](https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/6093943)

Release justification: Testing only change. 

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
@ajwerner @yuzefovich
3 people committed Aug 15, 2022
4 parents 41db784 + 5485899 + 7a5cb73 + c4e6a61 commit 6b7f005
Show file tree
Hide file tree
Showing 23 changed files with 264 additions and 318 deletions.
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,7 @@ GO_TARGETS = [
"//pkg/kv/kvclient/rangefeed/rangefeedcache:rangefeedcache_test",
"//pkg/kv/kvclient/rangefeed:rangefeed",
"//pkg/kv/kvclient/rangefeed:rangefeed_test",
"//pkg/kv/kvclient/rangestats:rangestats",
"//pkg/kv/kvclient:kvclient",
"//pkg/kv/kvnemesis:kvnemesis",
"//pkg/kv/kvnemesis:kvnemesis_test",
Expand Down Expand Up @@ -2327,6 +2328,7 @@ GET_X_DATA_TARGETS = [
"//pkg/kv/kvclient/rangefeed:get_x_data",
"//pkg/kv/kvclient/rangefeed/rangefeedbuffer:get_x_data",
"//pkg/kv/kvclient/rangefeed/rangefeedcache:get_x_data",
"//pkg/kv/kvclient/rangestats:get_x_data",
"//pkg/kv/kvnemesis:get_x_data",
"//pkg/kv/kvprober:get_x_data",
"//pkg/kv/kvserver:get_x_data",
Expand Down
7 changes: 0 additions & 7 deletions pkg/ccl/partitionccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,17 @@ go_library(
"//pkg/sql/catalog",
"//pkg/sql/catalog/catpb",
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/schemaexpr",
"//pkg/sql/catalog/tabledesc",
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/rowenc",
"//pkg/sql/rowenc/valueside",
"//pkg/sql/schemachanger/scdeps",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/normalize",
"//pkg/sql/sem/tree",
"//pkg/sql/sem/tree/treecmp",
"//pkg/sql/sem/volatility",
"//pkg/sql/types",
"//pkg/util/encoding",
"//pkg/util/errorutil/unimplemented",
"//pkg/util/log",
"@com_github_cockroachdb_errors//:errors",
],
)
Expand Down Expand Up @@ -79,7 +73,6 @@ go_test(
"//pkg/sql/rowenc",
"//pkg/sql/scrub",
"//pkg/sql/scrub/scrubtestutils",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/tests",
"//pkg/sql/types",
Expand Down
215 changes: 0 additions & 215 deletions pkg/ccl/partitionccl/partition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,17 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
"github.com/cockroachdb/cockroach/pkg/sql/rowenc/valueside"
"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scdeps"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/normalize"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp"
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/encoding"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/errors"
)

Expand Down Expand Up @@ -408,215 +402,6 @@ func createPartitioning(
return newImplicitCols, newPartitioning, err
}

// selectPartitionExprs constructs an expression for selecting all rows in the
// given partitions.
func selectPartitionExprs(
evalCtx *eval.Context, tableDesc catalog.TableDescriptor, partNames tree.NameList,
) (tree.Expr, error) {
exprsByPartName := make(map[string]tree.TypedExpr)
for _, partName := range partNames {
exprsByPartName[string(partName)] = nil
}

a := &tree.DatumAlloc{}
var prefixDatums []tree.Datum
if err := catalog.ForEachIndex(tableDesc, catalog.IndexOpts{
AddMutations: true,
}, func(idx catalog.Index) error {
return selectPartitionExprsByName(
a, evalCtx, tableDesc, idx, idx.GetPartitioning(), prefixDatums, exprsByPartName, true /* genExpr */)
}); err != nil {
return nil, err
}

var expr tree.TypedExpr = tree.DBoolFalse
for _, partName := range partNames {
partExpr, ok := exprsByPartName[string(partName)]
if !ok || partExpr == nil {
return nil, errors.Errorf("unknown partition: %s", partName)
}
expr = tree.NewTypedOrExpr(expr, partExpr)
}

var err error
expr, err = normalize.Expr(evalCtx, expr)
if err != nil {
return nil, err
}
// In order to typecheck during simplification and normalization, we used
// dummy IndexVars. Swap them out for actual column references.
finalExpr, err := tree.SimpleVisit(expr, func(e tree.Expr) (recurse bool, newExpr tree.Expr, _ error) {
if ivar, ok := e.(*tree.IndexedVar); ok {
col, err := tableDesc.FindColumnWithID(descpb.ColumnID(ivar.Idx))
if err != nil {
return false, nil, err
}
return false, &tree.ColumnItem{ColumnName: tree.Name(col.GetName())}, nil
}
return true, e, nil
})
return finalExpr, err
}

// selectPartitionExprsByName constructs an expression for selecting all rows in
// each partition and subpartition in the given index. To make it easy to
// simplify and normalize the exprs, references to table columns are represented
// as TypedOrdinalReferences with an ordinal of the column ID.
//
// NB Subpartitions do not affect the expression for their parent partitions. So
// if a partition foo (a=3) is then subpartitiond by (b=5) and no DEFAULT, the
// expression for foo is still `a=3`, not `a=3 AND b=5`. This means that if some
// partition is requested, we can omit all of the subpartitions, because they'll
// also necessarily select subsets of the rows it will. "requested" here is
// indicated by the caller by setting the corresponding name in the
// `exprsByPartName` map to nil. In this case, `genExpr` is then set to false
// for subpartitions of this call, which causes each subpartition to only
// register itself in the map with a placeholder entry (so we can still verify
// that the requested partitions are all valid).
func selectPartitionExprsByName(
a *tree.DatumAlloc,
evalCtx *eval.Context,
tableDesc catalog.TableDescriptor,
idx catalog.Index,
part catalog.Partitioning,
prefixDatums tree.Datums,
exprsByPartName map[string]tree.TypedExpr,
genExpr bool,
) error {
if part.NumColumns() == 0 {
return nil
}

// Setting genExpr to false skips the expression generation and only
// registers each descendent partition in the map with a placeholder entry.
if !genExpr {
err := part.ForEachList(func(name string, _ [][]byte, subPartitioning catalog.Partitioning) error {
exprsByPartName[name] = tree.DBoolFalse
var fakeDatums tree.Datums
return selectPartitionExprsByName(a, evalCtx, tableDesc, idx, subPartitioning, fakeDatums, exprsByPartName, genExpr)
})
if err != nil {
return err
}
return part.ForEachRange(func(name string, _, _ []byte) error {
exprsByPartName[name] = tree.DBoolFalse
return nil
})
}

var colVars tree.Exprs
{
// The recursive calls of selectPartitionExprsByName don't pass though
// the column ordinal references, so reconstruct them here.
colVars = make(tree.Exprs, len(prefixDatums)+part.NumColumns())
for i := range colVars {
col, err := tabledesc.FindPublicColumnWithID(tableDesc, idx.GetKeyColumnID(i))
if err != nil {
return err
}
colVars[i] = tree.NewTypedOrdinalReference(int(col.GetID()), col.GetType())
}
}

if part.NumLists() > 0 {
type exprAndPartName struct {
expr tree.TypedExpr
name string
}
// Any partitions using DEFAULT must specifically exclude any relevant
// higher specificity partitions (e.g for partitions `(1, DEFAULT)`,
// `(1, 2)`, the expr for the former must exclude the latter. This is
// done by bucketing the expression for each partition value by the
// number of DEFAULTs it involves.
partValueExprs := make([][]exprAndPartName, part.NumColumns()+1)

err := part.ForEachList(func(name string, values [][]byte, subPartitioning catalog.Partitioning) error {
for _, valueEncBuf := range values {
t, _, err := rowenc.DecodePartitionTuple(a, evalCtx.Codec, tableDesc, idx, part, valueEncBuf, prefixDatums)
if err != nil {
return err
}
allDatums := append(prefixDatums, t.Datums...)

// When len(allDatums) < len(colVars), the missing elements are DEFAULTs, so
// we can simply exclude them from the expr.
typContents := make([]*types.T, len(allDatums))
for i, d := range allDatums {
typContents[i] = d.ResolvedType()
}
tupleTyp := types.MakeTuple(typContents)
partValueExpr := tree.NewTypedComparisonExpr(
treecmp.MakeComparisonOperator(treecmp.EQ),
tree.NewTypedTuple(tupleTyp, colVars[:len(allDatums)]),
tree.NewDTuple(tupleTyp, allDatums...),
)
partValueExprs[len(t.Datums)] = append(partValueExprs[len(t.Datums)], exprAndPartName{
expr: partValueExpr,
name: name,
})

genExpr := true
if _, ok := exprsByPartName[name]; ok {
// Presence of a partition name in the exprsByPartName map
// means the caller has expressed an interested in this
// partition, which means any subpartitions can be skipped
// (because they must by definition be a subset of this
// partition). This saves us a little work and also helps
// out the normalization & simplification of the resulting
// expression, since it doesn't have to account for which
// partitions overlap.
genExpr = false
}
if err := selectPartitionExprsByName(
a, evalCtx, tableDesc, idx, subPartitioning, allDatums, exprsByPartName, genExpr,
); err != nil {
return err
}
}
return nil
})
if err != nil {
return err
}

// Walk backward through partValueExprs, so partition values with fewest
// DEFAULTs to most. As we go, keep an expression to be AND NOT'd with
// each partition value's expression in `excludeExpr`. This handles the
// exclusion of `(1, 2)` from the expression for `(1, DEFAULT)` in the
// example above.
//
// TODO(dan): The result of the way this currently works is correct but
// too broad. In a two column partitioning with cases for `(a, b)` and
// `(c, DEFAULT)`, the expression generated for `(c, DEFAULT)` will
// needlessly exclude `(a, b)`. Concretely, we end up with expressions
// like `(a) IN (1) AND ... (a, b) != (2, 3)`, where the `!= (2, 3)`
// part is irrelevant. This only happens in fairly unrealistic
// partitionings, so it's unclear if anything really needs to be done
// here.
excludeExpr := tree.TypedExpr(tree.DBoolFalse)
for i := len(partValueExprs) - 1; i >= 0; i-- {
nextExcludeExpr := tree.TypedExpr(tree.DBoolFalse)
for _, v := range partValueExprs[i] {
nextExcludeExpr = tree.NewTypedOrExpr(nextExcludeExpr, v.expr)
partValueExpr := tree.NewTypedAndExpr(v.expr, tree.NewTypedNotExpr(excludeExpr))
// We can get multiple expressions for the same partition in
// a single-col `PARTITION foo VALUES IN ((1), (2))`.
if e, ok := exprsByPartName[v.name]; !ok || e == nil {
exprsByPartName[v.name] = partValueExpr
} else {
exprsByPartName[v.name] = tree.NewTypedOrExpr(e, partValueExpr)
}
}
excludeExpr = tree.NewTypedOrExpr(excludeExpr, nextExcludeExpr)
}
}

if part.NumRanges() > 0 {
log.Fatal(evalCtx.Context, "TODO(dan): unsupported for range partitionings")
}
return nil
}

func init() {
sql.CreatePartitioningCCL = createPartitioning
scdeps.CreatePartitioningCCL = createPartitioning
Expand Down
80 changes: 0 additions & 80 deletions pkg/ccl/partitionccl/partition_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/importer"
"github.com/cockroachdb/cockroach/pkg/sql/parser"
"github.com/cockroachdb/cockroach/pkg/sql/randgen"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/tests"
"github.com/cockroachdb/cockroach/pkg/sql/types"
Expand Down Expand Up @@ -1241,85 +1240,6 @@ func TestInitialPartitioning(t *testing.T) {
}
}

func TestSelectPartitionExprs(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

// TODO(dan): PartitionExprs for range partitions is waiting on the new
// range partitioning syntax.
testData := partitioningTest{
name: `partition exprs`,
schema: `CREATE TABLE %s (
a INT, b INT, c INT, PRIMARY KEY (a, b, c)
) PARTITION BY LIST (a, b) (
PARTITION p33p44 VALUES IN ((3, 3), (4, 4)) PARTITION BY LIST (c) (
PARTITION p335p445 VALUES IN (5),
PARTITION p33dp44d VALUES IN (DEFAULT)
),
PARTITION p6d VALUES IN ((6, DEFAULT)),
PARTITION pdd VALUES IN ((DEFAULT, DEFAULT))
)`,
}
if err := testData.parse(); err != nil {
t.Fatalf("%+v", err)
}

tests := []struct {
// partitions is a comma-separated list of input partitions
partitions string
// expr is the expected output
expr string
}{
{`p33p44`, `((a, b) = (3, 3)) OR ((a, b) = (4, 4))`},
{`p335p445`, `((a, b, c) = (3, 3, 5)) OR ((a, b, c) = (4, 4, 5))`},
{`p33dp44d`, `(((a, b) = (3, 3)) AND (NOT ((a, b, c) = (3, 3, 5)))) OR (((a, b) = (4, 4)) AND (NOT ((a, b, c) = (4, 4, 5))))`},
// NB See the TODO in the impl for why this next case has some clearly
// unrelated `!=`s.
{`p6d`, `((a,) = (6,)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4))))`},
{`pdd`, `NOT ((((a, b) = (3, 3)) OR ((a, b) = (4, 4))) OR ((a,) = (6,)))`},

{`p335p445,p6d`, `(((a, b, c) = (3, 3, 5)) OR ((a, b, c) = (4, 4, 5))) OR (((a,) = (6,)) AND (NOT (((a, b) = (3, 3)) OR ((a, b) = (4, 4)))))`},

// TODO(dan): The expression simplification in this method is all done
// by our normal SQL expression simplification code. Seems like it could
// use some targeted work to clean these up. Ideally the following would
// all simplyify to `(a, b) IN ((3, 3), (4, 4))`. Some of them work
// because for every requested partition, all descendent partitions are
// omitted, which is an optimization to save a little work with the side
// benefit of making more of these what we want.
{`p335p445,p33dp44d`, `(((a, b, c) = (3, 3, 5)) OR ((a, b, c) = (4, 4, 5))) OR ((((a, b) = (3, 3)) AND (NOT ((a, b, c) = (3, 3, 5)))) OR (((a, b) = (4, 4)) AND (NOT ((a, b, c) = (4, 4, 5)))))`},
{`p33p44,p335p445`, `((a, b) = (3, 3)) OR ((a, b) = (4, 4))`},
{`p33p44,p335p445,p33dp44d`, `((a, b) = (3, 3)) OR ((a, b) = (4, 4))`},
}

evalCtx := &eval.Context{
Codec: keys.SystemSQLCodec,
Settings: cluster.MakeTestingClusterSettings(),
}
for _, test := range tests {
t.Run(test.partitions, func(t *testing.T) {
var partNames tree.NameList
for _, p := range strings.Split(test.partitions, `,`) {
partNames = append(partNames, tree.Name(p))
}
expr, err := selectPartitionExprs(evalCtx, testData.parsed.tableDesc, partNames)
if err != nil {
t.Fatalf("%+v", err)
}
if exprStr := expr.String(); exprStr != test.expr {
t.Errorf("got\n%s\nexpected\n%s", exprStr, test.expr)
}
})
}
t.Run("error", func(t *testing.T) {
partNames := tree.NameList{`p33p44`, `nope`}
_, err := selectPartitionExprs(evalCtx, testData.parsed.tableDesc, partNames)
if !testutils.IsError(err, `unknown partition`) {
t.Errorf(`expected "unknown partition" error got: %+v`, err)
}
})
}

func TestRepartitioning(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down
16 changes: 16 additions & 0 deletions pkg/kv/kvclient/rangestats/BUILD.bazel
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "rangestats",
srcs = ["fetcher.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangestats",
visibility = ["//visibility:public"],
deps = [
"//pkg/kv",
"//pkg/roachpb",
"//pkg/sql/sem/eval",
],
)

get_x_data(name = "get_x_data")
Loading

0 comments on commit 6b7f005

Please sign in to comment.