row/fetcher: clean up and harden TestRowFetcherMVCCMetadata

This commit refactors `TestRowFetcherMVCCMetadata` a bit to explicitly
request `crdb_internal_mvcc_timestamp` as opposed to fishing it out of
the fetcher, which allows removing an exported method from the latter.

This commit also hardens the test to de-flake it when write buffering is
enabled. The test scans the storage engine directly, so we need to tweak
a testing knob to ensure that COMMIT blocks until the writes are
committed into pebble.

Release note: None

Author: yuzefovich

pkg/sql/row/BUILD.bazel

@@ -127,6 +127,7 @@ go_test(
         "//pkg/sql",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/bootstrap",
+        "//pkg/sql/catalog/colinfo",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/desctestutils",

pkg/sql/row/fetcher.go

@@ -1284,12 +1284,6 @@ func (rf *Fetcher) NextRowDecodedInto(
 	return true, spanID, nil
 }
 
-// RowLastModified may only be called after NextRow has returned a non-nil row
-// and returns the timestamp of the last modification to that row.
-func (rf *Fetcher) RowLastModified() hlc.Timestamp {
-	return rf.table.rowLastModified
-}
-
 // RowIsDeleted may only be called after NextRow has returned a non-nil row and
 // returns true if that row was most recently deleted. This method is only
 // meaningful when the configured KVBatchFetcher returns deletion tombstones, which
@@ -1306,7 +1300,7 @@ func (rf *Fetcher) finalizeRow() error {
 		// TODO (rohany): Datums are immutable, so we can't store a DDecimal on the
 		//  fetcher and change its contents with each row. If that assumption gets
 		//  lifted, then we can avoid an allocation of a new decimal datum here.
-		dec := rf.args.Alloc.NewDDecimal(tree.DDecimal{Decimal: eval.TimestampToDecimal(rf.RowLastModified())})
+		dec := rf.args.Alloc.NewDDecimal(tree.DDecimal{Decimal: eval.TimestampToDecimal(rf.table.rowLastModified)})
 		table.row[table.timestampOutputIdx] = rowenc.EncDatum{Datum: dec}
 	}
 	if table.oidOutputIdx != noOutputColumn {

pkg/sql/row/fetcher_mvcc_test.go

@@ -15,11 +15,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/row"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -74,7 +74,17 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()
-	srv, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{})
+	srv, db, kvDB := serverutils.StartServer(t, base.TestServerArgs{
+		Knobs: base.TestingKnobs{
+			Store: &kvserver.StoreTestingKnobs{
+				// With write buffering enabled on the KV client, we need to
+				// ensure that committing a write to the raft log returns only
+				// when it's applied to pebble (since we scan the engine
+				// directly in slurpUserDataKVs).
+				DisableCanAckBeforeApplication: true,
+			},
+		},
+	})
 	defer srv.Stopper().Stop(ctx)
 	s := srv.ApplicationLayer()
 	codec := s.Codec()
@@ -88,9 +98,13 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 		FAMILY (a, b, c), FAMILY (d)
 	)`)
 	desc := desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, `d`, `parent`)
+	// Request crdb_internal_mvcc_timestamp column in addition to all user
+	// columns from the table.
+	fetchColumnIDs := desc.PublicColumnIDs()
+	fetchColumnIDs = append(fetchColumnIDs, colinfo.MVCCTimestampColumnID)
 	var spec fetchpb.IndexFetchSpec
 	if err := rowenc.InitIndexFetchSpec(
-		&spec, codec, desc, desc.GetPrimaryIndex(), desc.PublicColumnIDs(),
+		&spec, codec, desc, desc.GetPrimaryIndex(), fetchColumnIDs,
 	); err != nil {
 		t.Fatal(err)
 	}
@@ -129,12 +143,13 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 				break
 			}
 			row := rowWithMVCCMetadata{
-				RowIsDeleted:    rf.RowIsDeleted(),
-				RowLastModified: eval.TimestampToDecimalDatum(rf.RowLastModified()).String(),
+				RowIsDeleted: rf.RowIsDeleted(),
 			}
 			for _, datum := range datums {
 				if datum == tree.DNull {
 					row.PrimaryKey = append(row.PrimaryKey, `NULL`)
+				} else if d, ok := datum.(*tree.DDecimal); ok {
+					row.RowLastModified = d.String()
 				} else {
 					row.PrimaryKey = append(row.PrimaryKey, string(*datum.(*tree.DString)))
 				}
@@ -151,8 +166,8 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 	END;`).Scan(&ts1)
 
 	if actual, expected := kvsToRows(slurpUserDataKVs(t, store.TODOEngine(), codec)), []rowWithMVCCMetadata{
-		{[]string{`1`, `a`, `a`, `a`}, false, ts1},
-		{[]string{`2`, `b`, `b`, `b`}, false, ts1},
+		{PrimaryKey: []string{`1`, `a`, `a`, `a`}, RowIsDeleted: false, RowLastModified: ts1},
+		{PrimaryKey: []string{`2`, `b`, `b`, `b`}, RowIsDeleted: false, RowLastModified: ts1},
 	}; !reflect.DeepEqual(expected, actual) {
 		t.Errorf(`expected %v got %v`, expected, actual)
 	}
@@ -165,8 +180,8 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 	END;`).Scan(&ts2)
 
 	if actual, expected := kvsToRows(slurpUserDataKVs(t, store.TODOEngine(), codec)), []rowWithMVCCMetadata{
-		{[]string{`1`, `NULL`, `NULL`, `NULL`}, false, ts2},
-		{[]string{`2`, `b`, `b`, `NULL`}, false, ts2},
+		{PrimaryKey: []string{`1`, `NULL`, `NULL`, `NULL`}, RowIsDeleted: false, RowLastModified: ts2},
+		{PrimaryKey: []string{`2`, `b`, `b`, `NULL`}, RowIsDeleted: false, RowLastModified: ts2},
 	}; !reflect.DeepEqual(expected, actual) {
 		t.Errorf(`expected %v got %v`, expected, actual)
 	}
@@ -177,8 +192,8 @@ func TestRowFetcherMVCCMetadata(t *testing.T) {
 		SELECT cluster_logical_timestamp();
 	END;`).Scan(&ts3)
 	if actual, expected := kvsToRows(slurpUserDataKVs(t, store.TODOEngine(), codec)), []rowWithMVCCMetadata{
-		{[]string{`1`, `NULL`, `NULL`, `NULL`}, true, ts3},
-		{[]string{`2`, `b`, `b`, `NULL`}, false, ts2},
+		{PrimaryKey: []string{`1`, `NULL`, `NULL`, `NULL`}, RowIsDeleted: true, RowLastModified: ts3},
+		{PrimaryKey: []string{`2`, `b`, `b`, `NULL`}, RowIsDeleted: false, RowLastModified: ts2},
 	}; !reflect.DeepEqual(expected, actual) {
 		t.Errorf(`expected %v got %v`, expected, actual)
 	}