sql: allow the in-memory version to be the next fence version

[ajwerner]

In SHOW CLUSTER SETTING version we wait for the stored version to match the in-memory version. The in-memory version get pushed to the fence but the fence is not persisted, so, while migrations are ongoing, we won't see these values match. In practice this is a problem these days because the migrations take a long time.

Epic: none

Informs #99894

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[renatolabs]

In practice this is a problem these days because the migrations take a long time.

Do you mean individual migrations or all migrations that happen when we upgrade from two released binaries? I believe the former is what is relevant here, and as I mentioned in #99894, I don't see migrations taking anywhere near 2mins around the timestamp of the failure 🤔.

Another thing is that the "show cluster setting" retry loop has basically no backoff, right? So wouldn't it be unlikely that we're hitting a fence every time for 2 minutes?

[ajwerner]

Do you mean individual migrations or all migrations that happen when we upgrade from two released binaries?

All migrations. The deal is that at the start of a migration, we move the in-memory version to the next fence version (but we don't write the fence). Then when the migration completes, we move the in-memory version and then write to KV.

[ajwerner]

The point being that while migrations are running, it is likely that the in-memory version is at the fence.

[ajwerner]

Another thing is that the "show cluster setting" retry loop has basically no backoff, right? So wouldn't it be unlikely that we're hitting a fence every time for 2 minutes?

retry.WithMaxAttempts very much does exponential backoff

cockroach/pkg/sql/show_cluster_setting.go

Line 59 in a961061

return retry.WithMaxAttempts(ctx, retry.Options{}, math.MaxInt32, func() error {

[ajwerner]

What's funky is I don't see the upgrade migrations taking very long on master now.

[irfansharif]

Perhaps add a comment here explaining what you are in the commit message.

[ajwerner]

@irfansharif I added more structure, improved the error message, and added testing. PTAL

[ajwerner]

For your amusement, here's ChatGPT's summary of this change as a short poem:

In a cluster, settings compared,
Two byte slices, a challenge dared,
Decoding checks if they align,
Fence versions considered, by design.

If matching found, nil returned,
But difference breeds concern,
An error speaks of values clashed,
Cluster safety, our goal unmasked.

[ajwerner]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)