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.