I have a few comments but I'd like Andrew to take a look at this as well at some point, just to make sure. I'm paranoid about migrations.

Somewhat related and I might be wrong, but the backup case could perhaps be handled by adding a new upgrade step to theRunPostDeserializationChanges method of catalog.DescriptorBuilder, in which case it'd be appropriate to include in this PR.

nit: for the release note, i think we can make it something a bit more user-focused. It can just focus on what the end-user would notice, and the other details can stay just in the commit message.

Release note (bug fix): An internal upgrade was added that runs when a cluster is upgraded to v21.2, and which fixes the representation of database object privileges. This fixes a potential source of corruption that could leave the database in an unusable state.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)

pkg/migration/migrations/fix_privileges_migration.go, line 84 at r1 (raw file):

		if privilegeDesc.Version >= descpb.Version21_2 {
			log.Infof(ctx, "privilege descriptor has already been fixed, skipping %d", upgrade)

nit: will this get logged every time a node starts? maybe we'd want to be less verbose.

pkg/migration/migrations/fix_privileges_migration_external_test.go, line 36 at r1 (raw file):

)

func TestFixPrivilegesMigration(t *testing.T) {

cool tests! nit on adjusting the whtiespace for the sql queries in the comments.

Reviewed 4 of 14 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/migration/migrations/fix_privileges_migration.go, line 37 at r1 (raw file):

	var lastUpgradedID descpb.ID
	for {
		done, idToUpgrade, objectType, err := findNextDescriptorToFix(ctx, d.InternalExecutor, lastUpgradedID)

I find the control flow here to be surprising. It's mostly weird because we're scanning the descriptors table, decoding the thing, then throwing that away, getting it by ID etc.

Instead, what if we just literally scanned the descriptor table and then did the modification in place. I think it would simplify the code a bunch. You can still use descs.Txn to do the writing. catalogkv.NewBuilderWithMVCCTimestamp(&desc, ts) is your friend. Lastly, I think you'll benefit greatly from marking a bit if you did a repair -- I think you can avoid all of the specialized logic that occurs on a per-type basis, keeping all of the complexity in the builder.

Updated, did too much cargo culting previously. Reworked so all descriptors are upgraded in one transaction and refactored a bunch of logic including some of the old MaybeFixPrivilege logic, we can hopefully remove it after 21.2

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

pkg/migration/migrations/fix_privileges_migration.go, line 37 at r1 (raw file):

Agreed, too much cargo culting. I've changed a lot here.

Also, I think the Version field is akin to the bit for a repair (it determines whether we need to repair or not as descriptors created with version 21.2 are guaranteed to not have these issues)

pkg/migration/migrations/fix_privileges_migration.go, line 50 at r1 (raw file):

Done (I believe). Changed it so MaybeFixPrivileges is used in this migration.

pkg/migration/migrations/fix_privileges_migration.go, line 81 at r1 (raw file):

N/A

pkg/migration/migrations/fix_privileges_migration.go, line 84 at r1 (raw file):

Should only be called once for each ID during the migration, not during node start.

pkg/migration/migrations/fix_privileges_migration.go, line 102 at r1 (raw file):

Done.

pkg/migration/migrations/fix_privileges_migration.go, line 131 at r1 (raw file):

Done, changed it to "one big SQL query" or rather have all the descriptors updated in one txn

pkg/migration/migrations/fix_privileges_migration_external_test.go, line 36 at r1 (raw file):

Done.

pkg/migration/migrations/fix_privileges_migration_external_test.go, line 64 at r1 (raw file):

Should be fixed.

pkg/migration/migrations/fix_privileges_migration_external_test.go, line 173 at r1 (raw file):

Done, and slightly updated Validate.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_privileges_migration.go, line 43 at r2 (raw file):

	for ; ok; ok, err = rows.Next(ctx) {
		row := rows.Cur()
		id := descpb.ID(*row[0].(*tree.DInt))

nit: refactor the row parsing into a helper.

I think with the various helpers, this should read:

ok, err := rows.Next(ctx)
for ; ok, ok, err = rows.Next(ctx) {
    desc, err := decodeRow(ctx, row)
    if err != nil {
        return err // wrap the error in decodeRow with which row
    }
    if desc.GetPrivileges.Version > descpb.Version21_2 {
        continue
    }
    if err := upgradePrivileges(ctx, desc.GetID()); err != nil {
        return err
    }
}

pkg/migration/migrations/fix_privileges_migration.go, line 72 at r2 (raw file):

nit: factor this out into a helper.

pkg/migration/migrations/fix_privileges_migration.go, line 76 at r2 (raw file):

		err = descs.Txn(ctx, d.Settings, d.LeaseManager, d.InternalExecutor, d.DB, func(
			ctx context.Context, txn *kv.Txn, descriptors *descs.Collection) error {
			if privilegeDesc.Version >= descpb.Version21_2 {

why do this down here? I think it'd be better if you did this check above with a continue.

pkg/migration/migrations/fix_privileges_migration.go, line 85 at r2 (raw file):

nit: factor this out into a helper

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

pkg/migration/migrations/fix_privileges_migration.go, line 76 at r2 (raw file):

Done

General comment for @ajwerner @postamar
Now that this was added in #66599

This migration fails due to a validation error since we're injecting old descriptors with an older Primary Key version
pq: internal error: running migration for 21.1-14: relation "tb" (60): primary index "primary" has invalid version 0, expected 4

The Validation fails on fix_privileges_migration.go line 117 return descriptors.WriteDesc(ctx, false /* kvTrace */, desc, txn)
Since RunPostDeserialzation doesn't happen during the migration.

Does it make sense to add this fix into the migration as well?

changes.UpgradedIndexFormatVersion = maybeUpgradePrimaryIndexFormatVersion(desc)
	for i := range desc.Indexes {
		changes.UpgradedIndexFormatVersion = changes.UpgradedIndexFormatVersion || maybeUpgradeSecondaryIndexFormatVersion(&desc.Indexes[i])
	}
	for i := range desc.Mutations {
		if idx := desc.Mutations[i].GetIndex(); idx != nil {
			changes.UpgradedIndexFormatVersion = changes.UpgradedIndexFormatVersion || maybeUpgradeSecondaryIndexFormatVersion(idx)
		}
	}

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_privileges_migration.go, line 90 at r3 (raw file):

Done.

pkg/sql/doctor/doctor_test.go, line 294 at r3 (raw file):

Had to update this test case, since MaybeFixPrivileges is called in RunPostDeserializationChanges for types now, this no longer produces a nil dereference. I can't think of a way to do this off the top of my head, any suggestions?

pkg/sql/catalog/descpb/structured.go, line 393 at r3 (raw file):

Removed, thought I needed this before but stopped using it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, and @rafiss)

pkg/migration/migrations/fix_descriptor_migration.go, line 97 at r7 (raw file):

Fair enough, don

pkg/migration/migrations/fix_descriptor_migration.go, line 82 at r8 (raw file):

Done.

Reviewed 1 of 14 files at r1, 1 of 13 files at r4, 3 of 10 files at r6, 2 of 17 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/sql/catalog/catalog.go, line 49 at r10 (raw file):

	// GetChanged returns if the MutableDescriptor was changed after running
	// RunPostDeserializationChanges.
	GetChanged() bool

Can we call this something more descriptive. HasPostDeserializationChanges? Also, I was envisioning this method living on the builder rather than the descriptor, but this is fine.

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

What does that version mean? Are we getting value out of setting it?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

The version means we can check the validate the privileges strictly.

Validate is prior to and during the migration, in which we have less strict checks.

	// Validate can be called during the fix_privileges_migration introduced in
	// 21.2. It is possible for have invalid privileges prior to 21.2 in certain
	// cases due to bugs. We can strictly check privileges in 21.2 and onwards.
	if p.Version < Version21_2 {
		if objectType == privilege.Schema {
			// Prior to 21_2, it was possible for a schema to have some database
			// privileges on it. This was temporarily fixed by an upgrade on read
			// but in 21.2 onwards, it should be permanently fixed with a migration.
			allowedPrivilegesBits |= privilege.GetValidPrivilegesForObject(privilege.Database).ToBitField()
		}
		if objectType == privilege.Table || objectType == privilege.Database {
			// Prior to 21_2, it was possible for a table or database to have USAGE
			// privilege on it due to a bug when upgrading from 20.1 to 20.2.
			// In 21.2 onwards, it should be permanently fixed with a migration.
			allowedPrivilegesBits |= privilege.USAGE.Mask()
		}
	}

I do think that the code you're writing here is something we should find a way to package up for reuse. See how you feel about my batching suggestion.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

The fact that we're going to touch most descriptors makes me wonder if we should be accumulating a batch at a time (we probably should). What if we make a slice of lease.IDVersion and then pick some arbitrary batch size and go do the upgrade when the batch is full.

I'll try to pull out the descriptor getting / upgrade into a package.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

Seems reasonable to me, do you have an idea of what is a good arbitrary batch size? 100?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

That seems like the right order of magnitude. What if we did it based on byte size rather than number of descriptors? desc.DescriptorProto().Size() should be a good proxy. I think 512 KiB would be a good batch size.

I'll try to pull out the descriptor getting / upgrade into a package.

I think it's fine for it to be in the migrations package. Just think about some minor abstracting. We can further abstract it later.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

Added an abstraction for migrations that will go over all descriptors and fix them.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, and @rafiss)

pkg/sql/catalog/catalog.go, line 49 at r10 (raw file):

Done

pkg/migration/migrations/fix_descriptor_migration.go, line 46 at r8 (raw file):

Done I used 512 KiB as a batch size.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 65 at r13 (raw file):

	}

	return descriptorUpgradeMigration(ctx, rows, fixDescriptorFunc, 524288 /* batchSizeInBytes */)

1<<19 /* 512 KiB batch size */?

pkg/migration/migrations/fix_descriptor_migration.go, line 73 at r13 (raw file):

	ctx context.Context, d migration.TenantDeps, id descpb.ID, version descpb.DescriptorVersion,
) error {
	return descs.Txn(ctx, d.Settings, d.LeaseManager, d.InternalExecutor, d.DB, func(

The point of the batching is to batch the writes into a single write batch. Use descriptors.WriteDescToBatch

pkg/migration/migrations/fix_descriptor_migration.go, line 85 at r13 (raw file):

			return nil
		}
		log.Infof(ctx, "upgrading descriptor with id %d", desc.GetID())

nit: this is loud. I don't really want us logging 1M times if we fix 1M descriptors. Maybe lot out the batch?

pkg/migration/migrations/fix_descriptor_migration.go, line 124 at r13 (raw file):

		descs = append(descs, desc)
		timestamps = append(timestamps, ts)

I feel like I wasn't clear enough on the reason for the batching. We'd like to evaluate the predicate inside the loop here and then accumulate a batch to write to the store in a single shot.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)

pkg/migration/migrations/fix_descriptor_migration.go, line 73 at r13 (raw file):

Oops for some reason I thought I was doing that 🤦 done now

pkg/migration/migrations/fix_descriptor_migration.go, line 85 at r13 (raw file):

Logging per batch

pkg/migration/migrations/fix_descriptor_migration.go, line 124 at r13 (raw file):

Sorry, it was clear for some reason I just forgot how WriteDescToBatch worked.

Needs a make bazel-generate but

Reviewed 1 of 3 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @nihalpednekar, @postamar, @rafiss, and @RichardJCai)