Skip to content

Conversation

yuzefovich
Copy link
Member

Backport 1/1 commits from #151774.

/cc @cockroachdb/release


When processing batches that touch multiple ranges, the DistSender needs
to know whether to iterate across those ranges in the forward or reverse
manner (i.e. ASC or DESC for range keys). Currently, it only uses the
reverse direction if it finds at least one ReverseScan request in the
batch, and it uses the forward direction otherwise. This goes against
the needs of SQL which might issue point Gets when performing a SQL
revscan operation, and currently in such a scenario we would hit an
error (instead of returning the results in incorrect order).

This commit fixes the issue by introducing IsReverse boolean on the
batch header to explicitly indicate the direction for range iteration.
It seems reasonable that the caller should be explicit about this, and
we also add a validation that the boolean is set correctly (meaning that
it should be false when only forward range requests are present and
true when only reverse range requests are present).

In order to simplify the story a bit, the DistSender will no longer
allow batches that have both forward and reverse range requests.
Previously, this limitation only applied to the batches with limits, but
now it's extended to be unconditional. SQL never issues such batches, so
the limitation seems acceptable. This limitation required some updates
to the existing tests, including KVNemesis to not generate batches that
are now disallowed.

See also 619f395 for some related context.

Given that the new header field is only examined on the KV client, the
change can be backported with no mixed-version concerns.

Fixes: #146637.

Release note (bug fix): Previously, CockroachDB could hit an error
ERROR: span with results after resume span... when evaluating some
queries with ORDER BY ... DESC in an edge case. The bug has been present
since about v22.1 and is now fixed.

Release justification: bug fix for an issue with no good workaround.

When processing batches that touch multiple ranges, the DistSender needs
to know whether to iterate across those ranges in the forward or reverse
manner (i.e. ASC or DESC for range keys). Currently, it only uses the
reverse direction if it finds at least one ReverseScan request in the
batch, and it uses the forward direction otherwise. This goes against
the needs of SQL which might issue point Gets when performing a SQL
revscan operation, and currently in such a scenario we would hit an
error (instead of returning the results in incorrect order).

This commit fixes the issue by introducing `IsReverse` boolean on the
batch header to explicitly indicate the direction for range iteration.
It seems reasonable that the caller should be explicit about this, and
we also add a validation that the boolean is set correctly (meaning that
it should be `false` when only forward range requests are present and
`true` when only reverse range requests are present).

In order to simplify the story a bit, the DistSender will no longer
allow batches that have both forward and reverse range requests.
Previously, this limitation only applied to the batches with limits, but
now it's extended to be unconditional. SQL never issues such batches, so
the limitation seems acceptable. This limitation required some updates
to the existing tests, including KVNemesis to not generate batches that
are now disallowed.

See also 619f395 for some related context.

Given that the new header field is only examined on the KV client, the
change can be backported with no mixed-version concerns.

Release note (bug fix): Previously, CockroachDB could hit an error
`ERROR: span with results after resume span...` when evaluating some
queries with ORDER BY ... DESC in an edge case. The bug has been present
since about v22.1 and is now fixed.
@yuzefovich yuzefovich requested review from a team as code owners August 20, 2025 16:35
Copy link

blathers-crl bot commented Aug 20, 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 Aug 20, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link

blathers-crl bot commented Aug 20, 2025

✅ PR #152184 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's primary purpose is to fix a critical bug that affects SQL queries with ORDER BY DESC, primarily manifesting as an error in edge cases (listed as 'ERROR: span with results after resume span...'). The modifications include the addition of an 'IsReverse' flag in batch headers to explicitly handle the directionality of range iterations, which addresses incorrect results due to query execution order—this aligns with the critical bug fix categories for stability issues and bugs that cause the DB to return incorrect results. The PR meets the critical bug fix criteria and includes a 'Release justification: bug fix for an issue with no good workaround', thus exempting it from feature flag requirements. No backward-incompatible changes such as removing version gates or API changes are introduced. All files changed pertain to the database's core functionalities and include necessary test adjustments to reflect the new logic. The PR does not add any new features that would require gating.

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

@yuzefovich
Copy link
Member Author

There were some merge conflicts here and in #152185, but nothing major.

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.

LGTM

@yuzefovich yuzefovich merged commit f915419 into cockroachdb:release-25.2 Aug 20, 2025
16 checks passed
@yuzefovich yuzefovich deleted the backport25.2-151774 branch August 20, 2025 17:59
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 T-sql-queries SQL Queries Team v25.2.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants