Skip to content

sql/schemachanger: refactor update of parent schema ID in table descriptors#169345

Merged
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
bghal:sql-set-parent-fallthrough
Apr 30, 2026
Merged

sql/schemachanger: refactor update of parent schema ID in table descriptors#169345
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
bghal:sql-set-parent-fallthrough

Conversation

@bghal
Copy link
Copy Markdown
Contributor

@bghal bghal commented Apr 29, 2026

The schema ID field was not being set when walked over for an update
because of a silent fallthrough; another op was updating the field. The
change adds an error-default that exposes the silent fallthrough and
refactors handling of the table descriptor's update to the expected
place.

Supports: #168255

Part of: #164216

Epic: CRDB-31325

Release note: None

@bghal bghal requested a review from a team as a code owner April 29, 2026 15:45
@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Apr 29, 2026

😎 Merged successfully - details.

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 29, 2026

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.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@bghal bghal force-pushed the sql-set-parent-fallthrough branch 3 times, most recently from 16a287e to 473d554 Compare April 29, 2026 16:13
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

great catch. can we add a test that would have caught this bug?

assuming that this led to a user-facing issue, let's add release note (bug fix) and backport this to 26.1 where this bug was first manifested. (#158141)

Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

nice, just saw the test case that needed updating. could there also be a logictest that shows a user-facing issue?

@bghal bghal added backport-26.1.x Flags PRs that need to be backported to 26.1 backport-26.2.x Flags PRs that need to be backported to 26.2 labels Apr 29, 2026
…iptors

The schema ID field was not being set when walked over for an update
because of a silent fallthrough; another op was updating the field. The
change adds an error-default that exposes the silent fallthrough and
refactors handling of the table descriptor's update to the expected
place.

Supports: cockroachdb#168255

Part of: cockroachdb#164216

Epic: CRDB-31325

Release note: None
@bghal bghal force-pushed the sql-set-parent-fallthrough branch from 473d554 to aab7d7a Compare April 30, 2026 15:32
@bghal bghal changed the title sql/schemachanger: fix inconsistent parent schema ID field in table descriptors sql/schemachanger: refactor update of parent schema ID in table descriptors Apr 30, 2026
@bghal bghal removed backport-26.1.x Flags PRs that need to be backported to 26.1 backport-26.2.x Flags PRs that need to be backported to 26.2 labels Apr 30, 2026
@bghal
Copy link
Copy Markdown
Contributor Author

bghal commented Apr 30, 2026

From those tests, turns out it was being updated but it wasn't where it was expected so this is a refactor and tests.

@bghal bghal requested a review from rafiss April 30, 2026 15:36
bghal added a commit to bghal/cockroach that referenced this pull request Apr 30, 2026
The issue in cockroachdb#169345 demonstrated the risk of switch statements on types
if an unexpected type is passed to the vistor function. This change
guards against future bugs by adding errors-on-default that trip on
unexpected types.

Epic: CRDB-31325

Informed by: cockroachdb#169345

Release note: None
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks for the cleanup!

@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Apr 30, 2026

/trunk merge

@trunk-io trunk-io Bot merged commit 705b380 into cockroachdb:master Apr 30, 2026
26 checks passed
bghal added a commit to bghal/cockroach that referenced this pull request May 1, 2026
The issue in cockroachdb#169345 demonstrated the risk of switch statements on types
if an unexpected type is passed to the vistor function. This change
guards against future bugs by adding errors-on-default that trip on
unexpected types.

Epic: CRDB-31325

Informed by: cockroachdb#169345

Release note: None
bghal added a commit to bghal/cockroach that referenced this pull request May 1, 2026
The issue in cockroachdb#169345 demonstrated the risk of switch statements on types
if an unexpected type is passed to the vistor function. This change
guards against future bugs by adding errors-on-default that trip on
unexpected types.

Epic: CRDB-31325

Informed by: cockroachdb#169345

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants