Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: prevent stack overflow when deriving computed column filters #132701

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Oct 15, 2024

opt: add max-stack opttest option

This commit adds the max-stack option for opttests, allowing tests to
alter the maximum stack size for the goroutine that optimizes the query.

Release note: None

opt: prevent stack overflow when deriving computed column filters

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. 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, an OrExpr tree is never built. Instead,
CombineComputedColFilters returns a slice of ScalarExprs that
represents a disjunction of expressions. This effectively eliminates the
possibility of stack overflows in code that recursively traverses the
derived expressions. It also simplified the logic.

Fixes #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.

@mgartner mgartner requested a review from a team as a code owner October 15, 2024 19:53
@mgartner mgartner requested review from mw5h and a team and removed request for a team October 15, 2024 19:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Oct 15, 2024
@mgartner mgartner changed the title 132669 fix overflow opt: prevent stack overflow in derived computed column filters Oct 15, 2024
@mgartner mgartner changed the title opt: prevent stack overflow in derived computed column filters opt: prevent stack overflow when deriving computed column filters Oct 15, 2024
@mgartner mgartner force-pushed the 132669-fix-overflow branch from ebbe1af to 41eff36 Compare October 15, 2024 19:55
@mgartner

This comment was marked as outdated.

@mgartner mgartner requested a review from DrewKimball October 15, 2024 20:03
@mgartner mgartner force-pushed the 132669-fix-overflow branch 2 times, most recently from 2345c64 to 23c2f70 Compare October 16, 2024 12:30
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Elegant fix. Nice work!

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mw5h)

This commit adds the `max-stack` option for opttests, allowing tests to
alter the maximum stack size for the goroutine that optimizes the query.

Release note: None
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. 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, an `OrExpr` tree is never built. Instead,
`CombineComputedColFilters` returns a slice of `ScalarExpr`s that
represents a disjunction of expressions. This effectively eliminates the
possibility of stack overflows in code that recursively traverses the
derived expressions. It also simplified the logic.

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`.
@mgartner mgartner force-pushed the 132669-fix-overflow branch from 23c2f70 to a3d27e8 Compare October 17, 2024 19:31
@mgartner
Copy link
Collaborator Author

TFTR!

bors r+

@craig craig bot merged commit e59f52b into cockroachdb:master Oct 17, 2024
22 of 23 checks passed
Copy link

blathers-crl bot commented Oct 17, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 56d6b22 to blathers/backport-release-23.1-132701: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


error creating merge commit from a3d27e8 to blathers/backport-release-23.2-132701: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


error creating merge commit from a3d27e8 to blathers/backport-release-24.1-132701: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

blathers-crl bot pushed a commit that referenced this pull request Oct 29, 2024
This commit relaxes the maximum Go stack size in bytes for a test added
in #132701 from 50KB to 125KB. The very low max stack size was causing
stack overflows to occur in unrelated functions, like parsing, in some
nightly tests. I'm hoping that more than doubling this will eliminate
the flakes.

Fixes #133212

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Oct 31, 2024
This commit relaxes the maximum Go stack size in bytes for a test added
in #132701 from 50KB to 125KB. The very low max stack size was causing
stack overflows to occur in unrelated functions, like parsing, in some
nightly tests. I'm hoping that more than doubling this will eliminate
the flakes.

Fixes #133212

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Oct 31, 2024
This commit relaxes the maximum Go stack size in bytes for a test added
in #132701 from 50KB to 125KB. The very low max stack size was causing
stack overflows to occur in unrelated functions, like parsing, in some
nightly tests. I'm hoping that more than doubling this will eliminate
the flakes.

Fixes #133212

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 5, 2024
The `max-stack` opt tester option now run the test command in a separate
goroutine. A fresh stack makes tests using this setting more reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see cockroachdb#132701) to 100KB, between 65KB in which the
test fails after the fix in cockroachdb#132701 and 135KB in which the test fails
before the fix.

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 5, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see cockroachdb#132701) to 100KB, between 65KB in which the
test fails after the fix in cockroachdb#132701 and 135KB in which the test fails
before the fix.

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 5, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see cockroachdb#132701) to 100KB, between 65KB in which the
test fails after the fix in cockroachdb#132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
craig bot pushed a commit that referenced this pull request Nov 6, 2024
133328: sql: fix trigger interaction with computed columns r=DrewKimball a=DrewKimball

This commit makes the following changes to the way row-level BEFORE triggers interact with computed columns:
* BEFORE triggers now observe NULL values in place of computed columns.
* Modifications to computed columns by BEFORE triggers are ignored.
* Computed columns are re-computed after row-level BEFORE triggers execute.

These changes make the behavior consistent with that of Postgres.

Fixes #132979

Release note: None

134313: opt: make `max-stack` opt tester option more reliable r=mgartner a=mgartner

The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Epic: None

Release note: None


Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
craig bot pushed a commit that referenced this pull request Nov 6, 2024
133220: opt: plan row-level BEFORE triggers for cascading mutations r=DrewKimball a=DrewKimball

#### opt: mechanical changes for BEFORE triggers on cascades

This commit consists of the following changes to prepare for adding
support for row-level BEFORE triggers to cascading mutations:
* Added `unsafe_allow_triggers_modifying_cascades` which will determine
  whether a trigger can modify the rows mutated by a cascade, default off.
* Added a "cascade" argument to `buildRowLevelBeforeTriggers` to provide
  the context when building a row-level BEFORE trigger.
* Refactoring the logic for invoking `crdb_internal.plpgsql_raise` to be
  re-used for triggers.

Informs #132971

Release note: None

#### opt: plan row-level BEFORE triggers for cascading mutations

This commit adds logic to invoke the functions for row-level BEFORE
triggers that are fired by a cascade's mutation. This is mostly
straightforward, except for the fact that a row-level BEFORE trigger
can actually modify or filter rows-to-be-mutated. In Postgres, it's
possible to cause constraint violations via this behavior. This commit
adds a runtime check to ensure that trigger functions do not modify
cascading updates/deletes. The check is gated behind
`unsafe_allow_triggers_modifying_cascades`, default off.

Fixes #132971

Release note (sql change): Cascades can now fire row-level BEFORE triggers.
By default, attempting to modify or eliminate the cascading update/delete
will result in a `Triggered Data Change Violation` error. Users that wish
to do this anyway can set `unsafe_allow_triggers_modifying_cascades`.
Note that doing so could result in constraint violations, similar to Postgres.

#### sql: fix queuing behavior for checks and triggers

This commit fixes a bug in `PlanAndRunPostQueries` that could cause
check queries to be run more than once. This would cause problems with
unclosed resources after query completion. This commit also fixes a
minor bug that could cause triggers queued by triggers to be run
before newly-queued checks and cascades, instead of after. The following
commit includes a regression test.

Fixes #133792

Release note: None

#### opt: don't add spurious check queries after BEFORE trigger

This commit fixes a bug that could cause spurious constraint violation
errors when a BEFORE trigger was planned on a cascaded mutation. Since we
don't allow triggers to modify the rows of a cascaded mutation, it is safe
to avoid the extra check entirely. This commit also adds a diamond-pattern
cascade test, with triggers on each table.

Fixes #133784

Release note: None

134005: ui: delete old db pages r=xinhaoz a=xinhaoz

Delete legacy db pages and related functionality.
These pages are now replaced by the v2 db pages.
Deleted
- Database overview page and related apis
- Database details page and related apis
- Database table page and related apis

Epic: none

Release note (ui change): As of 25.1 the legacy db page which was previously available via Advanced Debug is no longer available.

134313: opt: make `max-stack` opt tester option more reliable r=mgartner a=mgartner

The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Epic: None

Release note: None


134353: mixedversion: update skip upgrades logic r=RaduBerinde a=RaduBerinde

This commit updates the logic that determines which versions support
skipping the previous major release during upgrade.

Epic: REL-1292
Release note: None

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
blathers-crl bot pushed a commit that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see #132701) to 100KB, between 65KB in which the
test fails after the fix in #132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see cockroachdb#132701) to 100KB, between 65KB in which the
test fails after the fix in cockroachdb#132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
mgartner added a commit to mgartner/cockroach that referenced this pull request Nov 6, 2024
The `max-stack` opt tester option now runs the test command in a
separate goroutine. A fresh stack makes tests using this setting more
reliable.

It also decreases the `max-stack` of the original test that motivated
the `max-stack` option (see cockroachdb#132701) to 100KB, between 65KB in which the
test fails after the fix in cockroachdb#132701 and 135KB in which the test fails
before the fix.

Finally, the test has been disabled under `race` builds which increase
the size of stack frames and would cause this test to fail.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opt: large IN filter causes stack overflow in CombineComputedColFilters
3 participants