*: craft rangefeed abstraction, support cluster settings in tenants

[ajwerner]

This PR is a reworking of @dt's excellent effort in #57926
to pick up rangefeeds for tenant cluster settings after sprucing up the libraries around
them. @tbg and @nvanbenschoten I'm tagging you accordingly given that you reviewed
that PR.

First two commits (kvclient/rangefeed package and adoption in catalog/lease)

The first commit in this PR abstract out some rangefeed client logic around
retries to make them more generally usable in other contexts. The second
commit adopts this package in sql/catalog/lease. These two commits can
(and probably should) be merged separately and exist in
#58361.

Next two commits (server/settingswatcher package and adoption in server)

These commits introduce code to update the settings using the above rangefeed
package and then hooks it up to the sql server.

Next three commits

Remove the restrictions from tenants updating settings, mark which settings can be
updated, generate docs.

These are lifted straight from @dt in #57926.

Maybe closes #56623.

[cockroach-teamcity]

This change is 

[ajwerner]

This still breaks one test which checks for an error starting up the tenant with the wrong codec. There are some obvious fixes to work around that test but it reveals an important problem here which is that not all errors are forever retryable. Namely the tenant mismatch and a gc threshold error. I'd love to talk about how to pursue solutions there before trying something.

[tbg]

Nice! Thanks for putting this together. One big thing that I'm not 100% sure was on your radar is that this needs to be a "safe" backport to 20.2 if it is going to benefit CC-Serverless (as it should). Perhaps that commit you added to replace the legacy code is sufficiently isolated, but it would be easier if that commit were to be removed from this PR and merged separately to keep the backport most straightforward (your call how you go about this eventually though).

I left a ton of comments but none of them substantial, when they are addressed this is good to go as far as I'm concerned. BTW, I did not re-review David's commits; if anything in there wants another look please let me know.

@andy-kimball can you chime in on this thread here: https://reviewable.io/reviews/cockroachdb/cockroach/57926#-MOf6JkX7xTgwjcFUck5 - Essentially, do we want an allowlisting or blocklisting approach for settings? Nathan and I favor allowlisting (i.e. if you don't mark a setting as accessible to tenants, then it is not - seeing how 99% of settings currently make no sense for tenants and I worry about giving tenants ways to shoot themselves in the foot by accident) whereas David argued for a blocklist (link above).

Reviewed 10 of 10 files at r1, 11 of 11 files at r2, 8 of 8 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 12 of 12 files at r6, 4 of 4 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @nvanbenschoten)

---

pkg/kv/kvclient/rangefeed/config.go, line 27 at r1 (raw file):

	retryOptions      retry.Options
	onInitialScanDone OnInitialScanDone
	withInitialScan   bool

this is just onInitialScanDone != nil? Might want to make it a method instead or add a comment why that won't work.

---

pkg/kv/kvclient/rangefeed/config.go, line 56 at r1 (raw file):

func WithDiff() Option { return withDiff }

var withDiff = optionFunc(func(c *config) { c.withDiff = true })

nit: I personally prefer

func WithDiff Option {
  return optionFunc(func(c *config) { c.withDiff = true })
}

because it avoids having to look around for the actual definition and is more consistent with the rest of the defs.

---

pkg/kv/kvclient/rangefeed/config.go, line 65 at r1 (raw file):

}

var defaultConfig = config{}

why this dangling var? You can just *c = config{} below and keep things local.

---

pkg/kv/kvclient/rangefeed/db_adapter.go, line 23 at r1 (raw file):

)

// dbAdapter is an implementation of db a *kv.DB.

Sentence is garbled and also I feel it could tell me more about what this thing is. I guess it wraps a *kv.DB into something that allows me to run range feeds and scans?

---

pkg/kv/kvclient/rangefeed/db_adapter.go, line 43 at r1 (raw file):

		targetScanBytes: defaultTargetScanBytes,
	}
	{

It's a shame that this is all necessary, but there's no good way around it without invasive refactors. (I was thinking to add a DistSender() method to *kv.DB, but that doesn't solve the problem - we still need to assert on the wrapped sender there because in tests it's definitely not a full dist sender). Could be fixed but let's not right now.

---

pkg/kv/kvclient/rangefeed/db_adapter.go, line 56 at r1 (raw file):

		dbClient.distSender = distSender
	}
	return &dbClient, nil

nit: set things up so that you can return &dbAdapter{ ... } with all fields filled in. This pattern where we set up a partial struct and then fill in some pieces should be a last resort imo (error prone), and don't think you need it here.

---

pkg/kv/kvclient/rangefeed/db_adapter_external_test.go, line 78 at r1 (raw file):

	// Ensure that pagination doesn't break anything.
	cli.SetTargetScanBytes(1)

super nit: cli is also a prominent package name, maybe avoid it here.

---

pkg/kv/kvclient/rangefeed/doc.go, line 19 at r1 (raw file):

package rangefeed

// TODO(ajwerner): Rework this logic to encapsulate the multi-span logic in

It would be useful to write out what the limitations of the current code are.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 125 at r1 (raw file):

}

// Close closes the RangeFeed and waits for it to shut down.

//
// Close is idempotent.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 136 at r1 (raw file):

// failures or general unavailability. We'll reset the retrier if the
// rangefeed runs for longer than the resetThreshold.
const resetThreshold = 30 * time.Second

I saw this elsewhere too, but didn't understand why. Mind adding a comment?

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 139 at r1 (raw file):

The context is hooked up to the stopper's quiescence.

what does this mean? Is that a statement about the incoming context (in which case it's maybe more misleading than anything and could be removed).

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 170 at r1 (raw file):

		start := timeutil.Now()

		// Note that the below channel send is safe because processEvents will

is safe -> will not block forever

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 171 at r1 (raw file):

		// Note that the below channel send is safe because processEvents will
		// wait for the worker to send. RunWorker is safe here because this

because processEvents is guaranteed to consume the error before returning.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 172 at r1 (raw file):

		// Note that the below channel send is safe because processEvents will
		// wait for the worker to send. RunWorker is safe here because this
		// goroutine is run as an AsyncTask.

Any particular reason not to use RunAsyncTask? The Worker/Task API is crap and it seems that they're interchangeable here. It would be nice to make do without assumptions on the caller which tend to rot more easily than local constraints.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 191 at r1 (raw file):

		if ranFor > resetThreshold {
			i = 1
			r.Reset()

This might also be a good place for a comment about the resetting.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 199 at r1 (raw file):

// requested. It will retry in the face of errors and will only return upon
// context cancellation.
func (f *RangeFeed) maybeRunInitialScan(

nit: move processEvents above this method so that it is closer to its caller.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 203 at r1 (raw file):

) (done bool) {
	if !f.withInitialScan {
		return ctx.Err() != nil // done

I would've expected true if we're not even requesting an initial scan. Why isn't that the case?

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 222 at r1 (raw file):

		// It's something of a bummer that we must allocate a new value for each
		// of these but the contract doesn't indicate that the value cannot be

s/cannot/can/?

---

pkg/kv/kvclient/rangefeed/rangefeed_external_test.go, line 47 at r1 (raw file):

	require.NoError(t, db.Put(ctx, mkKey("c"), 3))

	sp := roachpb.Span{

Will it add anything to have this span multiple ranges?

---

pkg/kv/kvclient/rangefeed/rangefeed_external_test.go, line 62 at r1 (raw file):

	initialScanDone := make(chan struct{})
	r, err := f.RangeFeed(ctx, "test", sp, afterB, func(ctx context.Context, value *roachpb.RangeFeedValue) {
		fmt.Println(value)

t.Log

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 339 at r1 (raw file):

	}
	restartingRE := regexp.MustCompile("restarting rangefeed.*after.*")
	log.Intercept(ctx, func(entry logpb.Entry) {

not a fan of this - this will also prevent logs from going to the files. Can you add a hook instead that you fire when you restart the rangefeed instead?

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 366 at r1 (raw file):

		return nil
	})
	seen.Lock()

?

---

pkg/sql/catalog/lease/lease.go, line 1799 at r2 (raw file):

		log.Infof(ctx, "using rangefeeds for lease manager updates")
	}
	distSender := db.NonTransactionalSender().(*kv.CrossRangeTxnWrapperSender).Wrapped().(*kvcoord.DistSender)

Are you comfortable with these changes getting backported? Since we are doing this work to enable CC, it will need a backport. Just keep that in mind - my intuition is that we should avoid touching old production code and use the new code only for tenants where it's needed. Then, on master only, we make the changes you are making here where the new package is used by existing system tenant code.

[tbg]

@andy-kimball there was more discussion about allow/blocklist on the original PR, #57926

[nvanbenschoten]

I agree with Tobi, this is a big improvement! Thanks for putting it together.

Reviewed 10 of 10 files at r1, 11 of 11 files at r2, 8 of 8 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 12 of 12 files at r6, 4 of 4 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @tbg)

---

pkg/kv/kvclient/rangefeed/config.go, line 27 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

this is just onInitialScanDone != nil? Might want to make it a method instead or add a comment why that won't work.

I think the intention is to allow .WithInitialScan(nil).

---

pkg/kv/kvclient/rangefeed/config.go, line 56 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: I personally prefer

func WithDiff Option {
  return optionFunc(func(c *config) { c.withDiff = true })
}

because it avoids having to look around for the actual definition and is more consistent with the rest of the defs.

But that's more expensive than what's here, as it results in a pair of allocations per WithDiff call, instead of statically performing both.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 134 at r1 (raw file):

retrier

typo? frontier?

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 198 at r1 (raw file):

// maybeRunInitialScan will attempt to perform an initial data scan if one was
// requested. It will retry in the face of errors and will only return upon
// context cancellation.

Or when the scan completes.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 199 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: move processEvents above this method so that it is closer to its caller.

I'm actually +1 on this order. It's the order that the methods are called from run.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 203 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I would've expected true if we're not even requesting an initial scan. Why isn't that the case?

+1

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 215 at r1 (raw file):

		v.Value.Timestamp = f.initialTimestamp

		// Supply the value from the scan as also the previous value to avoid

Do we do this in the changefeed processor as well?

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 266 at r1 (raw file):

			// Ensure that the RangeFeed goroutine stops.
			<-errCh
			return nil

Should we be returning ctx.Err()?

---

pkg/server/settingswatcher/settings_watcher.go, line 44 at r3 (raw file):

	clock *hlc.Clock,
	codec keys.SQLCodec,
	settings *cluster.Settings,

It wasn't as clear to me as it probably should have been that the purpose of the SettingsWatcher was to update this provided Settings object. Can we make that more clear through a combination of improving the commentary and maybe renaming this to settingsToUpdate?

---

pkg/server/settingswatcher/settings_watcher.go, line 70 at r3 (raw file):

	u := s.settings.MakeUpdater()
	initialScanDone := make(chan struct{})
	if _, err := s.f.RangeFeed(ctx, "settings", settingsTableSpan, now, func(

Do we want to leak the RangeFeed or should we provide a way for a client to stop the SettingsWatcher (and underlying RangeFeed)?

---

pkg/sql/set_cluster_setting.go, line 98 at r6 (raw file):

	if setting.SystemOnly() && !p.execCfg.Codec.ForSystemTenant() {
		return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
			"setting %s is only setable in the system tenant", name)

"settable"

---

pkg/sql/catalog/lease/lease.go, line 1833 at r2 (raw file):

		}
	}
	// Ignore errors here because they indicate that the server is shutting down.

Is this part of the function's contract. Should it be?

[knz]

One big thing that I'm not 100% sure was on your radar is that this needs to be a "safe" backport to 20.2 if it is going to benefit CC-Serverless (as it should).

That's not so clear that it should. As described in the linked review thread the consensus seems to be that CC free tier running on v20.2 will not offer cluster settings to tenants, while shared tier running on v21.1 will.

We're going to pick up this conversation in the following new issue: #58445

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @nvanbenschoten, and @tbg)

---

pkg/kv/kvclient/rangefeed/config.go, line 56 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

But that's more expensive than what's here, as it results in a pair of allocations per WithDiff call, instead of statically performing both.

That might be true (is it actually though?) but I think the nit still stands, as this is not code where this matters.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 199 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm actually +1 on this order. It's the order that the methods are called from run.

Oh, true. Carry on then.

[tbg]

You've probably had reasons to not touch this in the interim, but just on the off chance that it just slipped the radar, friendly ping.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @nvanbenschoten, and @tbg)

---

pkg/kv/kvclient/rangefeed/config.go, line 27 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

this is just onInitialScanDone != nil? Might want to make it a method instead or add a comment why that won't work.

I toyed with the interfaces here a lot. I'm not totally in love with where I ended up for what it's worth. I played with something that takes an interface{ Init(...); Row(...); Done(...) } for the initial scan. At that point it didn't really feel worth integrating into this interface. Perhaps it's not worth integrating but then I was annoyed at the need to write another retry loop.

---

pkg/kv/kvclient/rangefeed/config.go, line 56 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

That might be true (is it actually though?) but I think the nit still stands, as this is not code where this matters.

over-optimizing an allocation in an allocation-prone setting. Removed.

However, I don't understand your example, it definitely is:
https://play.golang.org/p/9B0mwnXlCdF

---

pkg/kv/kvclient/rangefeed/config.go, line 65 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

why this dangling var? You can just *c = config{} below and keep things local.

Sure, was thinking there might be some non-zero value defaults. Done.

---

pkg/kv/kvclient/rangefeed/db_adapter.go, line 23 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Sentence is garbled and also I feel it could tell me more about what this thing is. I guess it wraps a *kv.DB into something that allows me to run range feeds and scans?

I'd rather that the reader go read the kvDB interface. I've made it a hair clearer.

---

pkg/kv/kvclient/rangefeed/db_adapter.go, line 56 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: set things up so that you can return &dbAdapter{ ... } with all fields filled in. This pattern where we set up a partial struct and then fill in some pieces should be a last resort imo (error prone), and don't think you need it here.

Done.

---

pkg/kv/kvclient/rangefeed/db_adapter_external_test.go, line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

super nit: cli is also a prominent package name, maybe avoid it here.

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 125 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

//
// Close is idempotent.

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 134 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

retrier

typo? frontier?

Not a typo. Reworded for clarity.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 136 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I saw this elsewhere too, but didn't understand why. Mind adding a comment?

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 139 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The context is hooked up to the stopper's quiescence.

what does this mean? Is that a statement about the incoming context (in which case it's maybe more misleading than anything and could be removed).

Removed.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 171 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

because processEvents is guaranteed to consume the error before returning.

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 172 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Any particular reason not to use RunAsyncTask? The Worker/Task API is crap and it seems that they're interchangeable here. It would be nice to make do without assumptions on the caller which tend to rot more easily than local constraints.

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 191 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This might also be a good place for a comment about the resetting.

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 203 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

+1

I had a test that wanted to make sure we didn't kick off the rangefeed if the context is canceled prior to the Rangefeed call. I've reworked the context checking to make this true elsewhere.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 215 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we do this in the changefeed processor as well?

Yes.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 222 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

s/cannot/can/?

I think it's right as stated. The contract doesn't say anything about ownership so we take the pessimistic view.

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 266 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we be returning ctx.Err()?

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed_external_test.go, line 47 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Will it add anything to have this span multiple ranges?

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed_external_test.go, line 62 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

t.Log

detritus, removed

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 339 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

not a fan of this - this will also prevent logs from going to the files. Can you add a hook instead that you fire when you restart the rangefeed instead?

Done.

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 366 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

?

detritus, removed.

---

pkg/server/settingswatcher/settings_watcher.go, line 44 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It wasn't as clear to me as it probably should have been that the purpose of the SettingsWatcher was to update this provided Settings object. Can we make that more clear through a combination of improving the commentary and maybe renaming this to settingsToUpdate?

Done.

---

pkg/server/settingswatcher/settings_watcher.go, line 70 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we want to leak the RangeFeed or should we provide a way for a client to stop the SettingsWatcher (and underlying RangeFeed)?

Done.

---

pkg/sql/catalog/lease/lease.go, line 1799 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Are you comfortable with these changes getting backported? Since we are doing this work to enable CC, it will need a backport. Just keep that in mind - my intuition is that we should avoid touching old production code and use the new code only for tenants where it's needed. Then, on master only, we make the changes you are making here where the new package is used by existing system tenant code.

We're not trying to backport this.

---

pkg/sql/catalog/lease/lease.go, line 1833 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this part of the function's contract. Should it be?

Added.

[andy-kimball]

---

pkg/sql/catalog/lease/lease.go, line 1799 at r2 (raw file):

Previously, ajwerner wrote…

We're not trying to backport this.

Andrew's correct: this does not need to be back-ported, since we mainly need this for supporting upcoming features that are part of 21.1. Of course, that assumes we can upgrade from 20.2 to 21.1, which is not yet possible, but that's a different discussion.

[ajwerner]

This should be RFAL. Sorry I let it rot for so long. Working on getting it in and doing something on top to allow us to get tenants upgraded.

[andy-kimball]

So if this is necessary for getting tenants upgraded from 20.2 => 21.1, is it also safe to back-port to 20.2?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @nvanbenschoten, and @tbg)

[ajwerner]

So if this is necessary for getting tenants upgraded from 20.2 => 21.1, is it also safe to back-port to 20.2?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @nvanbenschoten, and @tbg)

No, but I don't think that matters. This change is compatible with 20.2. What we need is to make sure that when we boot 21.1 sql servers they can activate the 21.1 cluster version.

[nvanbenschoten]

Reviewed 2 of 44 files at r8, 6 of 10 files at r9, 11 of 15 files at r10, 9 of 9 files at r11, 1 of 1 files at r12, 1 of 4 files at r13, 11 of 12 files at r14, 4 of 4 files at r15.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @tbg)

---

pkg/kv/kvclient/rangefeed/rangefeed.go, line 114 at r15 (raw file):

		client:  f.client,
		stopper: f.stopper,
		knobs:   &f.knobs,

Should we just store the TestingKnobs as a pointer on Factory?

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 366 at r1 (raw file):

Previously, ajwerner wrote…

detritus, removed.

It doesn't look like this was removed.

---

pkg/kv/kvclient/rangefeed/rangefeed_mock_test.go, line 332 at r15 (raw file):

	}
	ctx := context.Background()
	var seen struct {

nit: would an atomic int be simpler?

[ajwerner]

TFTR!

bors r=nvanbenschoten

[craig]

Build succeeded:

• GitHub CI (Cockroach)