roachpb: split Intent proto message into Intent and LockUpdate messages

[nvanbenschoten]

The Intent message type has been a frequent source of complexity and confusion. @knz found this out first-hand in #42152/#42582 when he tried to add an IgnoredSeqNums field to the message type. This led to a series of questions that were not easily answered by simply reading about the message type, like:

• why would an Intent ever have a non-PENDING status?
• why would an Intent ever span multiple keys?
• where are Intents created?
• is this Intent message containing the authoritative state of the txn?
• does this Intent message have a populated IgnoredSeqNums slice?

The root problem was that the message type was serving two roles, as was documented in its comment. It both referenced on-disk write intents and served as a way to talk about updates that should be made to these write intents.

This commit addresses this issue by splitting Intent into two (wire-compatible) messages with distinct purposes - Intent and LockUpdate. Intent's new sole purpose is now to reference persistent on-disk write intents on return paths of MVCC operations. They are only ever created when on-disk intents are encountered. LockUpdate's sole purpose is to represent updates that are intended for all locks held by the transaction within its span. They are only ever created after their transaction's record has been consulted, either after an EndTxn or a PushTxn request. This split simplifies the use of each of these types and helps make the questions above easier to answer or impossible to even ask (e.g. Intent no longer has a Status field).

While here, the commit adds a durability field to LockUpdate. This is used to inform the message's recipient about the durability of the lock(s) it is being instructed to update. This will be important soon when #44976 formally introduces unreplicated locks to the key-value layer.

I would suggest that reviewers start with the proto changes in pkg/roachpb and fan-out from there. Everything else is essentially fallout from the typing change. This is still a bit of a WIP as there is more renaming necessary before this could be merged, but I want to get people's opinions on this sooner rather than later because it's partly on the critical path for #44976 and I'm feeling a little pressed for time with that change and the impending stability period.

[cockroach-teamcity]

This change is 

[tbg]

given how this PR sprays, I'd say go ahead and merge this. You can address my comments after.

Reviewed 42 of 42 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/roachpb/api.go, line 1284 at r1 (raw file):

		Status:         rir.Status,
		IgnoredSeqNums: rir.IgnoredSeqNums,
		Durability:     lock.Replicated,

Will we ever allow making a replicated lock unreplicated? Just curious.

---

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

}

// MakePendingIntent makes an intent in the pending state with the

The reference here to the pending state confused me until I realized that you want to drive the point home that an Intent always describes a pending intent (i.e. there is no status on, it but it's implicitly PENDING). Are you really trying to rename Intent to PendingIntent? If this is not appropriate, I wonder if we should drop the Pending and rely on the comment in data.proto to drive that point home.

---

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

  // NB: now that LockUpdate is split away from this message type, this span
  // field always contains a single-key. We could migrate it, but doing so
  // doesn't seem worth it.

Can we replace span below with the wire-compatible

// SingleKeySpan preseves wire compatibility with an earlier version of this proto which
// used a roachpb.Span.xxxxx
message SingleKeySpan {
  option (gogoproto.equal) = true;

  option (gogoproto.goproto_stringer) = false;
  option (gogoproto.populate) = true;

  reserved 1, 2, 4;
  // The start key of the key range.
  bytes key = 3 [(gogoproto.casttype) = "Key"];
}
SingleKeySpan span = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];

and avoid any kind of real migration? I think that might be worth it, to avoid having an EndKey around which will invite some of the confusion you're generally eliminating here.

[nvanbenschoten]

Thanks for the quick review!

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

---

pkg/roachpb/api.go, line 1284 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Will we ever allow making a replicated lock unreplicated? Just curious.

I think this would be possible if you roll back a replicated lock and then replace it with an unreplicated lock. But I don't think we'll ever need to support it for a single sequence number.

---

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

Previously, tbg (Tobias Grieger) wrote…

The reference here to the pending state confused me until I realized that you want to drive the point home that an Intent always describes a pending intent (i.e. there is no status on, it but it's implicitly PENDING). Are you really trying to rename Intent to PendingIntent? If this is not appropriate, I wonder if we should drop the Pending and rely on the comment in data.proto to drive that point home.

Done.

---

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

Previously, tbg (Tobias Grieger) wrote…

Can we replace span below with the wire-compatible

// SingleKeySpan preseves wire compatibility with an earlier version of this proto which
// used a roachpb.Span.xxxxx
message SingleKeySpan {
  option (gogoproto.equal) = true;

  option (gogoproto.goproto_stringer) = false;
  option (gogoproto.populate) = true;

  reserved 1, 2, 4;
  // The start key of the key range.
  bytes key = 3 [(gogoproto.casttype) = "Key"];
}
SingleKeySpan span = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];

and avoid any kind of real migration? I think that might be worth it, to avoid having an EndKey around which will invite some of the confusion you're generally eliminating here.

Done.

[nvanbenschoten]

bors r+

[craig]

Merge conflict (retrying...)

[craig]

Merge conflict (retrying...)

[craig]

Merge conflict (retrying...)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Merge conflict

[nvanbenschoten]

bors r+

[nvanbenschoten]

Cancelled

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)