Merge #45235

45235: roachpb: split Intent proto message into Intent and LockUpdate messages r=nvanbenschoten a=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.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

Makefile

@@ -1202,7 +1202,7 @@ UI_JS_OSS := pkg/ui/src/js/protos.js
 UI_TS_OSS := pkg/ui/src/js/protos.d.ts
 UI_PROTOS_OSS := $(UI_JS_OSS) $(UI_TS_OSS)
 
-CPP_PROTOS := $(filter %/roachpb/metadata.proto %/roachpb/data.proto %/roachpb/internal.proto %/roachpb/errors.proto %/roachpb/api.proto %util/tracing/recorded_span.proto %/engine/enginepb/mvcc.proto %/engine/enginepb/mvcc3.proto %/engine/enginepb/file_registry.proto %/engine/enginepb/rocksdb.proto %/hlc/legacy_timestamp.proto %/hlc/timestamp.proto %/log/log.proto %/unresolved_addr.proto,$(GO_PROTOS))
+CPP_PROTOS := $(filter %/roachpb/api.proto %/roachpb/metadata.proto %/roachpb/data.proto %/roachpb/internal.proto %/roachpb/errors.proto %util/tracing/recorded_span.proto %/concurrency/lock/locking.proto %/engine/enginepb/mvcc.proto %/engine/enginepb/mvcc3.proto %/engine/enginepb/file_registry.proto %/engine/enginepb/rocksdb.proto %/hlc/legacy_timestamp.proto %/hlc/timestamp.proto %/log/log.proto %/unresolved_addr.proto,$(GO_PROTOS))
 CPP_HEADERS := $(subst ./pkg,$(CPP_PROTO_ROOT),$(CPP_PROTOS:%.proto=%.pb.h))
 CPP_SOURCES := $(subst ./pkg,$(CPP_PROTO_ROOT),$(CPP_PROTOS:%.proto=%.pb.cc))
 

c-deps/libroach/CMakeLists.txt

@@ -44,20 +44,21 @@ add_library(roach
   sst_dump.cc
   table_props.cc
   utils.cc
+  protos/roachpb/api.pb.cc
   protos/roachpb/data.pb.cc
   protos/roachpb/errors.pb.cc
   protos/roachpb/internal.pb.cc
   protos/roachpb/metadata.pb.cc
+  protos/storage/concurrency/lock/locking.pb.cc
   protos/storage/engine/enginepb/mvcc.pb.cc
   protos/storage/engine/enginepb/mvcc3.pb.cc
   protos/storage/engine/enginepb/file_registry.pb.cc
   protos/storage/engine/enginepb/rocksdb.pb.cc
   protos/util/hlc/legacy_timestamp.pb.cc
   protos/util/hlc/timestamp.pb.cc
   protos/util/log/log.pb.cc
-  protos/util/unresolved_addr.pb.cc
-  protos/roachpb/api.pb.cc
   protos/util/tracing/recorded_span.pb.cc
+  protos/util/unresolved_addr.pb.cc
   rocksdbutils/env_encryption.cc
 )
 target_include_directories(roach

c-deps/libroach/db.cc

@@ -573,8 +573,8 @@ DBIterState DBCheckForKeyCollisions(DBIterator* existingIter, DBIterator* sstIte
         // target key space, which will require appropriate resolution logic.
         cockroach::roachpb::WriteIntentError err;
         cockroach::roachpb::Intent* intent = err.add_intents();
-        intent->mutable_span()->set_key(existingIter->rep->key().data(),
-                                        existingIter->rep->key().size());
+        intent->mutable_single_key_span()->set_key(existingIter->rep->key().data(),
+                                                   existingIter->rep->key().size());
         intent->mutable_txn()->CopyFrom(meta.txn());
 
         *write_intent = ToDBString(err.SerializeAsString());

c-deps/libroach/incremental_iterator.cc

@@ -161,7 +161,7 @@ void DBIncrementalIterator::advanceKey() {
           !legacyTimestampIsLess(end_time, meta.timestamp())) {
         cockroach::roachpb::WriteIntentError err;
         cockroach::roachpb::Intent* intent = err.add_intents();
-        intent->mutable_span()->set_key(key.data(), key.size());
+        intent->mutable_single_key_span()->set_key(key.data(), key.size());
         intent->mutable_txn()->CopyFrom(meta.txn());
 
         status = ToDBString("WriteIntentError");