storage: override MVCCMetadata.TxnDidNotUpdateMeta in mixed version cluster

Specifically, if the cluster version indicates that there can
be nodes with the broken SingleDelete logic for separated
intent resolution, the MVCCMetadata.TxnDidNotUpdateMeta
field will never be set to true.

See code comments and #69891 (comment)

Informs #69891

Release justification: fix for a release blocker that causes incorrect
behavior for transactional writes.

Release note: None

Author: sumeerbhola

pkg/ccl/storageccl/engineccl/encrypted_fs_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/baseccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl/enginepbccl"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -247,6 +248,7 @@ func TestPebbleEncryption(t *testing.T) {
 			StorageConfig: base.StorageConfig{
 				Attrs:             roachpb.Attributes{},
 				MaxSize:           512 << 20,
+				Settings:          cluster.MakeTestingClusterSettings(),
 				UseFileRegistry:   true,
 				EncryptionOptions: encOptionsBytes,
 			},

pkg/kv/kvserver/rangefeed/BUILD.bazel

@@ -48,6 +48,7 @@ go_test(
         "//pkg/base",
         "//pkg/keys",
         "//pkg/roachpb:with-mocks",
+        "//pkg/settings/cluster",
         "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/testutils",

pkg/kv/kvserver/rangefeed/catchup_scan_bench_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rangefeed"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
@@ -184,7 +185,7 @@ func setupMVCCPebble(b testing.TB, dir string, lBaseMaxBytes int64, readOnly boo
 	peb, err := storage.NewPebble(
 		context.Background(),
 		storage.PebbleConfig{
-			StorageConfig: base.StorageConfig{Dir: dir},
+			StorageConfig: base.StorageConfig{Dir: dir, Settings: cluster.MakeTestingClusterSettings()},
 			Opts:          opts,
 		})
 	if err != nil {

pkg/kv/kvserver/replica_test.go

@@ -248,7 +248,8 @@ func (tc *testContext) StartWithStoreConfigAndVersion(
 			storage.InMemory(),
 			storage.Attributes(roachpb.Attributes{Attrs: []string{"dc1", "mem"}}),
 			storage.MaxSize(1<<20),
-			storage.SetSeparatedIntents(disableSeparatedIntents))
+			storage.SetSeparatedIntents(disableSeparatedIntents),
+			storage.Settings(cfg.Settings))
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -6430,9 +6431,9 @@ func TestRangeStatsComputation(t *testing.T) {
 	}
 	expMS = baseStats
 	expMS.Add(enginepb.MVCCStats{
-		LiveBytes:   103,
+		LiveBytes:   101,
 		KeyBytes:    28,
-		ValBytes:    75,
+		ValBytes:    73,
 		IntentBytes: 23,
 		LiveCount:   2,
 		KeyCount:    2,
@@ -6442,6 +6443,10 @@ func TestRangeStatsComputation(t *testing.T) {
 	if tc.engine.IsSeparatedIntentsEnabledForTesting(ctx) {
 		expMS.SeparatedIntentCount++
 	}
+	if !tc.engine.OverrideTxnDidNotUpdateMetaToFalse(ctx) {
+		expMS.LiveBytes += 2
+		expMS.ValBytes += 2
+	}
 	if err := verifyRangeStats(tc.engine, tc.repl.RangeID, expMS); err != nil {
 		t.Fatal(err)
 	}

pkg/kv/kvserver/spanset/batch.go

@@ -676,6 +676,10 @@ func (s spanSetWriter) LogLogicalOp(
 	s.w.LogLogicalOp(op, details)
 }
 
+func (s spanSetWriter) OverrideTxnDidNotUpdateMetaToFalse(ctx context.Context) bool {
+	return s.w.OverrideTxnDidNotUpdateMetaToFalse(ctx)
+}
+
 // ReadWriter is used outside of the spanset package internally, in ccl.
 type ReadWriter struct {
 	spanSetReader

pkg/storage/engine.go

@@ -679,6 +679,31 @@ type Writer interface {
 	//
 	// It is safe to modify the contents of the arguments after it returns.
 	SingleClearEngineKey(key EngineKey) error
+
+	// OverrideTxnDidNotUpdateMetaToFalse is a temporary method that will be removed
+	// for 22.1.
+	//
+	// See #69891 for details on the bug related to usage of SingleDelete in
+	// separated intent resolution. The following is needed for correctly
+	// migrating from 21.1 to 21.2.
+	//
+	// We have fixed the intent resolution code path in 21.2-beta to use
+	// SingleDelete more conservatively. The 21.2-GA will also likely include
+	// Pebble changes to make the old buggy usage of SingleDelete correct.
+	// However, there is a problem if someone upgrades from 21.1 to
+	// 21.2-beta/21.2-GA:
+	// 21.1 nodes will not write separated intents while they are the
+	// leaseholder for a range. However they can become the leaseholder for a
+	// range after a separated intent was written (in a mixed version cluster).
+	// Hence they can resolve separated intents. The logic in 21.1 for using
+	// SingleDelete when resolving intents is similarly buggy, and the Pebble
+	// code included in 21.1 will not make this buggy usage correct. The
+	// solution is for 21.2 nodes to never set txnDidNotUpdateMeta=true when
+	// writing separated intents, until the cluster version is at the version
+	// when the buggy code was fixed in 21.2. So 21.1 code will never use
+	// SingleDelete when resolving these separated intents (since the only
+	// separated intents being written are by 21.2 nodes).
+	OverrideTxnDidNotUpdateMetaToFalse(ctx context.Context) bool
 }
 
 // ReadWriter is the read/write interface to an engine's data.

pkg/storage/mvcc.go

@@ -1075,8 +1075,7 @@ func (b *putBuffer) putIntentMeta(
 	ctx context.Context,
 	writer Writer,
 	key MVCCKey,
-	state PrecedingIntentState,
-	txnDidNotUpdateMeta bool,
+	helper txnDidNotUpdateMetaHelper,
 	meta *enginepb.MVCCMetadata,
 ) (keyBytes, valBytes int64, separatedIntentCountDelta int, err error) {
 	if meta.Txn != nil && meta.Timestamp.ToTimestamp() != meta.Txn.WriteTimestamp {
@@ -1085,32 +1084,62 @@ func (b *putBuffer) putIntentMeta(
 		return 0, 0, 0, errors.AssertionFailedf(
 			"meta.Timestamp != meta.Txn.WriteTimestamp: %s != %s", meta.Timestamp, meta.Txn.WriteTimestamp)
 	}
-	// All nodes in this cluster understand separated intents, so can fiddle
-	// with TxnDidNotUpdateMeta, which is not understood by older nodes (which
-	// are no longer present, and will never again be present).
-	//
-	// NB: the parameter txnDidNotUpdateMeta is about what happened prior to
-	// this Put, and is passed through to writer below. The field
-	// TxnDidNotUpdateMeta, in the MVCCMetadata we are about to write,
-	// includes what happened in this Put.
-	if state == NoExistingIntent {
-		meta.TxnDidNotUpdateMeta = &trueValue
-	} else {
-		// Absence represents false.
-		meta.TxnDidNotUpdateMeta = nil
-	}
-
+	helper.populateMeta(ctx, meta)
 	bytes, err := b.marshalMeta(meta)
 	if err != nil {
 		return 0, 0, 0, err
 	}
 	if separatedIntentCountDelta, err = writer.PutIntent(
-		ctx, key.Key, bytes, state, txnDidNotUpdateMeta, meta.Txn.ID); err != nil {
+		ctx, key.Key, bytes, helper.state, helper.valueForPutIntent(), meta.Txn.ID); err != nil {
 		return 0, 0, 0, err
 	}
 	return int64(key.EncodedSize()), int64(len(bytes)), separatedIntentCountDelta, nil
 }
 
+// txnDidNotUpdateMetaHelper is used to decide what to put in the MVCCMetadata
+// proto, and what value to pass in the txnDidNotUpdateMeta parameter of
+// PutIntent.
+//
+// Note that all nodes in this cluster understand separated intents, so can
+// fiddle with TxnDidNotUpdateMeta, which is not understood by older nodes
+// (which are no longer present, and will never again be present). Our
+// assumption is that 21.1 nodes will never be writing separated intents since
+// it is disabled by default and the code is known to have a bug. However they
+// can set MVCCMetadata.TxnDidNotUpdateMeta to true when writing interleaved
+// intents (this is harmless since interleaved intents do not invoke the
+// SingleDelete optimization).
+// - The txnDidNotUpdateMeta field is about what happened prior to this Put,
+//   and is intended to be passed through to Writer.PutIntent. It is only used
+//   by intentDemuxWriter when there was an existing separated intent that was
+//   written once and the writer is writing interleaved intents, and the true
+//   value enables SingleDelete of the existing separated intent. This
+//   transition can happen when an intent written at a 21.2 node is rewritten
+//   on a 21.1 node. But since 21.2 nodes never set
+//   MVCCMetadata.TxnDidNotUpdateMeta to true in a mixed version cluster (see
+//   next bullet), this will always be false in such a situation.
+// - The state field describes the preceding intent state. It is intended to
+//   be used for populating the MVCCMetadata.TxnDidNotUpdateMeta. This is
+//   where we use Writer.OverrideTxnDidNotUpdateMetaToFalse to override to
+//   false until there can never be 21.1 nodes.
+type txnDidNotUpdateMetaHelper struct {
+	txnDidNotUpdateMeta bool
+	state               PrecedingIntentState
+	w                   Writer
+}
+
+func (t txnDidNotUpdateMetaHelper) valueForPutIntent() bool {
+	return t.txnDidNotUpdateMeta
+}
+
+func (t txnDidNotUpdateMetaHelper) populateMeta(ctx context.Context, meta *enginepb.MVCCMetadata) {
+	if t.state == NoExistingIntent && !t.w.OverrideTxnDidNotUpdateMetaToFalse(ctx) {
+		meta.TxnDidNotUpdateMeta = &trueValue
+	} else {
+		// Absence represents false.
+		meta.TxnDidNotUpdateMeta = nil
+	}
+}
+
 // MVCCPut sets the value for a specified key. It will save the value
 // with different versions according to its timestamp and update the
 // key metadata. The timestamp must be passed as a parameter; using
@@ -1746,7 +1775,12 @@ func mvccPutInternal(
 	var separatedIntentCountDelta int
 	if newMeta.Txn != nil {
 		metaKeySize, metaValSize, separatedIntentCountDelta, err = buf.putIntentMeta(
-			ctx, writer, metaKey, precedingIntentState, txnDidNotUpdateMeta, newMeta)
+			ctx, writer, metaKey,
+			txnDidNotUpdateMetaHelper{
+				txnDidNotUpdateMeta: txnDidNotUpdateMeta,
+				state:               precedingIntentState,
+				w:                   writer,
+			}, newMeta)
 		if err != nil {
 			return err
 		}
@@ -3176,7 +3210,13 @@ func mvccResolveWriteIntent(
 			// to do anything to update the intent but to move the timestamp forward,
 			// even if it can.
 			metaKeySize, metaValSize, separatedIntentCountDelta, err = buf.putIntentMeta(
-				ctx, rw, metaKey, precedingIntentState, canSingleDelHelper.v(), &buf.newMeta)
+				ctx, rw, metaKey,
+				txnDidNotUpdateMetaHelper{
+					txnDidNotUpdateMeta: canSingleDelHelper.v(),
+					state:               precedingIntentState,
+					w:                   rw,
+				},
+				&buf.newMeta)
 		} else {
 			metaKeySize = int64(metaKey.EncodedSize())
 			separatedIntentCountDelta, err =

pkg/storage/mvcc_history_test.go

@@ -20,8 +20,10 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -109,7 +111,30 @@ func TestMVCCHistories(t *testing.T) {
 				"randomly setting enableSeparated: %t", enabledSeparated)
 		}
 		// We start from a clean slate in every test file.
-		engine, err := Open(ctx, InMemory(), CacheSize(1<<20 /* 1 MiB */), SetSeparatedIntents(!enabledSeparated))
+		engine, err := Open(ctx, InMemory(), CacheSize(1<<20 /* 1 MiB */),
+			SetSeparatedIntents(!enabledSeparated),
+			func(cfg *engineConfig) error {
+				if !overridden {
+					// Latest cluster version, since these tests are not ones where we
+					// are examining differences related to separated intents.
+					cfg.Settings = cluster.MakeTestingClusterSettings()
+				} else {
+					if !enabledSeparated {
+						// 21.1, which has the old code that is unaware about the changes
+						// we have made for OverrideTxnDidNotUpdateMetaToFalse. By using
+						// the latest cluster version, we effectively undo these changes.
+						cfg.Settings = cluster.MakeTestingClusterSettings()
+					} else if strings.Contains(path, "mixed_cluster") {
+						v21_1 := clusterversion.ByKey(clusterversion.V21_1)
+						cfg.Settings =
+							cluster.MakeTestingClusterSettingsWithVersions(v21_1, v21_1, true)
+					} else {
+						// Latest cluster version.
+						cfg.Settings = cluster.MakeTestingClusterSettings()
+					}
+				}
+				return nil
+			})
 		if err != nil {
 			t.Fatal(err)
 		}

pkg/storage/pebble.go

@@ -1010,6 +1010,11 @@ func (p *Pebble) PutEngineKey(key EngineKey, value []byte) error {
 	return p.db.Set(key.Encode(), value, pebble.Sync)
 }
 
+// OverrideTxnDidNotUpdateMetaToFalse implements the Engine interface.
+func (p *Pebble) OverrideTxnDidNotUpdateMetaToFalse(ctx context.Context) bool {
+	return overrideTxnDidNotUpdateMetaToFalse(ctx, p.settings)
+}
+
 // IsSeparatedIntentsEnabledForTesting implements the Engine interface.
 func (p *Pebble) IsSeparatedIntentsEnabledForTesting(ctx context.Context) bool {
 	return !p.disableSeparatedIntents
@@ -1265,7 +1270,7 @@ func (p *Pebble) GetAuxiliaryDir() string {
 func (p *Pebble) NewBatch() Batch {
 	return newPebbleBatch(
 		p.db, p.db.NewIndexedBatch(), false, /* writeOnly */
-		p.disableSeparatedIntents)
+		p.disableSeparatedIntents, overrideTxnDidNotUpdateMetaToFalse(context.TODO(), p.settings))
 }
 
 // NewReadOnly implements the Engine interface.
@@ -1275,7 +1280,8 @@ func (p *Pebble) NewReadOnly() ReadWriter {
 
 // NewUnindexedBatch implements the Engine interface.
 func (p *Pebble) NewUnindexedBatch(writeOnly bool) Batch {
-	return newPebbleBatch(p.db, p.db.NewBatch(), writeOnly, p.disableSeparatedIntents)
+	return newPebbleBatch(p.db, p.db.NewBatch(), writeOnly, p.disableSeparatedIntents,
+		overrideTxnDidNotUpdateMetaToFalse(context.TODO(), p.settings))
 }
 
 // NewSnapshot implements the Engine interface.
@@ -1806,6 +1812,10 @@ func (p *pebbleReadOnly) LogLogicalOp(op MVCCLogicalOpType, details MVCCLogicalO
 	panic("not implemented")
 }
 
+func (p *pebbleReadOnly) OverrideTxnDidNotUpdateMetaToFalse(ctx context.Context) bool {
+	panic("not implemented")
+}
+
 // pebbleSnapshot represents a snapshot created using Pebble.NewSnapshot().
 type pebbleSnapshot struct {
 	snapshot *pebble.Snapshot
@@ -2097,3 +2107,11 @@ func pebbleExportToSst(
 
 	return rows.BulkOpSummary, MVCCKey{Key: resumeKey, Timestamp: resumeTS}, nil
 }
+
+// See the comment for Writer.OverrideTxnDidNotUpdateMetaToFalse.
+func overrideTxnDidNotUpdateMetaToFalse(ctx context.Context, st *cluster.Settings) bool {
+	// The fix to the single delete bug in 21.2 has nothing to do with
+	// PebbleFormatVersioned, but both are part of the 21.2 beta, which will be
+	// the earliest production version of 21.2.
+	return !st.Version.ActiveVersionOrEmpty(ctx).IsActive(clusterversion.PebbleFormatVersioned)
+}

pkg/storage/pebble_batch.go

@@ -43,13 +43,14 @@ type pebbleBatch struct {
 	// engine state. This relies on the fact that all pebbleIterators created
 	// here are marked as reusable, which causes pebbleIterator.Close to not
 	// close iter. iter will be closed when pebbleBatch.Close is called.
-	prefixIter       pebbleIterator
-	normalIter       pebbleIterator
-	prefixEngineIter pebbleIterator
-	normalEngineIter pebbleIterator
-	iter             cloneableIter
-	writeOnly        bool
-	closed           bool
+	prefixIter                         pebbleIterator
+	normalIter                         pebbleIterator
+	prefixEngineIter                   pebbleIterator
+	normalEngineIter                   pebbleIterator
+	iter                               cloneableIter
+	writeOnly                          bool
+	closed                             bool
+	overrideTxnDidNotUpdateMetaToFalse bool
 
 	wrappedIntentWriter intentDemuxWriter
 	// scratch space for wrappedIntentWriter.
@@ -66,7 +67,11 @@ var pebbleBatchPool = sync.Pool{
 
 // Instantiates a new pebbleBatch.
 func newPebbleBatch(
-	db *pebble.DB, batch *pebble.Batch, writeOnly bool, disableSeparatedIntents bool,
+	db *pebble.DB,
+	batch *pebble.Batch,
+	writeOnly bool,
+	disableSeparatedIntents bool,
+	overrideTxnDidNotUpdateMetaToFalse bool,
 ) *pebbleBatch {
 	pb := pebbleBatchPool.Get().(*pebbleBatch)
 	*pb = pebbleBatch{
@@ -94,6 +99,10 @@ func newPebbleBatch(
 			reusable:      true,
 		},
 		writeOnly: writeOnly,
+		// A batch is not long-lived, so using the same value (which could be
+		// slightly stale) is fine for the lifetime of the batch. Staleness is not
+		// a correctness issue.
+		overrideTxnDidNotUpdateMetaToFalse: overrideTxnDidNotUpdateMetaToFalse,
 	}
 	pb.wrappedIntentWriter = wrapIntentWriter(context.Background(), pb, disableSeparatedIntents)
 	return pb
@@ -474,6 +483,11 @@ func (p *pebbleBatch) PutEngineKey(key EngineKey, value []byte) error {
 	return p.batch.Set(p.buf, value, nil)
 }
 
+// OverrideTxnDidNotUpdateMetaToFalse implements the Batch interface.
+func (p *pebbleBatch) OverrideTxnDidNotUpdateMetaToFalse(_ context.Context) bool {
+	return p.overrideTxnDidNotUpdateMetaToFalse
+}
+
 func (p *pebbleBatch) put(key MVCCKey, value []byte) error {
 	if len(key.Key) == 0 {
 		return emptyKeyError()