sql: run show cluster setting in a restartable txn

[jordanlewis]

Previously, SHOW CLUSTER SETTING would run in the parent txn. This was
troublesome because if it misses a KV update once (due to an older txn
timestamp), it'll miss it in perpetuity.

Now, we make a fresh transaction for the kv read, and allow it to
restart.

Closes #30225.

Release note: None

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[jordanlewis]

This is extremely science dog, but in principle I don't see any reason why it's not ok to make a new transaction here. The analogous SET CLUSTER SETTING code does it, for example.

[jordanlewis]

This is extremely science dog, but in principle I don't see any reason why it's not ok to make a new transaction here. The analogous SET CLUSTER SETTING code does it, for example.

[tschottdorf]

LGTM, but equally 👩‍🔬 🐕 as well, so let's get someone who would know... @andreimatei?

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

[andreimatei]

well so this patch introduces non-repeatable reads for transactions that call this showStateMachineSetting(). I'm not sure if it's a good idea (but I haven't checked how this function is used).
Why would we do this , exactly? Skimming through #30225, I understand that the problem is with a set statement. Can't we have that guy create new transactions for every iteration of its polling? (and also probably make that particular set statement illegal inside transactions?)

Reviewable status: complete! 0 of 0 LGTMs obtained

[jordanlewis]

Andrei, I think the problem is only in SHOW, not in SET.

[jordanlewis]

Andrei, I think the problem is only in SHOW, not in SET.

[andreimatei]

Then can you describe the problem, please? :)

[andreimatei]

Then can you describe the problem, please? :)

[jordanlewis]

Gossip can win against KV. If the two disagree, you need to restart your txn and check KV again. I think. Honestly, I'm not sure how these components interact exactly.

[jordanlewis]

Gossip can win against KV. If the two disagree, you need to restart your txn and check KV again. I think. Honestly, I'm not sure how these components interact exactly.

[andreimatei]

OK, so this is about show cluster setting version. I think you want to make this statement non-transactional (i.e. make it error out if it's ran inside an explicit txn). In fact, I think we should make all show cluster setting statements non-transactional. Then this change looks fine to me (modulo the fact that I think it's missing the retries).

[andreimatei]

OK, so this is about show cluster setting version. I think you want to make this statement non-transactional (i.e. make it error out if it's ran inside an explicit txn). In fact, I think we should make all show cluster setting statements non-transactional. Then this change looks fine to me (modulo the fact that I think it's missing the retries).

[jordanlewis]

What do you mean you think it's missing the retries?

[jordanlewis]

What do you mean you think it's missing the retries?

[andreimatei]

See my comment below

we used to retry on this, now we don't. Shouldn't we?

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/show_cluster_setting.go, line 86 at r2 (raw file):

		}
		if !bytes.Equal(gossipRawVal, kvRawVal) {
			return errors.Errorf("value differs between gossip (%v) and KV (%v); try again later (%v after %s)", gossipObj, kvObj, retryCtx.Err(), timeutil.Since(tBegin))

we used to retry on this, now we don't. Shouldn't we?

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/show_cluster_setting.go, line 86 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

we used to retry on this, now we don't. Shouldn't we?

Yes I suppose we should. Is the right way to do that to keep the outer Retry that I deleted and embed this Txn call within? Also, I don't understand what the last WithMaxAttempts was for - it seems to have been set to MaxInt32 retries.

[jordanlewis]

PTAL @andreimatei

[jordanlewis]

PTAL @andreimatei

[andreimatei]

LGTM

But what did you think about disallowing show cluster setting in a txn?

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/show_cluster_setting.go, line 23 at r3 (raw file):

	"strings"
	"time"

extra line

[jordanlewis]

But what did you think about disallowing show cluster setting in a txn?

What's the point? I guess to just prevent people from being confused? If we're doing this, I think we should do them all at once - and if that's the case, not in this PR.

[jordanlewis]

But what did you think about disallowing show cluster setting in a txn?

What's the point? I guess to just prevent people from being confused? If we're doing this, I think we should do them all at once - and if that's the case, not in this PR.

[jordanlewis]

TFTR!

[jordanlewis]

TFTR!

[jordanlewis]

bors r+

[jordanlewis]

bors r+

[andreimatei]

The point is that this patch has made show cluster setting non-transactional, as in non-repeatable read anomaly. Isn't that a biggie?

[andreimatei]

The point is that this patch has made show cluster setting non-transactional, as in non-repeatable read anomaly. Isn't that a biggie?

[jordanlewis]

Doesn't feel like a biggie to me. We have other things like that, such as schema changes, don't we?

[jordanlewis]

Doesn't feel like a biggie to me. We have other things like that, such as schema changes, don't we?

[andreimatei]

Schema changes are special beasts indeed, but I don't quite see the analogy. The atomicity of schema changes is questionable, leading to write-skew type anomalies, but that's different (even schema changes outside of txns would have that problem).

[andreimatei]

Schema changes are special beasts indeed, but I don't quite see the analogy. The atomicity of schema changes is questionable, leading to write-skew type anomalies, but that's different (even schema changes outside of txns would have that problem).

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[knz]

this needs a backport if you claim it fixes #28879.

[knz]

this needs a backport if you claim it fixes #28879.