workload/schemachanger: use show cluster settings for version

[fqazi]

Previously, we would do SHOW CLUSTER SETTING version, which could block until an upgrade occurred. This statement could also timeout, if the upgrade between versions took a long time. To address this, this patch will use
SHOW CLUSTER SETTINGS with a where clause.

Fixes: #99115

Release note: None

[cockroach-teamcity]

This change is 

[chengxiong-ruan]

LGTM!

[renatolabs]

I started a 100-run of this test on this branch this morning, mind if we wait for that to finish before merging? (Unless you already ran something similar on your end).

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan and @srosenberg)

[fqazi]

@renatolabs Thanks, I'm gonna wait for that

[renatolabs]

Saw one failure:

"message": "***UNEXPECTED COMMIT ERROR; Received an unexpected commit error: ERROR: write-job-info-delete: relation \"system.job_info\" does not exist (SQLSTATE 42P01)",

🤔

[fqazi]

@renatolabs Let me chase this with the jobs team think we have a bug with concurrent DDL

[renatolabs]

99162_artifacts.zip

[fqazi]

The only failure being seen here intermittently is: #99241. I'll wait for Aditya to merge a fix first

[tbg]

Could you point me at that code? Is that new behavior? I'm also seeing a flake in #99459 that seems like the same thing, but we had that test for years so I'm wondering which PR changed semantics and whether that was intentional.

[fqazi]

@tbg This is not a new change, but linked to the behaviour we have inside: getCurrentEncodedVersionSettingValue. We have some long-running upgrade that exceeds 2 minutes. Or that's what my theory here was, it's possible we have a hang too during the upgrade, which would be horrible. But we were stuck on the fence version about 2 minutes which is bad :(

[aliher1911]

Not PR author, just looking at the code, but i see that show cluster setting will fail if local value != queried value from settings table (by searching the test error), while show settings seem to be delegating to querying crdb_internal.cluster_settings which doesn't check local values? That's my best guess.

[fqazi]

The intent was for SHOW CLUSTER VERSION to guarantee that the version on disk and in memory is synced up, indicating that the upgrade is complete. Fence versions only get updated in memory, so this is probably by design.

[aliher1911]

This bit basically

cockroach/pkg/sql/show_cluster_setting.go

Lines 59 to 103 in a961061

	return retry.WithMaxAttempts(ctx, retry.Options{}, math.MaxInt32, func() error {
	return p.execCfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
	datums, err := txn.QueryRowEx(
	ctx, "read-setting",
	txn.KV(),
	sessiondata.RootUserSessionDataOverride,
	"SELECT value FROM system.settings WHERE name = $1", name,
	)
	if err != nil {
	return err
	}
	var kvRawVal []byte
	if len(datums) != 0 {
	dStr, ok := datums[0].(*tree.DString)
	if !ok {
	return errors.New("the existing value is not a string")
	}
	kvRawVal = []byte(string(*dStr))
	} else if !p.execCfg.Codec.ForSystemTenant() {
	// The tenant clusters 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},
	}
	encoded, err := protoutil.Marshal(&tenantDefaultVersion)
	if err != nil {
	return errors.WithAssertionFailure(err)
	}
	kvRawVal = encoded
	} else {
	// There should always be a version saved; there's a migration
	// populating it.
	return errors.AssertionFailedf("no value found for version setting")
	}
	localRawVal := []byte(s.Get(&st.SV))
	if !bytes.Equal(localRawVal, kvRawVal) {
	// NB: errors.Wrapf(nil, ...) returns nil.
	// nolint:errwrap
	return errors.Errorf(
	"value differs between local setting (%v) and KV (%v); try again later (%v after %s)",
	localRawVal, kvRawVal, ctx.Err(), timeutil.Since(tBegin))
	}

which is not applicable for show all settings.

[renatolabs]

What I don't understand is that in both #99115 and #99459, I don't see a migration taking more than a few seconds (let alone 2mins) to finish around the time of the test failure. Am I missing something?

[aliher1911]

IMHO once version querying code is fixed, we'll see the real issue because version that test expects will diverge and give us more sensible failure. Same in other test.
edit: this one isn't a test, so maybe we shouldn't change it until we understand why upgrade is stuck.