From 3d26bc51fb9d2af37c912df93751513cefc32745 Mon Sep 17 00:00:00 2001 From: Andrei Matei Date: Tue, 6 Aug 2019 10:59:53 -0400 Subject: [PATCH] kv: fix a buglet in SetSystemConfigTrigger and cleanup We were asking for the system config trigger to be executed on EndTransaction even when we had failed to anchor the transaction on the system config range. I think this was benign we weren't actually committing the transactions for which SetSystemConfigTrigger() returned errors. I've also cleaned up the code a bit. Before this patch, there was a systemConfigTrigger field on both the Txn and the TxnCoordSender. However, the TxnCoordSender didn't actually know anything about any trigger; it is just concerned with the anchoring of the transaction on the system config range. I've clarified this point. Release note: None --- pkg/internal/client/sender.go | 25 ++++++++++++++----------- pkg/internal/client/txn.go | 5 ++++- pkg/internal/client/txn_test.go | 22 ++++++++++++++++++++++ pkg/kv/txn_coord_sender.go | 24 ++++++++++-------------- 4 files changed, 50 insertions(+), 26 deletions(-) diff --git a/pkg/internal/client/sender.go b/pkg/internal/client/sender.go index 35c5e7144340..44460c497709 100644 --- a/pkg/internal/client/sender.go +++ b/pkg/internal/client/sender.go @@ -12,6 +12,7 @@ package client import ( "context" + "fmt" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb" @@ -104,16 +105,16 @@ type TxnSender interface { // is the only one which will be invoked. OnFinish(func(error)) - // SetSystemConfigTrigger sets the system db trigger to true on this transaction. - // This will impact the EndTransactionRequest. + // AnchorOnSystemConfigRange ensures that the transaction record, if/when it + // will be created, will be created on the system config range. This is useful + // because some commit triggers only work when the EndTransaction is evaluated + // on that range. // - // NOTE: The system db trigger will only execute correctly if the transaction - // record is located on the range that contains the system span. If a - // transaction is created which modifies both system *and* non-system data, it - // should be ensured that the transaction record itself is on the system span. - // This can be done by making sure a system key is the first key touched in the - // transaction. - SetSystemConfigTrigger() error + // An error is returned if the transaction's key has already been set (i.e. if + // the transaction already performed any writes). + // The note above notwithstanding, it is allowed to call this method multiple + // times (even if there's been writes in between the calls). + AnchorOnSystemConfigRange() error // GetMeta retrieves a copy of the TxnCoordMeta, which can be sent from root // to leaf transactions or the other way around. Can be combined via @@ -294,8 +295,10 @@ func (m *MockTransactionalSender) OnFinish(f func(error)) { } } -// SetSystemConfigTrigger is part of the TxnSender interface. -func (m *MockTransactionalSender) SetSystemConfigTrigger() error { panic("unimplemented") } +// AnchorOnSystemConfigRange is part of the TxnSender interface. +func (m *MockTransactionalSender) AnchorOnSystemConfigRange() error { + return fmt.Errorf("unimplemented") +} // TxnStatus is part of the TxnSender interface. func (m *MockTransactionalSender) TxnStatus() roachpb.TransactionStatus { diff --git a/pkg/internal/client/txn.go b/pkg/internal/client/txn.go index a2c6280e7052..14bb00fbcdcf 100644 --- a/pkg/internal/client/txn.go +++ b/pkg/internal/client/txn.go @@ -274,8 +274,11 @@ func (txn *Txn) CommitTimestampFixed() bool { func (txn *Txn) SetSystemConfigTrigger() error { txn.mu.Lock() defer txn.mu.Unlock() + if err := txn.mu.sender.AnchorOnSystemConfigRange(); err != nil { + return err + } txn.systemConfigTrigger = true - return txn.mu.sender.SetSystemConfigTrigger() + return nil } // DisablePipelining instructs the transaction not to pipeline requests. It diff --git a/pkg/internal/client/txn_test.go b/pkg/internal/client/txn_test.go index 8b651bfd7b82..6f0cae1c8c33 100644 --- a/pkg/internal/client/txn_test.go +++ b/pkg/internal/client/txn_test.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/pkg/errors" + "github.com/stretchr/testify/require" ) var ( @@ -487,3 +488,24 @@ func TestUpdateDeadlineMaybe(t *testing.T) { t.Errorf("unexpected deadline: %s", d) } } + +// Test that, if SetSystemConfigTrigger() fails, the systemConfigTrigger has not +// been set. +func TestAnchoringErrorNoTrigger(t *testing.T) { + defer leaktest.AfterTest(t)() + ctx := context.Background() + + mc := hlc.NewManualClock(1) + clock := hlc.NewClock(mc.UnixNano, time.Nanosecond) + db := NewDB( + testutils.MakeAmbientCtx(), + MakeMockTxnSenderFactory( + func(context.Context, *roachpb.Transaction, roachpb.BatchRequest, + ) (*roachpb.BatchResponse, *roachpb.Error) { + return nil, nil + }), + clock) + txn := NewTxn(ctx, db, 0 /* gatewayNodeID */, RootTxn) + require.Error(t, txn.SetSystemConfigTrigger(), "unimplemented") + require.False(t, txn.systemConfigTrigger) +} diff --git a/pkg/kv/txn_coord_sender.go b/pkg/kv/txn_coord_sender.go index 837a58e52acb..97db2dfccabf 100644 --- a/pkg/kv/txn_coord_sender.go +++ b/pkg/kv/txn_coord_sender.go @@ -11,6 +11,7 @@ package kv import ( + "bytes" "context" "fmt" "runtime/debug" @@ -118,11 +119,6 @@ type TxnCoordSender struct { // (a retryable TransactionAbortedError in case of the async abort). closed bool - // systemConfigTrigger is set to true when modifying keys from the - // SystemConfig span. This sets the SystemConfigTrigger on - // EndTransactionRequest. - systemConfigTrigger bool - // txn is the Transaction proto attached to all the requests and updated on // all the responses. txn roachpb.Transaction @@ -1110,18 +1106,18 @@ func (tc *TxnCoordSender) setTxnAnchorKeyLocked(key roachpb.Key) error { return nil } -// SetSystemConfigTrigger is part of the client.TxnSender interface. -func (tc *TxnCoordSender) SetSystemConfigTrigger() error { +// AnchorOnSystemConfigRange is part of the client.TxnSender interface. +func (tc *TxnCoordSender) AnchorOnSystemConfigRange() error { tc.mu.Lock() defer tc.mu.Unlock() - if !tc.mu.systemConfigTrigger { - tc.mu.systemConfigTrigger = true - // The system-config trigger must be run on the system-config range which - // means any transaction with the trigger set needs to be anchored to the - // system-config range. - return tc.setTxnAnchorKeyLocked(keys.SystemConfigSpan.Key) + // Allow this to be called more than once. + if bytes.Equal(tc.mu.txn.Key, keys.SystemConfigSpan.Key) { + return nil } - return nil + // The system-config trigger must be run on the system-config range which + // means any transaction with the trigger set needs to be anchored to the + // system-config range. + return tc.setTxnAnchorKeyLocked(keys.SystemConfigSpan.Key) } // TxnStatus is part of the client.TxnSender interface.