Introduce a BeginTransaction request. #2799

spencerkimball · 2015-10-08T07:24:42Z

BeginTransactionRequest is automatically inserted by the KV client
immediately before the first transactional write. On execution,
BeginTransaction creates the transaction record.

If a heartbeat arrives for a txn that has no transaction record, it's
ignored. If a push txn arrives for a txn that has no transaction record,
the txn is considered aborted.

This solves the problem of errant heartbeats or pushes recreating a GC'd
transaction record, addressing #2062.

spencerkimball · 2015-10-08T07:25:46Z

This PR isn't finished yet...there are two failing unittests (and looks like there are merge conflicts...again).

@bdarnell : the TestStoreRangeDownReplicate test is broken in
client_raft_test.go. This addition of requiring the txn to have
a BeginTransaction which writes the record before EndTransaction
can work is surfacing the bug. On occasion, when up-replicating
from 4 to 5 replicas, the most recently added replica isn't sent
the BeginTransaction request. It does however receive the
EndTransaction request, which causes it to fail. There's a bunch
of logging I added which indicates to me that state.addNode is in
fact being called correctly. I'm not sure why the
BeginTransaction request isn't sent through as expected. Perhaps
you'll have an easier time figuring out what's going wrong there.

@petermattis , @vivekmenezes : the sql LogicTest is failing with this
change and I haven't even started looking into why that's
happening. If it has something to do with the addition of a
BeginTransaction, it occurs to me that the SQL stuff may be
hiding errors.

petermattis · 2015-10-08T19:53:41Z

The SQL logic test failures that I looked at were all the same root problem. We were modifying the schema (e.g. adding a unique index) and found an error that was unrelated to the conditional puts we performed. For example, testdata/alter_table:63 fails because the error returned is associated with the key /3/1/1001/2, but that is a key in the descriptor table and in the batch we wrote to that key using Put, not CPut. The error returned is:

unexpected value: bytes:"\013\002" checksum:168851347 timestamp:<wall_time:1444333838303045793 logical:0 > tag:BYTES

That's a conditional put failure, but as I said we wrote to the key using Put. This makes me think that this change somehow screwed up the mapping of errors to results.

spencerkimball · 2015-10-09T00:00:47Z

@petermattis Ah, good call. IIRC, @tschottdorf introduced an index into the error for batches. That would get screwed up because we insert the begin transaction request and then remove it. I'll look into fixing that. The other problem with raft remains however.

tbg · 2015-10-09T10:30:05Z

kv/txn_coord_sender.go

+	// the first transactional write. Also enforce that no transactional
+	// writes occur before a begin transaction.
+	var haveBeginTxn bool
+	for _, req := range ba.Requests {


take a look at ba.GetArg(proto.BeginTransactionRequest).

I need to go through them all in order to check for error conditions.

tbg · 2015-10-09T11:31:42Z

Is the client code really the right place to insert this call? It seems like we're shoving more complexity into the client though we could do that in the coordinator transparently.
That we have this EndTransaction business in the client already may seem like that's where its counterpart should live, but really EndTransaction is there because only the client knows when we're done. But the coordinator knows perfectly well when we start the write portion of a transaction (pretty much at the beginning of TxnCoordSender.Send). What do you think?

I assumed that's what you were doing before I saw the client changes (it's not like you didn't explain it, but alas), so some of my comments will refer to this erroneously.

tbg · 2015-10-09T11:32:40Z

@petermattis Ah, good call. IIRC, @tschottdorf introduced an index into the error for batches. That would get screwed up because we insert the begin transaction request and then remove it. I'll look into fixing that. The other problem with raft remains however.

Yes, that'll be it. Just go for a hotfix for now; I'm filing an issue. We should move the error index out of the error.

bdarnell · 2015-10-09T21:43:06Z

On occasion, when up-replicating
from 4 to 5 replicas, the most recently added replica isn't sent
the BeginTransaction request. It does however receive the
EndTransaction request, which causes it to fail.

I can't reproduce this - do you have a log? I can, however, reproduce a timing-dependent failure in the down-replicate phase of this test. The transaction record is created successfully, but it's not there by the time EndTransaction is called. This looks like a race between the RangeGCQueue and the operations on this range. This test is artificially accelerating the clock to expire leases and running GC scans, which are the normal means by which we avoid this race in the real world (although we should probably also have something more iron-clad).

spencerkimball · 2015-10-23T22:50:33Z

@tschottdorf I'm going to leave this stuff in the client. There's parity there with the end transaction stuff and I'm by no means convinced that the coordinator will always have enough information (with future directions) to determine the beginning of a txn.

BeginTransactionRequest is automatically inserted by the KV client immediately before the first transactional write. On execution, BeginTransaction creates the transaction record. If a heartbeat arrives for a txn that has no transaction record, it's ignored. If a push txn arrives for a txn that has no transaction record, the txn is considered aborted. This solves the problem of errant heartbeats or pushes recreating a GC'd transaction record, addressing #2062.

Introduce a BeginTransaction request.

tamird · 2015-10-24T21:46:55Z

client/txn.go

-		if !haveTxnWrite {
-			haveTxnWrite = roachpb.IsTransactionWrite(args)
+		if roachpb.IsTransactionWrite(args) && firstWriteIndex == -1 {
+			firstWriteKey = args.Header().Key


doesn't this end up being the last write key?

err, disregard

tbg · 2015-10-26T13:57:14Z

I understand that you wanted to get this out of your queue, but please leave future PRs open for discussion at least up to the first LGTM.

tbg · 2015-10-26T14:00:24Z

client/txn.go

+			br.Responses = append(br.Responses[:firstWriteIndex], br.Responses[firstWriteIndex+1:]...)
+		}
+		// Handle case where inserted begin txn confused an indexed error.
+		if pErr != nil && pErr.Detail != nil {


this is just if iErr, ok := pErr.GoError().(roachpb.IndexedError) {

spencerkimball · 2015-10-26T15:53:33Z

My mistake. Thought this was already LGTM.

spencerkimball · 2015-10-26T15:53:46Z

@tschottdorf Will send another PR with fixes.

fixes cockroachdb#2717 (ok since cockroachdb#2799 has landed).

spencerkimball assigned tbg Oct 8, 2015

spencerkimball force-pushed the spencerkimball/begin-txn branch from eb64586 to 50d25e1 Compare October 8, 2015 07:32

tbg reviewed Oct 9, 2015
View reviewed changes

tbg mentioned this pull request Oct 9, 2015

roachpb: refactoring IndexedError #2809

Closed

tbg mentioned this pull request Oct 14, 2015

Re-enable auto-gc for one-shot transactions #2717

Closed

spencerkimball force-pushed the spencerkimball/begin-txn branch 2 times, most recently from 432c77b to ca6ef78 Compare October 24, 2015 17:09

spencerkimball added 2 commits October 24, 2015 14:19

Addressed feedback

ca96b85

spencerkimball force-pushed the spencerkimball/begin-txn branch from ca6ef78 to ca96b85 Compare October 24, 2015 20:01

spencerkimball added a commit that referenced this pull request Oct 24, 2015

Merge pull request #2799 from cockroachdb/spencerkimball/begin-txn

47fb047

Introduce a BeginTransaction request.

spencerkimball merged commit 47fb047 into master Oct 24, 2015

spencerkimball deleted the spencerkimball/begin-txn branch October 24, 2015 21:37

tamird reviewed Oct 24, 2015
View reviewed changes

tbg reviewed Oct 26, 2015
View reviewed changes

tbg added a commit to tbg/cockroach that referenced this pull request Nov 10, 2015

re-enable txn auto gc

c18c29d

fixes cockroachdb#2717 (ok since cockroachdb#2799 has landed).

tbg mentioned this pull request Nov 10, 2015

re-enable txn auto gc #3094

Merged

cucaroach mentioned this pull request Nov 4, 2021

sql: goroutines seemingly not cancelling #72445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a BeginTransaction request. #2799

Introduce a BeginTransaction request. #2799

spencerkimball commented Oct 8, 2015

spencerkimball commented Oct 8, 2015

petermattis commented Oct 8, 2015

spencerkimball commented Oct 9, 2015

tbg Oct 9, 2015

spencerkimball Oct 23, 2015

tbg commented Oct 9, 2015

tbg commented Oct 9, 2015

bdarnell commented Oct 9, 2015

spencerkimball commented Oct 23, 2015

tamird Oct 24, 2015

tamird Oct 24, 2015

tbg commented Oct 26, 2015

tbg Oct 26, 2015

spencerkimball Oct 26, 2015

spencerkimball commented Oct 26, 2015

spencerkimball commented Oct 26, 2015

Introduce a BeginTransaction request. #2799

Introduce a BeginTransaction request. #2799

Conversation

spencerkimball commented Oct 8, 2015

spencerkimball commented Oct 8, 2015

petermattis commented Oct 8, 2015

spencerkimball commented Oct 9, 2015

tbg Oct 9, 2015

Choose a reason for hiding this comment

spencerkimball Oct 23, 2015

Choose a reason for hiding this comment

tbg commented Oct 9, 2015

tbg commented Oct 9, 2015

bdarnell commented Oct 9, 2015

spencerkimball commented Oct 23, 2015

tamird Oct 24, 2015

Choose a reason for hiding this comment

tamird Oct 24, 2015

Choose a reason for hiding this comment

tbg commented Oct 26, 2015

tbg Oct 26, 2015

Choose a reason for hiding this comment

spencerkimball Oct 26, 2015

Choose a reason for hiding this comment

spencerkimball commented Oct 26, 2015

spencerkimball commented Oct 26, 2015