Merge pull request #154406 from spilchen/blathers/backport-release-25.4-154363

release-25.4: sql/types: reintroduce downgradeType for NAME compatibility in mixed clusters

Author: spilchen

pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog

@@ -149,4 +149,4 @@ SELECT id, strip_volatile(descriptor) FROM crdb_internal.kv_catalog_descriptor W
 111         {"table": {"checks": [{"columnIds": [1], "constraintId": 2, "expr": "k > 0:::INT8", "name": "ck"}], "columns": [{"id": 1, "name": "k", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "v", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "dependedOnBy": [{"columnIds": [1, 2], "id": 112}], "formatVersion": 3, "id": 111, "name": "kv", "nextColumnId": 3, "nextConstraintId": 3, "nextIndexId": 2, "nextMutationId": 1, "parentId": 106, "primaryIndex": {"constraintId": 1, "encodingType": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["k"], "name": "kv_pkey", "partitioning": {}, "sharded": {}, "storeColumnIds": [2], "storeColumnNames": ["v"], "unique": true, "vecConfig": {}, "version": 4}, "privileges": {"ownerProto": "root", "users": [{"privileges": "2", "userProto": "admin", "withGrantOption": "2"}, {"privileges": "2", "userProto": "root", "withGrantOption": "2"}], "version": 3}, "replacementOf": {"time": {}}, "schemaLocked": true, "unexposedParentSchemaId": 107, "version": "7"}}
 112         {"table": {"columns": [{"id": 1, "name": "k", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "v", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"defaultExpr": "unique_rowid()", "hidden": true, "id": 3, "name": "rowid", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "dependsOn": [111], "formatVersion": 3, "id": 112, "indexes": [{"createdExplicitly": true, "foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [2], "keyColumnNames": ["v"], "keySuffixColumnIds": [3], "name": "idx", "partitioning": {}, "sharded": {}, "vecConfig": {}, "version": 4}], "isMaterializedView": true, "name": "mv", "nextColumnId": 4, "nextConstraintId": 2, "nextIndexId": 4, "nextMutationId": 1, "parentId": 106, "primaryIndex": {"constraintId": 1, "encodingType": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [3], "keyColumnNames": ["rowid"], "name": "mv_pkey", "partitioning": {}, "sharded": {}, "storeColumnIds": [1, 2], "storeColumnNames": ["k", "v"], "unique": true, "vecConfig": {}, "version": 4}, "privileges": {"ownerProto": "root", "users": [{"privileges": "2", "userProto": "admin", "withGrantOption": "2"}, {"privileges": "2", "userProto": "root", "withGrantOption": "2"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 107, "version": "8", "viewQuery": "SELECT k, v FROM db.public.kv"}}
 113         {"function": {"functionBody": "SELECT json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(json_remove_path(d, ARRAY['table':::STRING, 'families':::STRING]:::STRING[]), ARRAY['table':::STRING, 'nextFamilyId':::STRING]:::STRING[]), ARRAY['table':::STRING, 'indexes':::STRING, '0':::STRING, 'createdAtNanos':::STRING]:::STRING[]), ARRAY['table':::STRING, 'indexes':::STRING, '1':::STRING, 'createdAtNanos':::STRING]:::STRING[]), ARRAY['table':::STRING, 'indexes':::STRING, '2':::STRING, 'createdAtNanos':::STRING]:::STRING[]), ARRAY['table':::STRING, 'primaryIndex':::STRING, 'createdAtNanos':::STRING]:::STRING[]), ARRAY['table':::STRING, 'createAsOfTime':::STRING]:::STRING[]), ARRAY['table':::STRING, 'modificationTime':::STRING]:::STRING[]), ARRAY['function':::STRING, 'modificationTime':::STRING]:::STRING[]), ARRAY['type':::STRING, 'modificationTime':::STRING]:::STRING[]), ARRAY['schema':::STRING, 'modificationTime':::STRING]:::STRING[]), ARRAY['database':::STRING, 'modificationTime':::STRING]:::STRING[]);", "id": 113, "lang": "SQL", "name": "strip_volatile", "nullInputBehavior": "CALLED_ON_NULL_INPUT", "params": [{"class": "IN", "name": "d", "type": {"family": "JsonFamily", "oid": 3802}}], "parentId": 104, "parentSchemaId": 105, "privileges": {"ownerProto": "root", "users": [{"privileges": "2", "userProto": "admin", "withGrantOption": "2"}, {"privileges": "1048576", "userProto": "public"}, {"privileges": "2", "userProto": "root", "withGrantOption": "2"}], "version": 3}, "returnType": {"type": {"family": "JsonFamily", "oid": 3802}}, "version": "1", "volatility": "STABLE"}}
-4294966962  {"table": {"columns": [{"id": 1, "name": "f_table_catalog", "nullable": true, "type": {"family": "StringFamily", "oid": 19}}, {"id": 2, "name": "f_table_schema", "nullable": true, "type": {"family": "StringFamily", "oid": 19}}, {"id": 3, "name": "f_table_name", "nullable": true, "type": {"family": "StringFamily", "oid": 19}}, {"id": 4, "name": "f_geography_column", "nullable": true, "type": {"family": "StringFamily", "oid": 19}}, {"id": 5, "name": "coord_dimension", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "srid", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 7, "name": "type", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294966962, "name": "geography_columns", "nextColumnId": 8, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}, "vecConfig": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294966963, "version": "1"}}
+4294966962  {"table": {"columns": [{"id": 1, "name": "f_table_catalog", "nullable": true, "type": {"family": 11, "oid": 19}}, {"id": 2, "name": "f_table_schema", "nullable": true, "type": {"family": 11, "oid": 19}}, {"id": 3, "name": "f_table_name", "nullable": true, "type": {"family": 11, "oid": 19}}, {"id": 4, "name": "f_geography_column", "nullable": true, "type": {"family": 11, "oid": 19}}, {"id": 5, "name": "coord_dimension", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "srid", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 7, "name": "type", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294966962, "name": "geography_columns", "nextColumnId": 8, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}, "vecConfig": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294966963, "version": "1"}}

pkg/sql/logictest/testdata/logic_test/mixed_version_char

@@ -1,5 +1,7 @@
 # LogicTest: cockroach-go-testserver-25.2
 
+subtest regression_152629
+
 # This test verifies that the "char" type is handled correctly in a mixed
 # version cluster. The reason it's being tested is because during 25.4
 # development we began removing the legacy code for serializing types in the
@@ -25,3 +27,48 @@ user root nodeidx=2
 
 statement ok
 insert into t values (ARRAY['abc'::STRING, ''::STRING], 'cat'::STRING);
+
+subtest end
+
+subtest regression_153322
+
+# This is a repro scenario for 153322. It involves the NAME data type, which is
+# a string type column type.
+
+# 1 is already upgraded but adding a redundant one here incase this subtest is
+# run in isolation.
+upgrade 1
+
+user root nodeidx=1
+
+# Create the function using the NAME data type in the upgraded version.
+statement ok
+CREATE FUNCTION test_name_func(p NAME) RETURNS TEXT LANGUAGE SQL AS 'SELECT p::TEXT'
+
+query T
+SELECT test_name_func('test_value')
+----
+test_value
+
+user root nodeidx=2
+
+# Attempt to access the function on the old non-upgraded versions. These had
+# previously failed to resolve the function.
+
+query T
+SELECT test_name_func('test_value')
+----
+test_value
+
+statement ok
+ALTER FUNCTION test_name_func(NAME) RENAME TO test_name_func_renamed
+
+query T
+SELECT test_name_func_renamed('renamed_test')
+----
+renamed_test
+
+statement ok
+DROP FUNCTION test_name_func_renamed(NAME);
+
+subtest end

pkg/sql/types/types.go

@@ -2526,7 +2526,18 @@ func (t *T) IsPseudoType() bool {
 //
 // Marshal is part of the protoutil.Message interface.
 func (t *T) Size() (n int) {
-	return t.InternalType.Size()
+	// non-NAME types don't need any serialization changes to be backwards compatible.
+	if t.InternalType.Oid != oid.T_name {
+		return t.InternalType.Size()
+	}
+	// Need to first downgrade the type before delegating to InternalType,
+	// because Marshal will downgrade.
+	temp := *t
+	err := temp.downgradeType()
+	if err != nil {
+		panic(errors.NewAssertionErrorWithWrappedErrf(err, "error during Size call"))
+	}
+	return temp.InternalType.Size()
 }
 
 // Identical is the internal implementation for T.Identical. See that comment
@@ -2832,12 +2843,50 @@ func (t *T) upgradeType() error {
 	return nil
 }
 
+// downgradeType transforms NAME types for backwards-compatibility with v25.3
+// and earlier versions during mixed-version clusters. For non-NAME types, this
+// is a no-op.
+//
+// NAME types are downgraded to use the legacy Family=name encoding so that v25.3
+// nodes can correctly interpret them without losing NAME semantics.
+//
+// TODO(sql-foundations): Remove this function entirely once v25.4 is the minimum
+// supported version and we no longer need to support mixed-version clusters with
+// v25.3 nodes.
+func (t *T) downgradeType() error {
+	// Set Family for NAME type for backwards-compatibility.
+	switch t.Family() {
+	case StringFamily, CollatedStringFamily:
+		t.InternalType.Family = name
+		// If locale is an empty string, we set it to nil to ensure older versions
+		// don’t misclassify the NAME as a collated string.
+		if t.InternalType.Locale != nil && len(*t.InternalType.Locale) == 0 {
+			t.InternalType.Locale = nil
+		}
+	}
+
+	return nil
+}
+
 // Marshal serializes a type into a byte representation using gogo protobuf
-// serialization rules. It returns the resulting bytes as a slice.
+// serialization rules. It returns the resulting bytes as a slice. The bytes
+// are serialized in a format that is backwards-compatible with the previous
+// version of CRDB so that clusters can run in mixed version mode during
+// upgrade.
 //
 //	bytes, err := protoutil.Marshal(&typ)
 func (t *T) Marshal() (data []byte, err error) {
-	return protoutil.Marshal(&t.InternalType)
+	// non-NAME types don't need any serialization changes to be backwards compatible.
+	if t.InternalType.Oid != oid.T_name {
+		return protoutil.Marshal(&t.InternalType)
+	}
+	// First downgrade to a struct that will be serialized in a backwards-
+	// compatible bytes format.
+	temp := *t
+	if err := temp.downgradeType(); err != nil {
+		return nil, err
+	}
+	return protoutil.Marshal(&temp.InternalType)
 }
 
 // MarshalToSizedBuffer is like Mashal, except that it deserializes to
@@ -2846,7 +2895,15 @@ func (t *T) Marshal() (data []byte, err error) {
 //
 // Marshal is part of the protoutil.Message interface.
 func (t *T) MarshalToSizedBuffer(data []byte) (int, error) {
-	return t.InternalType.MarshalToSizedBuffer(data)
+	// non-NAME types don't need any serialization changes to be backwards compatible.
+	if t.InternalType.Oid != oid.T_name {
+		return t.InternalType.MarshalToSizedBuffer(data)
+	}
+	temp := *t
+	if err := temp.downgradeType(); err != nil {
+		return 0, err
+	}
+	return temp.InternalType.MarshalToSizedBuffer(data)
 }
 
 // MarshalTo behaves like Marshal, except that it deserializes to an existing
@@ -2856,7 +2913,15 @@ func (t *T) MarshalToSizedBuffer(data []byte) (int, error) {
 //
 // Marshal is part of the protoutil.Message interface.
 func (t *T) MarshalTo(data []byte) (int, error) {
-	return t.InternalType.MarshalTo(data)
+	// non-NAME types don't need any serialization changes to be backwards compatible.
+	if t.InternalType.Oid != oid.T_name {
+		return t.InternalType.MarshalTo(data)
+	}
+	temp := *t
+	if err := temp.downgradeType(); err != nil {
+		return 0, err
+	}
+	return temp.InternalType.MarshalTo(data)
 }
 
 // String returns the name of the type, similar to the Name method. However, it

pkg/sql/types/types_jsonpb.go

@@ -19,11 +19,17 @@ import (
 // MarshalJSONPB marshals the T to json. This is necessary as otherwise
 // this field will be lost to the crdb_internal.pb_to_json and the likes.
 func (t *T) MarshalJSONPB(marshaler *jsonpb.Marshaler) ([]byte, error) {
+	temp := *t
+	// Only NAME types need changes to be backwards compatible.
+	if temp.InternalType.Oid == oid.T_name {
+		if err := temp.downgradeType(); err != nil {
+			return nil, err
+		}
+	}
 	// Map empty locale to nil so empty string does not appear in the JSON result.
 	// TODO(rafi): When we upgrade to go1.24, we can modify the proto definition
 	//  of the locale field to use `[(gogoproto.jsontag) = ",omitzero"]` instead of
 	//  this workaround.
-	temp := *t
 	if temp.InternalType.Locale != nil && len(*temp.InternalType.Locale) == 0 {
 		temp.InternalType.Locale = nil
 	}

pkg/sql/types/types_test.go

@@ -789,6 +789,7 @@ func TestUnmarshalCompat(t *testing.T) {
 		{InternalType{Family: StringFamily, VisibleType: visibleVARCHAR, Width: 20}, MakeVarChar(20)},
 		{InternalType{Family: StringFamily, VisibleType: visibleCHAR}, BPChar},
 		{InternalType{Family: StringFamily, VisibleType: visibleQCHAR, Width: 1}, QChar},
+		{InternalType{Family: name}, Name},
 	}
 
 	for _, tc := range testCases {
@@ -981,6 +982,17 @@ func TestUpgradeType(t *testing.T) {
 				Locale:      &emptyLocale,
 			}},
 		},
+		{
+			desc: "legacy NAME support",
+			input: &T{InternalType: InternalType{
+				Family: name,
+			}},
+			expected: &T{InternalType: InternalType{
+				Family: StringFamily,
+				Oid:    oid.T_name,
+				Locale: &emptyLocale,
+			}},
+		},
 	}
 
 	for _, tc := range testCases {