Skip to content

release-22.1: opt: prevent overflow when normalizing comparison with constants#88970

Merged
mgartner merged 3 commits intocockroachdb:release-22.1from
mgartner:backport22.1-88199-88969
Sep 30, 2022
Merged

release-22.1: opt: prevent overflow when normalizing comparison with constants#88970
mgartner merged 3 commits intocockroachdb:release-22.1from
mgartner:backport22.1-88199-88969

Conversation

@mgartner
Copy link
Copy Markdown
Contributor

@mgartner mgartner commented Sep 28, 2022

Backport:

Please see individual PRs for details.

Release justification: Fixes long-standing correctness bug in optimizer.

/cc @cockroachdb/release

Previously, the `NormalizeCmpPlusConst`, `NormalizeCmpMinusConst`, and
`NormalizeCmpMinusConst` would normalize expressions in the form
`var +/- const1 ? const2` and `const1 - var ? const2`, where `const1`
and `const2` were const constants and `?` is any comparison operator, to
attempt to isolate the variable on one side of the comparison. Below are
some examples of the transformations these rules made:

    a+1 > 10 =>  a > 10-1
    a-1 > 10 =>  a > 10+1
    1-a > 10 =>  1-10 > a

However, these transformations were invalid when the newly created
addition or subtraction operators overflowed. In the case of an
expression involving TIME and INTERVAL types, this could cause incorrect
query results. For example:

    t-'2 hr'::INTERVAL < '23:00:00'::TIME
    =>
    t < '23:00:00'::TIME+'2 hr'::INTERVAL
    =>
    t < '01:00:00'::TIME

The first expression and last expression have different semantic meaning
because the addition of the constant TIME and constant INTERVAL
overflowed.

This commit prevents the rules from transforming expressions if they
would cause an overflow/underflow. In order to detect overflow, there
are new restrictions that prevent these rules from firing in some cases.
Notably, the datum on the RHS of the newly constructed +/- operator must
be an integer, float, decimal, or interval. Also, the result type of the
newly constructed +/- operator must be equivalent to the LHS of the
operator. Both of these restrictions are required in order to detect
overflow/underflow.

Informs cockroachdb#88128

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of expressions in the form `col +/- const1 ? const2`, where
`const1` and `const2` are constant values and `?` is any comparison
operator. The bug was caused by operator overflow when the optimizer
attempted to simplify these expressions to have a single constant value.
@mgartner mgartner requested review from a team, DrewKimball, msirek and rytaft September 28, 2022 22:14
@mgartner mgartner requested a review from a team as a code owner September 28, 2022 22:14
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Sep 28, 2022

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?

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

It was recently discovered that normalization rules in the optimizer
were invalid (see cockroachdb#88128). These rules were adapted from the heuristic
optimizer several years ago, and they still remain in the `normalize`
package even though the heuristic optimizer no longer exists. The
`normalize` package is still used to normalize expressions during
backfilling, so the invalid rules can cause incorrect computed column
values and corrupt indexes. This commit removes these rules entirely.

Informs cockroachdb#88128

Release note: None
This commit fixes minor typos introduced in cockroachdb#88199.

Release note: None
@mgartner mgartner force-pushed the backport22.1-88199-88969 branch from 4f0f144 to 8db6c09 Compare September 30, 2022 20:27
@mgartner mgartner merged commit a769b09 into cockroachdb:release-22.1 Sep 30, 2022
@mgartner mgartner deleted the backport22.1-88199-88969 branch September 30, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants