version: provide backwards compatible implementation of String

WIP

Epic: none

Release note: None

pkg/clusterversion/cockroach_versions.go

@@ -436,3 +436,26 @@ func ListBetween(from, to roachpb.Version) []roachpb.Version {
 	}
 	return cvs
 }
+
+// StringForPersistence returns the string representation of the given
+// version in cases where that version needs to be persisted. This
+// takes backwards compatibility into account, making sure that we use
+// the old version formatting if we need to continue supporting
+// releases that don't understand it.
+//
+// TODO(renato): remove this function once MinSupported is at least 24.1.
+func StringForPersistence(v roachpb.Version) string {
+	return stringForPersistenceWithMinSupported(v, MinSupported.Version())
+}
+
+func stringForPersistenceWithMinSupported(v, minSupported roachpb.Version) string {
+	// newFormattingVersion is the version in which the new version
+	// formatting (#115223) was introduced.
+	newFormattingVersion := roachpb.Version{Major: 24, Minor: 1}
+
+	if minSupported.AtLeast(newFormattingVersion) {
+		return v.String()
+	}
+
+	return v.OldRepresentation()
+}

pkg/clusterversion/cockroach_versions_test.go

@@ -181,3 +181,38 @@ func TestReleaseSeries(t *testing.T) {
 		require.Equalf(t, expected, k.ReleaseSeries(), "version: %s", k)
 	}
 }
+
+func TestStringForPersistence(t *testing.T) {
+	testCases := []struct {
+		v            roachpb.Version
+		minSupported roachpb.Version
+		expected     string
+	}{
+		{
+			v:            roachpb.Version{Major: 23, Minor: 2},
+			minSupported: roachpb.Version{Major: 23, Minor: 2},
+			expected:     "23.2",
+		},
+		{
+			v:            roachpb.Version{Major: 24, Minor: 1},
+			minSupported: roachpb.Version{Major: 23, Minor: 2},
+			expected:     "24.1",
+		},
+		{
+			v:            roachpb.Version{Major: 24, Minor: 1, Internal: 10},
+			minSupported: roachpb.Version{Major: 23, Minor: 2},
+			expected:     "24.1-10",
+		},
+		{
+			v:            roachpb.Version{Major: 24, Minor: 1, Internal: 10},
+			minSupported: roachpb.Version{Major: 24, Minor: 1},
+			expected:     "24.1-upgrading-step-010",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run("", func(t *testing.T) {
+			require.Equal(t, tc.expected, stringForPersistenceWithMinSupported(tc.v, tc.minSupported))
+		})
+	}
+}

pkg/roachpb/version.go

@@ -86,6 +86,20 @@ func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {
 	}
 }
 
+// OldRepresentation returns the string representation of this
+// function prior to the change in formatting to make them more
+// readable (#115223). This is necessary in case we are persisting the
+// version and need to be backwards compatible.
+//
+// TODO(renato): remove this function once MinSupported is at least 24.1.
+func (v Version) OldRepresentation() string {
+	if v.IsFinal() {
+		return v.String()
+	}
+
+	return fmt.Sprintf("%d.%d-%d", v.Major, v.Minor, v.Internal)
+}
+
 // IsFinal returns true if this is a final version (as opposed to a transitional
 // internal version during upgrade).
 //

pkg/roachpb/version_test.go

@@ -28,11 +28,17 @@ func TestParseVersion(t *testing.T) {
 		{s: "23.1-upgrading-to-23.2-step-004", v: Version{Major: 23, Minor: 1, Internal: 4}, roundtrip: true},
 		{s: "1000023.1-upgrading-to-1000023.2-step-004", v: Version{Major: 1000023, Minor: 1, Internal: 4}, roundtrip: true},
 		{s: "23.1-4", v: Version{Major: 23, Minor: 1, Internal: 4}},
+		{ /* old representation */ v: Version{Major: 23, Minor: 1, Internal: 4}},
 		{s: "23.1-upgrading-step-004", v: Version{Major: 23, Minor: 1, Internal: 4}},
 	}
 	for _, tc := range testData {
 		t.Run("", func(t *testing.T) {
-			v, err := ParseVersion(tc.s)
+			versionStr := tc.s
+			if tc.s == "" {
+				versionStr = tc.v.OldRepresentation()
+			}
+
+			v, err := ParseVersion(versionStr)
 			require.NoError(t, err)
 			require.Equal(t, tc.v, v)
 			if tc.roundtrip {
@@ -128,3 +134,25 @@ func TestReleaseSeries(t *testing.T) {
 		})
 	}
 }
+
+func TestOldRepresentaiton(t *testing.T) {
+	testCases := []struct {
+		v        Version
+		expected string
+	}{
+		{
+			v:        Version{Major: 23, Minor: 1},
+			expected: "23.1",
+		},
+		{
+			v:        Version{Major: 23, Minor: 1, Internal: 10},
+			expected: "23.1-10",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run("", func(t *testing.T) {
+			require.Equal(t, tc.expected, tc.v.OldRepresentation())
+		})
+	}
+}

pkg/sql/sqlinstance/instancestorage/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/base",
+        "//pkg/clusterversion",
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/kv/kvclient/rangefeed",

pkg/sql/sqlinstance/instancestorage/row_codec.go

@@ -14,6 +14,7 @@ import (
 	"bytes"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -244,7 +245,7 @@ func (d *rowCodec) encodeValue(
 			return tree.NewDString(sqlAddr)
 		},
 		binaryVersionColumnIdx: func() tree.Datum {
-			return tree.NewDString(binaryVersion.String())
+			return tree.NewDString(clusterversion.StringForPersistence(binaryVersion))
 		},
 	}
 	for i, f := range columnsToEncode {