sqlbase: prohibit copying DatumAlloc by value

This commit adds `_ util.NoCopy` to `DatumAlloc` struct to prevent us
from misusing it. A few places failed the linter, and those have been
addressed, but there was no performance problems AFAICT due to the
removed copies by value.

Release note: None

pkg/sql/colexec/cfetcher.go

@@ -162,7 +162,7 @@ func (m colIdxMap) get(c sqlbase.ColumnID) (int, bool) {
 //   }
 type cFetcher struct {
 	// table is the table that's configured for fetching.
-	table cTableInfo
+	table *cTableInfo
 
 	// reverse denotes whether or not the spans should be read in reverse
 	// or not when StartScan is invoked.
@@ -266,7 +266,6 @@ func (rf *cFetcher) Init(
 	}
 
 	tableArgs := tables[0]
-	oldTable := rf.table
 
 	m := colIdxMap{
 		vals: make(sqlbase.ColumnIDs, 0, len(tableArgs.ColIdxMap)),
@@ -278,20 +277,13 @@ func (rf *cFetcher) Init(
 	}
 	sort.Sort(m)
 	colDescriptors := tableArgs.Cols
-	table := cTableInfo{
+	table := &cTableInfo{
 		spans:            tableArgs.Spans,
 		desc:             tableArgs.Desc,
 		colIdxMap:        m,
 		index:            tableArgs.Index,
 		isSecondaryIndex: tableArgs.IsSecondaryIndex,
 		cols:             colDescriptors,
-
-		// These slice fields might get re-allocated below, so reslice them from
-		// the old table here in case they've got enough capacity already.
-		indexColOrdinals:       oldTable.indexColOrdinals[:0],
-		extraValColOrdinals:    oldTable.extraValColOrdinals[:0],
-		allIndexColOrdinals:    oldTable.allIndexColOrdinals[:0],
-		allExtraValColOrdinals: oldTable.allExtraValColOrdinals[:0],
 	}
 
 	typs := make([]*types.T, len(colDescriptors))
@@ -403,7 +395,7 @@ func (rf *cFetcher) Init(
 	if err != nil {
 		return err
 	}
-	if cHasExtraCols(&table) {
+	if cHasExtraCols(table) {
 		// Unique secondary indexes have a value that is the
 		// primary index key.
 		// Primary indexes only contain ascendingly-encoded
@@ -908,7 +900,7 @@ func (rf *cFetcher) getDatumAt(colIdx int, rowIdx int, typ *types.T) tree.Datum
 func (rf *cFetcher) processValue(
 	ctx context.Context, familyID sqlbase.FamilyID,
 ) (prettyKey string, prettyValue string, err error) {
-	table := &rf.table
+	table := rf.table
 
 	if rf.traceKV {
 		var buf strings.Builder
@@ -1211,7 +1203,7 @@ func (rf *cFetcher) processValueTuple(
 }
 
 func (rf *cFetcher) fillNulls() error {
-	table := &rf.table
+	table := rf.table
 	if rf.machine.remainingValueColsByIdx.Empty() {
 		return nil
 	}

pkg/sql/conn_executor.go

@@ -592,7 +592,7 @@ func (s *Server) newConnExecutor(
 			settings: s.cfg.Settings,
 		},
 		memMetrics: memMetrics,
-		planner:    planner{execCfg: s.cfg},
+		planner:    planner{execCfg: s.cfg, alloc: &sqlbase.DatumAlloc{}},
 
 		// ctxHolder will be reset at the start of run(). We only define
 		// it here so that an early call to close() doesn't panic.

pkg/sql/copy.go

@@ -104,7 +104,7 @@ func newCopyMachine(
 		columns: n.Columns,
 		txnOpt:  txnOpt,
 		// The planner will be prepared before use.
-		p:              planner{execCfg: execCfg},
+		p:              planner{execCfg: execCfg, alloc: &sqlbase.DatumAlloc{}},
 		execInsertPlan: execInsertPlan,
 	}
 

pkg/sql/copy_file_upload.go

@@ -57,7 +57,7 @@ func newFileUploadMachine(
 	c := &copyMachine{
 		conn: conn,
 		// The planner will be prepared before use.
-		p: planner{execCfg: execCfg},
+		p: planner{execCfg: execCfg, alloc: &sqlbase.DatumAlloc{}},
 	}
 	f = &fileUploadMachine{
 		c:  c,

pkg/sql/create_table.go

@@ -394,7 +394,7 @@ func (n *createTableNode) startExec(params runParams) error {
 				desc.Columns,
 				row.SkipFKs,
 				nil, /* fkTables */
-				&params.p.alloc)
+				params.p.alloc)
 			if err != nil {
 				return err
 			}

pkg/sql/delete_range.go

@@ -192,7 +192,7 @@ func (d *deleteRangeNode) startExec(params runParams) error {
 		sqlbase.ScanLockingStrength_FOR_NONE,
 		false, /* returnRangeInfo */
 		false, /* isCheck */
-		&params.p.alloc,
+		params.p.alloc,
 		allTables...,
 	); err != nil {
 		return err

pkg/sql/join_test.go

@@ -24,7 +24,7 @@ import (
 func newTestScanNode(kvDB *kv.DB, tableName string) (*scanNode, error) {
 	desc := sqlbase.GetImmutableTableDescriptor(kvDB, keys.SystemSQLCodec, sqlutils.TestDB, tableName)
 
-	p := planner{}
+	p := planner{alloc: &sqlbase.DatumAlloc{}}
 	scan := p.Scan()
 	scan.desc = desc
 	err := scan.initDescDefaults(p.curPlan.deps, publicColumnsCfg)

pkg/sql/opt_exec_factory.go

@@ -1231,7 +1231,7 @@ func (ef *execFactory) ConstructInsert(
 	}
 	// Create the table inserter, which does the bulk of the work.
 	ri, err := row.MakeInserter(
-		ctx, ef.planner.txn, ef.planner.ExecCfg().Codec, tabDesc, colDescs, checkFKs, fkTables, &ef.planner.alloc,
+		ctx, ef.planner.txn, ef.planner.ExecCfg().Codec, tabDesc, colDescs, checkFKs, fkTables, ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1296,7 +1296,7 @@ func (ef *execFactory) ConstructInsertFastPath(
 
 	// Create the table inserter, which does the bulk of the work.
 	ri, err := row.MakeInserter(
-		ctx, ef.planner.txn, ef.planner.ExecCfg().Codec, tabDesc, colDescs, row.SkipFKs, nil /* fkTables */, &ef.planner.alloc,
+		ctx, ef.planner.txn, ef.planner.ExecCfg().Codec, tabDesc, colDescs, row.SkipFKs, nil /* fkTables */, ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1408,7 +1408,7 @@ func (ef *execFactory) ConstructUpdate(
 		row.UpdaterDefault,
 		checkFKs,
 		ef.planner.EvalContext(),
-		&ef.planner.alloc,
+		ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1554,7 +1554,7 @@ func (ef *execFactory) ConstructUpsert(
 		insertColDescs,
 		checkFKs,
 		fkTables,
-		&ef.planner.alloc,
+		ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1572,7 +1572,7 @@ func (ef *execFactory) ConstructUpsert(
 		row.UpdaterDefault,
 		checkFKs,
 		ef.planner.EvalContext(),
-		&ef.planner.alloc,
+		ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1599,7 +1599,7 @@ func (ef *execFactory) ConstructUpsert(
 			insertCols: ri.InsertCols,
 			tw: optTableUpserter{
 				ri:            ri,
-				alloc:         &ef.planner.alloc,
+				alloc:         ef.planner.alloc,
 				canaryOrdinal: int(canaryCol),
 				fkTables:      fkTables,
 				fetchCols:     fetchColDescs,
@@ -1688,7 +1688,7 @@ func (ef *execFactory) ConstructDelete(
 		fetchColDescs,
 		checkFKs,
 		ef.planner.EvalContext(),
-		&ef.planner.alloc,
+		ef.planner.alloc,
 	)
 	if err != nil {
 		return nil, err
@@ -1703,7 +1703,7 @@ func (ef *execFactory) ConstructDelete(
 	*del = deleteNode{
 		source: input.(planNode),
 		run: deleteRun{
-			td: tableDeleter{rd: rd, alloc: &ef.planner.alloc},
+			td: tableDeleter{rd: rd, alloc: ef.planner.alloc},
 		},
 	}
 

pkg/sql/planner.go

@@ -182,7 +182,7 @@ type planner struct {
 	// Use a common datum allocator across all the plan nodes. This separates the
 	// plan lifetime from the lifetime of returned results allowing plan nodes to
 	// be pool allocated.
-	alloc sqlbase.DatumAlloc
+	alloc *sqlbase.DatumAlloc
 
 	// optPlanningCtx stores the optimizer planning context, which contains
 	// data structures that can be reused between queries (for efficiency).
@@ -269,7 +269,7 @@ func newInternalPlanner(
 		ts = readTimestamp.GoTime()
 	}
 
-	p := &planner{execCfg: execCfg}
+	p := &planner{execCfg: execCfg, alloc: &sqlbase.DatumAlloc{}}
 
 	p.txn = txn
 	p.stmt = nil

pkg/sql/planner_test.go

@@ -15,12 +15,13 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 )
 
 func TestTypeAsString(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	p := planner{}
+	p := planner{alloc: &sqlbase.DatumAlloc{}}
 	testData := []struct {
 		expr        tree.Expr
 		expected    string

pkg/sql/rowcontainer/disk_row_container.go

@@ -65,7 +65,7 @@ type DiskRowContainer struct {
 	diskMonitor *mon.BytesMonitor
 	engine      diskmap.Factory
 
-	datumAlloc sqlbase.DatumAlloc
+	datumAlloc *sqlbase.DatumAlloc
 }
 
 var _ SortableRowContainer = &DiskRowContainer{}
@@ -92,6 +92,7 @@ func MakeDiskRowContainer(
 		scratchEncRow: make(sqlbase.EncDatumRow, len(types)),
 		diskMonitor:   diskMonitor,
 		engine:        e,
+		datumAlloc:    &sqlbase.DatumAlloc{},
 	}
 	d.bufferedRows = d.diskMap.NewBatchWriter()
 
@@ -144,14 +145,14 @@ func (d *DiskRowContainer) AddRow(ctx context.Context, row sqlbase.EncDatumRow)
 	for i, orderInfo := range d.ordering {
 		col := orderInfo.ColIdx
 		var err error
-		d.scratchKey, err = row[col].Encode(d.types[col], &d.datumAlloc, d.encodings[i], d.scratchKey)
+		d.scratchKey, err = row[col].Encode(d.types[col], d.datumAlloc, d.encodings[i], d.scratchKey)
 		if err != nil {
 			return err
 		}
 	}
 	for _, i := range d.valueIdxs {
 		var err error
-		d.scratchVal, err = row[i].Encode(d.types[i], &d.datumAlloc, sqlbase.DatumEncoding_VALUE, d.scratchVal)
+		d.scratchVal, err = row[i].Encode(d.types[i], d.datumAlloc, sqlbase.DatumEncoding_VALUE, d.scratchVal)
 		if err != nil {
 			return err
 		}

pkg/sql/rowcontainer/disk_row_container_test.go

@@ -150,14 +150,14 @@ func TestDiskRowContainer(t *testing.T) {
 					// Ensure the datum fields are set and no errors occur when
 					// decoding.
 					for i, encDatum := range readRow {
-						if err := encDatum.EnsureDecoded(typs[i], &d.datumAlloc); err != nil {
+						if err := encDatum.EnsureDecoded(typs[i], d.datumAlloc); err != nil {
 							t.Fatal(err)
 						}
 					}
 
 					// Check equality of the row we wrote and the row we read.
 					for i := range row {
-						if cmp, err := readRow[i].Compare(typs[i], &d.datumAlloc, &evalCtx, &row[i]); err != nil {
+						if cmp, err := readRow[i].Compare(typs[i], d.datumAlloc, &evalCtx, &row[i]); err != nil {
 							t.Fatal(err)
 						} else if cmp != 0 {
 							t.Fatalf("encoded %s but decoded %s", row.String(typs), readRow.String(typs))
@@ -213,14 +213,14 @@ func TestDiskRowContainer(t *testing.T) {
 					// Ensure datum fields are set and no errors occur when
 					// decoding.
 					for i, encDatum := range row {
-						if err := encDatum.EnsureDecoded(types[i], &d.datumAlloc); err != nil {
+						if err := encDatum.EnsureDecoded(types[i], d.datumAlloc); err != nil {
 							t.Fatal(err)
 						}
 					}
 
 					// Check sorted order.
 					if cmp, err := compareRows(
-						types, sortedRows.EncRow(numKeysRead), row, &evalCtx, &d.datumAlloc, ordering,
+						types, sortedRows.EncRow(numKeysRead), row, &evalCtx, d.datumAlloc, ordering,
 					); err != nil {
 						t.Fatal(err)
 					} else if cmp != 0 {

pkg/sql/rowexec/distinct.go

@@ -112,12 +112,20 @@ func newDistinct(
 	var returnProcessor execinfra.RowSourcedProcessor = d
 	if allSorted {
 		// We can use the faster sortedDistinct processor.
+		// TODO(asubiotto): We should have a distinctBase, rather than making a copy
+		// of a distinct processor.
 		sd := &sortedDistinct{
-			distinct: *d,
+			distinct: distinct{
+				input:            input,
+				orderedCols:      spec.OrderedColumns,
+				distinctCols:     distinctCols,
+				memAcc:           memMonitor.MakeBoundAccount(),
+				types:            input.OutputTypes(),
+				nullsAreDistinct: spec.NullsAreDistinct,
+				errorOnDup:       spec.ErrorOnDup,
+			},
 		}
 		// Set d to the new distinct copy for further initialization.
-		// TODO(asubiotto): We should have a distinctBase, rather than making a copy
-		// of a distinct processor.
 		d = &sd.distinct
 		returnProcessor = sd
 	}

pkg/sql/rowexec/indexbackfiller.go

@@ -106,7 +106,7 @@ func (ib *indexBackfiller) prepare(ctx context.Context) error {
 	return nil
 }
 
-func (ib indexBackfiller) close(ctx context.Context) {
+func (ib *indexBackfiller) close(ctx context.Context) {
 	ib.adder.Close(ctx)
 }
 

pkg/sql/sqlbase/datum_alloc.go

@@ -10,11 +10,17 @@
 
 package sqlbase
 
-import "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+import (
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/util"
+)
 
 // DatumAlloc provides batch allocation of datum pointers, amortizing the cost
 // of the allocations.
+// NOTE: it *must* be passed in by a pointer.
 type DatumAlloc struct {
+	_ util.NoCopy
+
 	datumAlloc        []tree.Datum
 	dintAlloc         []tree.DInt
 	dfloatAlloc       []tree.DFloat