Skip to content

Commit

Permalink
roachpb: split Intent message into Intent and LockUpdate
Browse files Browse the repository at this point in the history
The Intent message type has been a frequent source of complexity and
confusion. cockroachdb#42152/cockroachdb#42582 found this out first hand when it 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 cockroachdb#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 cockroachdb#44976 and I'm feeling a little pressed for time with that change and
the impending stability period.
  • Loading branch information
nvanbenschoten committed Feb 21, 2020
1 parent aedf622 commit 6f81abe
Show file tree
Hide file tree
Showing 48 changed files with 2,147 additions and 593 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
5 changes: 3 additions & 2 deletions c-deps/libroach/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions c-deps/libroach/db.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion c-deps/libroach/incremental_iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Loading

0 comments on commit 6f81abe

Please sign in to comment.