Skip to content

Commit

Permalink
roachpb: set isRead for DeleteRequest
Browse files Browse the repository at this point in the history 
`DeleteRequest` performs a read to determine whether an existing, live
key was deleted, in order to populate the `DeleteResponse.FoundKey`
field. It must therefore have the `isRead` flag in order to avoid
deferring the `WriteTooOldError` and returning a possibly incorrect
value in the case of a write-write conflict (if the delete was submitted
below a newer key).

This has no externally visible effect on KV callers, because the
`WriteTooOldError` is retried on the client side (in
`txnSpanRefresher`), but it does violate the server contract. The flag
may also avoid an unnecessary Raft round trip to lay down an intent that
would be replaced later anyway.

Release note: None
  • Loading branch information
@erikgrinaker
erikgrinaker committed Oct 25, 2022
1 parent c5decae commit 45d609a
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 3 deletions.
6 changes: 5 additions & 1 deletion pkg/roachpb/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1281,7 +1281,11 @@ func (*IncrementRequest) flags() flag {
}

func (*DeleteRequest) flags() flag {
return isWrite | isTxn | isLocking | isIntentWrite | appliesTSCache | canBackpressure
// isRead because of the FoundKey boolean in the response, indicating whether
// an existing key was deleted at the read timestamp. isIntentWrite allows
// omitting needsRefresh. For background, see:
// https://github.com/cockroachdb/cockroach/pull/89375
return isRead | isWrite | isTxn | isLocking | isIntentWrite | appliesTSCache | canBackpressure
}

func (drr *DeleteRangeRequest) flags() flag {
Expand Down
8 changes: 6 additions & 2 deletions pkg/storage/mvcc_history_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1075,11 +1075,15 @@ func cmdDelete(e *evalCtx) error {
localTs := hlc.ClockTimestamp(e.getTsWithName("localTs"))
resolve, resolveStatus := e.getResolve()
return e.withWriter("del", func(rw storage.ReadWriter) error {
deletedKey, err := storage.MVCCDelete(e.ctx, rw, e.ms, key, ts, localTs, txn)
foundKey, err := storage.MVCCDelete(e.ctx, rw, e.ms, key, ts, localTs, txn)
if err == nil || errors.HasType(err, &roachpb.WriteTooOldError{}) {
// We want to output foundKey even if a WriteTooOldError is returned,
// since the error may be swallowed/deferred during evaluation.
e.results.buf.Printf("del: %v: found key %v\n", key, foundKey)
}
if err != nil {
return err
}
e.results.buf.Printf("del: %v: found key %v\n", key, deletedKey)
if resolve {
return e.resolveIntent(rw, key, txn, resolveStatus, hlc.ClockTimestamp{})
}
Expand Down
158 changes: 158 additions & 0 deletions pkg/storage/testdata/mvcc_histories/deletes
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,161 @@ data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>


# Try deleting historical versions using a txn with an old read timestamp, and
# ensure "found key" is reported correctly for various timestamps along with the
# WriteTooOldError (which may be deferred).
run error
with t=A
txn_begin ts=43
txn_advance ts=50
del k=a
----
del: "a": found key false
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=43.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>
error: (*roachpb.WriteTooOldError:) WriteTooOldError: write for key "a" at timestamp 43.000000000,0 too old; wrote at 50.000000000,0

run ok
with t=A
resolve_intent k=a status=ABORTED
txn_remove
----
>> at end:
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>

run error
with t=A
txn_begin ts=44
txn_advance ts=50
del k=a
----
del: "a": found key false
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=44.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>
error: (*roachpb.WriteTooOldError:) WriteTooOldError: write for key "a" at timestamp 44.000000000,0 too old; wrote at 50.000000000,0

run ok
with t=A
resolve_intent k=a status=ABORTED
txn_remove
----
>> at end:
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>

run error
with t=A
txn_begin ts=46
txn_advance ts=50
del k=a
----
del: "a": found key true
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=46.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>
error: (*roachpb.WriteTooOldError:) WriteTooOldError: write for key "a" at timestamp 46.000000000,0 too old; wrote at 50.000000000,0

run ok
with t=A
resolve_intent k=a status=ABORTED
txn_remove
----
>> at end:
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>

run error
with t=A
txn_begin ts=47
txn_advance ts=50
del k=a
----
del: "a": found key false
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=47.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>
error: (*roachpb.WriteTooOldError:) WriteTooOldError: write for key "a" at timestamp 47.000000000,0 too old; wrote at 50.000000000,0

run ok
with t=A
resolve_intent k=a status=ABORTED
txn_remove
----
>> at end:
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>

run ok
with t=A
txn_begin ts=49
txn_advance ts=50
del k=a
----
del: "a": found key false
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=49.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>

# Delete an inline value, both missing and existing.
run ok
del k=i
put k=i v=inline
del k=i
----
del: "i": found key false
del: "i": found key true
>> at end:
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=50.000000000,0 min=0,0 seq=0} ts=50.000000000,0 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
data: "a"/50.000000000,0 -> /<empty>
data: "a"/48.000000000,0 -> /<empty>
data: "a"/47.000000000,0 -> /<empty>
data: "a"/46.000000000,0 -> /BYTES/abc
data: "a"/44.000000000,0 -> /<empty>
data: "b"/49.000000000,0 -> /<empty>
1 change: 1 addition & 0 deletions pkg/storage/testdata/mvcc_histories/write_too_old
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ with t=A
txn_begin ts=33
del k=a
----
del: "a": found key false
>> at end:
txn: "A" meta={id=00000000 key=/Min pri=0.00000000 epo=0 ts=33.000000000,0 min=0,0 seq=0} lock=true stat=PENDING rts=33.000000000,0 wto=false gul=0,0
meta: "a"/0,0 -> txn={id=00000000 key=/Min pri=0.00000000 epo=0 ts=44.000000000,1 min=0,0 seq=0} ts=44.000000000,1 del=true klen=12 vlen=0 mergeTs=<nil> txnDidNotUpdateMeta=true
Expand Down

0 comments on commit 45d609a

Please sign in to comment.