builtins: add timestamp to crdb_internal.scan/list_sql_keys_in_range

[adityamaru]

This change adds a third return column to the builtins
that corresponds to the timestamp at which the value for
a particular key was written.

Release note (sql change): crdb_internal.scan and
crdb_internal.list_sql_keys_in_range return
the timestamp at which the value for a key was written, in
addition to the raw key and value.

[cockroach-teamcity]

This change is 

[stevendanna]

You probably need to alter the rangeKeyIterator code. I think you probably need to change this:

// Values implements the tree.ValueGenerator interface.
func (rk *rangeKeyIterator) Values() (tree.Datums, error) {
	kv := rk.kvs[rk.index]
	rk.buf[0] = tree.NewDString(kv.Key.String())
	rk.buf[1] = tree.NewDString(kv.Value.PrettyPrint())
	return rk.buf[:], nil
}

to make sure it only returns the first two elements of the buffer. (or change the return type of that function as well.

[adityamaru]

You probably need to alter the rangeKeyIterator code. I think you probably need to change this:

heh, you're too quick! Decided to add the ts to that builtin too, let me know if you'd rather I go the other way.

[stevendanna]

I reviewed the uses of sql_keys_in_range in Slack and didn't see anyone using it in a way that would be seriously impacted by the new column, so I think adding this new column there is OK by me.

Left one question re the return type that I'll leave to your judgement.

[stevendanna]

I wonder: Should we make this new column types.Decimal? That is the type of crdb_internal_mvcc_timestamp column is. I could go either way..

[adityamaru]

In https://github.com/cockroachdb/cockroach/pull/90848/files#diff-ed2b0d994e60d34e9f853343fd43ee6e14ca234027786774a143e5b73b5a2698R111 im currently hashing the hlc.Timestamp.String() representation of an MVCC timestamp. As discussed offline I wanted uniformity between the output of this builtin and what we hash so that in the future we can write statements such as:

WITH sfp AS (SELECT xor_agg(fnv64(key, ts::BYTES, value)) AS scanfp from crdb_internal.scan(crdb_internal.table_span('t'::regclass::int)))
  SELECT count(*) FROM crdb_internal.fingerprint(crdb_internal.table_span('t'::regclass::int),'$before_insert'::TIMESTAMPTZ, false) AS efp, sfp WHERE efp = sfp.scanfp;

to test that our fingerprinting is working as expected. We could go either way here:

1. hash the decimal representation of the timestamp and change scan to return a decimal instead
2. hash the string representation of the timestamp and keep hashing behaviour what it is at the moment

In the interest of moving things ahead, I'll leave this as a string for now but while reviewing #90848 if we decide decimal is a better representation in both places, I'll make the switch there.

[adityamaru]

TFTR!

bors r=stevendanna

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)