sql: change index backfill merger to use batch api #77055
Conversation
rhu713
requested review from
a team and
stevendanna
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
pkg/sql/backfill.go
Outdated
| settings.TenantWritable, | ||
| "bulkio.index_backfill.merge_batch_bytes", | ||
| "the max number of bytes we merge between temporary and adding indexes in a single batch", | ||
| 16<<10, |
There was a problem hiding this comment.
Where did this number come from? It feels small. Ideally what we'd do is both have this be larger and pull this much out of the memory monitor before issuing it.
At the very least, comment how this number was chosen.
There was a problem hiding this comment.
If we want to choose based on some prior art, we use 16 MB over in changefeeds:
Although we run those scans in parallel which we don't do yet here.I opened an issue for the memory monitor here: #77123
There was a problem hiding this comment.
Done, I'll use 16MiB here. It looks like it should be larger than 1000 KVs from looking at indexes in the demo. Also added the memory monitor and pull the merge_batch_bytes out of it before sending the request.
| optional int64 chunk_size = 6 [(gogoproto.nullable) = false]; | ||
| optional int64 chunk_bytes = 7 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
in retrospect I don't think it's a great idea to put either of these in the spec. Instead, can we just have each of the processors read from the setting per batch? That way you could change it at runtime without a pause-resume cycle. Just a suggestion.
There was a problem hiding this comment.
Done, implemented reading the setting at beginning of each batch.
| sourceKV := &kvs[i] | ||
| destKeys := make([]roachpb.Key, len(resp.Rows)) | ||
| for i := range resp.Rows { | ||
| sourceKV := &resp.Rows[i] |
There was a problem hiding this comment.
Drive-by: it feel sort of lame to allocate a new byte slice here for each key in destKeys. We could amortize the allocations by using a single slice of buffer and always appending to it and then sub-slicing it.
There was a problem hiding this comment.
Done, changed implementation to use a single slice buffer.
rhu713
force-pushed
the
ib-merge-api
branch
3 times, most recently
from
af1f309
to
a657e1c
Compare
Use Batch API instead of txn.Scan() in order to limit the number of bytes per batch response in the index backfill merger. Fixes cockroachdb#76685. Release justification: scan API change without changing functionality Release note: None Release justification:
|
bors r+ |
|
Build succeeded: |
Use Batch API instead of txn.Scan() in order to limit the number of bytes per
batch response in the index backfill merger.
Fixes #76685.
Release note: None