This change is 

, but probably you'll want a second pair of eyeballs.

Reviewed 4 of 4 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/internal/client/sender.go, line 42 at r2 (raw file):

	// ForgetTxn assures that the transaction is no longer being tracked
	// by the TxnCoordSender. Aborting the transaction and stopping its
	// heartbeat loop if it is. Implementations should be idepotent, and

s/idep/idemp/

pkg/internal/client/txn.go, line 1071 at r2 (raw file):

	_ = txn.updateStateOnRetryableErrLocked(
		ctx, *newErr,
		// We're passing the current ID as the "request"'s. In doing so, we're

This comment isn't new, but could you explain what's going on here?

pkg/internal/client/txn.go, line 1124 at r2 (raw file):

		// incarnation of this Txn. This should be handled by the TxnCoordSender
		// itself when it sees the abort error, but because responses can
		// be reordered between the TxnCoordSender and Txn.Send, it may

So what happened here was that TxnCoordSender drops state about a txn, but an outstanding request comes back, and then a new heartbeat loop is started? Yikesies.

pkg/sql/executor.go, line 1421 at r2 (raw file):

			// fixed.
			//
			// TODO DURING REVIEW: Should we remove the restriction but synchronize

That sounds like the right thing to do, right?

pkg/sql/session.go, line 665 at r2 (raw file):

		// the transaction epoch to invalidate any writes performed by any workers
		// after the retry updated the txn's proto but before we synchronized. Note
		// that we don't Txn need lock the client.Txn because we're synchronized.

this sentence is off.

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

			}},
		}
		shuffle.Shuffle(injections)

But if it fails, you don't even know the order. Should have a seed and print it.

pkg/sql/txn_restart_test.go, line 605 at r2 (raw file):

// with parallel statements. We test that when a statement running in parallel with
// others gets a retryable error, the other statements synchronize and retry without
// leaving around any unintended data. This was the case before #17197 was addressed.

"leaving around" doesn't seem so bad, but it they were leaving around committable data -- might be worth adding that word.

pkg/sql/txn_restart_test.go, line 684 at r2 (raw file):

					// Verify that the txns succeeded by reading the rows from each table.
					// Each should have written exactly one row.

How quickly does this fail without your fixes in this PR?

Comments from Reviewable

Reviewed 2 of 4 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

pkg/internal/client/sender.go, line 33 at r2 (raw file):

// the Txn client and DistSQL hacky powers when handling errors that
// happened on remote nodes or with concurrent requests.
type SenderWithTxnCoordBackdoor interface {

The Sender abstraction is pretty well broken at this point: it's no longer safe to interpose another Sender between client.Txn and TxnCoordSender as the interface would imply. File a cleanup issue to make client.Txn take a *TxnCoordSender instead of a Sender (unless this all becomes moot first when Txn and TxnCoordSender are merged).

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

This comes for free if you add our randutil.SeedForTests() in main_test.go

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

Not really, unless everything that happens before this test is deterministic (at least in its usage of the rand).

Comments from Reviewable

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

Or if you're running this test in isolation. So it's not too useful for CI runs, but you could still use it for stress runs of this test. But anyway, being explicit in tests that deliberately use randomness is a good idea.

Comments from Reviewable

Reviewed 2 of 4 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

pkg/internal/client/sender.go, line 44 at r2 (raw file):

	// heartbeat loop if it is. Implementations should be idepotent, and
	// should be a no-op if the transaction is not being tracked.
	ForgetTxn(ctx context.Context, txn roachpb.Transaction)

if a txnID is sufficient here instead of the whole proto, I think it'd be cleaner to just take that.
If we stick we the proto, we're passing it my pointer everywhere else, so I think we should be consistent here too.

pkg/internal/client/txn.go, line 1106 at r2 (raw file):

		// didn't start heartbeating again because of this delayed error.
		txn.forgetTxnOnAbort(ctx, newTxn)
		return roachpb.NewError(&roachpb.RespForPrevTxnIDError{})

I'm not sure why we're returning this error here?
We don't return an error for the other cases (error for previous incarnations) - i.e. the wrong epoch. Why not swallow this?

I think returning the error is OK in the other place (the non-error case in txn.Send), but perhaps TxnAbortedAsyncErr would be a better name. And I'd suggest we try to unify this error with what the TxnCoordSender returns from maybeRejectClientLocked(). Does that make sense?

pkg/internal/client/txn.go, line 1137 at r2 (raw file):

		// TxnCoordSender.
		txn.mu.Proto = *newTxn
	} else /* restartErr */ {

restartErr is not a thing. I'd remove that inline comment.

pkg/internal/client/txn.go, line 1152 at r2 (raw file):

}

// forgetTxnOnAbort assures that is the Txn's wrapped sender is a TxnCoordSender,

s/is/if

pkg/roachpb/data.go, line 862 at r2 (raw file):

}

// BumpEpoch increments the transaction's epoch, allowing for an in-place

I don't know what an "in-place restart" is :)
I'd say that it allows a client to discard all the previous writes before restarting and it is to be used in cases where those previous writes might have been performed at the wrong epoch.

pkg/sql/executor.go, line 1421 at r2 (raw file):

Yes, I think that's what we should do, at least morally. But actually, was I confused when I wrote this? Can the parallelizeQueue.Empty() ever be false here, given that we're executing another statement that's not in the queue?

Anyway, if we add the synchronization here, I'd also leave a TODO about figuring out how to cancel those statements.
And also, I guess we should look at the error from queue.synchronize() and bail if it's not retryable. But then again maybe the whole thing doesn't make sense since the current contract for returning errors would imply that the queue is empty...

pkg/sql/parallel_stmts.go, line 44 at r1 (raw file):

//    executed before the plan added second.
// 3. No plans will begin execution once an error has been seen until Wait is
//    called to drain the plans and reset the error set state.

nit: s/error set/error-set (otherwise parsing is hard)

pkg/sql/parallel_stmts.go, line 56 at r1 (raw file):

	// errs is the set of error seen since the last call to ParallelizeQueue.Wait.
	// Referred to as the current "parallel batch's error set".

consider giving some hints here about how this set is going to be used, or what the higher level might want to do with it

pkg/sql/session.go, line 664 at r2 (raw file):

		// incarnation of this transaction. Before doing so though, we need to bump
		// the transaction epoch to invalidate any writes performed by any workers
		// after the retry updated the txn's proto but before we synchronized. Note

add "(some of these writes might have been performed at the wrong epoch)".

Comments from Reviewable

Thanks for the reviews

Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

pkg/internal/client/sender.go, line 33 at r2 (raw file):

Done. #17657.

pkg/internal/client/sender.go, line 42 at r2 (raw file):

Done.

pkg/internal/client/sender.go, line 44 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 1071 at r2 (raw file):

It looks like DistSQL doesn't support concurrent transaction batches because it can't handle one of them getting an Abort error.

pkg/internal/client/txn.go, line 1106 at r2 (raw file):

You're right, swallowing the error is the right thing to do. Also changed the name to TxnAbortedAsyncErr. I'm not as convinced about merging the errors, as the error returned from maybeRejectClientLocked can be a symptom of other issues as well.

pkg/internal/client/txn.go, line 1137 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 1152 at r2 (raw file):

Done.

pkg/sql/executor.go, line 1421 at r2 (raw file):

Let me know if my comment makes sense.

Also, I'm getting rid of parallelizeQueue.Empty() because it's misleading. Statements could have already been run and their errors could be waiting for a synchronization to be cleared. If we went with the current approach, those errors could be left over for the next iteration of statements.

pkg/sql/parallel_stmts.go, line 44 at r1 (raw file):

Done.

pkg/sql/parallel_stmts.go, line 56 at r1 (raw file):

Done.

pkg/sql/session.go, line 664 at r2 (raw file):

Done.

pkg/sql/session.go, line 665 at r2 (raw file):

Done.

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

So you want me to call randutil.SeedForTests() again at the beginning of this test? It is already in sql's TestMain.

pkg/sql/txn_restart_test.go, line 605 at r2 (raw file):

Done.

pkg/sql/txn_restart_test.go, line 684 at r2 (raw file):

Within 100 iterations, though we are injecting a lot of retry errors in each test.

Comments from Reviewable

Reviewed 13 of 13 files at r3, 13 of 13 files at r4.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks pending.

pkg/sql/txn_restart_test.go, line 135 at r2 (raw file):

Yeah, I guess (ideally Shuffle would accept an RNG). But I don't feel strongly.

Comments from Reviewable

I realized that the extra hooks into TxnCoordSender were only needed because of a failure in our tests, not because of a real need. In tests we were injecting TxnAbortedErrors without the transaction record having actually been aborted. This meant that if a concurrent request attempted to heartbeat the txn record, it would find that it was still pending, which resulted in the deadlock mentioned previously.

I've changed the test to properly abort the meta record when it injects a TxnAbortedError, which allows us to remove the new hooks into TxnCoordSender and simplify the rest of this significantly. PTAL

Ping :) This really needs to get into 1.1.

LGTM except the last commit, for which you'll definitely want Andrei's eyes. Nice simplification though!

Reviewed 13 of 13 files at r5, 1 of 1 files at r6, 12 of 12 files at r7.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks pending.

Comments from Reviewable

LGTM

Reviewed 1 of 1 files at r6, 12 of 12 files at r7.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/internal/client/txn.go, line 1096 at r7 (raw file):

	newTxn := &retryErr.Transaction

	abortedPreviouslyDuringTx := requestTxnID != txn.mu.Proto.ID

Get rid of the temporary variable; the comment does a better job of explaining what's going on than the variable name does.

pkg/roachpb/errors.proto, line 333 at r7 (raw file):

// transaction. This can happen in cases where concurrent requests are made
// for a Transaction and one of the requests results in a Txn abort.
message TxnAbortedAsyncErr {

s/AbortedAsyncErr/PrevAttemptAbortedError/? Or maybe just TxnPrevAttemptError, since the relationship between this condition and an abort is not obvious.

(I think of regular TxnAbortedErrors as being asynchronous in some sense, because they're the result of an abort that is asynchronous with respect to the txn being aborted)

pkg/internal/client/sender.go, line 33 at r2 (raw file):

#17657

Close that issue if it's no longer relevant with the latest refactor of this change.

Comments from Reviewable

Reviewed 1 of 13 files at r5, 5 of 12 files at r7.
Review status: all files reviewed at latest revision, 14 unresolved discussions, all commit checks successful.

pkg/roachpb/errors.proto, line 273 at r7 (raw file):

}

// UnhandledRetryableError tells the recipient that a KV request must be

superfluous move of UnhandledRetryableError and HandledRetryableError? Nothing changed to them, right?
I'd try to move them back to avoid the diff.

pkg/roachpb/errors.proto, line 324 at r7 (raw file):

// An UntrackedTxnError indicates that an attempt to transmit a batch request
// for a Transaction failed because the Transaction was not being tracked by
// the client properly.

consider clarifying "by the client" a bit - "by the TxnCoordSender"?

pkg/roachpb/errors.proto, line 363 at r7 (raw file):

  optional ReplicaCorruptionError replica_corruption = 17;
  optional ReplicaTooOldError replica_too_old = 18;
  reserved 25;

consider putting these 3 lines back where they were

pkg/sql/executor.go, line 1366 at r7 (raw file):

			// the stmts and clear the parallel batch's error-set before
			// restarting.
			if parErr := session.synchronizeParallelStmts(session.context); parErr != nil {

add a TODO here to cancel the parallel queries?

pkg/sql/executor.go, line 1366 at r7 (raw file):

			// the stmts and clear the parallel batch's error-set before
			// restarting.
			if parErr := session.synchronizeParallelStmts(session.context); parErr != nil {

also comment that a big reason for the sync is that it'd be incorrect to run statements after the txn.Reset below cause the might write at the wrong epoch.

pkg/sql/executor.go, line 1405 at r7 (raw file):

	_, independentFromParallelStmts := stmt.AST.(parser.IndependentFromParallelizedPriors)
	if !(parallelize || independentFromParallelStmts) {
		if err := session.synchronizeParallelStmts(session.context); err != nil {

I think you want session.Ctx() here and below.

Comments from Reviewable

Review status: all files reviewed at latest revision, 14 unresolved discussions, all commit checks successful.

pkg/internal/client/txn.go, line 1096 at r7 (raw file):

Done.

pkg/roachpb/errors.proto, line 273 at r7 (raw file):

I move them because the ordering was getting all messed up here. The pattern in this file is that the ErrorDeatil types were all listed up here in order. The mixed up order made it harder to read. Split into a separate commit for explicitness.

pkg/roachpb/errors.proto, line 324 at r7 (raw file):

Done.

pkg/roachpb/errors.proto, line 333 at r7 (raw file):

Done. Changes to TxnPrevAttemptError.

pkg/roachpb/errors.proto, line 363 at r7 (raw file):

These should all be in order, since this is just a giant union. The move happens in its own commit now though, for clarity of the blame.

pkg/sql/executor.go, line 1366 at r7 (raw file):

Done.

pkg/sql/executor.go, line 1366 at r7 (raw file):

Done.

pkg/sql/executor.go, line 1405 at r7 (raw file):

Done.

pkg/internal/client/sender.go, line 33 at r2 (raw file):

Done.

Comments from Reviewable

Isn't this that should be backported to 1.0?

@knz yes it should, along with #17868. What's the process for doing so?

1. Fork the branch release-1.0.
2. Make an equivalent patch against that branch. (e.g. via git cherry-pick).
3. Create a PR on github as usual; don't forget to select release-1.0 as base branch for the PR).
4. Prefix the PR title with "cherrypick".
5. @cockroachdb/release in a comment to the PR.

👍 Thanks for the instructions!