This change is 

Heads up: I saw this and will ignore it until you ask for a review.

@tschottdorf still not ready to review, but I'd like to understand the epoch-based lease restriction with closed timestamps a little further. The more I work with this, the more I'd like to remove 137a1b0 as a hard requirement for rangefeeds because it makes things significantly more difficult to test and to use. Ranges will often swap from expiration-based leases to epoch-based leases and it's surprisingly tricky to ensure that we're never establishing a rangefeed to a range with the first type of lease.

@nvanbenschoten I think it's fair to say that if the CT updates are used correctly, then there is no danger in trying to use them on an expiration-based range. What I mean by that is that "correct" usage of CT information requires taking into account the Epoch of the lease. An expiration-based lease has Epoch zero (or rather, an expiration-base lease should be treated as one with Epoch zero). Closed timestamp updates are never emitted for Epoch zero, and so the result of this is that no read request will ever come to the conclusion that it's OK to be served from a follower, at least not until there's a new, Epoch-based, lease.
I don't know if that's where you're going, but this would allow you to establish rangefeeds wherever, and to check at opportune moments whether this rangefeed can actually make progress given the current lease.
Did this address your concern at all?

What I mean by that is that "correct" usage of CT information requires taking into account the Epoch of the lease. An expiration-based lease has Epoch zero (or rather, an expiration-base lease should be treated as one with Epoch zero). Closed timestamp updates are never emitted for Epoch zero, and so the result of this is that no read request will ever come to the conclusion that it's OK to be served from a follower, at least not until there's a new, Epoch-based, lease.

So then it's also the case that the MaxClosed method added here would return an empty timestamp when a range has an expiration-based lease (meaning that the resolved timestamp would never advance) but that things would also work correctly as it transitions to an epoch-based lease, correct?

In that case, what do you think about simply rejecting rangefeeds on ranges where Replica.requiresExpiringLeaseRLocked returns true instead of actually looking at the lease like we do in 137a1b0? That will still require us to split ranges off in tests, but will allow us to avoid waiting for the new range's lease to upgrade to an epoch-based lease.

So then it's also the case that the MaxClosed method added here would return an empty timestamp when a range has an expiration-based lease (meaning that the resolved timestamp would never advance) but that things would also work correctly as it transitions to an epoch-based lease, correct?

Yep, that's what should happen :-)

In that case, what do you think about simply rejecting rangefeeds on ranges where Replica.requiresExpiringLeaseRLocked returns true instead of actually looking at the lease like we do in 137a1b0? That will still require us to split ranges off in tests, but will allow us to avoid waiting for the new range's lease to upgrade to an epoch-based lease.

That should work fine, if you want to make sure that "hopeless" RangeFeeds never even get created. I do like the explicit validation that RangeFeeds on expiration-based ranges don't do anything wrong, but even if you let the system create them you'd probably add a trigger that terminates them so quickly that things have very little chance to go wrong in the first place - so, what you're proposing seems better.

That error would bubble up all the way to the ChangeFeed, right?

@tschottdorf the last two commits should be ready for review now. The first three are still #28912 because merging anything is just about impossible today.

Rebased of master now that #28912 merged.

With this change and a small DistSender fix I'll send out soon I was able to get cdc/w=100/nodes=3/init=true passing. However, doing so required a few tweaks:

• I had to disable the rangefeed release valve entirely by setting EventChanTimeout to 0 here
• I had to bump the initial scan limit in the test
• I had to "poke" empty ranges which were not getting closed timestamp updates. I talked to @tschottdorf about this an its possible that if a range has never applied a single write then it won't start incrementing its closed timestamp. I need to explore this further.

Awesome! 🙌

I looked into the tpcc-1000 roachtest failure yesterday and it looks like we're being really slow about dumping into kafka for some reason, which is backing up writing to the buffer pretty considerably. I tried some retunings of sarama and pprof-ing the roachtest and a local benchmark. So for it's been inconclusive. Anyway, I bet this is contributing to the release valve issues you're seeing. We definitely need to address the buffer TODO but even if we did that, I think the sink throughput is currently bad enough that'd we'd still have some problems.

Also @tschottdorf, I was unsure of whether I need to call r.store.cfg.ClosedTimestamp.Clients.Request when a closed timestamp is empty for a range.

With this change and #29219, all tests in ccl/changefeedccl pass except TestChangefeedTruncateRenameDrop and TestValidations when rangefeeds are enabled. However, a few take a while (~20 seconds) because they need to wait for expiration-based leases to switch over to epoch-based leases.

TFTRs!

bors r+

Build succeeded

• GitHub CI (Cockroach)