Skip to content

Commit

Permalink
opt: prevent stack overflow when deriving computed column filters
Browse files Browse the repository at this point in the history
Previously, a filter of the form `col IN (elem0, elem1, ..., elemN)`
could result in a stack overflow in the optimizer when `N` is very
large, e.g., 1.6 million or more . The problem would occur when trying
to derive constraints for indexes on computed column, specifically when
those computed columns are dependent on the column in the filter, e.g.,
`col` in the example.

The current implementation builds an `OrExpr` tree with depth equal to
the length of the `IN` list (see the TODO in `makeSpansForExpr`
suggesting that this should be avoided). It then constructs a
`FiltersItem` with the `OrExpr` tree as the condition, causing logical
properties to be built for the expression. When building logical
properties, the `OrExpr` tree is traversed recursively, which causes the
stack to overflow when the tree is very deep.

Now, a `FiltersItems` is never constructed, thus logical properties are
not built for the `OrExpr` tree, avoiding this particular type of stack
overflow.

Fixes cockroachdb#132669

Release note (bug fix): A bug in the query optimizer which could cause
CockroachDB nodes to crash in rare cases has been fixed. The bug could
occur when a query contains a filter in the form
`col IN (elem0, elem1, ..., elemN)` only when `N` is very large, e.g.,
1.6+ million, and when `col` exists in a hash-sharded index or exists a
table with an indexed, computed column dependent on `col`.
  • Loading branch information
mgartner committed Oct 15, 2024
1 parent 2c67d3b commit 9f9791f
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 17 deletions.
19 changes: 10 additions & 9 deletions pkg/sql/opt/idxconstraint/index_constraints.go
Original file line number Diff line number Diff line change
Expand Up @@ -641,22 +641,23 @@ func (c *indexConstraintCtx) makeSpansForExpr(
// Attempt to convert the constraint into a disjunction of ANDed IS
// predicates, with additional derived IS conjuncts on computed
// columns based on columns in the constraint spans.
// TODO(msirek/mgartner): Modify CombineComputedColFilters to build a
// `Constraint` or `constraint.Set` directly instead of building a
// filter and calling `makeSpansForExpr`.
computedColumnFilters := norm.CombineComputedColFilters(
// TODO(msirek/mgartner): Modify CombineComputedColFilters to
// build a `Constraint` or `constraint.Set` directly instead of
// building a filter and calling `makeSpansForExpr`.
computedColumnExpr := norm.CombineComputedColFilters(
c.computedCols,
c.keyCols,
c.colsInComputedColsExpressions,
constraints.Constraint(0),
c.factory,
)
if len(computedColumnFilters) == 1 {
// All predicates in `computedColumnFilters[0].Condition` fully
// represent the original condition plus derived predicates, so we
// only have to make spans on the new condition.
if computedColumnExpr != nil {
// All predicates in `computedColumnFilters[0].Condition`
// fully represent the original condition plus derived
// predicates, so we only have to make spans on the new
// condition.
c.skipComputedColPredDerivation = true
localTight := c.makeSpansForExpr(offset, computedColumnFilters[0].Condition, out)
localTight := c.makeSpansForExpr(offset, computedColumnExpr, out)
c.skipComputedColPredDerivation = false
return localTight
}
Expand Down
13 changes: 5 additions & 8 deletions pkg/sql/opt/norm/general_funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,9 @@ func (c *CustomFuncs) tryFoldComputedCol(
// which seeks to fold computed column expressions using values from single-key
// constraint spans in order to build new predicates on those computed columns
// and AND them with predicates built from the constraint span key used to
// compute the computed column value.
// compute the computed column value. If successful, it returns a tree of
// OrExprs, with a depth equal to the number of spans in the constraint. If
// unsuccessful, nil is returned.
//
// New derived keys cannot be saved for the filters as a whole, for later
// processing, because a given combination of span key values is only applicable
Expand Down Expand Up @@ -1090,14 +1092,13 @@ func CombineComputedColFilters(
colsInComputedColsExpressions opt.ColSet,
cons *constraint.Constraint,
f *Factory,
) memo.FiltersExpr {
) opt.ScalarExpr {
if len(computedCols) == 0 {
return nil
}
if !f.evalCtx.SessionData().OptimizerUseImprovedComputedColumnFiltersDerivation {
return nil
}
var combinedComputedColFilters memo.FiltersExpr
var orOp opt.ScalarExpr
if !cons.Columns.ColSet().Intersects(colsInComputedColsExpressions) {
// If this constraint doesn't involve any columns used to construct a
Expand Down Expand Up @@ -1148,11 +1149,7 @@ func CombineComputedColFilters(
return nil
}
}
if orOp != nil {
combinedComputedColFilters =
append(combinedComputedColFilters, f.ConstructFiltersItem(orOp))
}
return combinedComputedColFilters
return orOp
}

// buildConstColsMapFromConstraint converts a constraint on one or more columns
Expand Down
50 changes: 50 additions & 0 deletions pkg/sql/opt/xform/testdata/rules/select
Original file line number Diff line number Diff line change
Expand Up @@ -2806,6 +2806,56 @@ project
└── filters
└── a:2 = b:3 [outer=(2,3), constraints=(/2: (/NULL - ]; /3: (/NULL - ]), fd=(2)==(3), (3)==(2)]

# Regression test for #132669. Deriving computed column constraints should not
# cause the stack to grow excessively.
exec-ddl
CREATE TABLE t132669 (
a INT,
b INT NOT NULL AS (mod(a, 16)) VIRTUAL,
INDEX (b, a)
);
----

# Optimize the query with a significantly reduced max stack size. This allows
# unnecessary recursion to trigger a stack overflow without having to make the
# `IN` list below huge - triggering a stack overflow with Go's default max stack
# size requires a list of ~1.6 million elements.
opt max-stack=125KB format=hide-all
SELECT * FROM t132669
WHERE a IN (
1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
31, 32, 33, 34, 35, 36, 37, 38, 39, 40,
41, 42, 43, 44, 45, 46, 47, 48, 49, 50,
51, 52, 53, 54, 55, 56, 57, 58, 59, 60,
61, 62, 63, 64, 65, 66, 67, 68, 69, 70,
71, 72, 73, 74, 75, 76, 77, 78, 79, 80,
81, 82, 83, 84, 85, 86, 87, 88, 89, 90,
91, 92, 93, 94, 95, 96, 97, 98, 99, 100,
101, 102, 103, 104, 105, 106, 107, 108, 109, 110,
111, 112, 113, 114, 115, 116, 117, 118, 119, 120,
121, 122, 123, 124, 125, 126, 127, 128, 129, 130,
131, 132, 133, 134, 135, 136, 137, 138, 139, 140,
141, 142, 143, 144, 145, 146, 147, 148, 149, 150,
151, 152, 153, 154, 155, 156, 157, 158, 159, 160,
161, 162, 163, 164, 165, 166, 167, 168, 169, 170,
171, 172, 173, 174, 175, 176, 177, 178, 179, 180,
181, 182, 183, 184, 185, 186, 187, 188, 189, 190,
191, 192, 193, 194, 195, 196, 197, 198, 199, 200
)
----
project
├── select
│ ├── scan t132669
│ │ └── computed column expressions
│ │ └── b
│ │ └── mod(a, 16)
│ └── filters
│ └── a IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200)
└── projections
└── mod(a, 16)

# --------------------------------------------------
# GenerateInvertedIndexScans
# --------------------------------------------------
Expand Down

0 comments on commit 9f9791f

Please sign in to comment.