From db1ee684d92467786e62c65341af53df92d74ab8 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 28 Mar 2019 16:57:27 -0400 Subject: [PATCH] storage: require QueryTxn requests set a sufficient timestamp This commit adds a check that QueryTxn requests set sufficiently high timestamps on their request header. This is in the spirit of #35297. Even though QueryTxn will never change the timestamp cache, I think there is a very rare hazard if it doesn't provide a timestamp at least as large as the transaction's timestamp in its batch header. If it doesn't do this, it seems possible that it could evaluate concurrently with a lease transfer (the lease transfer would have to start right after). The timestamp cache could then rotate pages a number of times until its low water mark is above the lease transfer timestamp (which is crazy as this would need to take at least 10s), and the QueryTxn could then consider a transaction ABORTED while evaluating because of the timestamp cache low water mark. The new leaseholder would have a lower timestamp cache low water mark and the transaction might still be able to create its transaction record. I don't think we would have ever seen this in the wild because of the 10s stall and the repeat timestamp cache rotations rotations, but this seems like a good change to make anyway. Release note: None --- docs/generated/settings/settings.html | 2 +- pkg/kv/txn_coord_sender_test.go | 1 + pkg/settings/cluster/cockroach_versions.go | 12 ++++++++++-- pkg/storage/batcheval/cmd_push_txn.go | 20 +++++++++++--------- pkg/storage/batcheval/cmd_query_txn.go | 12 +++++++++++- pkg/storage/batcheval/cmd_recover_txn.go | 4 ++-- pkg/storage/txnrecovery/manager.go | 1 + pkg/storage/txnwait/txnqueue.go | 1 + 8 files changed, 38 insertions(+), 15 deletions(-) diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index a460586ad829..6e69dd6b6d6b 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -121,6 +121,6 @@ trace.debug.enablebooleanfalseif set, traces for recent requests can be seen in the /debug page trace.lightstep.tokenstringif set, traces go to Lightstep using this token trace.zipkin.collectorstringif set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set -versioncustom validation19.1-1set the active cluster version in the format '.' +versioncustom validation19.1-2set the active cluster version in the format '.' diff --git a/pkg/kv/txn_coord_sender_test.go b/pkg/kv/txn_coord_sender_test.go index a6103b18abaa..5eedee9a0745 100644 --- a/pkg/kv/txn_coord_sender_test.go +++ b/pkg/kv/txn_coord_sender_test.go @@ -404,6 +404,7 @@ func getTxn(ctx context.Context, txn *client.Txn) (*roachpb.Transaction, *roachp } ba := roachpb.BatchRequest{} + ba.Timestamp = txnMeta.Timestamp ba.Add(qt) db := txn.DB() diff --git a/pkg/settings/cluster/cockroach_versions.go b/pkg/settings/cluster/cockroach_versions.go index 854f240b1dce..d2a423924575 100644 --- a/pkg/settings/cluster/cockroach_versions.go +++ b/pkg/settings/cluster/cockroach_versions.go @@ -69,6 +69,7 @@ const ( VersionSnapshotsWithoutLog Version19_1 VersionStart19_2 + VersionQueryTxnTimestamp // Add new versions here (step one of two). @@ -450,15 +451,22 @@ var versionsSingleton = keyedVersions([]keyedVersion{ // VersionSnapshotsWithoutLog is https://github.com/cockroachdb/cockroach/pull/36714. Key: VersionSnapshotsWithoutLog, Version: roachpb.Version{Major: 2, Minor: 1, Unstable: 11}, - }, { + }, + { // Version19_1 is CockroachDB v19.1. It's used for all v19.1.x patch releases. Key: Version19_1, Version: roachpb.Version{Major: 19, Minor: 1}, - }, { + }, + { // Version19_2_Start demarcates work towards CockroachDB v19.2. Key: VersionStart19_2, Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 1}, }, + { + // VersionQueryTxnTimestamp is https://github.com/cockroachdb/cockroach/pull/36307. + Key: VersionQueryTxnTimestamp, + Version: roachpb.Version{Major: 19, Minor: 1, Unstable: 2}, + }, // Add new versions here (step two of two). diff --git a/pkg/storage/batcheval/cmd_push_txn.go b/pkg/storage/batcheval/cmd_push_txn.go index a791f594da8c..58a287f851ba 100644 --- a/pkg/storage/batcheval/cmd_push_txn.go +++ b/pkg/storage/batcheval/cmd_push_txn.go @@ -115,18 +115,20 @@ func PushTxn( if h.Txn != nil { return result.Result{}, ErrTransactionUnsupported } - - // Verify that the PushTxn's timestamp is not less than the timestamp that - // the request intends to push the transaction to. Transactions should not - // be pushed into the future or their effect may not be fully reflected in - // a future leaseholder's timestamp cache. This is analogous to how reads - // should not be performed at a timestamp in the future. if h.Timestamp.Less(args.PushTo) { - return result.Result{}, errors.Errorf("PushTo %s larger than PushRequest header timestamp %s", args.PushTo, h.Timestamp) + // Verify that the PushTxn's timestamp is not less than the timestamp that + // the request intends to push the transaction to. Transactions should not + // be pushed into the future or their effect may not be fully reflected in + // a future leaseholder's timestamp cache. This is analogous to how reads + // should not be performed at a timestamp in the future. + return result.Result{}, errors.Errorf("request timestamp %s less than PushTo timestamp %s", h.Timestamp, args.PushTo) + } + if h.Timestamp.Less(args.PusheeTxn.Timestamp) { + // This condition must hold for the timestamp cache access/update to be safe. + return result.Result{}, errors.Errorf("request timestamp %s less than pushee txn timestamp %s", h.Timestamp, args.PusheeTxn.Timestamp) } - if !bytes.Equal(args.Key, args.PusheeTxn.Key) { - return result.Result{}, errors.Errorf("request key %s should match pushee's txn key %s", args.Key, args.PusheeTxn.Key) + return result.Result{}, errors.Errorf("request key %s should match pushee txn key %s", args.Key, args.PusheeTxn.Key) } key := keys.TransactionKey(args.PusheeTxn.Key, args.PusheeTxn.ID) diff --git a/pkg/storage/batcheval/cmd_query_txn.go b/pkg/storage/batcheval/cmd_query_txn.go index 56e4cc2c1d15..99def7979eb3 100644 --- a/pkg/storage/batcheval/cmd_query_txn.go +++ b/pkg/storage/batcheval/cmd_query_txn.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/storage/batcheval/result" "github.com/cockroachdb/cockroach/pkg/storage/engine" "github.com/cockroachdb/cockroach/pkg/storage/spanset" @@ -49,11 +50,20 @@ func QueryTxn( ctx context.Context, batch engine.ReadWriter, cArgs CommandArgs, resp roachpb.Response, ) (result.Result, error) { args := cArgs.Args.(*roachpb.QueryTxnRequest) + h := cArgs.Header reply := resp.(*roachpb.QueryTxnResponse) - if cArgs.Header.Txn != nil { + if h.Txn != nil { return result.Result{}, ErrTransactionUnsupported } + // TODO(nvanbenschoten): old clusters didn't attach header timestamps to + // QueryTxn requests, so only perform this check for clusters that will + // always attach a valid timestamps. + checkHeaderTS := cArgs.EvalCtx.ClusterSettings().Version.IsActive(cluster.VersionQueryTxnTimestamp) + if h.Timestamp.Less(args.Txn.Timestamp) && checkHeaderTS { + // This condition must hold for the timestamp cache access to be safe. + return result.Result{}, errors.Errorf("request timestamp %s less than txn timestamp %s", h.Timestamp, args.Txn.Timestamp) + } if !bytes.Equal(args.Key, args.Txn.Key) { return result.Result{}, errors.Errorf("request key %s does not match txn key %s", args.Key, args.Txn.Key) } diff --git a/pkg/storage/batcheval/cmd_recover_txn.go b/pkg/storage/batcheval/cmd_recover_txn.go index 5e5c1f815538..8a73463de8ea 100644 --- a/pkg/storage/batcheval/cmd_recover_txn.go +++ b/pkg/storage/batcheval/cmd_recover_txn.go @@ -64,8 +64,8 @@ func RecoverTxn( return result.Result{}, errors.Errorf("request key %s does not match txn key %s", args.Key, args.Txn.Key) } if h.Timestamp.Less(args.Txn.Timestamp) { - // This condition must hold for the timestamp cache update to be safe. - return result.Result{}, errors.Errorf("request timestamp %v less than txn timestamp %v", h.Timestamp, args.Txn.Timestamp) + // This condition must hold for the timestamp cache access/update to be safe. + return result.Result{}, errors.Errorf("request timestamp %s less than txn timestamp %s", h.Timestamp, args.Txn.Timestamp) } key := keys.TransactionKey(args.Txn.Key, args.Txn.ID) diff --git a/pkg/storage/txnrecovery/manager.go b/pkg/storage/txnrecovery/manager.go index 8eda45812084..cba8dca00c55 100644 --- a/pkg/storage/txnrecovery/manager.go +++ b/pkg/storage/txnrecovery/manager.go @@ -252,6 +252,7 @@ func (m *manager) resolveIndeterminateCommitForTxnProbe( // write is prevented, or we run out of in-flight writes to query. for len(queryIntentReqs) > 0 { var b client.Batch + b.Header.Timestamp = m.clock.Now() b.AddRawRequest(&queryTxnReq) for i := 0; i < defaultBatchSize && len(queryIntentReqs) > 0; i++ { b.AddRawRequest(&queryIntentReqs[0]) diff --git a/pkg/storage/txnwait/txnqueue.go b/pkg/storage/txnwait/txnqueue.go index 3b464a3a1535..1eae427c3e99 100644 --- a/pkg/storage/txnwait/txnqueue.go +++ b/pkg/storage/txnwait/txnqueue.go @@ -825,6 +825,7 @@ func (q *Queue) queryTxnStatus( now hlc.Timestamp, ) (*roachpb.Transaction, []uuid.UUID, *roachpb.Error) { b := &client.Batch{} + b.Header.Timestamp = q.store.Clock().Now() b.AddRawRequest(&roachpb.QueryTxnRequest{ RequestHeader: roachpb.RequestHeader{ Key: txnMeta.Key,