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

release-22.2: constraint: perform cancel checking when combining constraints #112075

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Oct 9, 2023

Backport 1/1 commits from #111979 on behalf of @msirek.

/cc @cockroachdb/release


Previously, the combining of constraint spans when one constraint is a
suffix of the other could take a long time and cause the
statement_timeout session setting to not be honored when each
constraint has hundreds or thousands of spans.

The issue is that constraint.Combine has double nested loops to
consider every combination of one span from one constraint with one span
of the other constraint. The building of possibly millions of spans
may take excessive CPU time and allocate excessive amounts of memory.

The fix is to maintain a counter in constraint.Combine and call the
query cancel check function every 16 iterations. The cancel check
function itself will check for query timeout every 1024 iterations, so
effectively every 16K iterations constraint.Combine will perform
cancel checking and abort the query if the timeout has been reached.

Epic: none
Fixes: #111862

Release note (bug fix): This patch fixes an issue where the optimizer
fails to honor the statement_timeout session setting when generating
constrained index scans for queries with large IN lists or = ANY
predicates on multiple index key columns, which may lead to an out
of memory condition on the node.


Release justification: Critical fix which allows the statement_timeout setting to be honored and avoid OOMs on queries with many constants in = ANY or IN list predicates.

Previously, the combining of constraint spans when one constraint is a
suffix of the other could take a long time and cause the
`statement_timeout` session setting to not be honored when each
constraint has hundreds or thousands of spans.

The issue is that `constraint.Combine` has double nested loops to
consider every combination of one span from one constraint with one span
of the other constraint. The building of possibly millions of spans
may take excessive CPU time and allocate excessive amounts of memory.

The fix is to maintain a counter in `constraint.Combine` and call the
query cancel check function every 16 iterations. The cancel check
function itself will check for query timeout every 1024 iterations, so
effectively every 16K iterations `constraint.Combine` will perform
cancel checking and abort the query if the timeout has been reached.

Epic: none
Fixes: #111862

Release note (bug fix): This patch fixes an issue where the optimizer
fails to honor the `statement_timeout` session setting when generating
constrained index scans for queries with large IN lists or `= ANY`
predicates on multiple index key columns, which may lead to an out
of memory condition on the node.
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.2-111979 branch from c003f83 to 5e43c33 Compare October 9, 2023 23:55
@blathers-crl blathers-crl bot requested a review from a team as a code owner October 9, 2023 23:55
@blathers-crl blathers-crl bot added the blathers-backport This is a backport that Blathers created automatically. label Oct 9, 2023
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.2-111979 branch from 8cd8255 to 96bcc65 Compare October 9, 2023 23:55
@blathers-crl blathers-crl bot added the O-robot Originated from a bot. label Oct 9, 2023
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 9, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Oct 9, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, and @msirek)

@msirek msirek removed the request for review from cucaroach October 10, 2023 00:02
@msirek msirek merged commit fc76278 into release-22.2 Oct 10, 2023
@msirek msirek deleted the blathers/backport-release-22.2-111979 branch October 10, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants