This is nice. The only major hesitation I have is around the SystemDatabaseCache. The rest of my comments are minor.

Reviewed 1 of 1 files at r1, 34 of 34 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @rhu713)

pkg/sql/catalog/descs/descriptor.go line 225 at r2 (raw file):

	//
	// TODO(ajwerner): More generally leverage this set of kv descriptors on
	// the resolution path.

I think I'd say Jason achieved this TODO.

Code quote:

	// TODO(ajwerner): More generally leverage this set of kv descriptors on
	// the resolution path.

pkg/sql/catalog/descs/descriptor.go line 338 at r2 (raw file):

// and (2) if mutable descriptors are requested, these are present in the
// uncommitted descriptors layer.
// Known validation levels can be provided via validationLevels, otherwise when

nit: my brain had a hard time parsing otherwise when nil safe defaults are used instead. Can you sprinkle in a comma or two?

Code quote:

// Known validation levels can be provided via validationLevels, otherwise when
// nil safe defaults are used instead. In any case, after validation is

pkg/sql/catalog/descs/descriptor.go line 348 at r2 (raw file):

	validationLevels []catalog.ValidationLevel,
) error {
	if validationLevels == nil {

add a defensive check that if validationLevels is not nil that it has the same length as descs?

pkg/sql/catalog/descs/uncommitted_descriptors.go line 181 at r2 (raw file):

		return errors.Wrap(err, "Memory usage exceeds limit for uncommittedDescriptors")
	}
	ud.mutable.Upsert(mut, true /* skipNameMap */)

why are we unconditionally skipping the name map? It feels like if we're going to always skip the nameMap we should use a data structure that does not have one.

pkg/sql/catalog/internal/catkv/stored_catalog.go line 38 at r2 (raw file):

	// cache mirrors the descriptors in storage.
	// This map does not store descriptors by name.

I said it elsewhere: let's export an object from nstree which wraps the existing byIDMap and avoid the confusion this thing is causing

pkg/sql/catalog/internal/catkv/system_database_cache.go line 34 at r2 (raw file):

// the lease manager to cache all system table IDs.
type SystemDatabaseCache struct {
	syncutil.RWMutex

I don't think it's right to export the mutex methods. The style here is generally to embed the mutex inside of an anonymous, embedded struct. I think that would work nicely here.

type SystemDatabaseCache struct {
	mu struct {
		syncutil.RWMutex
		m map[roachpb.Version]*nstree.MutableCatalog
        }
}

pkg/sql/catalog/internal/catkv/system_database_cache.go line 35 at r2 (raw file):

type SystemDatabaseCache struct {
	syncutil.RWMutex
	m map[roachpb.Version]*nstree.MutableCatalog

The semantics of the versions in use here are interesting. I think that right now, it doesn't really matter because we never rename something in the system database and we only use this for caching names, right?

pkg/sql/catalog/internal/catkv/system_database_cache.go line 107 at r2 (raw file):

// Anything not pertaining to the system database is ignored.
// Presently, system descriptors are all ignored and only namespace entries are
// used.

what's your vision here? My plan for 23.1 is to start treating system descriptors, to the extent possible, like regular descriptors and not to make them more special. We'll need this to allow for deeper configuration of multi-region primitives

Code quote:

// Presently, system descriptors are all ignored and only namespace entries are
// used.

Thanks for the review.

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

pkg/sql/catalog/descs/descriptor.go line 225 at r2 (raw file):

Removing.

pkg/sql/catalog/descs/descriptor.go line 338 at r2 (raw file):

Rewriting.

pkg/sql/catalog/descs/descriptor.go line 348 at r2 (raw file):

Will do.

pkg/sql/catalog/descs/uncommitted_descriptors.go line 181 at r2 (raw file):

Will do.

pkg/sql/catalog/internal/catkv/stored_catalog.go line 38 at r2 (raw file):

Will do. Agreed.

pkg/sql/catalog/internal/catkv/system_database_cache.go line 34 at r2 (raw file):

Whoops, right. Will do.

pkg/sql/catalog/internal/catkv/system_database_cache.go line 35 at r2 (raw file):

Indeed it's just me being defensive. Adding commentary.

pkg/sql/catalog/internal/catkv/system_database_cache.go line 107 at r2 (raw file):

My vision is the same as yours. Adding commentary to explain why we don't actually cache system descriptors other than the system DB.

my comments are nits, but in general, I'd prefer we not export methods unless we need them to conform to some interface explicitly. I spent a little while looking around for such an interface.

The internal executor change I sort of get, but I don't fully get. I feel like we can tighten up the code and commentary around that change.

Reviewed 3 of 34 files at r3, 19 of 34 files at r4, 8 of 17 files at r5, 1 of 2 files at r6, 9 of 11 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rhu713)

-- commits line 69 at r6:

which it never has when using the planner's state.

Is that true? Consider:

Code quote:

  prevent that by attaching the logic to reset the synthetic descriptors to
  whether the internal executor has attached synthetic descriptors, which
  it never has when using the planner's state.

-- commits line 5 at r7:
I anticipate this all worked because the backups don't store dropped descriptors. Does this commit fix any known bug? Should it be in this PR or should it be pulled out into a separate one such that it might be tested?

Code quote:

  Recent changes to the descriptors collection have uncovered some
  brittleness in the logic for writing namespace entries for dropped
  descriptors when creating backups.

pkg/sql/internal.go line 337 at r6 (raw file):

			ex.extraTxnState.descCollection = ie.extraTxnState.descCollection
			ex.extraTxnState.fromOuterTxn = true
			ex.extraTxnState.shouldResetSyntheticDescriptors = len(ie.syntheticDescriptors) > 0

can you add more commentary here?

pkg/sql/internal.go line 334 at r7 (raw file):

	// and job collections from the caller.
	postSetupFn := func(ex *connExecutor) {
		if ie.extraTxnState != nil && ie.extraTxnState.descCollection != nil {

what's the case where extraTxnState is not nil but descCollection is? Also, while you're here, if you have to do another revision, rename to descsCollection?

pkg/sql/catalog/descs/synthetic_descriptors.go line 23 at r7 (raw file):

}

func (sd *syntheticDescriptors) Add(desc catalog.Descriptor) {

why are all these methods now exported?

pkg/sql/catalog/descs/uncommitted_descriptors.go line 63 at r7 (raw file):

// Reset zeroes the object for re-use in a new transaction.
func (ud *uncommittedDescriptors) Reset(ctx context.Context) {

Why are all of the methods on this unexported struct exported? It doesn't seem like it's embedded anywhere