This is close. Some of the logic is quite subtle and deserves a bit more commentary and another pass to try to make the control flow clearer.

Reviewed 13 of 22 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)

pkg/sql/sqlinstance/instancestorage/instancecache.go line 223 at r2 (raw file):

// from the old implementation to the new implementation when the version
// changes to V23_1_SystemRbrReadNew.
func newMigrationCache(

this function is a beast.

pkg/sql/sqlinstance/instancestorage/instancecache.go line 253 at r2 (raw file):

		// Use the background context because we don't want RPC cancellation to
		// prevent starting the new range feed.
		err := stopper.RunAsyncTask(context.Background(), "start-new-cache-implementation", func(ctx context.Context) {

nit: annotate the context with logtags. Probably cacheCtx := logtags.AddTags(context.Background(), logtags.FromContext(ctx))

pkg/sql/sqlinstance/instancestorage/instancecache.go line 256 at r2 (raw file):

			log.Ops.Info(ctx, "starting new system.sql_instance cache")

			cache, err := newCache(ctx)

I'd feel better if there were a defer which sent on this channel the return value of a function. In general, I feel like it'd be nicer if you made closures for two functions which send on the channels and lay out the invariants about who is going to send on which channel. After having scrutinized this a bunch, I don't see obvious bugs if there are no panics, but this thing is surely complex.

pkg/sql/sqlinstance/instancestorage/instancecache.go line 306 at r2 (raw file):

	select {
	case err = <-newReady:

I think there's a subtle race here. Imagine that the version change notification happens and then the stopper gets shut down. There's a case where the versionChanged field could get set to true, and the RunAsyncTask code can choose not to run. We'll log a warning but nobody will send on either channel. I think you can fix that by sending on the channel when unable to start the new cache.

All of this code is pretty subtle. My sense is that splitting out some of the logic into helpers can make the behavior a bit clearer.

pkg/sql/tests/system_table_test.go line 223 at r2 (raw file):

		require.NoError(t, desctestutils.TestingValidateSelf(gen))

		switch gen.GetID() {

Explain this in commentary

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

pkg/sql/sqlinstance/instancestorage/instancecache.go line 223 at r2 (raw file):

I reworked the function and tried to clean up some of the state. Let me know what you think.

pkg/sql/sqlinstance/instancestorage/instancecache.go line 256 at r2 (raw file):

I've reworked the implementation. Let me know if it is still difficult to follow and I will take another crack at it.

pkg/sql/sqlinstance/instancestorage/instancecache.go line 306 at r2 (raw file):

Done.

pkg/sql/tests/system_table_test.go line 223 at r2 (raw file):

Done.

LGTM