storage: narrow down use of SingleDel to avoid anomalies

[tbg]

Fixes #69414.

Only use SingleDel if

• the meta tracking allows it (i.e. the sole previous condition), and
• epoch zero, and
• no ignored txn seqno ranges.

These three together imply that the engine history of the metadata key
is a single Set, so we can safely use SingleDel.

Release justification: fix for a release blocker
Release note: None

[cockroach-teamcity]

This change is 

[sumeerbhola]

Looks good!

nit: why is this "below-Raft" given this is being done by the leaseholder when evaluating the proposal?

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

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

// to change with #69891):
//
// - Set (Set' Del) Set'' SingleDel

nit: are the parentheses supposed to represent what was in a single compaction?
If so, the example is not quite accurate. We won't compact (Set' Del) into nothing. The Del will live because there is an older Set.
A bad sequence (which happens even if the first Del is a SingleDel) is
Set Set' (Del Set'') SingleDel
Set Set' (Set'' SingleDel)
Set Set'

---

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

// https://github.com/cockroachdb/cockroach/issues/69891
type singleDelOptimizationHelper struct {
	_didNotUpdateMeta *bool

why underscore prefixes in the names?

---

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

	}
	// TODO(sumeerbhole): changing this to `return true` does not
	// yield any test failures. This seems problematic.

I didn't understand this comment. We currently don't populate this optional field unless it is true, but that is just an implementation detail.

---

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

}

// onClearIntent returns true if the SingleDel optimization is available

nit: how about calling thisonAbortIntent with a code comment saying that this should not be mistaken to mean txn abort, since it only applies to the intent? onClearIntent seems a bit confusing since we clear the intent even in the case of committing (intentDemuxWriter.ClearIntent)

[tbg]

nit: why is this "below-Raft" given this is being done by the leaseholder when evaluating the proposal?

🙈 it's not, fixed, thank you.

RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

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

Previously, sumeerbhola wrote…

why underscore prefixes in the names?

It's my attempt to make it less likely that they will be accessed instead of the getters by accident.

---

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

Previously, sumeerbhola wrote…

I didn't understand this comment. We currently don't populate this optional field unless it is true, but that is just an implementation detail.

Ah, that makes sense, thank you for clarifying.

[sumeerbhola]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)

---

-- commits, line 13 at r3:
btw, I'd feel better if we also fix the compatibility issue with 21.1 before beta #69891 (comment) (I realize we don't expect to upgrade from 21.1 to 21.2 beta for serverless, so it isn't strictly necessary, but remembering why partial fixes are ok make my head hurt).

---

pkg/storage/mvcc.go, line 2937 at r3 (raw file):

//   BEGIN;
//     SAVEPOINT foo;
//       INSERT INTO kv VALUES(1, 1);

nit: can you add back the UPDATE in the example that was there in your previous version. That is more consistent with the use of Del below.

[tbg]

bors r=sumeerbhola
TFTR!
Going to leave the comments as is since I don't have time for another CI turnaround tonight. I expect there'll be another chance to touch these up with progress on #69891 (comment).

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[tbg]

blathers backport 21.2