kvserver: document reproposals

[tbg]

Reproposals are a deep rabbit hole and an area in which past changes
were all related to subtle bugs. Write it all up and in particular make
some simplifications that ought to be possible if my understanding is
correct:

• have proposals always enter (*Replica).propose without a
  MaxLeaseIndex or prior encoded command set, i.e. propose
  behaves the same for reproposals as for first proposals.
• assert that after a failed call to tryReproposeWithNewLeaseIndex,
  the command is not in the proposals map, i.e. check absence of
  a leak.
• replace code that should be impossible to reach (and had me confused
  for a good amount of time) with an assertion.
• add long comment on r.mu.proposals.

This commit also moves tryReproposeWithNewLeaseIndex off (*Replica),
which is possible due to recent changes¹. In doing so, I realized
there was a (small) data race (now fixed): when returning a
NotLeaseholderError from that method, we weren't acquiring r.mu. It
may have looked as though we were holding it already since we're
accessing r.mu.propBuf, however that field has special semantics - it
wraps r.mu and acquires it when needed.

Epic: CRDB-220
Release note: None

Footnotes

1. The "below raft" test mentioned in the previous comment was
   changed in https://github.com/cockroachdb/cockroach/pull/93785 and
   no longer causes a false positive. ↩

[cockroach-teamcity]

This change is 

[andreimatei]

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

---

pkg/kv/kvserver/replica.go line 506 at r1 (raw file):

		//    is not superseded, see below) and attach the value to the entry.
		// 2. for each entry:
		//				- stage written and in-memory effects of the entry (some may apply as no-ops

indentation

---

pkg/kv/kvserver/replica.go line 596 at r1 (raw file):

		// [^1]: https://github.com/cockroachdb/cockroach/blob/59ce13b6052a99a0318e3dfe017908ff5630db30/pkg/kv/kvserver/replica_raft.go#L1224
		// [^2]: https://github.com/cockroachdb/cockroach/blob/59ce13b6052a99a0318e3dfe017908ff5630db30/pkg/kv/kvserver/replica_application_result.go#L148
		// [^3]: it's debatable how useful this is. It was introduced in

what is "this"?

[nvanbenschoten]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pavelkalinnikov and @tbg)

---

pkg/kv/kvserver/replica.go line 523 at r1 (raw file):

		// We can always safely create an identical copy (i.e. (1)) because of the
		// replay protection conferred by the MaxLeaseIndex - all but the first
		// proposal will be rejected (i.e. apply as a no-op).

"all but the first proposal that successfully achieves consensus"

---

pkg/kv/kvserver/replica.go line 531 at r1 (raw file):

with various different MaxLeaseIndex values

Is it possible for there two be more than two different MaxLeaseIndex values in the unapplied log at a time? And can they be interleaved?

---

pkg/kv/kvserver/replica.go line 552 at r1 (raw file):

		// where we assume that the `MaxLeaseIndex` 100 is invalid, i.e. when we see the
		// first copy of the command being applied, we've already applied some command
		// with a higher `MaxLeaseIndex`. In a world without mechanism (2), `N` would

"an equal or higher"

---

pkg/kv/kvserver/replica.go line 553 at r1 (raw file):

		// first copy of the command being applied, we've already applied some command
		// with a higher `MaxLeaseIndex`. In a world without mechanism (2), `N` would
		// be rejected, and would finalize the proposal (i.e. signal the client with

Should we define "finalize" up above in a broader discussion of when entries are added to the proposals map, when they're updated, and when they're removed?

---

pkg/kv/kvserver/replica.go line 584 at r1 (raw file):

		// then can (2) engage. In addition, an entry that doesn't pass this equality
		// check must not signal the proposer and/or unlink from the proposals map (as a
		// newer reproposal is likely in the log)[^4].

"as a newer reproposal is likely in the log which may succeed"

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):

	p.command.MaxLeaseIndex = 0
	p.encodedCommand = nil
	pErr := r.propose(ctx, p, tok.Move(ctx))

Replica.propose doesn't assign the MaxLeaseIndex synchronously, it only does when the proposal buffer is flushed. Are we ok with the command having a MaxLeaseIndex of 0 in the meantime? The need to reset the fields on error raises questions.

---

pkg/kv/kvserver/replica_application_state_machine.go line 236 at r1 (raw file):

			log.Fatalf(ctx, "command already applied: %+v; unexpected successful result", cmd)
		}
		// If any reproposals at a higher MaxLeaseIndex exist we know that they will

I don't understand this branch. It dates back to 0e81674, but I can't make sense of it — it sounds like it was just dead code. Glad you got rid of it and replaced it with an assertion.

---

pkg/kv/kvserver/replica_proposal.go line 135 at r1 (raw file):

// rejected and should not be reproposed.
//
// Note that some commands (such as leases) don't use MaxLeaseIndex.

"lease requests"

Lease transfers do use MaxLeaseIndex.

[tbg]

Thanks for the reviews! I rebased and added a fix-up commit, so if reviewable gets confused you might just want to use GH review for the last commit instead.

Dismissed @andreimatei from 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @pavelkalinnikov)

---

pkg/kv/kvserver/replica.go line 531 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

with various different MaxLeaseIndex values

Is it possible for there two be more than two different MaxLeaseIndex values in the unapplied log at a time? And can they be interleaved?

I think in practice we won't see too much of that but in theory anything goes, because you always have to assume that all previous incarnations are still floating around out there as raft reproposals that could reach the leader at any time & in any order. Updated the comment with some detail and simplified it a bit too.

---

pkg/kv/kvserver/replica.go line 596 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

what is "this"?

The below-raft reproposal mechanism, updated.

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Replica.propose doesn't assign the MaxLeaseIndex synchronously, it only does when the proposal buffer is flushed. Are we ok with the command having a MaxLeaseIndex of 0 in the meantime? The need to reset the fields on error raises questions.

The command is not in the map at this point, so I am not only ok with this, but intentionally want this. I want r.propose to not have to care about whether it sees a proposal for the first time or fifth time.

---

pkg/kv/kvserver/replica_application_state_machine.go line 236 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't understand this branch. It dates back to 0e81674, but I can't make sense of it — it sounds like it was just dead code. Glad you got rid of it and replaced it with an assertion.

Ack, thanks for the check. I had a similar conclusion. The logic is still quite complicated and I wish it were a little less subtle to know when a proposal is in which "state" but that's for another day.

[nvanbenschoten]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pavelkalinnikov)

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The command is not in the map at this point, so I am not only ok with this, but intentionally want this. I want r.propose to not have to care about whether it sees a proposal for the first time or fifth time.

The part that feels fragile to me then is the need to reset the fields on error. I probably just don't understand that well enough yet, but why is an error here treated differently than an error when flushing the proposal buffer back into the proposals map?

[tbg]

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

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):
In this method, the command is not in the proposals map. If r.propose returns an error, the command never made it into the proposals map. So here we just pretend that we never even called r.propose. The proposal will then fail up to the caller, and this is probably not a particularly well-tested code path anyway, but it will not be reproposed and so it shouldn't matter what the MLAI and prev encoded command was. But it felt right to restore the previous state, to avoid having to reason about yet one more code path at the caller.

I'm not defending this code, btw - this is the clearest I was able to make it without really stirring the pot. I wish we didn't have these below-raft reproposals and would "just" delegate to an above-raft goroutine pool to repropose on behalf of clients who used pipelined writes. I bet that would be tricky in its own right, but conceptually clearer.

but why is an error here treated differently than an error when flushing the proposal buffer back into the proposals map?

I might be wrong, but: if we catch an error flushing the buffer, we return it up:

cockroach/pkg/kv/kvserver/replica_proposal_buf.go

Line 540 in a94858b

return 0, firstErr

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 727 to 728 in b637f03

	err := r.withRaftGroupLocked(true, func(raftGroup *raft.RawNode) (bool, error) {
	numFlushed, err := r.mu.proposalBuf.FlushLockedWithRaftGroup(ctx, raftGroup)

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 756 to 758 in b637f03

	} else if err != nil {
	return stats, errors.Wrap(err, "checking raft group for Ready")
	}

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 1110 to 1113 in b637f03

	default:
	log.FatalfDepth(ctx, 1, "%+v", err)
	panic("unreachable")
	}

basically flushing the buffer "has to work". But in this method, we can run into the closed timestamp, so it has to be fallible and we need to return an error to the caller.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[nvanbenschoten]

Reviewed 1 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @pavelkalinnikov)

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

In this method, the command is not in the proposals map. If r.propose returns an error, the command never made it into the proposals map. So here we just pretend that we never even called r.propose. The proposal will then fail up to the caller, and this is probably not a particularly well-tested code path anyway, but it will not be reproposed and so it shouldn't matter what the MLAI and prev encoded command was. But it felt right to restore the previous state, to avoid having to reason about yet one more code path at the caller.

I'm not defending this code, btw - this is the clearest I was able to make it without really stirring the pot. I wish we didn't have these below-raft reproposals and would "just" delegate to an above-raft goroutine pool to repropose on behalf of clients who used pipelined writes. I bet that would be tricky in its own right, but conceptually clearer.

but why is an error here treated differently than an error when flushing the proposal buffer back into the proposals map?

I might be wrong, but: if we catch an error flushing the buffer, we return it up:

cockroach/pkg/kv/kvserver/replica_proposal_buf.go

Line 540 in a94858b

return 0, firstErr

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 727 to 728 in b637f03

	err := r.withRaftGroupLocked(true, func(raftGroup *raft.RawNode) (bool, error) {
	numFlushed, err := r.mu.proposalBuf.FlushLockedWithRaftGroup(ctx, raftGroup)

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 756 to 758 in b637f03

	} else if err != nil {
	return stats, errors.Wrap(err, "checking raft group for Ready")
	}

and up:

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 1110 to 1113 in b637f03

	default:
	log.FatalfDepth(ctx, 1, "%+v", err)
	panic("unreachable")
	}

basically flushing the buffer "has to work". But in this method, we can run into the closed timestamp, so it has to be fallible and we need to return an error to the caller.

Thanks for explaining. This makes sense. If you wanted to add another sentence to the comment stating that an error to propose causes the request to fail while a success means that it's back in the proposal pipeline, I'd be supportive. It's also fine if not.

[tbg]

bors r=nvanbenschoten

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

---

pkg/kv/kvserver/replica_application_result.go line 259 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Thanks for explaining. This makes sense. If you wanted to add another sentence to the comment stating that an error to propose causes the request to fail while a success means that it's back in the proposal pipeline, I'd be supportive. It's also fine if not.

Good idea., done.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)