Skip to content

Conversation

@blathers-crl
Copy link

@blathers-crl blathers-crl bot commented Oct 24, 2025

Backport 1/1 commits from #156105 on behalf of @mw5h.


Previously, when the optimizer stored the ordinal of an index to use for writing tombstones in non-SERIALIZABLE transactions, it would call the Ordinal() method, which returns the value from the table's descriptor. We would later use this value as an input to the Index() method to retrieve the index in question. However, Ordinal() can differ from the catalog's view of the schema during schema change, resulting in sometimes getting the wrong index ordinal from the optimizer's point of view, sometimes resulting in an out of bounds error.

This patch switches the ordinal we save to be the actual index used when creating the optTable, which feels janky, but is at least correct.

Fixes: #151477
Release note (bug fix): A bug which could occasionally cause DML on regional by row tables with unique indexes that don't reference the region to fail under READ-COMMITTED isolation has been corrected.


Release justification: Simple fix for problem found in testing.

Previously, when the optimizer stored the ordinal of an index to use for
writing tombstones in non-SERIALIZABLE transactions, it would call the
Ordinal() method, which returns the value from the table's descriptor.
We would later use this value as an input to the Index() method to
retrieve the index in question. However, Ordinal() can differ from the
catalog's view of the schema during schema change, resulting in
sometimes getting the wrong index ordinal from the optimizer's point of
view, sometimes resulting in an out of bounds error.

This patch switches the ordinal we save to be the actual index used when
creating the optTable, which feels janky, but is at least correct.

Fixes: #151477
Release note (bug fix): A bug which could occasionally cause DML on
regional by row tables with unique indexes that don't reference the
region to fail under READ-COMMITTED isolation has been corrected.
@blathers-crl blathers-crl bot requested a review from a team as a code owner October 24, 2025 21:45
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-24.3-156105 branch from 4d4cdbb to cdd312b Compare October 24, 2025 21:45
@blathers-crl blathers-crl bot requested review from mw5h and removed request for a team October 24, 2025 21:45
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Oct 24, 2025
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 24, 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 requested a review from michae2 October 24, 2025 21:45
@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-sql-queries SQL Queries Team labels Oct 24, 2025
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 24, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 24, 2025

❌ PR #156133 does not comply with backport policy

Confidence: medium
Explanation: The PR changes production code in the 'pkg/sql/opt_catalog.go' file, which indicates changes to the database internals. The 'Release justification' in the PR body is insufficiently detailed and does not explicitly state why this change is critical, which is necessary for policy exemption. Moreover, the change addresses a bug related to using the wrong index ordinal, potentially leading to failures under specific transaction isolation conditions. However, without a clear indication that this bug meets the critical bug criteria in terms of data loss or major performance regression, it does not automatically qualify for backport without feature flag gating or stronger justification.

As per the PR, the bug fix corrects a condition that could cause failures under specific circumstances ('occasionally cause DML... to fail under READ-COMMITTED isolation') impacting transaction performance and correctness. Although this might lead to incorrect results or suboptimal performance, a more explicit alignment with critical bug criteria or a description of the impact's severity would strengthen the justification.

The changes do not seem to include a new feature flag for gating, and the backport does not directly mention its necessity due to critical nature or feature mitigation strategies.
Recommendation: Add a justification as to why this is a critical bug fix that can be safely backported or consider gating the PR behind a feature flag.

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants