Merge #153982 #154055 #154174

153982: backup: remove metamorphic backup manifest read r=jeffswenson a=msbutler

Epic: none

Release note: none

154055: tree: generate a random tuple type in TestStringConcat r=jeffswenson a=jeffswenson

Previously, TestStringConcat was using the AnyTuple type to generate a randum datum. `AnyTuple` is not a real type that can be assigned to a value. Rather its a sentinel type used to represent any type that is a tuple.

This worked before  PR #153789, because randgen would pick random datums and then generate a new tuple type that matches the datums. But that behavior was broken for `NULL` values and did not match the behavior of the value decoder.

Release note: none
Fixes: #153789

154174: update: bump descriptor versions during repair r=fqazi a=fqazi

Previously, the post deserialization repair incorrectly wrote the descriptors at the exact same version. While the leasing manager will tolerate this for non-historical descriptors, any historical queries can end up failing after the first upgrade. This patch adds a version bump when running post deserialization changes to avoid these issues.

Fixes: #153358

Release note: None

Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Jeff Swenson <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>

Author: 4 people

pkg/backup/backup_job.go

@@ -61,7 +61,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logutil"
-	"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -82,8 +81,6 @@ var BackupCheckpointInterval = settings.RegisterDurationSetting(
 	"the minimum time between writing progress checkpoints during a backup",
 	time.Minute)
 
-var forceReadBackupManifest = metamorphic.ConstantWithTestBool("backup-read-manifest", false)
-
 var useBulkOracle = settings.RegisterBoolSetting(
 	settings.ApplicationLevel,
 	"bulkio.backup.balanced_distribution.enabled",
@@ -708,7 +705,7 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 	defer mem.Close(ctx)
 	var memSize int64
 
-	if backupManifest == nil || forceReadBackupManifest {
+	if backupManifest == nil {
 		backupManifest, memSize, err = b.readManifestOnResume(ctx, &mem, p.ExecCfg(), defaultStore,
 			details, p.User(), &kmsEnv)
 		if err != nil {

pkg/sql/sem/tree/expr_test.go

@@ -75,10 +75,13 @@ func TestStringConcat(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 	rng, _ := randutil.NewTestRand()
+
+	tuple := randgen.RandTupleFromSlice(rng, types.Scalar)
+
 	ctx := context.Background()
 	evalCtx := eval.MakeTestingEvalContext(cluster.MakeTestingClusterSettings())
 	defer evalCtx.Stop(ctx)
-	for _, typ := range append([]*types.T{types.AnyTuple}, types.Scalar...) {
+	for _, typ := range append([]*types.T{tuple}, types.Scalar...) {
 		// Strings and Bytes are handled specially.
 		if typ.Identical(types.String) || typ.Identical(types.Bytes) {
 			continue

pkg/upgrade/upgrades/first_upgrade.go

@@ -127,7 +127,11 @@ func upgradeDescriptors(
 				}
 				b := txn.KV().NewBatch()
 				for _, mut := range muts {
-					if !mut.GetPostDeserializationChanges().HasChanges() {
+					// Both newly created and altered descriptors never write the modification time
+					// to storage. This post-deserialization change is always expected now.
+					changes := mut.GetPostDeserializationChanges()
+					hasChanges := changes.Len() > 1 || !changes.Contains(catalog.SetModTimeToMVCCTimestamp)
+					if !hasChanges {
 						// In the upgrade to 25.4, we do a one-time rewrite of all
 						// descriptors in order to upgrade them to use the new type
 						// serialization format.
@@ -137,6 +141,7 @@ func upgradeDescriptors(
 						}
 					}
 					key := catalogkeys.MakeDescMetadataKey(d.Codec, mut.GetID())
+					mut.MaybeIncrementVersion()
 					b.CPut(key, mut.DescriptorProto(), mut.GetRawBytesInStorage())
 				}
 				return txn.KV().Run(ctx, b)

pkg/upgrade/upgrades/first_upgrade_test.go

@@ -79,14 +79,16 @@ func TestFirstUpgrade(t *testing.T) {
 		"USE test",
 		"CREATE TABLE foo (i INT PRIMARY KEY, j INT, INDEX idx(j))",
 	)
-
+	// Select a timestamp for when the tables were created.
+	startTime := testServer.Clock().Now()
 	// Corrupt the table descriptor in an unrecoverable manner. We are not able to automatically repair this
 	// descriptor.
 	tbl := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "test", "foo")
 	descKey := catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, tbl.GetID())
 	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		mut := tabledesc.NewBuilder(tbl.TableDesc()).BuildExistingMutableTable()
 		mut.NextIndexID = 1
+		mut.MaybeIncrementVersion()
 		return txn.Put(ctx, descKey, mut.DescriptorProto())
 	}))
 
@@ -106,6 +108,8 @@ func TestFirstUpgrade(t *testing.T) {
 	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 		mut := tabledesc.NewBuilder(tbl.TableDesc()).BuildExistingMutableTable()
 		mut.ModificationTime = hlc.Timestamp{}
+		mut.Indexes[0].NotVisible = true
+		mut.Version += 2 // Increment by 2 because we wrote a version above.
 		return txn.Put(ctx, descKey, mut.DescriptorProto())
 	}))
 
@@ -127,8 +131,9 @@ func TestFirstUpgrade(t *testing.T) {
 	// the MVCC timestamp.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
 	changes := readDescFromStorage().GetPostDeserializationChanges()
-	require.Equal(t, changes.Len(), 1)
+	require.Equal(t, 2, changes.Len())
 	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
+	require.True(t, changes.Contains(catalog.SetIndexInvisibility))
 
 	// Wait long enough for precondition check to see the unbroken table descriptor.
 	execStmts(t, "CREATE DATABASE test3")
@@ -137,18 +142,18 @@ func TestFirstUpgrade(t *testing.T) {
 	// Upgrade the cluster version.
 	tdb.Exec(t, qUpgrade)
 
-	// The table descriptor protobuf should still have the modification time set;
-	// the only post-deserialization change should be SetModTimeToMVCCTimestamp.
+	// Run a schema change on the bar table.
+	tdb.Exec(t, "ALTER TABLE foo ADD COLUMN z INT")
+
+	// The only post-deserialization change should be SetModTimeToMVCCTimestamp,
+	// because the MVCC timestamp gets on each write.
 	require.False(t, readDescFromStorage().GetModificationTime().IsEmpty())
 	changes = readDescFromStorage().GetPostDeserializationChanges()
-	if v1.Equal(clusterversion.V25_4.Version()) {
-		// In 25.4, we do a one-time rewrite of all descriptors, so there should be
-		// no changes here. In later versions, there should be one change.
-		require.Equal(t, 0, changes.Len())
-	} else {
-		require.Equal(t, 1, changes.Len())
-		require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
-	}
+	require.Equal(t, 1, changes.Len())
+	require.True(t, changes.Contains(catalog.SetModTimeToMVCCTimestamp))
+	// Validate we can read bar before this upgrade. The lease manager should see
+	// distinct version numbers in the history. Otherwise, it can panic.
+	tdb.Exec(t, fmt.Sprintf("SELECT * FROM foo AS OF SYSTEM TIME '%s'", startTime.AsOfSystemTime()))
 }
 
 // TestFirstUpgradeRepair tests the correct repair behavior of upgrade
@@ -241,12 +246,14 @@ func TestFirstUpgradeRepair(t *testing.T) {
 			ConstraintID:        tbl.NextConstraintID,
 		}}
 		tbl.NextConstraintID++
+		tbl.MaybeIncrementVersion()
 		b.Put(catalogkeys.MakeDescMetadataKey(codec, tbl.GetID()), tbl.DescriptorProto())
 		fn := funcdesc.NewBuilder(fooFn.FuncDesc()).BuildExistingMutableFunction()
 		fn.DependedOnBy = []descpb.FunctionDescriptor_Reference{{
 			ID:        123456789,
 			ColumnIDs: []descpb.ColumnID{1},
 		}}
+		fn.MaybeIncrementVersion()
 		b.Put(catalogkeys.MakeDescMetadataKey(codec, fn.GetID()), fn.DescriptorProto())
 		return txn.Run(ctx, b)
 	}))
@@ -340,18 +347,34 @@ func TestFirstUpgradeRepair(t *testing.T) {
 	tdb.CheckQueryResults(t, qDetectRepairableCorruption, [][]string{{"0"}})
 	close(upgradeCompleted)
 	require.NoError(t, grp.Wait())
+	expectedVersionBump := 1
+	// Upgrades from before 25.4 will round-trip all descriptors once, so we
+	// expect two version bumps. Once for the repair and the next for the
+	// round trip.
+	if v0.Less(clusterversion.V25_4.Version()) {
+		expectedVersionBump += 1
+	}
 	// Assert that a version upgrade is reflected for repaired descriptors (stricly one version upgrade).
 	for _, d := range corruptDescs {
 		descId := d.GetID()
 		desc := readDescFromStorage(descId)
-		require.Equalf(t, descOldVersionMap[descId]+1, desc.GetVersion(), desc.GetName())
+		require.Equalf(t, descOldVersionMap[descId]+descpb.DescriptorVersion(expectedVersionBump), desc.GetVersion(), desc.GetName())
 	}
 
 	// Assert that no version upgrade is reflected for non-repaired descriptors.
+	expectedVersionBump = 0
+	// Upgrading from before 25.4 all non-database descriptors will be round tripped.
+	if v0.Less(clusterversion.V25_4.Version()) {
+		expectedVersionBump += 1
+	}
 	for _, d := range nonCorruptDescs {
 		descId := d.GetID()
 		desc := readDescFromStorage(descId)
-		require.Equalf(t, descOldVersionMap[descId], desc.GetVersion(), desc.GetName())
+		if desc.DescriptorType() != catalog.Database {
+			require.Equalf(t, descOldVersionMap[descId]+descpb.DescriptorVersion(expectedVersionBump), desc.GetVersion(), desc.GetName())
+		} else {
+			require.Equalf(t, descOldVersionMap[descId], desc.GetVersion(), desc.GetName())
+		}
 	}
 
 	// Check that the repaired table and function are OK.