Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @healthy-pod, @lidorcarmel, and @miretskiy)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go line 206 at r1 (raw file):

}

var isV221Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {

nit: I think we can remove isV221Active altogether - it's ok for checks to be nil.

Reviewed 27 of 28 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @lidorcarmel, and @miretskiy)

pkg/clusterversion/cockroach_versions.go line 219 at r2 (raw file):

	// TODODelete_V22_2SetSystemUsersUserIDColumnNotNull sets the user_id column in system.users to not null.
	TODODelete_V22_2SetSystemUsersUserIDColumnNotNull
	// Permanent_V22_2SQLSchemaTelemetryScheduledJobs adds an automatic schedule for SQL schema

Maybe keep the full comment for this one.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @lidorcarmel, and @miretskiy)

pkg/clusterversion/cockroach_versions.go line 180 at r2 (raw file):

	//
	// This is a permanent migration which should exist forever.
	Permanent_V22_2SQLSchemaTelemetryScheduledJobs

Maybe we want to move this to a "Primordial" version? (feels like this is what they are for, but I'm not exactly sure)

pkg/sql/schemachanger/screl/scalars.go line 106 at r2 (raw file):

// VersionSupportsElementUse checks if an element may be used at a given version.
func VersionSupportsElementUse(el scpb.Element, version clusterversion.ClusterVersion) bool {

Nice refactoring!

pkg/sql/schemachanger/screl/scalars.go line 124 at r2 (raw file):

		// These elements need v22.1 so they can be used without checking any version gates.
		return true
	case *scpb.CompositeType, *scpb.CompositeTypeAttrType, *scpb.CompositeTypeAttrName:

[nit] these cases can be consolidated and sorted by version

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @lidorcarmel, and @miretskiy)

pkg/clusterversion/cockroach_versions.go line 171 at r2 (raw file):

	VPrimordial5
	VPrimordial6
	// NOTE(andrei): Do not introduce new upgrades corresponding to VPrimordial

@knz do you understand this comment? My current guess is that it says to not introduce upgrades tied to an existing VPrimordialX version, and that we should instead add a new version?

It feels that all permanent upgrades would use one of these versions? Otherwise what is the point of "tying" a permanent upgrade to a dev version, if it is going to run no matter what?

Unfortunately Andrei and Andrew had the most context here I think :/

pkg/upgrade/upgrades/upgrades.go line 89 at r2 (raw file):

	),
	// Introduced in v2.1.
	// TODO(knz): bake this migration into v19.1.

I think we can remove this comment

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @lidorcarmel, @miretskiy, and @RaduBerinde)

pkg/clusterversion/cockroach_versions.go line 171 at r2 (raw file):

Without reversing this code, i don't feel comfortable reusing the VPrimordial versions for anything. We also didn't do it in the last release cycle.

There are some open issues around this, listed under 4 here: #95530
Can you mark this PR as fixing them?

Specifically:

• Fixes: #96762, #96761, #96765

Thank you!

Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @knz, @lidorcarmel, @miretskiy, and @RaduBerinde)

pkg/clusterversion/cockroach_versions.go line 180 at r2 (raw file):

I'm not 100% sure how Primordial versions are used either tbh (compared to normal version gates).

At the very least, we should leave version value (order in which the system migrates though each gate). So perhaps we leave Permanent versions as-is (in the same order), unless others have more context/guidance here.

SQL Foundations related files look good

Reviewed all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @healthy-pod, @knz, @lidorcarmel, @miretskiy, and @RaduBerinde)