Merge #123520

123520: sqlinstance: write backwards compatible binary version r=RaduBerinde a=renatolabs

We keep track of sql instances in multi-tenant deployments using the
`system.sql_instances` table. One of the columns in this table is
`binary_version`: this is the encoding of the version that the sql
instance is running. SQL instances know how to reach each other by
reading from that table (more accurately, setting up a rangefeed on
that table and updating a local cache).

In #115223, we introduced a new, more understandable string
representation of cockroach internal versions. This new format is,
however, backwards incompatible: older releases of cockroach are not
able to parse it. As a result, a mixed-version multi-tenant
deployments may face errors if some of the instances are running an
internal version: the older releases won't be able to parse the new
version format. As a result, the cache will be stale and we might see
query errors and distsql timeouts.

In this commit, we introduce a backwards compatible implementation of
the the string representation of a version. Specifically, we continue
to use the old format if the minimum supported version is less than
24.1 (the version in which the new formatting was added). This commit
should eventually be reverted when we no longer support versions older
than 24.1.

Epic: none

Release note: None

Co-authored-by: Renato Costa <renato@cockroachlabs.com>

pkg/clusterversion/cockroach_versions.go

@@ -10,7 +10,11 @@
 
 package clusterversion
 
-import "github.com/cockroachdb/cockroach/pkg/roachpb"
+import (
+	"fmt"
+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+)
 
 // Key is a unique identifier for a version of CockroachDB.
 type Key int
@@ -426,3 +430,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) || v.IsFinal() {
+		return v.String()
+	}
+
+	return fmt.Sprintf("%d.%d-%d", v.Major, v.Minor, v.Internal)
+}

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/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 {