cli/start: check if the special version initialization for secondary tenants can go away

[knz]

Describe the problem

In the server startup for secondary tenant, we see the following special-purpose code:

    // This value is injected in order to have something populated during startup.
    // In the initial 20.2 release of multi-tenant clusters, no version state was
    // ever populated in the version cluster setting. A value is populated during
    // the activation of 21.1. See the documentation attached to the TenantCluster
    // in upgrade/upgradecluster for more details on the tenant upgrade flow.
    // Note that a the value of 21.1 is populated when a tenant cluster is created
    // during 21.1 in crdb_internal.create_tenant.
    //
    // Note that the tenant will read the value in the system.settings table
    // before accepting SQL connections.
    //
    // TODO(knz): Check if this special initialization can be removed.
    // See:
    st ≔ serverCfg.BaseConfig.Settings
    return clusterversion.Initialize(
      ctx, st.Version.BinaryMinSupportedVersion(), &st.SV,
    )

Is this initialization still relevant? Can we remove it?

cc @dt @ajstorm for advice.

Jira issue: CRDB-20962

Epic CRDB-10829

[ajstorm]

Yeah, I noticed this too in my work this week and was planning on deleting it. Feel free to do so yourself if you get to it first, unless @dt disagrees.

[knz]

So it's safe to delete?

[ajstorm]

That's my opinion, unless I'm misunderstanding something. It seems like ever since 21.1 we were populating this value correctly here:

cockroach/pkg/sql/tenant.go

Lines 267 to 278 in 8cdaa31

	// Populate the version setting for the tenant. This will allow the tenant
	// to know what migrations need to be run in the future. The choice to use
	// the active cluster version here is intentional; it allows tenants
	// created during the mixed-version state in the host cluster to avoid
	// using code which may be too new. The expectation is that the tenant
	// clusters will be updated to a version only after the system tenant has
	// been upgraded.
	v := p.EvalContext().Settings.Version.ActiveVersion(ctx)
	tenantSettingKV, err := generateTenantClusterSettingKV(codec, v)
	if err != nil {
	return err
	}

).

I think this means that we should also be able to remove this code:

cockroach/pkg/sql/set_cluster_setting.go

Lines 484 to 490 in 8cdaa31

	// The tenant cluster in 20.2 did not ever initialize this value and
	// utilized this hard-coded value instead. In 21.1, the builtin
	// which creates tenants sets up the cluster version state. It also
	// is set when the version is upgraded.
	tenantDefaultVersion := clusterversion.ClusterVersion{
	Version: roachpb.Version{Major: 20, Minor: 2},
	}

[knz]

ok i'll remove it in #90176

[knz]

Sadly, that bit is not optional. Without it, SQL server initialization fails:

F221031 13:42:42.810253 346 clusterversion/setting.go:124  [nsql2] 40  version not initialized

triggered by sql.(*TemporaryObjectCleaner).Start.

This tells me that we need some value in the in-memory setting before the actual first value is fetched from KV.

[knz]

This was closed by mistake. Still current.

[knz]

cc @healthy-pod for info

[knz]

ok now I understand why this must be there. No need for this issue any more.