Thanks!

I'm a little concerned with the ambiguity of the term "local", both because it seems imprecise ("local to what?") and because it won't match the semantics we're planning on giving the flag. I imagine that "local" is meant to mean "with the same region locality tier as the gateway". So that would imply that if no replicas for a range are in the same region as the gateway, then we should always fail. Similarly, it would imply that if two replicas for a range are in the same region as the gateway, then we should try both but not try any other replicas.

That's not quite what we are planning to implement. Instead, what we'll implement will be more aligned with "nearest_only" or "closest_only". We'll use the closest available replica (like the info text mentions) regardless of whether it is in the same region as the gateway, and that's the only replica we'll try to use.

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

docs/RFCS/20210519_bounded_staleness_reads.md, line 94 at r1 (raw file):

`local_only`argument  that

nit: the spacing looks off here. Same thing with down below.

docs/RFCS/20210519_bounded_staleness_reads.md, line 175 at r1 (raw file):

if the table descriptor is not available locally

What do you mean by this? Do you mean that we will reject the query if the system.descriptor table's leaseholder is not local and we don't have a lease on the table descriptor at the time of the query?

docs/RFCS/20210519_bounded_staleness_reads.md, line 540 at r1 (raw file):

This is still the case if `local_only` is specified.

Why does local_only have an impact on this? We'll still iterate up the schema history when local_only is set and a newer version is unavailable. It's only when we get to the earliest usable schema version that local_only makes a difference.

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

				return withMinTimestamp(ctx, tree.MustBeDTimestampTZ(args[0]).Time)
			},
			PreferredOverload: true,

Why do both WithMinTimestampFunctionName functions set PreferredOverload to true but both WithMaxStalenessFunctionName leave it as false?

i've changed it to nearest_only. i misunderstood it to mean "local" as in local to the region, but the difference you describe makes sense.

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

docs/RFCS/20210519_bounded_staleness_reads.md, line 175 at r1 (raw file):

i've clarified this a little bit.

docs/RFCS/20210519_bounded_staleness_reads.md, line 540 at r1 (raw file):

fixed this up a bit

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

oops, that doesn't need to be set.
i've removed PreferredOverload mentions.

Reviewed 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @otan)

docs/RFCS/20210519_bounded_staleness_reads.md, line 6 at r1 (raw file):

- Authors: Nathan VanBenschoten, Rebecca Taft, Oliver Tan
- RFC PR: #66020.
- Cockroach Issue: #25405.

nit: while here, want to delete the period above as well?

docs/RFCS/20210519_bounded_staleness_reads.md, line 175 at r1 (raw file):

I see. Is this actually what we are planning on implementing though? I wasn't thinking that nearest_only would have any interaction with table leases. It seems like a pretty rough failure mode that if a table descriptor is not leased at some point in time and we only have bounded staleness reads on the table, we'll never grab a lease and begin rejecting all queries.

We assume that the system.descriptor table is always going to be available, so a missing lease will lead to a blip in latency, but I think that's ok since it won't block indefinitely during a region outage. If users truly want a hard latency cap for all operations, they should still set timeouts.

Put another way, I don't think we need to make the nearest_only flag result in all cross-region operations being rejected. I think it's enough to keep it very constrained to its interaction with dynamic timestamp selection.

docs/RFCS/20210519_bounded_staleness_reads.md, line 12 at r3 (raw file):

Bounded staleness reads are a form of historical read-only queries that use a
dynamic, system-determined timestamp, subject to a user-provided staleness
bound, to read nearby local replicas while minimizing data staleness. They

Should we revert this change?

docs/RFCS/20210519_bounded_staleness_reads.md, line 39 at r3 (raw file):

but avoids the use of the leaseholder.

It doesn't even necessarily do this. The leaseholder can be used if it is the closest replica.

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

const nearestOnlyInfo = `

If nearest_only is set to true, reads that cannot be served using a nearby replica

s/a nearby replica/the nearest available replica/

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

docs/RFCS/20210519_bounded_staleness_reads.md, line 175 at r1 (raw file):

i see. so system.descriptor is always available on some replica is the assumption, i think that's reasonable. i was confusing it with my initial interpretation of local.

docs/RFCS/20210519_bounded_staleness_reads.md, line 12 at r3 (raw file):

oops

Reviewed 3 of 3 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @otan)