Skip to content

Conversation

@celiala
Copy link
Collaborator

@celiala celiala commented Oct 29, 2025

Backport 1/1 commits from #156374.

/cc @cockroachdb/release


This PR fixes incorrect ordering in the rulesForReleases array:

Background

The rulesForReleases array in pkg/sql/schemachanger/scplan/plan.go stores schema changer rule registries for different cluster versions. The array is documented to be in descending order (newest version first), and the GetRulesRegistryForRelease() function depends on this ordering to correctly match cluster versions with their corresponding rule sets.

Bug

The array had V25_2 and V25_3 in ascending order instead of descending:

Before (incorrect):

var rulesForReleases = []rulesForRelease{
	{activeVersion: clusterversion.Latest, rulesRegistry: current.GetRegistry()},
	{activeVersion: clusterversion.V25_2, rulesRegistry: release_25_2.GetRegistry()},
	{activeVersion: clusterversion.V25_3, rulesRegistry: release_25_3.GetRegistry()},
}

After (correct):

var rulesForReleases = []rulesForRelease{
	// NB: sort versions in descending order, i.e. newest supported version first.
	{activeVersion: clusterversion.Latest, rulesRegistry: current.GetRegistry()},
	{activeVersion: clusterversion.V25_3, rulesRegistry: release_25_3.GetRegistry()},
	{activeVersion: clusterversion.V25_2, rulesRegistry: release_25_2.GetRegistry()},
}

Impact

GetRulesRegistryForRelease() iterates through the array in order and returns the first entry where activeVersion.IsActive() is true. With the incorrect ascending order, a cluster running version 25.3 would incorrectly get the 25.2 rules instead of the 25.3 rules.

Changes

  • Fixed ordering in pkg/sql/schemachanger/scplan/plan.go
  • Updated test expectations in pkg/cli/testdata/declarative-rules/invalid_version
  • Regenerated test output in pkg/cli/testdata/declarative-rules/deprules

Epic: None
Release note (bug fix): Fix rulesForReleases ordering to correctly match cluster versions with schema changer rule sets.

Release justification: GetRulesRegistryForRelease() iterates through the array in order and returns the first entry where activeVersion.IsActive() is true. With the incorrect ascending order, a cluster running version 25.3 would incorrectly get the 25.2 rules instead of the 25.3 rules.

@celiala celiala requested a review from a team as a code owner October 29, 2025 02:05
@blathers-crl
Copy link

blathers-crl bot commented Oct 29, 2025

Thanks for opening a backport.

Before merging, please confirm that the change does not break backwards compatibility and otherwise complies with the backport policy. Include a brief release justification in the PR description explaining why the backport is appropriate. All backports must be reviewed by the TL for the owning area. While the stricter LTS policy does not yet apply, please exercise judgment and consider gating non-critical changes behind a disabled-by-default feature flag when appropriate.

@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-code-systems labels Oct 29, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@celiala celiala force-pushed the backportrelease-25.4-156374 branch from 9327ba4 to 040bc4d Compare October 29, 2025 02:08
This fix addresses incorrect ordering in the rulesForReleases array
and adds a runtime check to prevent future ordering errors.

Bug Summary:
Location: pkg/sql/schemachanger/scplan/plan.go:157-162

Issue: The rulesForReleases array has incorrect ordering. Entries V25_2
and V25_3 are in ascending order, but the documented requirement
(line 158) and the search logic in GetRulesRegistryForRelease() require
descending order (newest first).

Current (incorrect):
  {Latest},    // V25_4
  {V25_2},     // ← wrong position
  {V25_3},     // ← wrong position

Should be (descending):
  {Latest},    // V25_4
  {V25_3},     // ← correct
  {V25_2},     // ← correct

The GetRulesRegistryForRelease function iterates through rulesForReleases
in order and returns the first entry where activeVersion.IsActive() is
true. With ascending order, it would incorrectly return V25_2 rules when
V25_3 is active.

Changes:
- Fixed ordering in pkg/sql/schemachanger/scplan/plan.go
- Added init-time assertion to validate descending order
- Fixed trailing space in error message in pkg/cli/declarative_print_rules.go

Epic: None
Release note (bug fix): Fix rulesForReleases ordering to correctly match
cluster versions with schema changer rule sets.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@celiala celiala force-pushed the backportrelease-25.4-156374 branch from 040bc4d to 8ad4b6c Compare October 29, 2025 02:11
@celiala celiala requested review from fqazi and rafiss October 29, 2025 02:13
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

@celiala Thank you for doing this!

:lgtm:

@fqazi reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

@celiala celiala merged commit f33e6f1 into cockroachdb:release-25.4 Oct 29, 2025
15 checks passed
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-code-systems v25.4.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants