roachpb,kv: new TxnCoordSender API for step-wise execution

[knz]

Supports #42864.

This patch introduces the following methods on client.TxnSender:

	// Step creates a sequencing point in the current transaction. A
	// sequencing point establishes a snapshot baseline for subsequent
	// read operations: until the next sequencing point, read operations
	// observe the data at the time the snapshot was established and
	// ignore writes performed since.
	//
	// Before the first step is taken, the transaction operates as if
	// there was a step after every write: each read to a key is able to
	// see the latest write before it. This makes the step behavior
	// opt-in and backward-compatible with existing code which does not
	// need it.
	// The method is idempotent.
	Step() error

	// DisableStepping disables the sequencing point behavior and
	// ensures that every read can read the latest write. The
	// effect remains disabled until the next call to Step().
	// The method is idempotent.
	DisableStepping() error

Additionally it implements it in the TxnCoordSender.

Step() is the most important and will be used to introduce sequence
points between SQL statements.

DisableStepping() is a convenience function, so as to avoid
introducing many Step() calls in the code that performs schema
changes.

Release note: none

[cockroach-teamcity]

This change is 

[knz]

@nvanbenschoten I think I will need your help on this one. I have tried two different approaches and each of them breaks for an "interesting" reason.

1. use the special value readSeq == 0 to disable step-wise execution. (this is the current choice of the PR).

   The first valid read seq is then 1. To hold the invariant readSeq <= seqGen, this forces to assign seqGen := 1 if there were no writes yet in the txn.

   Pros: just 1 extra field in the seqno allocator and TxnCoordMeta.

   Cons: this breaks the 1PC optimization, as demonstrated by TestUpsertFastPath in sql: make SQL statements operate on a read snapshot #42862.

2. use a separate boolean to enable step-wise execution, and make readSeq == 0 a valid read seqnum.

   Pros: code perhaps easier to read.

   Cons:

   • the MVCC read-at-fixed-seqnum logic does not activate if the seqnum of a read operation is 0. So the logic breaks for a SQL mutation that executes first thing in a txn.
   • 2 extra fields in seqno allocator + TxnCoordMeta.

I think I really want to take approach (1) but I don't understand how/where to preserve the 1PC optimization.

[andreimatei]

Can you spell out why 1PC batches need sequence number 0? Is there a good reason?

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

[knz]

Can you spell out why 1PC batches need sequence number 0? Is there a good reason?

No — I can't spell it out because I don't understand the optimization completely and I did not design it.

However my working theory is that the thing checks that there was just 1 batch by verifying that the operation that sets the sequence number (e.g. 1) is also present in the batch — with the logic proposed here, it won't find it.

However I don't know where this code lives. I really hope you or Nathan can help.

[andreimatei]

Oh I see. Check this out:
https://github.com/cockroachdb/cockroach/blob/6e1d241720245a97a1b859622282c98e131ca642/pkg/roachpb/batch.go#L256-L255

Would making it expect 2 instead of 1 fix your problem?

[knz]

Would making it expect 2 instead of 1 fix your problem?

Maybe. My problem is that my code currently lets seqGen start at 0 by default and so it only starts at 1 when step-wise execution is requested.

But I could change the default to become 1.

The problem I see with that is cross-version clusters. If I change the code you point me to, then during an upgrade the optimization will be disabled for all requests coming from pre-upgrade nodes. Is that acceptable?

[knz]

I have updated the PR after chatting with Nathan and Andrei:

• the logic is modified to adopt alternative 2 described above, so that the value 0 remains a valid sequence number. The seqnum interceptor now uses a separate internal field to make this happen.

• I have fixed the storage bug which did not respect the seqnum for read operations when it was initialized to 0, as discussed offline.

[knz]

Ok this and #42862 now do the job properly. RFAL

[andreimatei]

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

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 67 at r1 (raw file):

	writeSeq enginepb.TxnSeq

	// readSeqPlusOne is either:

This plus one business is a bit confusing. I'd try grouping writeSeq and readSeq into a separate seqCounter struct, with a IncWriteSeq(), LockReadSeq(), GetReadSeq() methods, have a bool for whether the stepping is enabled, and have GetReadSeq() return writeSeq or readSeq depending on the bool.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 104 at r1 (raw file):

			// enabled, then we want the read operation to read at the read
			// seqnum, not the latest write seqnum.
			oldHeader.Sequence = s.readSeqPlusOne - 1

Let's make put a note here about read+write requests operating at writeSeq because they're special, to make it clear that it's intentional.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 139 at r1 (raw file):

func (s *txnSeqNumAllocator) stepLocked() error {
	if s.readSeqPlusOne-1 > s.writeSeq {
		return errors.AssertionFailedf("cannot step() after mistaken initialization (%d,%d)", s.writeSeq, s.readSeqPlusOne)

long line

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 139 at r1 (raw file):

func (s *txnSeqNumAllocator) stepLocked() error {
	if s.readSeqPlusOne-1 > s.writeSeq {
		return errors.AssertionFailedf("cannot step() after mistaken initialization (%d,%d)", s.writeSeq, s.readSeqPlusOne)

nit: I'd just say "invalid read seq" or such

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 148 at r1 (raw file):

// restores read-latest-write behavior.
// Used by the TxnCoordSender's DisableStepping() method.
func (s *txnSeqNumAllocator) disableSteppingLocked() error {

get rid of the error return

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 157 at r1 (raw file):

	s.commandCount = 0
	s.writeSeq = 0
	if s.readSeqPlusOne > 0 {

Wouldn't a more natural behavior be for epochBumpedLocked() to imply disableSteppingLocked()? Like, let's start every epoch with stepping disabled in the spirit of forgetting everything that was done on the transaction before the restart.
As it stands, the behavior is inconsistent with what happens to a txn after a TransactionAbortedError where I don't see code to enable stepping on the new txn.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 159 at r1 (raw file):

	if s.readSeqPlusOne > 0 {
		s.readSeqPlusOne = 1
	} else {

I think the else is unnecessary.

---

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

  // Current read seqnum, plus 1. The special value of zero indicates synchronous
  // read-own-writes, where every KV read is able to observe the latest writes.
  // As soon as the value is increased past 0, (value - 1) becomes the sequence number

nit: bad wrapping

[andreimatei]

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

---

pkg/storage/replica_evaluate.go, line 246 at r1 (raw file):

		args := union.GetInner()
		if baHeader.Txn != nil {
			// Set the Request's sequence number on the TxnMeta for this

TxnMeta will no longer be accurate here, right?

[nvanbenschoten]

Reviewed 9 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

---

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

	SerializeTxn() *roachpb.Transaction

	// Step creates a sequencing point in the current transaction. A

Could you replace "read operations" with "read-only operations" throughout these descriptions?

---

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

	// effect remains disabled until the next call to Step().
	// The method is idempotent.
	DisableStepping() error

Make it clear somewhere that transactions start with stepping disabled.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 67 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

This plus one business is a bit confusing. I'd try grouping writeSeq and readSeq into a separate seqCounter struct, with a IncWriteSeq(), LockReadSeq(), GetReadSeq() methods, have a bool for whether the stepping is enabled, and have GetReadSeq() return writeSeq or readSeq depending on the bool.

Yeah, I like this idea. The "plus one" business seems to spread across a lot of places.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 100 at r1 (raw file):

		// Default case: operate at the current seqnum.
		oldHeader.Sequence = s.writeSeq
		if s.readSeqPlusOne > 0 && roachpb.IsReadOnly(req) {

How do you feel about restructuring this to something like:

var seq enginepb.TxnSeq
if roachpb.IsTransactionWrite(req) || req.Method() == roachpb.EndTransaction {
	s.writeSeq++
	seq = s.writeSeq
} else /* if roachpb.IsReadOnly(req) */ {
	if s.readSeqPlusOne > 0 {
		seq = s.readSeqPlusOne - 1
	} else {
		seq = s.writeSeq
	}
}

[knz]

Guys if you want me to do something else than readSeqPlusOne then I need you to tell me what I should put in the TxnCoordMeta and the logic you recommend for the Update function. Thank you in advance.

[knz]

discussed with Andrei: the proposed structure works because we only care about Root->Leaf conversion

[knz]

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

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could you replace "read operations" with "read-only operations" throughout these descriptions?

Done.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Make it clear somewhere that transactions start with stepping disabled.

Done.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 67 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, I like this idea. The "plus one" business seems to spread across a lot of places.

I simplified this. PTAL.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 100 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How do you feel about restructuring this to something like:

var seq enginepb.TxnSeq
if roachpb.IsTransactionWrite(req) || req.Method() == roachpb.EndTransaction {
	s.writeSeq++
	seq = s.writeSeq
} else /* if roachpb.IsReadOnly(req) */ {
	if s.readSeqPlusOne > 0 {
		seq = s.readSeqPlusOne - 1
	} else {
		seq = s.writeSeq
	}
}

I can't do this: The complement of IsTransactionWrite is not IsReadOnly.
Anyway with the new field usage, the code is slightly easier on the eye.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 104 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Let's make put a note here about read+write requests operating at writeSeq because they're special, to make it clear that it's intentional.

Done.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 139 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

long line

Done.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 148 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

get rid of the error return

I'd rather not. I want to add an assert in here when #43032 lands.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 157 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Wouldn't a more natural behavior be for epochBumpedLocked() to imply disableSteppingLocked()? Like, let's start every epoch with stepping disabled in the spirit of forgetting everything that was done on the transaction before the restart.
As it stands, the behavior is inconsistent with what happens to a txn after a TransactionAbortedError where I don't see code to enable stepping on the new txn.

Thanks for explaining. Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

nit: bad wrapping

Done.

---

pkg/storage/replica_evaluate.go, line 246 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

TxnMeta will no longer be accurate here, right?

What do you mean?

[knz]

Rebased this on top of now-ready #43032. RFAL.

@andreimatei check that the API definition works for you (and complies to RFC)

@nvanbenschoten (optionally) check that the sequence number allocation is correct. I think you've checked that before and I haven't changed the logic since your last review.

[knz]

Friendly ping.

[andreimatei]

LGTM

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

---

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

	// Step creates a sequencing point in the current transaction. A
	// sequencing point establishes a snapshot baseline for subsequent
	// read-only operations: until the next sequencing point, read-only operations

pretty please wrap the lines

---

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

	// effect remains disabled until the next call to Step().
	// The method is idempotent.
	// Note that a TxnCoordSender is initially in the non-

I don't think we should mention the TCS in this comment. All implementors of this interface must be in "non-stepping mode" initially.

---

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

	// Load the in-flight writes in the pipeliner.
	tcs.interceptorAlloc.txnPipeliner.initializeLeaf(tis)
	// Load the read seqnum into the seq num allocator.

nit: I think this comment and the one above are bound to go stale. They also duplicate the comments on the respective methods.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 148 at r1 (raw file):

Previously, knz (kena) wrote…

I'd rather not. I want to add an assert in here when #43032 lands.

#43032 and I see no assertion...

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 62 at r2 (raw file):

	wrapped lockedSender

	// writeSeq is the current write seqnum, or the value last assigned

nit: the "or" is ambiguous, it'd replace it with a "(...)" to make it clear that what follows is an explanation.

---

pkg/roachpb/data.proto, line 623 at r2 (raw file):

  // regardless of the current seqnum generated for writes. This is
  // updated via the (client.TxnSender).Step() operation.
  int32 read_seq_num_plus_one = 9 [

you don't want to add the bool here too instead of the plus one business?

---

pkg/storage/replica_evaluate.go, line 246 at r1 (raw file):

Previously, knz (kena) wrote…

What do you mean?

I don't know what I meant :). nvm.

[knz]

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

---

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

Previously, andreimatei (Andrei Matei) wrote…

pretty please wrap the lines

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

I don't think we should mention the TCS in this comment. All implementors of this interface must be in "non-stepping mode" initially.

Done.

---

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

Previously, andreimatei (Andrei Matei) wrote…

nit: I think this comment and the one above are bound to go stale. They also duplicate the comments on the respective methods.

Thanks. Changed the comment to hint about what to do in the future.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 148 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

#43032 and I see no assertion...

Done.

---

pkg/kv/txn_interceptor_seq_num_allocator.go, line 62 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: the "or" is ambiguous, it'd replace it with a "(...)" to make it clear that what follows is an explanation.

Done.

---

pkg/roachpb/data.proto, line 623 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

you don't want to add the bool here too instead of the plus one business?

I don't have strong feelings for either. What is best?

[knz]

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

---

pkg/roachpb/data.proto, line 623 at r2 (raw file):

Previously, knz (kena) wrote…

I don't have strong feelings for either. What is best?

Separated the fields as per previous suggestion.

[knz]

TFYRs!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)