From 45d609aaec560834662452cd2b7a88275cb2f418 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Thu, 20 Oct 2022 10:59:10 +0000 Subject: [PATCH] roachpb: set `isRead` for `DeleteRequest` `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 --- pkg/roachpb/api.go | 6 +- pkg/storage/mvcc_history_test.go | 8 +- pkg/storage/testdata/mvcc_histories/deletes | 158 ++++++++++++++++++ .../testdata/mvcc_histories/write_too_old | 1 + 4 files changed, 170 insertions(+), 3 deletions(-) diff --git a/pkg/roachpb/api.go b/pkg/roachpb/api.go index f72eb145be31..f32d69394125 100644 --- a/pkg/roachpb/api.go +++ b/pkg/roachpb/api.go @@ -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 { diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index 08adc1e976f5..f090fb37f118 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -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{}) } diff --git a/pkg/storage/testdata/mvcc_histories/deletes b/pkg/storage/testdata/mvcc_histories/deletes index 47772257bddf..ef8725fa5160 100644 --- a/pkg/storage/testdata/mvcc_histories/deletes +++ b/pkg/storage/testdata/mvcc_histories/deletes @@ -84,3 +84,161 @@ data: "a"/47.000000000,0 -> / data: "a"/46.000000000,0 -> /BYTES/abc data: "a"/44.000000000,0 -> / data: "b"/49.000000000,0 -> / + + +# 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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / +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 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / + +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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / +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 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / + +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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / +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 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / + +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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / +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 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / + +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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / + +# 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= txnDidNotUpdateMeta=true +data: "a"/50.000000000,0 -> / +data: "a"/48.000000000,0 -> / +data: "a"/47.000000000,0 -> / +data: "a"/46.000000000,0 -> /BYTES/abc +data: "a"/44.000000000,0 -> / +data: "b"/49.000000000,0 -> / diff --git a/pkg/storage/testdata/mvcc_histories/write_too_old b/pkg/storage/testdata/mvcc_histories/write_too_old index 6ad2e566d5a9..c2dbd69193c0 100644 --- a/pkg/storage/testdata/mvcc_histories/write_too_old +++ b/pkg/storage/testdata/mvcc_histories/write_too_old @@ -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= txnDidNotUpdateMeta=true