sql/rowenc: reduce index key prefix calls

[annrpom]

sql/rowenc: reduce index key prefix calls

This patch removes redundant calls to MakeIndexKeyPrefix during
the construction of IndexEntrys by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: #137798

Release note: None

---

sql/rowexec: run BenchmarkIndexBackfill on-disk

Epic: none
Release note: None

[cockroach-teamcity]

This change is 

[annrpom]

using benchdiff:

name                             old time/op    new time/op    delta
IndexBackfill/10,000_rows-10       53.6ms ±14%    49.4ms ± 2%  -7.76%  (p=0.027 n=10+8)
IndexBackfill/100_rows-10          43.6ms ± 9%    43.9ms ± 4%    ~     (p=0.631 n=10+10)
IndexBackfill/1,000,000_rows-10     11.2s ± 7%     11.1s ± 5%    ~     (p=0.730 n=9+9)

name                             old alloc/op   new alloc/op   delta
IndexBackfill/100_rows-10          7.46MB ±13%    7.68MB ± 8%    ~     (p=0.218 n=10+10)
IndexBackfill/10,000_rows-10       32.1MB ±12%    32.7MB ± 7%    ~     (p=0.393 n=10+10)
IndexBackfill/1,000,000_rows-10    6.06GB ± 7%    6.24GB ± 6%    ~     (p=0.123 n=10+10)

name                             old allocs/op  new allocs/op  delta
IndexBackfill/10,000_rows-10         136k ± 0%      126k ± 1%  -7.36%  (p=0.000 n=9+9)
IndexBackfill/1,000,000_rows-10     26.2M ± 4%     25.9M ± 0%    ~     (p=1.000 n=10+8)
IndexBackfill/100_rows-10           40.6k ± 2%     43.2k ± 7%  +6.22%  (p=0.001 n=9+10)

we seem to consistently get an improvement for allocs/op for our 10,000_rows table; everything else is gone with the wind

[annrpom]

rebased with the concurrency updates on master:

name                             old time/op    new time/op    delta
IndexBackfill/100_rows-10          21.1ms ± 2%    21.4ms ± 4%    ~     (p=0.173 n=8+10)
IndexBackfill/10,000_rows-10       38.1ms ± 3%    38.1ms ± 6%    ~     (p=0.696 n=8+10)
IndexBackfill/1,000,000_rows-10     10.9s ± 5%     10.7s ± 1%    ~     (p=0.278 n=10+9)

name                             old alloc/op   new alloc/op   delta
IndexBackfill/100_rows-10          4.52MB ± 3%    4.81MB ±20%    ~     (p=0.053 n=9+10)
IndexBackfill/10,000_rows-10       25.8MB ± 8%    24.9MB ± 3%    ~     (p=0.156 n=10+9)
IndexBackfill/1,000,000_rows-10    6.25GB ± 3%    6.21GB ± 3%    ~     (p=0.353 n=10+10)

name                             old allocs/op  new allocs/op  delta
IndexBackfill/10,000_rows-10         120k ± 2%      109k ± 2%  -9.40%  (p=0.000 n=10+10)
IndexBackfill/1,000,000_rows-10     26.9M ± 0%     25.8M ± 1%  -3.84%  (p=0.000 n=8+10)
IndexBackfill/100_rows-10           24.0k ± 1%     24.7k ± 8%    ~     (p=0.156 n=9+10)

we now consistently see both 10k and 1mil improvements in allocs/op

[annrpom]

i'm still not 100% sure why yet, but the inverted index tests were failing due to the shared key prefixes change; specifically, we need to look at a case where the inverted column (?term) has dupes:

cockroach/pkg/sql/logictest/testdata/logic_test/inverted_index

Line 1551 in 5a31d79

INSERT INTO c VALUES(1, ARRAY[], ARRAY['foo', 'bar', 'baz'])

cockroach/pkg/sql/logictest/testdata/logic_test/inverted_index

Line 1592 in 5a31d79

INSERT INTO c VALUES(5, ARRAY[], ARRAY[NULL, NULL])

when we are creating our batch of index entries, an encoding for (1, [], ...) gets created, making our entries look something like [... (/Tenant/10/Table/116/3/[]/1/0, ...) ...]. as soon as we get to constructing the encoded kv pair for our other [] value, our entries all of the sudden looks like this:
[... (/Tenant/10/Table/116/3/[]/5/0, ...) ... (/Tenant/10/Table/116/3/[]/5/0, ...)]
instead of [... (/Tenant/10/Table/116/3/[]/1/0, ...) ... (/Tenant/10/Table/116/3/[]/5/0, ...)]

i tried to see if this was an instance of me assuming something is passed/call by value instead of by ref, but no luck so far

[rafiss]

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

---

pkg/sql/rowenc/index_encoding.go line 1651 at r3 (raw file):

	tableDesc catalog.TableDescriptor,
	indexes []catalog.Index,
	keyPrefixMap map[descpb.IndexID][]byte,

instead of making this a map, let's make this a slice. the function comment should be updated to say that the indexes and the keyPrefixes slice should both have the same ordering. i expect that to result in less overhead, since looking up in a slice is cheaper than looking up in a map.

we can create this keyPrefixes slice in the same place where we create the indexesToEncode slice:

cockroach/pkg/sql/backfill/backfill.go

Lines 775 to 778 in cca9535

	ib.indexesToEncode = ib.added
	if len(ib.predicates) > 0 {
	ib.indexesToEncode = make([]catalog.Index, 0, len(ib.added))
	}

cockroach/pkg/sql/backfill/backfill.go

Lines 944 to 965 in cca9535

	ib.indexesToEncode = ib.indexesToEncode[:0]
	for _, idx := range ib.added {
	if !idx.IsPartial() {
	// If the index is not a partial index, all rows should have
	// an entry.
	ib.indexesToEncode = append(ib.indexesToEncode, idx)
	continue
	}
	// If the index is a partial index, only include it if the
	// predicate expression evaluates to true.
	texpr := ib.predicates[idx.GetID()]
	val, err := eval.Expr(ctx, ib.evalCtx, texpr)
	if err != nil {
	return nil, nil, memUsedPerChunk, err
	}
	if val == tree.DBoolTrue {
	ib.indexesToEncode = append(ib.indexesToEncode, idx)
	}
	}

unrelated to this PR, that second snippet makes me realize how much more expensive this is if any partial indexes are being backfilled. we have to reallocate re-populate ib.indexesToEncode for each row in that case.

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @yuzefovich)

---

pkg/sql/rowenc/index_encoding.go line 1651 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

instead of making this a map, let's make this a slice. the function comment should be updated to say that the indexes and the keyPrefixes slice should both have the same ordering. i expect that to result in less overhead, since looking up in a slice is cheaper than looking up in a map.

we can create this keyPrefixes slice in the same place where we create the indexesToEncode slice:

cockroach/pkg/sql/backfill/backfill.go

Lines 775 to 778 in cca9535

	ib.indexesToEncode = ib.added
	if len(ib.predicates) > 0 {
	ib.indexesToEncode = make([]catalog.Index, 0, len(ib.added))
	}

cockroach/pkg/sql/backfill/backfill.go

Lines 944 to 965 in cca9535

	ib.indexesToEncode = ib.indexesToEncode[:0]
	for _, idx := range ib.added {
	if !idx.IsPartial() {
	// If the index is not a partial index, all rows should have
	// an entry.
	ib.indexesToEncode = append(ib.indexesToEncode, idx)
	continue
	}
	// If the index is a partial index, only include it if the
	// predicate expression evaluates to true.
	texpr := ib.predicates[idx.GetID()]
	val, err := eval.Expr(ctx, ib.evalCtx, texpr)
	if err != nil {
	return nil, nil, memUsedPerChunk, err
	}
	if val == tree.DBoolTrue {
	ib.indexesToEncode = append(ib.indexesToEncode, idx)
	}
	}

unrelated to this PR, that second snippet makes me realize how much more expensive this is if any partial indexes are being backfilled. we have to reallocate ib.indexesToEncode for each row in that case.

ok done; can you check me on this change i made:

cockroach/pkg/sql/backfill/backfill.go

Lines 970 to 972 in 774bc22

	} else {
	ib.keyPrefixes[i] = nil
	}

my thought was that unless we clear out these key prefixes, our indexes and keyPrefixes slice are not guaranteed to maintain the same ordering as eachother (since in the case of partial indexes, we only include the index if the pred evals to true for that row). though this would mean no improvement in the case of any partial indexes being backfilled. maybe there is another way to handle this case with slices, wdyt

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @yuzefovich)

---

pkg/sql/rowenc/index_encoding.go line 1651 at r3 (raw file):

Previously, annrpom (annie pompa) wrote…

ok done; can you check me on this change i made:

cockroach/pkg/sql/backfill/backfill.go

Lines 970 to 972 in 774bc22

	} else {
	ib.keyPrefixes[i] = nil
	}

my thought was that unless we clear out these key prefixes, our indexes and keyPrefixes slice are not guaranteed to maintain the same ordering as eachother (since in the case of partial indexes, we only include the index if the pred evals to true for that row). though this would mean no improvement in the case of any partial indexes being backfilled. maybe there is another way to handle this case with slices, wdyt

i believe the logic works, but IMO it's hard to understand the code for two reasons:

• making the computation lazy makes it harder to understand which parts of the code will actually compute the value.
• modifying function parameters makes it harder to reason about what the function's inputs and outputs are.

i left a suggestion above

---

pkg/sql/backfill/backfill.go line 780 at r5 (raw file):

	// reset in BuildIndexEntriesChunk for every row added.
	ib.indexesToEncode = ib.added
	ib.keyPrefixes = make([][]byte, len(ib.added))

instead of computing the prefixes lazily inside of EncodeSecondaryIndexes, they can all just be computed here with an additional loop.

---

pkg/sql/backfill/backfill.go line 970 at r5 (raw file):

				if val == tree.DBoolTrue {
					ib.indexesToEncode = append(ib.indexesToEncode, idx)
				} else {

IMO this would be easier to understand if the keyPrefixes logic mirrored the indexesToEncode logic:

		if len(ib.predicates) > 0 {
			ib.indexesToEncode = ib.indexesToEncode[:0]
			ib.keyPrefixes = ib.keyPrefixes[:0]
			for i, idx := range ib.added {
				if !idx.IsPartial() {
					// If the index is not a partial index, all rows should have
					// an entry.
					ib.indexesToEncode = append(ib.indexesToEncode, idx)
					ib.keyPrefixes = append(ib.keyPrefixes, MakeIndexKeyPrefix(...))
					continue
				}

				// If the index is a partial index, only include it if the
				// predicate expression evaluates to true.
				texpr := ib.predicates[idx.GetID()]

				val, err := eval.Expr(ctx, ib.evalCtx, texpr)
				if err != nil {
					return nil, nil, memUsedPerChunk, err
				}

				if val == tree.DBoolTrue {
					ib.indexesToEncode = append(ib.indexesToEncode, idx)
					ib.keyPrefixes = append(ib.indexesToEncode, MakeIndexKeyPrefix(...))
				}
			}
		}

we can worry about optimizing this for partial indexes later (for example, by saving all of the prefixes somewhere so we can avoid calling MakeIndexKeyPrefix for each row).