Skip to content

Commit

Permalink
engine: set the MVCC timestamp on reads due to historical intents
Browse files Browse the repository at this point in the history 
Before this commit, we'd return a zero-value MVCC timestamp when reading an
intent from the intent history. This was problematic because elsewhere in the
code we assume that we will always get a non-zero MVCC timestamp when a read
returns a value. This is especially bizarre given that a read of the latest
intent will return its write timestamp.

The semantics here are such that we'll return the timestamp of the MVCC
metadata for the row. I think this is the most reasonable thing to do as
that timestamp ought to reflect the timestamp we return when we return the
current intent and furthermore is the only timestamp we really have around.
We could return the transactions current read or write timestamp but those
both seem like worse choices.

It's also worth noting that in the case where we need to read a non-zero
value, we don't really care what that value is and the fact that we are
reading this intent itself is somewhat suspect. That being said, I still
think we should make this change in addition to any change made to prevent
the code referenced in #49266 from needing it.

Fixes #49266.
Informs #50102.

Release note: None
  • Loading branch information
@ajwerner
ajwerner committed Sep 29, 2020
1 parent 5a4e410 commit deb3ced
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 34 deletions.
16 changes: 15 additions & 1 deletion c-deps/libroach/mvcc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,21 @@ template <bool reverse> class mvccScanner {

rocksdb::Slice value = intent.value();
if (value.size() > 0 || tombstones_) {
kvs_->Put(cur_raw_key_, value);
// If we're adding a value due to a previous intent, as indicated by the
// zero-valued timestamp, we want to populate the timestamp as of current
// metaTimestamp. Note that this may be controversial as this maybe be
// neither the write timestamp when this intent was written. However, this
// was the only case in which a value could have been returned from a read
// without an MVCC timestamp.
if (cur_timestamp_ == kZeroTimestamp) {
auto meta_timestamp = meta_.timestamp();
auto key = EncodeKey(cur_key_,
meta_timestamp.wall_time(),
meta_timestamp.logical());
kvs_->Put(key, value);
} else {
kvs_->Put(cur_raw_key_, value);
}
}
return true;
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/storage/pebble_mvcc_scanner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,17 @@ func (p *pebbleMVCCScanner) getFromIntentHistory() bool {
}
intent := p.meta.IntentHistory[upIdx-1]
if len(intent.Value) > 0 || p.tombstones {
p.results.put(p.curMVCCKey(), intent.Value)
key := p.curMVCCKey()
// If we're adding a value due to a previous intent, as indicated by the
// zero-valued timestamp, we want to populate the timestamp as of current
// metaTimestamp. Note that this may be controversial as this maybe be
// neither the write timestamp when this intent was written. However, this
// was the only case in which a value could have been returned from a read
// without an MVCC timestamp.
if key.Timestamp.IsEmpty() {
key.Timestamp = hlc.Timestamp(p.meta.Timestamp)
}
p.results.put(key, intent.Value)
}
return true
}
Expand Down
34 changes: 17 additions & 17 deletions pkg/storage/testdata/mvcc_histories/ignored_seq_nums
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ with t=A
txn_ignore_seqs seqs=(29-30)
get k=k
----
scan: "k" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "k/10" -> /BYTES/10 @0.000000011,0
scan: "k/20" -> /BYTES/20 @0.000000011,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0.000000011,0
get: "k" -> /BYTES/b @0.000000011,0
get: "k" -> /BYTES/b @0.000000011,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=40} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=1

Expand Down Expand Up @@ -87,9 +87,9 @@ with t=A
scan k=k end=-k
get k=k
----
scan: "k" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "k/20" -> /BYTES/20 @0.000000011,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0.000000011,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=40} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=2

Expand All @@ -103,9 +103,9 @@ with t=A
scan k=k end=-k
get k=k
----
scan: "k" -> /BYTES/a @0,0
scan: "k" -> /BYTES/a @0.000000011,0
scan: "k/10" -> /BYTES/10 @0.000000011,0
get: "k" -> /BYTES/a @0,0
get: "k" -> /BYTES/a @0.000000011,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=12} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=1

Expand All @@ -119,9 +119,9 @@ with t=A
scan k=k end=-k
get k=k
----
scan: "k" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "k/20" -> /BYTES/20 @0.000000011,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0.000000011,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=22} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=1

Expand All @@ -135,10 +135,10 @@ with t=A
scan k=k end=-k
get k=k
----
scan: "k" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "k/10" -> /BYTES/10 @0.000000011,0
scan: "k/20" -> /BYTES/20 @0.000000011,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0.000000011,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=32} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=1

Expand All @@ -159,8 +159,8 @@ with t=A
get k=k
----
meta: "k" -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=30} ts=0.000000011,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}}
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0,0
get: "k" -> /BYTES/b @0.000000011,0
get: "k" -> /BYTES/b @0.000000011,0
meta: "k" -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000014,0 min=0,0 seq=20} ts=0.000000014,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}}
get: "k" -> /BYTES/b @0.000000014,0
>> at end:
Expand All @@ -184,10 +184,10 @@ with t=A
scan k=k end=-k
get k=k
----
scan: "k" -> /BYTES/a @0,0
scan: "k" -> /BYTES/a @0.000000014,0
scan: "k/10" -> /BYTES/10 @0.000000011,0
scan: "k/30" -> /BYTES/30 @0.000000011,0
get: "k" -> /BYTES/a @0,0
get: "k" -> /BYTES/a @0.000000014,0
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000014,0 min=0,0 seq=40} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0 isn=1

Expand Down Expand Up @@ -339,7 +339,7 @@ with t=C
resolve_intent k=m status=COMMITTED
----
meta: "m" -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000030,0 min=0,0 seq=30} ts=0.000000030,0 del=false klen=12 vlen=6 ih={{10 /BYTES/a}{20 /BYTES/b}}
get: "m" -> /BYTES/a @0,0
get: "m" -> /BYTES/a @0.000000030,0
>> at end:
txn: "C" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000030,0 min=0,0 seq=30} lock=true stat=PENDING rts=0.000000030,0 wto=false max=0,0 isn=1
data: "k"/0.000000014,0 -> /BYTES/b
Expand Down
18 changes: 9 additions & 9 deletions pkg/storage/testdata/mvcc_histories/max_keys
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ with t=A ts=11,0 max=3
scan k=k end=o
scan k=k end=o reverse=true
----
scan: "k" -> /BYTES/b @0,0
scan: "l" -> /BYTES/b @0,0
scan: "m" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "l" -> /BYTES/b @0.000000011,0
scan: "m" -> /BYTES/b @0.000000011,0
scan: resume span ["n","o")
scan: "n" -> /BYTES/b @0,0
scan: "m" -> /BYTES/b @0,0
scan: "l" -> /BYTES/b @0,0
scan: "n" -> /BYTES/b @0.000000011,0
scan: "m" -> /BYTES/b @0.000000011,0
scan: "l" -> /BYTES/b @0.000000011,0
scan: resume span ["k","k\x00")
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0
Expand Down Expand Up @@ -176,9 +176,9 @@ with t=B ts=12,0 max=3
txn_step seq=20
scan k=k end=o
----
scan: "k" -> /BYTES/b @0,0
scan: "l" -> /BYTES/b @0,0
scan: "m" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000012,0
scan: "l" -> /BYTES/b @0.000000012,0
scan: "m" -> /BYTES/b @0.000000012,0
scan: resume span ["n","o")
>> at end:
txn: "B" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000012,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000012,0 wto=false max=0,0
Expand Down
12 changes: 6 additions & 6 deletions pkg/storage/testdata/mvcc_histories/target_bytes
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,14 +272,14 @@ with t=A ts=11,0 targetbytes=32
scan k=k end=o
scan k=k end=o reverse=true
----
scan: "k" -> /BYTES/b @0,0
scan: "l" -> /BYTES/b @0,0
scan: "k" -> /BYTES/b @0.000000011,0
scan: "l" -> /BYTES/b @0.000000011,0
scan: resume span ["m","o")
scan: 32 bytes (target 32)
scan: "n" -> /BYTES/b @0,0
scan: "m" -> /BYTES/b @0,0
scan: 50 bytes (target 32)
scan: "n" -> /BYTES/b @0.000000011,0
scan: "m" -> /BYTES/b @0.000000011,0
scan: resume span ["k","l\x00")
scan: 32 bytes (target 32)
scan: 50 bytes (target 32)
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=0.000000011,0 min=0,0 seq=20} lock=true stat=PENDING rts=0.000000011,0 wto=false max=0,0
data: "a"/0.000000123,45 -> /BYTES/abcdef
Expand Down

0 comments on commit deb3ced

Please sign in to comment.