sql/types: reintroduce downgradeType for NAME compatibility in mixed clusters#154363
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Sep 29, 2025
Conversation
Member
|
This change is |
3e49b9c to
47a0640
Compare
rafiss
approved these changes
Sep 29, 2025
Collaborator
rafiss
left a comment
There was a problem hiding this comment.
nice find! just had one question about skipping the downgradeType logic conditionally
| return t.InternalType.Size() | ||
| // Need to first downgrade the type before delegating to InternalType, | ||
| // because Marshal will downgrade. | ||
| temp := *t |
Collaborator
There was a problem hiding this comment.
this line with the pointer dereference was actually the original cause of the race condition. could we add an early-exit so we don't do anything for all types that do not have oid == T_name?
Contributor
Author
There was a problem hiding this comment.
Good idea. I applied this comment.
| return protoutil.Marshal(&t.InternalType) | ||
| // First downgrade to a struct that will be serialized in a backwards- | ||
| // compatible bytes format. | ||
| temp := *t |
Collaborator
There was a problem hiding this comment.
similar to above -- we can skip this pointer dereference if the oid is not T_name
…clusters This patch restores the `downgradeType` hook to maintain serialization compatibility for the NAME type in mixed-version clusters (v25.3 and 25.4). In previous releases, a NAME type could be misinterpreted by older nodes. Specifically, a v25.3 node deserializing a NAME encoded by a v25.4 node would mistakenly treat it as text (`oid.T_text`). This was due to the old `upgradeType` logic overwriting the OID when no `VisibleType` was set, causing `T_name` to degrade to `T_text`. PR cockroachdb#148694 addressed this issue for v25.4 by changing `upgradeType` (to avoid clobbering a provided OID) and removing the `downgradeType` function. However, the follow-up fix for mixed-version serialization (PR cockroachdb#152633) relied on the `InternalType.VisibleType` field to tag string types for older nodes. Because the NAME type doesn’t use a `VisibleType` (it’s represented as a STRING family with a special OID), that change didn’t cover NAME. As a result, NAME remained broken in a 25.3/25.4 mixed cluster. This commit explicitly handles NAME during serialization. When sending a NAME type to the wire, we downgrade it to the legacy encoding: we set its type family to `Family=name` (legacy enum value 11) so that v25.3 nodes recognize it as a NAME. Additionally, if the locale is an empty string, we set it to `nil` to ensure older versions don’t misclassify the NAME as a collated string. Together, these steps preserve the correct type interpretation on older nodes. Once all nodes are on v25.4, this compatibility shim can be removed. At that point, the improved upgrade logic in v25.4 (which no longer overrides a valid OID) means we won’t need to downgrade types for backward compatibility. We may also perform a one time descriptor migration to update any NAME descriptors to the modern format after the cluster upgrade. In short, `downgradeType` for NAME is a temporary measure to bridge the v25.3 gap. Fixes cockroachdb#153322 Release note: None
47a0640 to
036b796
Compare
spilchen
commented
Sep 29, 2025
Contributor
Author
spilchen
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
| return t.InternalType.Size() | ||
| // Need to first downgrade the type before delegating to InternalType, | ||
| // because Marshal will downgrade. | ||
| temp := *t |
Contributor
Author
There was a problem hiding this comment.
Good idea. I applied this comment.
| return protoutil.Marshal(&t.InternalType) | ||
| // First downgrade to a struct that will be serialized in a backwards- | ||
| // compatible bytes format. | ||
| temp := *t |
Contributor
Author
|
TFTR! bors r+ |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch restores the
downgradeTypehook to maintain serialization compatibility for the NAME type in mixed-version clusters (v25.3 and 25.4).In previous releases, a NAME type could be misinterpreted by older nodes. Specifically, a v25.3 node deserializing a NAME encoded by a v25.4 node would mistakenly treat it as text (
oid.T_text). This was due to the oldupgradeTypelogic overwriting the OID when noVisibleTypewas set, causingT_nameto degrade toT_text. PR #148694 addressed this issue for v25.4 by changingupgradeType(to avoid clobbering a provided OID) and removing thedowngradeTypefunction.However, the follow-up fix for mixed-version serialization (PR #152633) relied on the
InternalType.VisibleTypefield to tag string types for older nodes. Because the NAME type doesn’t use aVisibleType(it’s represented as a STRING family with a special OID), that change didn’t cover NAME. As a result, NAME remained broken in a 25.3/25.4 mixed cluster.This commit explicitly handles NAME during serialization. When sending a NAME type to the wire, we downgrade it to the legacy encoding: we set its type family to
Family=name(legacy enum value 11) so that v25.3 nodes recognize it as a NAME. Additionally, if the locale is an empty string, we set it tonilto ensure older versions don’t misclassify the NAME as a collated string. Together, these steps preserve the correct type interpretation on older nodes.Once all nodes are on v25.4, this compatibility shim can be removed. At that point, the improved upgrade logic in v25.4 (which no longer overrides a valid OID) means we won’t need to downgrade types for backward compatibility. We may also perform a one time descriptor migration to update any NAME descriptors to the modern format after the cluster upgrade. In short,
downgradeTypefor NAME is a temporary measure to bridge the v25.3 gap.Fixes #153322
Release note: None