distsql: uncertainty reads under DistSQL don't benefit from read span refresh mechanism

[andreimatei]

When a regular Scan encounters a ReadWithinUncertaintyInterval error, the TxnCoordSender will immediately try to refresh the txn's read spans and, if successful, retry the batch. This doesn't apply to DistSQL reads which don't go through the TxnCoordSender.
We should figure out another level at which to retry.
Separately, if the whole flow is scheduled on the gateway, everything could go through the TxnCoordSender, I think.

Jira issue: CRDB-5744

[andreimatei]

As the only-visible-to-crdb referenced issue above shows, this is suspected to cause a significant regression in higher percentage select latency on a customer workload.

[jordanlewis]

Hmm, @andreimatei can we talk about this? Seems like something we should tackle soon.

[andreimatei]

Months later, DistSQL reads go through the TxnCoordSender, but the txnSpanRefresher is neutered.
Remote flows do return their read spans to the gateway so, amusingly, the root can attempt to refresh if an error is encountered later by some other query, but not when an error is encountered by DistSQL itself.

[knz]

The only way I see is by attaching a token to every row flowing through DistSQL, tracking the highest timestamp of a read that contributed to that row.

What do you do with a filter, an aggregation or an anti-join, where the row carrying the tag is filtered out?

[andreimatei]

I would carry forward the tag even when a row is filtered by infecting all the next rows. I'd have each processor keep track of the highest timestamp that any of its input rows have been tagged with, and I'd tag every output row with that (and also tag the "absence of any output rows" by including this timestamp in each processor's trailing metadata (collected when processors drain). I think that works?

Now that I think about it again, I'm not sure why I phrased this as "tagging rows" rather than describing it in terms of broadcasting metadata and taking advantage of the DistSQL ordered message streams: processors that do KV operations (TableReader, IndexJoiner, etc) would notice when a scan they've done was actually performed at a new (higher) timestamp and would broadcast this information to all their consumers as a ProducerMetadata before sending any more rows to any consumer (the "all consumers" part is important; for example a hash router would send this along on all its output streams). Then, every other processor would respect the convention that such a metadata record is forwarded immediately (as opposed to how we currently handle metadata by deferring its forwarding to later).

[knz]

A distsql processor can have no output row. Indeed it seems like something that's not part of the flow but instead part of the "metadata"

(I really think that this word "metadata" is really bad and should never have been used. The better abstraction is a difference between control plane and data plane. You're playing with the control plane here regardless of what flows data-wise.)

[knz]

There's another challenge in there though. Suppose you have two concurrent processors A and B.

Processor A fails with a logic error (says some SQL built-in function errors out).
Concurrently B is scanning ahead some data.

Today the repatriation of the "metadata" payload will cause the logic error to cancel out whatever result comes from B. That would trash the information bits needed in your algorithm.

If we ever implement savepoint rollbacks in combination with txn refreshes, it's important that the magic that you want to implement does not get invalidated by such a logic error.

[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!

[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
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

[knz]

seems still relevant