sql: Client-side retries with WriteTooOldError in UPDATE

[bdarnell]

WriteTooOldErrors are handled specially: We try to delay returning this error until the end of the transaction in order to lay down more intents and do more discovery of conflicting timestamps. However, this delay undermines the SQL-level "rewind" functionality which can transparently retry the first statement in a transaction.

We have a strong preference for transparent retries whenever possible. In #22234, we disabled the delayed WriteTooOldError behavior for CPut and Increment, because we know that a transparent RefreshSpans will fail, and if we delay the WriteTooOldError to COMMIT, it will trigger a client-visible retry. This had the effect of ensuring that if the first statement was an INSERT, we'd retry internally, but if it was an UPDATE (including an UPDATE clause in an INSERT ON CONFLICT) we may trigger a retry at COMMIT time.

I think that #22234 didn't go far enough: Nearly all Put operations currently originated by SQL are logically "conditional puts" that are done in separate Get and Put steps. The RefreshSpans is guaranteed to fail just as it is for CPut. We should be returning WriteTooOldError immediately for these regular puts too. Doing this for all Puts effectively disables the "delayed WTOE" behavior, although I think Puts where the delayed WTOE would be beneficial are rare in our current usage.

I also think that #22234 went too far in making WTOE immediate for all CPuts. In the first statement of a transaction, an immediate WTOE maximizes our chance of a transparent retry. But in later statements, CPuts as well as regular Puts benefit from continuing to lay down intents. (This would prevent the O(n^2) behavior from #22234 (comment). In scenarios with high contention on multiple keys, we currently have quadratic behavior for operations using CPut but better performance for operations using Put)

I suggest addressing this in two parts. First, disable the "delayed WTOE" behavior for Puts (and probably just for everything). I think this is probably backportable (although it has the risk of making the aforementioned. O(n^2) behavior more likely for high-contention UPDATEs). Second, introduce a new flag to be plumbed down from SQL to indicate whether we are currently able to autoretry an immediate WTOE, and use this flag instead of the type of request to decide whether WTOEs are immediate or delayed. (I'll only do the first part myself; I need to hand off the latter part to someone else).

[awoods187]

cc @andy-kimball i think we should try to get this done for 19.2

[bdarnell]

The first part of this is trivial and I intend to get it into 19.1.3. I'm not sure the second part is worth trying to get into 19.2 at this stage in the development cycle.

[bdarnell]

The fix is in #38668. After working through the tests I'm a little less eager to get this into 19.1.3. I think it's probably still safe but I missed a few things on my initial implementation so it might be better to wait.

[awoods187]

Second, introduce a new flag to be plumbed down from SQL to indicate whether we are currently able to autoretry an immediate WTOE, and use this flag instead of the type of request to decide whether WTOEs are immediate or delayed

@bdarnell could you clarify how big the work effort is for the second part?

[bdarnell]

I think it's fairly small, probably a couple days of work. But testing it may be tricky and it may get more complex than expected once you start pulling on that thread.

[bdarnell]

Copying this from #38668 (review)

And FYI, we also have this code. It detects pushes while the txn can still auto-retry, but it doesn't do anything unless there's no chance for the refresh to succeed. Maybe instead of this patch we could improve that code - have it actually try a refresh (or, in the case of a write_too_old flag, just assume that the refresh will fail - which I guess would keep the write_too_old field somewhat useful). This way KV could continue to defer WTOE (and we would also defer them for CPut) and SQL would be in charge of deciding when deferring is no longer a good idea.

cockroach/pkg/sql/conn_executor_exec.go

Lines 425 to 445 in d024107

	if !os.ImplicitTxn.Get() && txn.IsSerializablePushAndRefreshNotPossible() {
	rc, canAutoRetry := ex.getRewindTxnCapability()
	if canAutoRetry {
	ev := eventRetriableErr{
	IsCommit: fsm.FromBool(isCommit(stmt.AST)),
	CanAutoRetry: fsm.FromBool(canAutoRetry),
	}
	txn.ManualRestart(ctx, ex.server.cfg.Clock.Now())
	payload := eventRetriableErrPayload{
	err: roachpb.NewTransactionRetryWithProtoRefreshError(
	"serializable transaction timestamp pushed (detected by connExecutor)",
	txn.ID(),
	// No updated transaction required; we've already manually updated our
	// client.Txn.
	roachpb.Transaction{},
	),
	rewCap: rc,
	}
	return ev, payload, nil
	}
	}

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!