Skip to content

Conversation

@blathers-crl
Copy link

@blathers-crl blathers-crl bot commented Nov 6, 2025

Backport 1/1 commits from #156962 on behalf of @mgartner.


This commit fixes a bug in the vectorized engine that caused internal
errors when projections operated on constant NULL values. The
proj_const_right_ops operators do not correctly handle NULL inputs. So,
we avoid these code paths when operator does not operate on NULL values
and the LHS or RHS of an operator is NULL by simply projecting NULLs.
This is similar to the change made in #128123 and addresses a TODO added
in that PR.

This code path is unlikely to hit (maybe impossible) without generic
query plans. In an optimized cusotm plan, the optimizer will fold
expressions with NULL values at optimization time, and the execution
engine will never see expressions of this form.

Fixes #152771

Release note (bug fix): A bug has been fixed that could cause internal
errors for queries using generic query plans with NULL placeholder
values.


Release justification: Low-risk bug fix.

This commit fixes a bug in the vectorized engine that caused internal
errors when projections operated on constant NULL values. The
proj_const_right_ops operators do not correctly handle NULL inputs. So,
we avoid these code paths when operator does not operate on NULL values
and the LHS or RHS of an operator is NULL by simply projecting NULLs.
This is similar to the change made in #128123 and addresses a TODO added
in that PR.

This code path is unlikely to hit (maybe impossible) without generic
query plans. In an optimized cusotm plan, the optimizer will fold
expressions with `NULL` values at optimization time, and the execution
engine will never see expressions of this form.

Fixes #152771

Release note (bug fix): A bug has been fixed that could cause internal
errors for queries using generic query plans with `NULL` placeholder
values.
@blathers-crl blathers-crl bot requested a review from a team as a code owner November 6, 2025 00:16
@blathers-crl blathers-crl bot requested review from michae2 and removed request for a team November 6, 2025 00:16
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Nov 6, 2025
@blathers-crl blathers-crl bot requested review from mgartner and yuzefovich November 6, 2025 00:16
@blathers-crl
Copy link
Author

blathers-crl bot commented Nov 6, 2025

Thanks for opening a backport.

Before merging, please confirm that it falls into one of the following categories (select one):

  • Non-production code changes. Includes test-only changes, build system changes, etc.
  • Fixes for serious issues. Defined in the policy as correctness, stability, or security issues, data corruption/loss, significant performance regressions, breaking working and widely used functionality, or an inability to detect and debug production issues.
  • Other approved changes. These changes must be gated behind a disabled-by-default feature flag unless there is a strong justification not to.

Add a brief release justification to the PR description explaining your selection.

Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy.

All backports must be reviewed by the TL and EM for the owning area.

@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-sql-queries SQL Queries Team labels Nov 6, 2025
@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.

This probably doesn't qualify for backport to 25.2, will defer to @michae2 on this one.

@yuzefovich reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

@blathers-crl
Copy link
Author

blathers-crl bot commented Nov 6, 2025

✅ PR #156977 is compliant with backport policy

Confidence: high
Critical bug criteria met: [Stability or security issues Bugs that can cause the DB to return incorrect results or result in suboptimal performance]
Backward compatible: true
Explanation: The pull request is compliant with the CockroachDB backport policy for several reasons:

  1. Release Justification: The PR body includes a 'Release justification:' statement indicating that this is a 'Low-risk bug fix.' This statement helps justify the backport as per the backport policy exceptions for release justification.
  2. Critical Bug Fix: The changes fix an issue where projections in the vectorized engine could lead to internal errors when operated on constant NULL values. Such errors pertain to the bug categories of 'Stability issues' or 'Bugs that can cause the DB to return incorrect results,' which makes this a critical bug needing immediate fixing, thereby exempting it from feature flag requirements.
  3. File Changes: The files changed (pkg/sql/colexec/colbuilder/execplan.go and pkg/sql/logictest/testdata/logic_test/vectorize) are involved directly in query execution and logic test, which suggests the fix is critical for stable system operation. The tests added help validate the fix without altering the behavior of unrelated system parts.
  4. Backward Compatibility: The changes are additions and modifications of logic within existing operational paths, which do not remove version gates or disrupt backward compatibility.

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

@mgartner
Copy link
Collaborator

mgartner commented Nov 6, 2025

This probably doesn't qualify for backport to 25.2, will defer to @michae2 on this one.

Ya, maybe not. There are risks on both sides.

Hitting this is much more likely in v25.2 which enabled generic query plans by default. If we do not backport, then we risk workloads hitting this internal error—and it will be hit sporadically depending on the arguments and whether or not a generic query plan is selected. That being said, one could argue that passing NULL placeholder values inside binary expressions like this is probably uncommon, so the chance of hitting the error is low.

If we backport, there is some chance that this change is incorrect or dangerous (or unlocks other incorrect or dangerous code paths). In the worst case we'd see incorrect results or nodes panic. It's difficult to quantify this risk.

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

I agree with blather's analysis. I think we can consider this a serious issue, because it becomes much more likely in 25.2 with generic query plans. Spurious query failures after upgrade to 25.2 would be an escalation to us. So I don't think we need an ENGREQ for this. And I think this qualifies for the feature flag exception because it's < 5 loc and no customer would consider it a breaking change.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

@yuzefovich
Copy link
Member

(Just to be clear, I have no concerns whatsoever about the risk of this change, and if it was up to me, I'd backport this without reservations. My comment was mostly about having enough justification given the stricter backport policy. 😉 )

@mgartner mgartner merged commit 7d8f333 into release-25.2 Nov 7, 2025
16 checks passed
@mgartner mgartner deleted the blathers/backport-release-25.2-156962 branch November 7, 2025 22:36
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. T-sql-queries SQL Queries Team target-release-25.2.10

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants