kv: remove GetMeta, AugmentMeta and TxnCoordMeta

[knz]

Recommended/requested by @nvanbenschoten.
Discussed with @andreimatei.
Prerequisite to completing #42854.

Prior to this patch, the same data structure TxnCoordMeta was used
both to initialize a LeafTxn from a RootTxn, and a RootTxn from a
LeafTxn. Moreover, the same method on TxnCoordSender (AugmentMeta)
was used to "configure" a txn into a root or a leaf, and to update
a root from the final state of leaves.

This was causing difficult questions when adding features (all the
fields in TxnCoordMeta needed to produce effects in one direction
and no-ops in the other). It was also making it hard to read and
understand the API.

This patch alleviates this problem by separating the two protocols:

// From roots:
func (txn *Txn) GetLeafTxnInputStateOrRejectClient(context.Context) (roachpb.LeafTxnInputState, error)

// to create a new leaf:
func NewLeafTxn(context.Context, *DB, roachpb.NodeID, *roachpb.LeafTxnInputState) *Txn

// From leaves, at end of use:
func (txn *Txn) GetLeafTxnFinalState(context.Context) roachpb.LeafTxnFinalState

// Back into roots:
func (txn *Txn) UpdateRootWithLeafFinalState(context.Context, tfs *roachpb.LeafTxnFinalState)

Additionally, this patch:

• removes the general-purpose Serialize() method, and replaces it
  by TestingCloneTxn() specifically purposed for use in testing.

• removes direct access to the TxnMeta WriteTimestamp in the SQL
  conn executor (to establish a high water mark for table lease
  expiration), and replaces it by a call to a new method
  ProvisionalCommitTimestamp().

Release note: None

[cockroach-teamcity]

This change is 

[knz]

Action item from #43035: remove RefreshWrites from the root<->leaf protos.

[knz]

note to self: I can simplify the IsStarted() further by using the active field.

[andreimatei]

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

---

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

	// AugmentMeta().
	//
	// If AnyTxnStatus is passed, then this function never returns errors.

I think this part is still true / good.

---

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

	// the push and thus calls this method again to receive the new
	// timestamp.
	CommitTimestamp() hlc.Timestamp

The comment on this is not very good. Can you clarify that it fixes the commit timestamp pls? Otherwise it's not clear how it differs from ProvisionalCommitTimestmap.

---

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

	// TODO(knz): Remove this, see
	// https://github.com/cockroachdb/cockroach/issues/15012
	IsStarted() bool

s/IsStarted/Active

---

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

	// TestingCloneTxn returns a clone of the transaction's current proto.
	// This is for use by tests only. Leaf transactions are to be derived

nit: s/ Leaf transactions are to be derived using GetLeafTxnInitialState()./ Use GetLeaf... instead when creating a Leaf txn.

---

pkg/internal/client/txn.go, line 134 at r1 (raw file):

}

// NewLeafTxn is like NewTxn, except it returns a new LeafTxn from

there's not much that's "like NewTxn" here. Most of the comment on that method does not apply. I'd scratch it.

---

pkg/kv/txn_coord_sender.go, line 184 at r1 (raw file):

	// initializeFromTxn populates the interceptor's initial
	// state from a given Transaction.
	initializeFromTxn(*roachpb.Transaction)

s/initializeFromTxn/init

Can you please clarify if one of initializeFromTxn()/initializeLeaf() will be called in the beginning?

---

pkg/kv/txn_coord_sender.go, line 623 at r1 (raw file):

	}

	if txn.Status != roachpb.PENDING {

move this to the top of the function

---

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

  // if all spans encountered during the transaction which need refreshing have
  // been collected to the refresh_reads and refresh_writes span slices.
  bool refresh_invalid = 7;

The comment on this field doesn't quite compute. There's no refresh_reads / refresh_writes to speak of around here.
I haven't actually checked the code, but I believe that in the context of a leaf transferring this field is simply used to disable the accumulation of read spans by the leaf if the root has already given up on accumulating them. I don't think this optimization is worth thinking about. I think we should get rid of the field and let the leaf always accumulate the read spans.

---

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

  bool refresh_invalid = 7;
  // in_flight_writes stores all writes that are in-flight and have not yet
  // been proven to have succeeded. Any client wishing to send a request that

(since you're taking the blame on these lines) s/Any client wishing to send a request that overlaps/Overlapping requests

---

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

message LeafTxnFinalState {
  // txn is a copy of the transaction record.
  Transaction txn = 1 [(gogoproto.nullable) = false];

What do you think about truly making history here and returning the fields from the txn that can actually be updated by the leaf?
I think they are simply:

• none from TxnMeta
• Status
• ObservedTimestamps

We can have the txn field too for backwards compatibility, but stop setting it based on a cluster version.

---

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

  Transaction txn = 1 [(gogoproto.nullable) = false];
  reserved 2;
  // command_count indicates that at least one request

This comment doesn't quite compute - an int indicating that "at least one"... Put more words.
Or, rather, remind me please why we can't simply increment the count on the root by one regardless of what the leaves do, and get rid of communicating this field from leaf to root.

---

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

  // the transaction can continue.
  repeated Span refresh_reads = 4 [(gogoproto.nullable) = false];
  repeated Span refresh_writes = 5 [(gogoproto.nullable) = false];

no refresh_writes (per #43035)

---

pkg/sql/execinfra/base.go, line 256 at r1 (raw file):

// GetLeafTxnFinalState returns the txn metadata from a transaction if
// it is present and the transaction is a leaf transaction, otherwise
// nil.

Do me a favor and change the last sentence to: This method returns nil when called on a Root. This is done as a convenience allowing DistSQL processors to be oblivious about whether they're running in a Leaf or a Root.

---

pkg/sql/execinfra/base.go, line 268 at r1 (raw file):

	txnMeta := txn.GetLeafTxnFinalState(ctx)
	if txnMeta.Txn.ID == uuid.Nil {
		return nil

not your code, but can you remind me/add a comment about when this fires?

[knz]

I processed a few comments already and will process the rest later after a green CI build.

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

---

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

Previously, andreimatei (Andrei Matei) wrote…

I think this part is still true / good.

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

The comment on this is not very good. Can you clarify that it fixes the commit timestamp pls? Otherwise it's not clear how it differs from ProvisionalCommitTimestmap.

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

s/IsStarted/Active

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

nit: s/ Leaf transactions are to be derived using GetLeafTxnInitialState()./ Use GetLeaf... instead when creating a Leaf txn.

Done.

---

pkg/internal/client/txn.go, line 134 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

there's not much that's "like NewTxn" here. Most of the comment on that method does not apply. I'd scratch it.

Done.

---

pkg/kv/txn_coord_sender.go, line 623 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

move this to the top of the function

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

(since you're taking the blame on these lines) s/Any client wishing to send a request that overlaps/Overlapping requests

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

This comment doesn't quite compute - an int indicating that "at least one"... Put more words.
Or, rather, remind me please why we can't simply increment the count on the root by one regardless of what the leaves do, and get rid of communicating this field from leaf to root.

"""
Please note that some users currently require being able to make metadata requests against virtual tables before issuing the savepoint. Any solution to this should preserve this behavior.
"""
#15012 (comment)

It's also mentioned in a comment in the caller to this method in conn_executor_exec.go.
Should I copy this comment here?

---

pkg/sql/execinfra/base.go, line 256 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Do me a favor and change the last sentence to: This method returns nil when called on a Root. This is done as a convenience allowing DistSQL processors to be oblivious about whether they're running in a Leaf or a Root.

Done.

---

pkg/sql/execinfra/base.go, line 268 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

not your code, but can you remind me/add a comment about when this fires?

No idea - this code was there back then when this code was first written (by you). Should I remove it?

[knz]

RFAL.

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

---

pkg/kv/txn_coord_sender.go, line 184 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/initializeFromTxn/init

Can you please clarify if one of initializeFromTxn()/initializeLeaf() will be called in the beginning?

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

The comment on this field doesn't quite compute. There's no refresh_reads / refresh_writes to speak of around here.
I haven't actually checked the code, but I believe that in the context of a leaf transferring this field is simply used to disable the accumulation of read spans by the leaf if the root has already given up on accumulating them. I don't think this optimization is worth thinking about. I think we should get rid of the field and let the leaf always accumulate the read spans.

Updated the comment to explain.

---

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

Previously, andreimatei (Andrei Matei) wrote…

What do you think about truly making history here and returning the fields from the txn that can actually be updated by the leaf?
I think they are simply:

• none from TxnMeta
• Status
• ObservedTimestamps

We can have the txn field too for backwards compatibility, but stop setting it based on a cluster version.

Good idea, but I'd prefer a separate PR. Filed #43192.

---

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

Previously, knz (kena) wrote…

"""
Please note that some users currently require being able to make metadata requests against virtual tables before issuing the savepoint. Any solution to this should preserve this behavior.
"""
#15012 (comment)

It's also mentioned in a comment in the caller to this method in conn_executor_exec.go.
Should I copy this comment here?

I decided to remove this in favor of using the active field on TxnCoordSender. This is sufficient to enable the use case described above.

---

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

Previously, andreimatei (Andrei Matei) wrote…

no refresh_writes (per #43035)

Done.

[andreimatei]

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

---

pkg/internal/client/sender.go, line 113 at r2 (raw file):

	GetLeafTxnInputState(context.Context, TxnStatusOpt) (roachpb.LeafTxnInputState, error)

	// InitializeLeafTxn sets up a LeafTxn using the provided input

this seems to never be called...

---

pkg/kv/txn_coord_sender.go, line 167 at r2 (raw file):

	//
	// This is called once, both for root and leaf txns.
	loadTxn(*roachpb.Transaction)

This loadTxn guy is only implemented by one interceptor - the seqNumAllocator (and only for one measly line too). It seems hardly necessary to me to make it part of this interface. Particularly since its relationship with initializeLeaf and other initialization is quite confusing.
How about moving what it's doing into initCommonInterceptors()?
For roots, also assert there that txn.sequence == 0.

---

pkg/kv/txn_coord_sender.go, line 173 at r2 (raw file):

	//
	// This is called once upon leaf txn initialization, immediately
	// after init() above.

init is no more

---

pkg/kv/txn_coord_sender.go, line 174 at r2 (raw file):

	// This is called once upon leaf txn initialization, immediately
	// after init() above.
	initializeLeaf(*roachpb.LeafTxnInputState)

I think we should get rid of this. It's only implemented by the pipeliner and the spanRefresher. I think the spanRefresher can go away, and for the pipeliner I'd just do that initialization in newLeafTxnCoordSender().

---

pkg/kv/txn_coord_sender.go, line 204 at r2 (raw file):

	if txn.Status != roachpb.PENDING {
		// It's invalid to create a new TCS if the transaction is not in a pending state.

nit: this comment does not add anything

---

pkg/kv/txn_coord_sender.go, line 364 at r2 (raw file):

		// spans to be reported to the Root. But the gateway doesn't do much
		// with them; see #24798.
		&tcs.interceptorAlloc.txnSpanRefresher,

Since we kept RefreshInvalid, can we use it here to skip chaining this interceptor completely? And then I think initializeLeaf() can go away - see comment on that interface.

[knz]

RFAL

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

---

pkg/internal/client/sender.go, line 113 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this seems to never be called...

Good catch. Removed.

---

pkg/kv/txn_coord_sender.go, line 167 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

This loadTxn guy is only implemented by one interceptor - the seqNumAllocator (and only for one measly line too). It seems hardly necessary to me to make it part of this interface. Particularly since its relationship with initializeLeaf and other initialization is quite confusing.
How about moving what it's doing into initCommonInterceptors()?
For roots, also assert there that txn.sequence == 0.

Done.

---

pkg/kv/txn_coord_sender.go, line 173 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

init is no more

Done.

---

pkg/kv/txn_coord_sender.go, line 174 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think we should get rid of this. It's only implemented by the pipeliner and the spanRefresher. I think the spanRefresher can go away, and for the pipeliner I'd just do that initialization in newLeafTxnCoordSender().

Done.

---

pkg/kv/txn_coord_sender.go, line 364 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Since we kept RefreshInvalid, can we use it here to skip chaining this interceptor completely? And then I think initializeLeaf() can go away - see comment on that interface.

Ooooh clever I like it ❇️

Done.

[andreimatei]

LGTM

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

---

pkg/internal/client/txn.go, line 808 at r3 (raw file):

func (txn *Txn) PrepareForRetry(ctx context.Context, err error) {
	if txn.typ != RootTxn {
		panic(errors.WithContextTags(errors.NewAssertionErrorWithWrappedErrf(err, "PrepareForRetry() called on leaf txn"), ctx))

nit, not necessarily for this PR: you use this WithContextTags(NewAsssertion...) combination a lot, but not everywhere. I think it'd be a good idea to ask for a context for all assertion errors - in the errors library directly, or maybe in a cockroach wrapper.

---

pkg/kv/txn_coord_sender.go, line 359 at r3 (raw file):

	// because they do not serve a role on leaves.

	// If refresh span collection is disabled, we only need the first two

nit: s/.../If the read spans are not needed by the root, we don't need the txnSpanRefresher.

---

pkg/kv/txn_coord_sender.go, line 829 at r3 (raw file):

}

var errCannotChangePriority = errors.New("cannot change the user priority of a running transaction")

long line. But why did you extract this variable?

[knz]

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

---

pkg/internal/client/txn.go, line 808 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit, not necessarily for this PR: you use this WithContextTags(NewAsssertion...) combination a lot, but not everywhere. I think it'd be a good idea to ask for a context for all assertion errors - in the errors library directly, or maybe in a cockroach wrapper.

Good idea. Filed cockroachdb/errors#18

---

pkg/kv/txn_coord_sender.go, line 359 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: s/.../If the read spans are not needed by the root, we don't need the txnSpanRefresher.

Done.

---

pkg/kv/txn_coord_sender.go, line 829 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

long line. But why did you extract this variable?

An earlier version of the PR was using it in two places. Reverted. (Good catch)

[knz]

🎉

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)