Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
83875: opt: fix memo cycle caused by join ordering r=DrewKimball a=DrewKimball

In some rare cases when null-rejection rules fail to fire, a redundant filter
can be inferred in an `InnerJoin` - `LeftJoin` complex. This could previously
result in the `JoinOrderBuilder` attempting to add a `Select` to the same memo
group as its input, which would create a memo cycle. This patch prevents the
`JoinOrderBuilder` from adding the `Select` to the memo in such cases.

What follows is a more detailed explanation of the conditions that could
previously cause a memo cycle.

`InnerJoin` operators have two properties that make them more 'reorderable'
than other types of joins: (1) their conjuncts can be reordered separately
and (2) new conjuncts can be inferred from equalities. As a special case of
(1), an `InnerJoin` can be pushed into the left side of a `LeftJoin`, leaving
behind any `Select` conjuncts that reference the right side of the `LeftJoin`.

This allows the `JoinOrderBuilder` to make the following transformation:
```
(InnerJoin
   A
   (InnerJoin
      B
      (LeftJoin
         C
         D
         c=d
      )
      b=c
   )
   a=b, a=d
)
=>
(InnerJoin
   A
   (Select
      (LeftJoin
         (InnerJoin
            B
            C
            b=c
         )
         D
         c=d
      )
      b=d // Inferred filter!
   )
   a=b, a=d
)
```
Note the new `b=d` filter that was inferred and subsequently left on
a `Select` operator after the reordering. The crucial point is that
this filter is redundant - the input to the `Select` is already a
valid reordering of the `BCD` join complex.

The `JoinOrderBuilder` avoids adding redundant filters to `InnerJoin`
operators, but does not do the same for the `Select` case because it
was assumed that the filters left on the `Select` would never be redundant.
This is because the `a=d` filter *should* rejects nulls, so the `LeftJoin`
should have been simplified. However, in rare cases null-rejection does not
take place. Because the input to the `Select` is already a valid reordering,
the `JoinOrderBuilder` ends up trying to add the `Select` to the same group
as its input - namely, the `BCD` join group. This causes a cycle in the memo.

Fixes #80901

Release note (bug fix): Fixed a bug that could cause an optimizer
panic in rare cases when a query had a left join in the input of
an inner join.

83945: storage/metamorphic: Add MVCC delete range using tombstone r=erikgrinaker a=itsbilal

This change adds MVCCDeleteRangeUsingTombstone to the MVCC
metamorphic tests. MVCCDeleteRange had already existed in
this test suite, so this ended up being a relatively simple
addition.

One part of #82545, with possibly more parts to follow
as other MVCC-level operations are added that utilize
`writer.{Put,Clear}{MVCC,Engine}RangeKey`.

Release note: None.

83992: server: log stacks on 500 errors r=ajwerner a=ajwerner

Before this change, we'd just log the error body, which often is not very
helpful. The format specifier makes me think that the intention was to log
the stacks. This made debugging [this](#83677 (comment))
trivial as opposed to hard.

Release note: None

84015: bazel: clear configurations when running `git grep` in `check.sh` r=miretskiy,mari-crl a=rickystewart

The configurations `grep.{column,lineNumber,fullName}` can be set
globally or on a per-user basis and change the output of `git grep`,
which breaks checks that do exact-string matching. We manually clear
these configurations for the `git grep`s in this file to ensure the
output is predictable on any machine.

Release note: None

Co-authored-by: Andrew Kimball <andrewekimball@gmail.com>
Co-authored-by: Bilal Akhtar <bilal@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
  • Loading branch information
5 people committed Jul 7, 2022
5 parents e67e47f + ac77889 + 9901620 + 688f535 + a6bbc5f commit 6b7082a
Show file tree
Hide file tree
Showing 9 changed files with 419 additions and 52 deletions.
11 changes: 7 additions & 4 deletions build/bazelutil/check.sh
Expand Up @@ -6,6 +6,9 @@ set -euo pipefail
# wrong with the Bazel build. This is run in CI as well as by `dev generate`.
# Set COCKROACH_BAZEL_CHECK_FAST to skip the longer-running logic in this file.

CONFIGS="-c grep.column=false -c grep.lineNumber=false -c grep.fullName=false"
GIT_GREP="git $CONFIGS grep"

EXISTING_GO_GENERATE_COMMENTS="
pkg/roachprod/vm/aws/config.go://go:generate go-bindata -mode 0600 -modtime 1400000000 -pkg aws -o embedded.go config.json old.json
pkg/roachprod/vm/aws/config.go://go:generate gofmt -s -w embedded.go
Expand Down Expand Up @@ -63,7 +66,7 @@ pkg/util/buildutil/crdb_test_on.go://go:build crdb_test && !crdb_test_off
"

if [ -z "${COCKROACH_BAZEL_CHECK_FAST:-}" ]; then
git grep 'go:generate stringer' pkg | while read LINE; do
$GIT_GREP 'go:generate stringer' pkg | while read LINE; do
dir=$(dirname $(echo $LINE | cut -d: -f1))
type=$(echo $LINE | grep -o -- '-type[= ][^ ]*' | sed 's/-type[= ]//g' | awk '{print tolower($0)}')
build_out=$(bazel query --output=build "//$dir:${type}_string.go")
Expand All @@ -79,7 +82,7 @@ fi
# We exclude stringer and add-leaktest.sh -- the former is already all
# Bazelfied, and the latter can be safely ignored since we have a lint to check
# the same thing: https://github.com/cockroachdb/cockroach/issues/64440
git grep '//go:generate' 'pkg/**/*.go' | grep -v stringer | grep -v 'add-leaktest\.sh' | while read LINE; do
$GIT_GREP '//go:generate' 'pkg/**/*.go' | grep -v stringer | grep -v 'add-leaktest\.sh' | while read LINE; do
if [[ "$EXISTING_GO_GENERATE_COMMENTS" == *"$LINE"* ]]; then
# Grandfathered.
continue
Expand All @@ -93,7 +96,7 @@ git grep '//go:generate' 'pkg/**/*.go' | grep -v stringer | grep -v 'add-leaktes
exit 1
done

git grep 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | grep -v pkg/cli/BUILD.bazel | grep -v generate-test-suites | cut -d: -f1 | while read LINE; do
$GIT_GREP 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | grep -v pkg/cli/BUILD.bazel | grep -v generate-test-suites | cut -d: -f1 | while read LINE; do
if [[ "$EXISTING_BROKEN_TESTS_IN_BAZEL" == *"$LINE"* ]]; then
# Grandfathered.
continue
Expand All @@ -103,7 +106,7 @@ git grep 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | g
exit 1
done

git grep '//go:build' pkg | grep crdb_test | while read LINE; do
$GIT_GREP '//go:build' pkg | grep crdb_test | while read LINE; do
if [[ "$EXISTING_CRDB_TEST_BUILD_CONSTRAINTS" == *"$LINE"* ]]; then
# Grandfathered.
continue
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/admin.go
Expand Up @@ -186,7 +186,7 @@ func (s *adminServer) RegisterGateway(
// serverError logs the provided error and returns an error that should be returned by
// the RPC endpoint method.
func serverError(ctx context.Context, err error) error {
log.ErrorfDepth(ctx, 1, "%+s", err)
log.ErrorfDepth(ctx, 1, "%+v", err)
return errAdminAPIError
}

Expand Down
6 changes: 6 additions & 0 deletions pkg/sql/opt/testutils/opttester/opt_tester.go
Expand Up @@ -528,6 +528,9 @@ func New(catalog cat.Catalog, sql string) *OptTester {
// - group-limit: used with check-size to set a max limit on the number of
// groups that can be added to the memo before a testing error is returned.
//
// - memo-cycles: used with memo to search the memo for cycles and output a
// path with a cycle if one is found.
//
// - use-multi-col-stats sets the value for
// SessionData.OptimizerUseMultiColStats which indicates whether or not
// multi-column statistics are used for cardinality estimation in the
Expand Down Expand Up @@ -1115,6 +1118,9 @@ func (f *Flags) Set(arg datadriven.CmdArg) error {
}
f.UseMultiColStats = b

case "memo-cycles":
f.MemoFormat = xform.FmtCycle

default:
return fmt.Errorf("unknown argument: %s", arg.Key)
}
Expand Down
40 changes: 23 additions & 17 deletions pkg/sql/opt/testutils/opttester/reorder_joins.go
Expand Up @@ -73,23 +73,29 @@ func (ot *OptTester) ReorderJoins() (string, error) {
relsJoinedLast = ""
})

o.JoinOrderBuilder().NotifyOnAddJoin(func(left, right, all, refs []memo.RelExpr, op opt.Operator) {
relsToJoin := jof.formatVertexSet(all)
if relsToJoin != relsJoinedLast {
ot.output(fmt.Sprintf("Joining %s\n", relsToJoin))
relsJoinedLast = relsToJoin
}
ot.indent(
fmt.Sprintf(
"%s %s [%s, refs=%s]",
jof.formatVertexSet(left),
jof.formatVertexSet(right),
joinOpLabel(op),
jof.formatVertexSet(refs),
),
)
joinsConsidered++
})
o.JoinOrderBuilder().NotifyOnAddJoin(
func(left, right, all, joinRefs, selRefs []memo.RelExpr, op opt.Operator) {
relsToJoin := jof.formatVertexSet(all)
if relsToJoin != relsJoinedLast {
ot.output(fmt.Sprintf("Joining %s\n", relsToJoin))
relsJoinedLast = relsToJoin
}
var selString string
if len(selRefs) > 0 {
selString = fmt.Sprintf(" [select, refs=%s]", jof.formatVertexSet(selRefs))
}
ot.indent(
fmt.Sprintf(
"%s %s [%s, refs=%s]%s",
jof.formatVertexSet(left),
jof.formatVertexSet(right),
joinOpLabel(op),
jof.formatVertexSet(joinRefs),
selString,
),
)
joinsConsidered++
})

expr, err := ot.optimizeExpr(o, nil)
if err != nil {
Expand Down
82 changes: 54 additions & 28 deletions pkg/sql/opt/xform/join_order_builder.go
Expand Up @@ -62,7 +62,7 @@ type OnReorderRuleParam struct {
// the base relations of the left and right inputs of the join, the set of all
// base relations currently being considered, the base relations referenced by
// the join's ON condition, and the type of join.
type OnAddJoinFunc func(left, right, all, refs []memo.RelExpr, op opt.Operator)
type OnAddJoinFunc func(left, right, all, joinRefs, selectRefs []memo.RelExpr, op opt.Operator)

// JoinOrderBuilder is used to add valid orderings of a given join tree to the
// memo during exploration.
Expand Down Expand Up @@ -689,7 +689,7 @@ func (jb *JoinOrderBuilder) addJoin(
}
if jb.onAddJoinFunc != nil {
// Hook for testing purposes.
jb.callOnAddJoinFunc(s1, s2, joinFilters, op)
jb.callOnAddJoinFunc(s1, s2, joinFilters, selectFilters, op)
}

left := jb.plans[s1]
Expand All @@ -715,7 +715,7 @@ func (jb *JoinOrderBuilder) addJoin(

if jb.onAddJoinFunc != nil {
// Hook for testing purposes.
jb.callOnAddJoinFunc(s2, s1, joinFilters, op)
jb.callOnAddJoinFunc(s2, s1, joinFilters, selectFilters, op)
}
}
}
Expand Down Expand Up @@ -754,6 +754,12 @@ func (jb *JoinOrderBuilder) addToGroup(
) {
if len(selectFilters) > 0 {
joinExpr := jb.memoize(op, left, right, on, nil)
if joinExpr.FirstExpr() == grp.FirstExpr() {
// In rare cases, the select filters may be redundant. In this case,
// adding a select to the group with the redundant filters would create a
// memo cycle (see #80901).
return
}
selectExpr := &memo.SelectExpr{
Input: joinExpr,
Filters: selectFilters,
Expand Down Expand Up @@ -950,13 +956,14 @@ func (jb *JoinOrderBuilder) callOnReorderFunc(join memo.RelExpr) {
// callOnAddJoinFunc calls the onAddJoinFunc callback function. Panics if the
// function is nil.
func (jb *JoinOrderBuilder) callOnAddJoinFunc(
s1, s2 vertexSet, edges memo.FiltersExpr, op opt.Operator,
s1, s2 vertexSet, joinFilters, selectFilters memo.FiltersExpr, op opt.Operator,
) {
jb.onAddJoinFunc(
jb.getRelationSlice(s1),
jb.getRelationSlice(s2),
jb.getRelationSlice(s1.union(s2)),
jb.getRelationSlice(jb.getRelations(jb.getFreeVars(edges))),
jb.getRelationSlice(jb.getRelations(jb.getFreeVars(joinFilters))),
jb.getRelationSlice(jb.getRelations(jb.getFreeVars(selectFilters))),
op,
)
}
Expand Down Expand Up @@ -1269,6 +1276,22 @@ func (e *edge) checkNonInnerJoin(s1, s2 vertexSet) bool {
// assoc(left-join, inner-join) is false.
// 3. The TES now includes all three relations, meaning that the inner join
// cannot join any two relations together (including xy and uv).
//
// Note that checking that the TES intersects both s1 and s2 diverges slightly
// from the paper. This makes explicit the fact that we forbid the
// introduction of cross joins that did not exist in the original normalized
// plan. (The paper checks if the left and right tables intersect s1 and s2
// respectively). However, the check is exactly equivalent to that given in
// the paper for the following reasons:
// 1. For degenerate predicates (one or both inputs not referenced) we add
// all base relations from the unreferenced input(s) to the TES
// (see calcTES).
// 2. (1) ensures that (TES ∩ S != ∅) implies (TABLES(input) ∩ S != ∅).
// 3. Since we discard join orders that introduce new cross-products anyway,
// we always filter out cases where (TABLES(input) ∩ S != ∅) but
// (TES ∩ S == ∅).
// Therefore, the check we use here prevents exactly the same reorderings as
// the check used in the paper.
return e.tes.intersection(e.op.leftVertexes).isSubsetOf(s1) &&
e.tes.intersection(e.op.rightVertexes).isSubsetOf(s2) &&
e.tes.intersects(s1) && e.tes.intersects(s2)
Expand All @@ -1277,29 +1300,6 @@ func (e *edge) checkNonInnerJoin(s1, s2 vertexSet) bool {
// checkInnerJoin performs an applicability check for an inner join between the
// two given sets of base relations. If it returns true, an inner join can be
// constructed using the filters from this edge and the two given relation sets.
//
// Why is the inner join check different from the non-inner join check?
// In effect, the difference between the inner and non-inner edge checks is that
// for inner joins, relations can be moved 'across' the join relative to their
// positions in the original join tree. This is necessary in order to allow
// inner join conjuncts from different joins to be combined into new join
// operators. For example, take this perfectly valid (and desirable)
// transformation:
//
// SELECT * FROM xy
// INNER JOIN (SELECT * FROM ab INNER JOIN uv ON a = u)
// ON x = a AND x = u
// =>
// SELECT * FROM ab
// INNER JOIN (SELECT * FROM xy INNER JOIN uv ON x = u)
// ON x = a AND a = u
//
// Note that, from the perspective of the x = a edge, it looks like the join has
// been commuted (the xy and ab relations switched sides). From the perspective
// of the a = u edge, however, all relations that were previously on the left
// are still on the left, and all relations that were on the right are still on
// the right. The stricter requirements of checkNonInnerJoin would not allow
// this transformation to take place.
func (e *edge) checkInnerJoin(s1, s2 vertexSet) bool {
if !e.checkRules(s1, s2) {
// The conflict rules for this edge are not satisfied for a join between s1
Expand All @@ -1310,6 +1310,32 @@ func (e *edge) checkInnerJoin(s1, s2 vertexSet) bool {
// The TES must be a subset of the relations of the candidate join inputs. In
// addition, the TES must intersect both s1 and s2 (the edge must connect the
// two vertex sets).
//
// Why is the inner join check different from the non-inner join check?
// In effect, the difference between the inner and non-inner edge checks is
// that for inner joins, relations can be moved 'across' the join relative to
// their positions in the original join tree. This is necessary in order to
// allow inner join conjuncts from different joins to be combined into new
// join operators. For example, take this perfectly valid (and desirable)
// transformation:
//
// SELECT * FROM xy
// INNER JOIN (SELECT * FROM ab INNER JOIN uv ON a = u)
// ON x = a AND x = u
// =>
// SELECT * FROM ab
// INNER JOIN (SELECT * FROM xy INNER JOIN uv ON x = u)
// ON x = a AND a = u
//
// Note that, from the perspective of the x = a edge, it looks like the join
// has been commuted (the xy and ab relations switched sides). From the
// perspective of the a = u edge, however, all relations that were previously
// on the left are still on the left, and all relations that were on the right
// are still on the right. The stricter requirements of checkNonInnerJoin
// would not allow this transformation to take place.
//
// See the checkNonInnerJoin comments for an explanation of why the
// intersection checks differ from those shown in the paper.
return e.tes.isSubsetOf(s1.union(s2)) && e.tes.intersects(s1) && e.tes.intersects(s2)
}

Expand Down

0 comments on commit 6b7082a

Please sign in to comment.