Great that you got to the bottom of this so fast! Man, that method is hard to follow. I gave this what I hope is a thorough review but will give it another one when you've figured out the lost update.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @nvanbenschoten, and @tbg)

pkg/storage/engine/mvcc.go, line 1466 at r1 (raw file):

			}
		} else if !metaTimestamp.Less(readTimestamp) {
			// This is the case where we're trying to write under a committed

Not necessarily, right? So far we only know that there's a committed write above the read timestamp (but the write timestamp could be above the committed value). Nvm, you mull on that below.

pkg/storage/engine/mvcc.go, line 1483 at r1 (raw file):

			// we'll need to refresh anyway, right? It might also be interacting
			// poorly with our logic in `evaluateBatch`. Any idea @bdarnell or
			// @tbg?

For snapshot transactions, this is just lost update -- two txn start at origts=100, their commit ts advances to 150 and 200 respectively, and then txn1 writes key=value1 (and commits) and txn2 cputs key from nil to value2, in effect wiping out value1 without ever having to observe it. I also don't see how this applies to serializable txns, though. As you mention, if we lay down an intent we're going to have to refresh anyway, and that should see the update that would be lost and force a restart. Worth getting to the bottom of even though I'm not sure avoiding the WTO is a good idea here. After all, what are the odds the write will survive the refresh? At that point we might as well check right here whether it would survive a refresh, though that seems to be optimizing for something we don't know is worth the added complexity.

PS this method probably has a cyclomatic complexity of MaxUint64... not that I see any easy way of changing that.

pkg/storage/engine/mvcc.go, line 1484 at r1 (raw file):

			// poorly with our logic in `evaluateBatch`. Any idea @bdarnell or
			// @tbg?
			writeTimestamp.Forward(metaTimestamp.Next())

Does this writeTimestamp ever make it back to the txn in the response?

pkg/storage/engine/mvcc.go, line 1497 at r1 (raw file):

			} else {
				// Outside of a transaction, read the latest value and advance
				// the write timestamp to the latest value's timestamp + 1. The

This part of the comment seems out of place because this was already done (also before this PR).

pkg/storage/engine/mvcc.go, line 1498 at r1 (raw file):

				// Outside of a transaction, read the latest value and advance
				// the write timestamp to the latest value's timestamp + 1. The
				// new timestamp is returned to the caller in maybeTooOldErr.

Confused by this. So we do actually write but then also return maybeTooOldErr? I doubt that.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @tbg)

pkg/storage/engine/mvcc.go, line 1466 at r1 (raw file):

Ack.

pkg/storage/engine/mvcc.go, line 1483 at r1 (raw file):

Mystery solved!

pkg/storage/engine/mvcc.go, line 1484 at r1 (raw file):

Yes, the writeTimestamp makes it back into the txn response here:

pkg/storage/engine/mvcc.go, line 1497 at r1 (raw file):

Updated.

pkg/storage/engine/mvcc.go, line 1498 at r1 (raw file):

It's surprising, but yes we do. Dating all the way back to 4220d4a!

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @nvanbenschoten)

pkg/storage/engine/mvcc.go, line 1483 at r1 (raw file):

How does this mechanism work exactly? If a CPut runs into a WTOErr, it "locally" (store?) retries the batch at the suggested timestamp and then returns the txn retry error? How does your attempted change break that? Shouldn't we be removing the WTO, but still returning a txn that needs to refresh if it wants to have any chance of committing? Why does it get to commit anyway?

pkg/storage/engine/mvcc.go, line 1498 at r1 (raw file):

But we're not in a transaction in this branch? The write must not be committed, so why do it?
This is probably not the right PR to be pulling on this thread, but add a TODO or some explanation, if applicable.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @tbg)

pkg/storage/engine/mvcc.go, line 1483 at r1 (raw file):

The mechanism is all in evaluateBatch. The idea isn't to locally retry anything. Instead, that function's purpose is to run all commands in a batch even if they are throwing WTO errors to determine the highest timestamp needed for later retries.

The function then makes a few decision. First, for non-transactional requests, it always returns the largest WriteTooOldError. So to answer the question below, we will still return the error instead of committing, but we will wait until the entire non-transactional batch has finished evaluating so that we can return the "best" error.

For transactional requests, things are more interesting. For suitable request types like blind Puts, it sets the batch responses timestamp and its WriteTooOld flag. It can then go ahead and writes these intents. However, for others that are not blind like CPut, it returns the WriteTooOld error immediately instead of writing the intents. This allows these request types to avoid ever needing to be refreshed. This is also what broke when I changed this to rely on CPuts refreshing.

I added some more commentary in the code.

pkg/storage/engine/mvcc.go, line 1498 at r1 (raw file):

Answered above.

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)