Nice, thanks for sending this! but I'm also not an expert here, so cc-ing @solongordon.

Reviewed 11 of 11 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rohany)

docs/generated/sql/functions.md, line 2568 at r1 (raw file):

<tr><td><a name="crdb_internal.lease_holder"></a><code>crdb_internal.lease_holder(key: <a href="bytes.html">bytes</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used to fetch the leaseholder corresponding to a request key</p>
</span></td></tr>
<tr><td><a name="crdb_internal.list_sql_keys_in_range"></a><code>crdb_internal.list_sql_keys_in_range(range_id: <a href="int.html">int</a>) &rarr; tuple{string AS key, string AS value}</code></td><td><span class="funcdesc"><p>TODO (rohany): Fill this out</p>

Remember to address this.

pkg/sql/logictest/testdata/logic_test/sql_keys, line 1 at r1 (raw file):

# LogicTest: local

Mind moving this test into crdb_internal?

pkg/sql/logictest/testdata/logic_test/sql_keys, line 13 at r1 (raw file):

# TODO (rohany): I thought ranges were split automatically on table creation, but
#  I seem to have to do the SPLIT AT above. Is something different in logic tests?

Dunno, but I think we're having to do the same thing here:

pkg/sql/sem/builtins/generator_builtins.go, line 1313 at r1 (raw file):

	}
	// Scan all of the K/V's within the target range.
	// TODO (rohany): Is it OK to have maxRows = 0 here? Since range sizes are capped (64 mb),

Range sizes now 512mb by default, and given we'd typically expect to use this when debugging live, minimizing any sort of impact seems prudent. Is it much work to fold it into the iterator instead?

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

pkg/sql/sem/builtins/generator_builtins.go, line 1315 at r1 (raw file):

	// TODO (rohany): Is it OK to have maxRows = 0 here? Since range sizes are capped (64 mb),
	//  we shouldn't OOM here. If not, we can do these scans as part of the iterator.
	kvs, err := txn.Scan(ctx, rangeDesc.StartKey, rangeDesc.EndKey, 0 /* maxRows */)

Please don't issue the Scan for all the rows inside Start() and then load them in RAM.

There should be buffering in the generator struct, and a conditional re-call of Scan in the Next() function instead.

At any point in time there should be no more than N bytes of data buffered inside the builtin struct, where N is fixed, small-ish (let's say, less than 1KiB) and independent of the range's actual size.

Also add a TODO: the KV interface currently has a maxRows parameters, but we're changing that to have a maxBytes parameter instead. That's what we really want. If some keys happen to be very large, maxRows doesn't help with memory usage.

very nice. thanks.
just noticed that the meta scan has the same issue, see comment

Reviewed 1 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @rohany)

pkg/sql/sem/builtins/generator_builtins.go, line 1248 at r2 (raw file):

// rangeKeyIterator requests at a time. If this changes, make sure
// to update the test in sql_keys.
// TODO (kv): The current KV API only supports a maxRows limitation

nit: there's a linter that wants TODO(kv) without a space

pkg/sql/sem/builtins/generator_builtins.go, line 1254 at r2 (raw file):

var rangeKeyIteratorType = types.MakeLabeledTuple(
	// TODO (rohany): These could be bytes if we don't want to display the

ditto

pkg/sql/sem/builtins/generator_builtins.go, line 1307 at r2 (raw file):

func (rk *rangeKeyIterator) Start(ctx context.Context, txn *kv.Txn) error {
	rk.txn = txn
	// Scan the range meta K/V's to find the target range.

similar comment to previous: if there are many ranges in the cluster, then this is going to blow up too.
I'd suggest to page through meta with no more than 100 or 1000 ranges at a time. (This can be achieved with maxRows.)

LGTM modulo placing the code in the right place(s).

Reviewed 2 of 6 files at r2, 10 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rohany)

pkg/sql/sem/builtins/generator_builtins.go, line 1308 at r3 (raw file):

	// Scan the range meta K/V's to find the target range. We do this in a
	// chunk-wise fashion to avoid loading all ranges into memory.
	var ranges []kv.KeyValue

I appreciate that the existing kv scan function was not appropriate, but the code you have written here is not really appropriate for a "SQL builtins" package either. It's very much low level and belongs to KV.

Therefore I'd encourage you to move this code into a new function in the appropriate package, and call it from here.

Reviewed 16 of 16 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rohany)