All the renames are now done.
Reviewable seems to refuse to show commits in isolation (cause there's too many of them?). If you prefer, I can split it into multiple PRs. But the only large commit that's not mechanic is unify OrigTimestamp and RefreshedTimestamp.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

💯 this is great! Thanks for doing the cleanup.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

pkg/internal/client/sender.go, line 344 at r1 (raw file):

	// For backwards compatibility with 19.2, set the DeprecatedOrigTimestamp too (although
	// not really needed by this Mock sender).

Add a TODO.

pkg/kv/txn_interceptor_span_refresher.go, line 279 at r1 (raw file):

// timestamp. Returns whether the refresh was successful or not.
func (sr *txnSpanRefresher) tryUpdatingTxnSpans(
	ctx context.Context, refreshTxn *roachpb.Transaction, refreshFrom hlc.Timestamp,

It's too bad that we lost the comment on this when we made it a param. I'm also not sure how I feel about getting this from the proto instead of sr.refreshedTimestamp. Why did you make that change.

Also, I think we should have a test for the behavior specified in:

// More importantly, reads that have happened since we've previously
// refreshed don't need to be checked below below the timestamp at which
// they've been read (which is the timestamp to which we've previously
// refreshed). Checking below that timestamp (like we would, for example,
// if we simply used txn.OrigTimestamp here), could cause false-positives
// that would fail the refresh.

pkg/roachpb/api.proto, line 1510 at r1 (raw file):

// RefreshRequest is arguments to the Refresh() method, which verifies
// that no write has occurred since the a provided timestamp

"since the a provided"

pkg/roachpb/api.proto, line 1514 at r1 (raw file):

// updated according to the write parameter. A transaction must be
// supplied with this request. If the key has been written more
// recently than the provided txn timestamp, an error is returned

s/provided txn timestamp/provided "refresh from" timestamp/?

pkg/roachpb/api.proto, line 1517 at r1 (raw file):

// and the timestamp cache is not updated.
//
// The timestamp cache is updated to txn.read_timestamp, like it is for all

nit: wrap to same width as rest of comment.

pkg/roachpb/batch.go, line 41 at r1 (raw file):

		// commit timestamp has been forwarded, so that all reads within a txn
		// observe the same snapshot of the database regardless of how the
		// provisional commit timestamp evolves..

Double period.

pkg/roachpb/data.go, line 1261 at r1 (raw file):

// specified in the supplied error can be retried at a refreshed timestamp to
// avoid a client-side transaction restart. If true, returns a cloned, updated
// Transaction object with the commit timestamp and refreshed timestamp set

"provisional commit timestamp"

pkg/roachpb/data.proto, line 334 at r1 (raw file):

  // retry.
  bool commit_timestamp_fixed = 16;
  // The refreshed timestamp is the transaction's read timestamp. All reads are

This comment needs some love now that "refreshed timestamp" is no longer a thing.

pkg/storage/gc_queue_test.go, line 450 at r1 (raw file):

				txn.ReadTimestamp = datum.ts
				txn.WriteTimestamp = datum.ts
				txn.DeprecatedOrigTimestamp = datum.ts

Why are we still providing this in tests? We shouldn't be requiring it to be set anywhere on the server or we won't be able to get rid of it in 20.2.

pkg/storage/batcheval/cmd_end_transaction.go, line 241 at r1 (raw file):

					"programming error: epoch regression: %d", h.Txn.Epoch,
				))
			} else if h.Txn.Epoch == reply.Txn.Epoch && reply.Txn.Timestamp.Less(h.Txn.OrigTimestamp) {

You could compare against h.Txn.MinTimestamp. I'm not sure that that's worth it though.

pkg/storage/batcheval/cmd_refresh.go, line 50 at r1 (raw file):

	refreshFrom := args.RefreshFrom
	if refreshFrom.IsEmpty() {
		// Compatibility with 19.2 nodes, which didn't set the args.RefreshFrom field.

Leave a TODO to remove this with a description of when that is possible.

pkg/storage/batcheval/cmd_refresh.go, line 66 at r1 (raw file):

		return result.Result{}, err
	} else if val != nil {
		if ts := val.Timestamp; !ts.Less(refreshFrom) {

No need for the batcheval: clarify comment about pessimization commit since it's completely removed by the end of this PR.

pkg/storage/batcheval/cmd_refresh_range.go, line 50 at r1 (raw file):

	refreshFrom := args.RefreshFrom
	if refreshFrom.IsEmpty() {
		// Compatibility with 19.2 nodes, which didn't set the args.RefreshFrom field.

Leave a TODO to remove this with a description of when that is possible.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/internal/client/sender.go, line 344 at r1 (raw file):

it's clear...

pkg/kv/txn_interceptor_span_refresher.go, line 279 at r1 (raw file):

Well, I had made the change because OrigTimestamp going away made things awkward for keeping track of what the "refresh from" should be. But you're right, it is better to use sr.refreshedTimestamp. I've now switched to using sr.refreshedTimestamp, but for that I had set that at the beginning of the txn (i.e. on the first batch that flows through) too. PTAL.

Spurred by this, I've also added a commit at the end cleaning up the protection we had against concurrent txn use. PTAL at that too.

The comment is now back. I will write the respective test tomorrow.

pkg/roachpb/api.proto, line 1510 at r1 (raw file):

done

pkg/roachpb/api.proto, line 1514 at r1 (raw file):

done

pkg/roachpb/api.proto, line 1517 at r1 (raw file):

done

pkg/roachpb/batch.go, line 41 at r1 (raw file):

done

pkg/roachpb/data.go, line 1261 at r1 (raw file):

done

pkg/roachpb/data.proto, line 334 at r1 (raw file):

done

pkg/storage/gc_queue_test.go, line 450 at r1 (raw file):

Because we need to override what newTransaction() sets (through the regular txn ctor). In the ctor, setting the OrigTS is not gated by a cluster version cause I didn't take the trouble.
But this can all go in 20.2.

pkg/storage/batcheval/cmd_end_transaction.go, line 241 at r1 (raw file):

yeah... I'd leave it alone.

pkg/storage/batcheval/cmd_refresh.go, line 50 at r1 (raw file):

it clear...

pkg/storage/batcheval/cmd_refresh.go, line 66 at r1 (raw file):

I'll leave it for the history

pkg/storage/batcheval/cmd_refresh_range.go, line 50 at r1 (raw file):

but it's clear...

And thanks for the read!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/txn_interceptor_span_refresher.go, line 279 at r1 (raw file):

Added the test in the commit about "kv: optimize refresh timestamp ranges" commit.

nice test.

Reviewed 2 of 86 files at r1, 15 of 15 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

pkg/internal/client/sender.go, line 344 at r1 (raw file):

The point isn't that this isn't clear, it's that the TODO allows you/me/someone to search for TODO.*20.2 and find all the code we can cleanup right after the next release. I guess as long as you have once of these TODOs on the DeprecatedOrigTimestamp field then its fine.

pkg/kv/dist_sender_server_test.go, line 2890 at r2 (raw file):

// - someone else writes "a" @ 200
// - txn attempts to write "a" and is pushed to 200. The refresh succeeds.
// - txn reads something that has a value in [100,200]. For example, "a", which

I think 200 is exclusive here, so below "as it would find b@200" also needs to change.

pkg/kv/txn_interceptor_span_refresher.go, line 169 at r1 (raw file):

		ba.Txn.ReadTimestamp.Forward(largestRefreshTS)
		if !sr.appendRefreshSpans(ctx, ba, br) {
			// The refresh spans are out of date, return a generic client-side retry error.

What allowed us to get rid of this? The concurrent txn detection? Is that enough to detect concurrent use with dist sql flows?

pkg/kv/txn_interceptor_span_refresher.go, line 130 at r2 (raw file):

	autoRetryCounter *metric.Counter

	// requestInFlight is set while a request is being processed by the next

Why is this detection in the span refresher? It seems like a good idea, but I don't see why it belongs here. I'd move it to the TxnCoordSender itself.

pkg/kv/txn_interceptor_span_refresher.go, line 162 at r2 (raw file):

	} else if sr.refreshedTimestamp != batchReadTimestamp {
		// Sanity check: we're supposed to control the read timestamp. What we're
		// tracking in sr. refreshedTimestamp is not supposed to get out of sync

sr.refreshedTimestamp

pkg/storage/gc_queue_test.go, line 450 at r1 (raw file):

Ack. Thanks for the explanation.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

pkg/internal/client/sender.go, line 344 at r1 (raw file):

right - one TODO on DeprecatedOrigTimestamp to rule them all

pkg/kv/dist_sender_server_test.go, line 2890 at r2 (raw file):

I clarified that the txn is pushed to (200,1). And below it should have been a@200.
See now.

pkg/kv/txn_interceptor_span_refresher.go, line 169 at r1 (raw file):

Yes to all the questions.
But as discussed, I've added yet another assertion error back here.

pkg/kv/txn_interceptor_span_refresher.go, line 130 at r2 (raw file):

moved to the gatekeeper

pkg/kv/txn_interceptor_span_refresher.go, line 162 at r2 (raw file):

done

Reviewed 2 of 86 files at r1, 6 of 6 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)