upgrade/upgrades: allow CreatedAtNanos to be set when validating migr…

…ation

Schema change upgrade migrations to system tables are made idempotent by
checking that the descriptor reaches some expected state. In order to ensure
that it is in that expected state, some volatile fields need to be masked.
We forgot to mask CreatedAtNanos. We also lost the testing which came with
these helper functions we use.

The vast majority of this PR is reviving testing from cockroachdb#66889.

Fixes cockroachdb#85228.

Release justification: Import bug fix for backport

Release note (bug fix): Some upgrade migrations perform schema changes on
system tables. Those upgrades which added indexes could, in some cases, get
caught retrying because they failed to detect that the migration had already
occurred due to the existence of a populated field. When that happens, the
finalization of the new version can hang indefinitely and require manual
intervention. This bug has been fixed.

pkg/migration/migrations/BUILD.bazel

@@ -83,6 +83,8 @@ go_test(
         "public_schema_migration_external_test.go",
         "raft_applied_index_term_external_test.go",
         "remove_invalid_database_privileges_external_test.go",
+        "schema_changes_external_test.go",
+        "schema_changes_helpers_test.go",
     ],
     data = glob(["testdata/**"]),
     embed = [":migrations"],
@@ -91,12 +93,14 @@ go_test(
         "//pkg/base",
         "//pkg/clusterversion",
         "//pkg/jobs",
+        "//pkg/jobs/jobspb",
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/kv/kvclient/rangefeed",
         "//pkg/kv/kvclient/rangefeed/rangefeedcache",
         "//pkg/kv/kvserver",
         "//pkg/kv/kvserver/stateloader",
+        "//pkg/migration",
         "//pkg/roachpb",
         "//pkg/security",
         "//pkg/security/securitytest",

pkg/migration/migrations/helpers_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
@@ -45,14 +46,20 @@ type Schema struct {
 // Migrate runs cluster migration by changing the 'version' cluster setting.
 func Migrate(
 	t *testing.T, sqlDB *gosql.DB, key clusterversion.Key, done chan struct{}, expectError bool,
+) {
+	UpgradeToVersion(t, sqlDB, clusterversion.ByKey(key), done, expectError)
+}
+
+func UpgradeToVersion(
+	t *testing.T, sqlDB *gosql.DB, v roachpb.Version, done chan struct{}, expectError bool,
 ) {
 	defer func() {
 		if done != nil {
 			done <- struct{}{}
 		}
 	}()
 	_, err := sqlDB.Exec(`SET CLUSTER SETTING version = $1`,
-		clusterversion.ByKey(key).String())
+		v.String())
 	if expectError {
 		assert.Error(t, err)
 		return
@@ -151,3 +158,6 @@ func GetTable(
 	require.NoError(t, err)
 	return table
 }
+
+// WaitForJobStatement is exported so that it can be detected by a testing knob.
+const WaitForJobStatement = waitForJobStatement

pkg/migration/migrations/schema_changes.go

@@ -43,6 +43,10 @@ type operation struct {
 	schemaExistsFn func(catalog.TableDescriptor, catalog.TableDescriptor, string) (bool, error)
 }
 
+// waitForJobStatement is the statement used to wait for an ongoing job to
+// complete.
+const waitForJobStatement = "SHOW JOBS WHEN COMPLETE VALUES ($1)"
+
 // migrateTable is run during a migration to a new version and changes an existing
 // table's schema based on schemaChangeQuery. The schema-change is ignored if the
 // table already has the required changes.
@@ -92,7 +96,7 @@ func migrateTable(
 			for _, mutation := range mutations {
 				log.Infof(ctx, "waiting for the mutation job %v to complete", mutation.JobID)
 				if _, err := d.InternalExecutor.Exec(ctx, "migration-mutations-wait",
-					nil, "SHOW JOB WHEN COMPLETE $1", mutation.JobID); err != nil {
+					nil, waitForJobStatement, mutation.JobID); err != nil {
 					return err
 				}
 			}
@@ -242,6 +246,8 @@ func hasIndex(storedTable, expectedTable catalog.TableDescriptor, indexName stri
 	expectedCopy.StoreColumnNames = []string{}
 	storedCopy.StoreColumnIDs = []descpb.ColumnID{0, 0, 0}
 	expectedCopy.StoreColumnIDs = []descpb.ColumnID{0, 0, 0}
+	storedCopy.CreatedAtNanos = 0
+	expectedCopy.CreatedAtNanos = 0
 
 	if err = ensureProtoMessagesAreEqual(&expectedCopy, &storedCopy); err != nil {
 		return false, err