sql/catalog/descs: unify and fix TxnWithExecutor

[ajwerner]

The code was bogus before because it was trying to use the transaction
after committing. That's not allowed. Also, the retry behavior was unsound.
We make it sound by tracking when the invariant violation has happened
and propagate that up for a true restart.

Release justification: fixes very broken and needed code

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

@ZhouXing19 and/or @rafiss can y'all review this? I'm reasonably pleased with where it ended up. @maryliag would very much like #86433 merged ASAP.

[ZhouXing19]

Thanks for fixing up the new interface, and LGTM! But maybe we want a stamp from @rafiss too.

Just wanted to confirm my understanding of some critical changes in this PR:

1. Previously there were 2 sets of logic for validating the schema changes, one in CollectionFactory.Txn() and the other in ex.commitSQLTransactionInternal(). And even worse, in CollectionFactory.TxnWithExecutor(), we used both of them. We now unify them to the latter one.
2. The retry invoked by a two-version invariant violation has been changed to first roll back the current kv.Txn, then propagate a retry error to the sql layer, and finally trigger the conn executor to reset the txn.

Do I understand it correctly?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)

---

-- commits line 20 at r4:
nit: release justification is missing

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)

---

pkg/sql/internal.go line 1042 at r1 (raw file):

	}
	defer ex.close(ctx, externalTxnClose)
	return ex.commitSQLTransactionInternal(ctx)

Should we call ex.commitSQLTransaction() instead? Otherwise we're not applying the changes to provoke conn executor to reset txn when the two-version invariant is violated.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @miretskiy, and @ZhouXing19)

---

pkg/sql/internal.go line 1042 at r1 (raw file):

Otherwise we're not applying the changes to provoke conn executor to reset txn when the two-version invariant is violated.

The internal executor in question doesn't live past this function call, so what value does resetting its transaction have?

[ajwerner]

The retry invoked by a two-version invariant violation has been changed to first roll back the current kv.Txn, then propagate a retry error to the sql layer, and finally trigger the conn executor to reset the txn.

This isn't quite right. We always rolled back the transaction when the invariant was violated, we just did it with a more confusing API. Before this change, we called (*kv.Txn).CleanupOnError and we had to synthesize an error to pass it. We then depended on how that error was handled implicitly rather than explicitly, but we also depended on an accompanying bool return value.

See CleanupOnError:

cockroach/pkg/kv/txn.go

Lines 691 to 700 in 00af7d7

	// CleanupOnError cleans up the transaction as a result of an error.
	func (txn *Txn) CleanupOnError(ctx context.Context, err error) {
	if txn.typ != RootTxn {
	panic(errors.WithContextTags(errors.AssertionFailedf("CleanupOnError() called on leaf txn"), ctx))
	}
	if err == nil {
	panic(errors.WithContextTags(errors.AssertionFailedf("CleanupOnError() called with nil error"), ctx))
	}
	if replyErr := txn.rollback(ctx); replyErr != nil {

And Rollback:

cockroach/pkg/kv/txn.go

Lines 835 to 840 in 00af7d7

	func (txn *Txn) Rollback(ctx context.Context) error {
	if txn.typ != RootTxn {
	return errors.WithContextTags(errors.AssertionFailedf("Rollback() called on leaf txn"), ctx)
	}
	return txn.rollback(ctx).GoError()

The code doesn't change the behavior in CheckTwoVersionInvariant, it just makes the way the result is communicated more explicit; rather than relying on a combination of a fake restart error and a boolean, it relies on an explicit error which can be detected.

[rafiss]

nice! thanks for the new datadriven test too

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @ZhouXing19)

---

-- commits line 20 at r4:

Previously, ZhouXing19 (Jane Xing) wrote…

nit: release justification is missing

i believe the justification is only needed in the PR (but confusingly there is a git hook that adds it to the commit msg too)

[ZhouXing19]

I see, thanks for the detailed explanation!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @miretskiy, and @ZhouXing19)

---

pkg/sql/internal.go line 1042 at r1 (raw file):

Previously, ajwerner wrote…

Otherwise we're not applying the changes to provoke conn executor to reset txn when the two-version invariant is violated.

The internal executor in question doesn't live past this function call, so what value does resetting its transaction have?

Ah I see, sorry I misunderstood.

[ajwerner]

TFTR!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)