From 6dad7b6f3ba64adebf5977820a965df538f11eaf Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 28 Jun 2018 15:18:00 -0400 Subject: [PATCH] storage: return TransactionRetryError from QueryIntent on pushed intent This change adjusts how QueryIntent handles pushed intents. First, it changes how pushed intents interact with SNAPSHOT transactions. Next, it makes sure to update the response transaction in the case of pushed intents. Finally, it returns a RETRY_SERIALIZABLE TransactionRetryError for SERIALIZABLE transactions who observe a pushed intent with the RETURN_ERROR behavior. Release note: None --- pkg/roachpb/api.pb.go | 13 +- pkg/roachpb/api.proto | 13 +- pkg/storage/batcheval/cmd_query_intent.go | 33 +++- pkg/storage/replica_test.go | 231 ++++++++++++---------- 4 files changed, 175 insertions(+), 115 deletions(-) diff --git a/pkg/roachpb/api.pb.go b/pkg/roachpb/api.pb.go index fa36510e8579..20db7f229091 100644 --- a/pkg/roachpb/api.pb.go +++ b/pkg/roachpb/api.pb.go @@ -374,7 +374,9 @@ const ( // Don't do anything special, just note that the intent was not found in the // response. QueryIntentRequest_DO_NOTHING QueryIntentRequest_IfMissingBehavior = 0 - // Return an IntentMissingError. + // Return an IntentMissingError. Special-cased to return a SERIALIZABLE + // retry error if a SERIALIZABLE transaction queries its own intent and + // finds it has been pushed. QueryIntentRequest_RETURN_ERROR QueryIntentRequest_IfMissingBehavior = 1 // Prevent the intent from ever being written in the future. If set as the // behavior, a response with found_intent=false implies that an intent will @@ -1254,9 +1256,12 @@ type QueryIntentRequest struct { // be committed by the provided transaction. If an intent is found at the // specified key, the intent is only considered a match if it has the same ID, // the same epoch, and a provisional commit timestamp that is equal to or less - // than that in the provided transaction. If the intent's timestamp is greater - // than that in the provided transaction, it would prevent the transaction - // from committing and is therefore not a match. + // than that in the provided transaction. For SERIALIZABLE transactions, if + // the intent's timestamp is greater than that in the provided transaction, it + // would prevent the transaction from committing and is therefore not a match. + // However, for SNAPSHOT transactions, if the intent's timestamp is greater + // than that in the provided transaction, it would not prevent the transaction + // from committing and therefore is a match. // // Additionally, the intent is only considered a match if its sequence number // is equal to or greater than the expected txn's sequence number. The diff --git a/pkg/roachpb/api.proto b/pkg/roachpb/api.proto index 59abbd0e9c22..5d07f3a126e0 100644 --- a/pkg/roachpb/api.proto +++ b/pkg/roachpb/api.proto @@ -749,9 +749,12 @@ message QueryIntentRequest { // be committed by the provided transaction. If an intent is found at the // specified key, the intent is only considered a match if it has the same ID, // the same epoch, and a provisional commit timestamp that is equal to or less - // than that in the provided transaction. If the intent's timestamp is greater - // than that in the provided transaction, it would prevent the transaction - // from committing and is therefore not a match. + // than that in the provided transaction. For SERIALIZABLE transactions, if + // the intent's timestamp is greater than that in the provided transaction, it + // would prevent the transaction from committing and is therefore not a match. + // However, for SNAPSHOT transactions, if the intent's timestamp is greater + // than that in the provided transaction, it would not prevent the transaction + // from committing and therefore is a match. // // Additionally, the intent is only considered a match if its sequence number // is equal to or greater than the expected txn's sequence number. The @@ -767,7 +770,9 @@ message QueryIntentRequest { // Don't do anything special, just note that the intent was not found in the // response. DO_NOTHING = 0; - // Return an IntentMissingError. + // Return an IntentMissingError. Special-cased to return a SERIALIZABLE + // retry error if a SERIALIZABLE transaction queries its own intent and + // finds it has been pushed. RETURN_ERROR = 1; // Prevent the intent from ever being written in the future. If set as the // behavior, a response with found_intent=false implies that an intent will diff --git a/pkg/storage/batcheval/cmd_query_intent.go b/pkg/storage/batcheval/cmd_query_intent.go index 21f40a7d3c0f..7f6a1468ff1d 100644 --- a/pkg/storage/batcheval/cmd_query_intent.go +++ b/pkg/storage/batcheval/cmd_query_intent.go @@ -38,6 +38,7 @@ func QueryIntent( ctx context.Context, batch engine.ReadWriter, cArgs CommandArgs, resp roachpb.Response, ) (result.Result, error) { args := cArgs.Args.(*roachpb.QueryIntentRequest) + h := cArgs.Header reply := resp.(*roachpb.QueryIntentResponse) // Snapshot transactions cannot be prevented using a QueryIntent command. @@ -65,7 +66,11 @@ func QueryIntent( return result.Result{}, err } + // Determine if the request is querying an intent in its own transaction. + ownTxn := h.Txn != nil && h.Txn.ID == args.Txn.ID + var curIntent *roachpb.Intent + var curIntentPushed bool switch len(intents) { case 0: reply.FoundIntent = false @@ -75,8 +80,27 @@ func QueryIntent( // comparison. reply.FoundIntent = (args.Txn.ID == curIntent.Txn.ID) && (args.Txn.Epoch == curIntent.Txn.Epoch) && - (!args.Txn.Timestamp.Less(curIntent.Txn.Timestamp)) && (args.Txn.Sequence <= curIntent.Txn.Sequence) + + // Check whether the intent was pushed past its expected timestamp. + if reply.FoundIntent && args.Txn.Timestamp.Less(curIntent.Txn.Timestamp) { + // The intent matched but was pushed to a later timestamp. + curIntentPushed = true + + // If the transaction is SERIALIZABLE, consider a pushed intent as a + // missing intent. If the transaction is SNAPSHOT, don't. + if args.Txn.Isolation == enginepb.SERIALIZABLE { + reply.FoundIntent = false + } + + // If the request was querying an intent in its own transaction, update + // the response transaction. + if ownTxn { + clonedTxn := h.Txn.Clone() + reply.Txn = &clonedTxn + reply.Txn.Timestamp.Forward(curIntent.Txn.Timestamp) + } + } default: log.Fatalf(ctx, "more than 1 intent on single key: %v", intents) } @@ -86,6 +110,13 @@ func QueryIntent( case roachpb.QueryIntentRequest_DO_NOTHING: // Do nothing. case roachpb.QueryIntentRequest_RETURN_ERROR: + if ownTxn && curIntentPushed { + // If the transaction's own intent was pushed, go ahead and + // return a TransactionRetryError immediately with an updated + // transaction proto. This is an optimization that can help + // the txn use refresh spans more effectively. + return result.Result{}, roachpb.NewTransactionRetryError(roachpb.RETRY_SERIALIZABLE) + } return result.Result{}, roachpb.NewIntentMissingError(curIntent) case roachpb.QueryIntentRequest_PREVENT: // The intent will be prevented by bumping the timestamp cache for diff --git a/pkg/storage/replica_test.go b/pkg/storage/replica_test.go index 04314c95375b..b6502134d8c3 100644 --- a/pkg/storage/replica_test.go +++ b/pkg/storage/replica_test.go @@ -5791,120 +5791,139 @@ func TestPushTxnSerializableRestart(t *testing.T) { func TestQueryIntentRequest(t *testing.T) { defer leaktest.AfterTest(t)() - for _, behavior := range []roachpb.QueryIntentRequest_IfMissingBehavior{ - roachpb.QueryIntentRequest_DO_NOTHING, - roachpb.QueryIntentRequest_RETURN_ERROR, - roachpb.QueryIntentRequest_PREVENT, - } { - t.Run(fmt.Sprintf("behavior=%s", behavior), func(t *testing.T) { - tc := testContext{} - stopper := stop.NewStopper() - defer stopper.Stop(context.TODO()) - tc.Start(t, stopper) + for _, iso := range []enginepb.IsolationType{enginepb.SNAPSHOT, enginepb.SERIALIZABLE} { + t.Run(iso.String(), func(t *testing.T) { + for _, behavior := range []roachpb.QueryIntentRequest_IfMissingBehavior{ + roachpb.QueryIntentRequest_DO_NOTHING, + roachpb.QueryIntentRequest_RETURN_ERROR, + roachpb.QueryIntentRequest_PREVENT, + } { + if iso == enginepb.SNAPSHOT && behavior == roachpb.QueryIntentRequest_PREVENT { + // Cannot prevent SNAPSHOT transaction with QueryIntent. + continue + } + t.Run(fmt.Sprintf("behavior=%s", behavior), func(t *testing.T) { - key1 := roachpb.Key("a") - key2 := roachpb.Key("b") - txn := newTransaction("test", key1, 1, enginepb.SERIALIZABLE, tc.Clock()) + tc := testContext{} + stopper := stop.NewStopper() + defer stopper.Stop(context.TODO()) + tc.Start(t, stopper) - pArgs := putArgs(key1, []byte("value1")) - assignSeqNumsForReqs(txn, &pArgs) - if _, pErr := tc.SendWrappedWith(roachpb.Header{Txn: txn}, &pArgs); pErr != nil { - t.Fatal(pErr) - } + key1 := roachpb.Key("a") + key2 := roachpb.Key("b") + txn := newTransaction("test", key1, 1, iso, tc.Clock()) + txn2 := newTransaction("test2", key2, 1, iso, tc.Clock()) - queryIntent := func( - key []byte, - txnMeta enginepb.TxnMeta, - baTxn *roachpb.Transaction, - expectIntent bool, - ) { - t.Helper() - qiArgs := queryIntentArgs(key, txnMeta, behavior) - qiRes, pErr := tc.SendWrappedWith(roachpb.Header{Txn: baTxn}, &qiArgs) - if behavior == roachpb.QueryIntentRequest_RETURN_ERROR && !expectIntent { - if _, ok := pErr.GetDetail().(*roachpb.IntentMissingError); !ok { - t.Fatalf("expected IntentMissingError, found %v", pErr) + pArgs := putArgs(key1, []byte("value1")) + assignSeqNumsForReqs(txn, &pArgs) + if _, pErr := tc.SendWrappedWith(roachpb.Header{Txn: txn}, &pArgs); pErr != nil { + t.Fatal(pErr) } - } else { + + queryIntent := func( + key []byte, + txnMeta enginepb.TxnMeta, + baTxn *roachpb.Transaction, + expectIntent bool, + ) { + t.Helper() + qiArgs := queryIntentArgs(key, txnMeta, behavior) + qiRes, pErr := tc.SendWrappedWith(roachpb.Header{Txn: baTxn}, &qiArgs) + if behavior == roachpb.QueryIntentRequest_RETURN_ERROR && !expectIntent { + ownIntent := baTxn != nil && baTxn.ID == txnMeta.ID + if ownIntent && txnMeta.Timestamp.Less(txn.Timestamp) { + if _, ok := pErr.GetDetail().(*roachpb.TransactionRetryError); !ok { + t.Fatalf("expected TransactionRetryError, found %v %v", txnMeta, pErr) + } + } else { + if _, ok := pErr.GetDetail().(*roachpb.IntentMissingError); !ok { + t.Fatalf("expected IntentMissingError, found %v", pErr) + } + } + } else { + if pErr != nil { + t.Fatal(pErr) + } + if e, a := expectIntent, qiRes.(*roachpb.QueryIntentResponse).FoundIntent; e != a { + t.Fatalf("expected FoundIntent=%t but FoundIntent=%t", e, a) + } + } + } + + for _, baTxn := range []*roachpb.Transaction{nil, txn, txn2} { + // Query the intent with the correct txn meta. Should see intent regardless + // of whether we're inside the txn or not. + queryIntent(key1, txn.TxnMeta, baTxn, true) + + // Query an intent on a different key for the same transaction. Should not + // see an intent. + queryIntent(key2, txn.TxnMeta, baTxn, false) + + // Query an intent on the same key for a different transaction. Should not + // see an intent. + diffIDMeta := txn.TxnMeta + diffIDMeta.ID = txn2.ID + queryIntent(key1, diffIDMeta, baTxn, false) + + // Query the intent with a larger epoch. Should not see an intent. + largerEpochMeta := txn.TxnMeta + largerEpochMeta.Epoch++ + queryIntent(key1, largerEpochMeta, baTxn, false) + + // Query the intent with a smaller epoch. Should not see an intent. + smallerEpochMeta := txn.TxnMeta + smallerEpochMeta.Epoch-- + queryIntent(key1, smallerEpochMeta, baTxn, false) + + // Query the intent with a larger timestamp. Should see an intent. + // See the comment on QueryIntentRequest.Txn for an explanation of why + // the request behaves like this. + largerTSMeta := txn.TxnMeta + largerTSMeta.Timestamp = largerTSMeta.Timestamp.Next() + queryIntent(key1, largerTSMeta, baTxn, true) + + // Query the intent with a smaller timestamp. Should not see an intent + // if transaction is serializable. Should see an intent if the transaction + // is SNAPSHOT. + smallerTSMeta := txn.TxnMeta + smallerTSMeta.Timestamp = smallerTSMeta.Timestamp.Prev() + queryIntent(key1, smallerTSMeta, baTxn, iso == enginepb.SNAPSHOT) + + // Query the intent with a larger sequence number. Should not see an intent. + largerSeqMeta := txn.TxnMeta + largerSeqMeta.Sequence++ + queryIntent(key1, largerSeqMeta, baTxn, false) + + // Query the intent with a smaller sequence number. Should see an intent. + // See the comment on QueryIntentRequest.Txn for an explanation of why + // the request behaves like this. + smallerSeqMeta := txn.TxnMeta + smallerSeqMeta.Sequence-- + queryIntent(key1, smallerSeqMeta, baTxn, true) + } + + // Perform a write at key2. Depending on the behavior of the queryIntent + // that queried that key, this write should have different results. + pArgs2 := putArgs(key2, []byte("value2")) + assignSeqNumsForReqs(txn, &pArgs2) + ba := roachpb.BatchRequest{} + ba.Header = roachpb.Header{Txn: txn} + ba.Add(&pArgs2) + br, pErr := tc.Sender().Send(context.Background(), ba) if pErr != nil { t.Fatal(pErr) } - if e, a := expectIntent, qiRes.(*roachpb.QueryIntentResponse).FoundIntent; e != a { - t.Fatalf("expected FoundIntent=%T but FoundIntent=%T", e, a) + tsBumped := br.Txn.Timestamp != br.Txn.OrigTimestamp + if behavior == roachpb.QueryIntentRequest_PREVENT { + if !tsBumped { + t.Fatalf("transaction timestamp not bumped: %v", br.Txn) + } + } else { + if tsBumped { + t.Fatalf("unexpected transaction timestamp bumped: %v", br.Txn) + } } - } - } - - for _, baTxn := range []*roachpb.Transaction{nil, txn} { - // Query the intent with the correct txn meta. Should see intent regardless - // of whether we're inside the txn or not. - queryIntent(key1, txn.TxnMeta, baTxn, true) - - // Query an intent on a different key for the same transaction. Should not - // see an intent. - queryIntent(key2, txn.TxnMeta, baTxn, false) - - // Query an intent on the same key for a different transaction. Should not - // see an intent. - diffIDMeta := txn.TxnMeta - diffIDMeta.ID = uuid.MakeV4() - queryIntent(key1, diffIDMeta, baTxn, false) - - // Query the intent with a larger epoch. Should not see an intent. - largerEpochMeta := txn.TxnMeta - largerEpochMeta.Epoch++ - queryIntent(key1, largerEpochMeta, baTxn, false) - - // Query the intent with a smaller epoch. Should not see an intent. - smallerEpochMeta := txn.TxnMeta - smallerEpochMeta.Epoch-- - queryIntent(key1, smallerEpochMeta, baTxn, false) - - // Query the intent with a larger timestamp. Should see an intent. - // See the comment on QueryIntentRequest.Txn for an explanation of why - // the request behaves like this. - largerTSMeta := txn.TxnMeta - largerTSMeta.Timestamp = largerTSMeta.Timestamp.Next() - queryIntent(key1, largerTSMeta, baTxn, true) - - // Query the intent with a smaller timestamp. Should not see an intent. - smallerTSMeta := txn.TxnMeta - smallerTSMeta.Timestamp = smallerTSMeta.Timestamp.Prev() - queryIntent(key1, smallerTSMeta, baTxn, false) - - // Query the intent with a larger sequence number. Should not see an intent. - largerSeqMeta := txn.TxnMeta - largerSeqMeta.Sequence++ - queryIntent(key1, largerSeqMeta, baTxn, false) - - // Query the intent with a smaller sequence number. Should see an intent. - // See the comment on QueryIntentRequest.Txn for an explanation of why - // the request behaves like this. - smallerSeqMeta := txn.TxnMeta - smallerSeqMeta.Sequence-- - queryIntent(key1, smallerSeqMeta, baTxn, true) - } - - // Perform a write at key2. Depending on the behavior of the queryIntent - // that queried that key, this write should have different results. - pArgs2 := putArgs(key2, []byte("value2")) - assignSeqNumsForReqs(txn, &pArgs2) - ba := roachpb.BatchRequest{} - ba.Header = roachpb.Header{Txn: txn} - ba.Add(&pArgs2) - br, pErr := tc.Sender().Send(context.Background(), ba) - if pErr != nil { - t.Fatal(pErr) - } - tsBumped := br.Txn.Timestamp != br.Txn.OrigTimestamp - if behavior == roachpb.QueryIntentRequest_PREVENT { - if !tsBumped { - t.Fatalf("transaction timestamp not bumped: %v", br.Txn) - } - } else { - if tsBumped { - t.Fatalf("unexpected transaction timestamp bumped: %v", br.Txn) - } + }) } }) }