Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

zoneconfig,sql/catalog: ensure dynamic system tables support rangefeeds #73045

Closed
ajwerner opened this issue Nov 22, 2021 · 3 comments · Fixed by #74555
Closed

zoneconfig,sql/catalog: ensure dynamic system tables support rangefeeds #73045

ajwerner opened this issue Nov 22, 2021 · 3 comments · Fixed by #74555
Labels
A-zone-configs C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Nov 22, 2021 •
edited
Loading

Is your feature request related to a problem? Please describe.

Today, all system tables offer rangefeeds. This is helpful for implementing various caching primitives on top of these tables. Rangefeeds are not enabled by default on all ranges. Instead, there's a cluster setting and then a hard-coded configuration which enables rangefeeds for all ranges which contain a system table.

See

cockroach/pkg/kv/kvserver/replica_rangefeed.go

Lines 150 to 153 in d7094d7

if !r.isSystemRange() && !RangefeedEnabled.Get(&r.store.cfg.Settings.SV) {
return roachpb.NewErrorf("rangefeeds require the kv.rangefeed.enabled setting. See %s",
docs.URL(`change-data-capture.html#enable-rangefeeds-to-reduce-latency`))
}

Describe the solution you'd like

I'd like to delegate control of whether or not rangefeeds are enabled to the zoneconfig/spanconfig infra (see #70614 (comment)). Then we should enable these rangefeeds in the system database unconditionally. Perhaps we need to enable them by default to make sure that there is not a gap when they are not available.

Describe alternatives you've considered
We could make sure that all system tables which need rangefeeds have a small ID, but that seems tough given how quickly we're running out of IDs.

Additional context

This is made pressing due to the exhaustion of descriptor IDs. See #57531.

Epic: CRDB-10772
@ajwerner ajwerner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Nov 22, 2021
@irfansharif
Copy link
Contributor

Is this issue a specific version of #70614? (#70614 (comment))

@ajwerner
Copy link
Contributor Author

Is this issue a specific version of #70614? (#70614 (comment))

🤦 the link in the solutions was supposed to be to that issue, updated. Indeed, but with a few more considerations and a direct tie-in to #57531.

@exalate-issue-sync exalate-issue-sync bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Nov 22, 2021
@ajwerner
Copy link
Contributor Author

#73754 is making the check whether a range is a system range more expensive as part of #57531. We'll need to do something here for 22.1.

@irfansharif irfansharif mentioned this issue Dec 15, 2021
Closed
24 tasks
@blathers-crl blathers-crl bot added the T-kv KV Team label Jan 5, 2022
irfansharif added a commit to irfansharif/cockroach that referenced this issue Jan 6, 2022
@irfansharif
spanconfig: support rangefeeds for dynamic system tables
a475ea7 
Fixes cockroachdb#73045.

We're running out of system table IDs (see cockroachdb#57531), and as a result
we're now introducing the notion of dynamic system IDs throughout the
system. Previously KV baked-in the assumption of static system IDs at
two points:
- When deciding to allow rangefeeds on a given range;
- When enforcing strict GC TTL;

It did so by decoding the range's key span and comparing against the
hard-coded maximum system ID, all to determine whether the range in
question contained system tables. If so, we allowed rangefeeds to be
declared over it, and also did not enforce strict GC TTL (only really
applies to user tables). This way of doing things does not compose with
dynamically allocated system table IDs. With arbitrary, possibly
non-contiguous IDs, we don't have the convenient key-comparison
properties to rely on.

To that end, we use the span configs infrastructure to to delegate
control of whether rangefeeds are enabled over a given range and whether
strict GC is enforced. This scheme allows SQL code to declare "system
table configs" over arbitrary schemas, and have KV still respect it.
This PR does not expose these span config settings as part of zone
configs -- there's no need to (though we could in the future). To
account for the asynchronous nature of the span configs infra, we need
to ensure that ranges without an available config default to enabling
rangefeeds.

Release note: None
@irfansharif irfansharif mentioned this issue Jan 6, 2022
Merged
irfansharif added a commit to irfansharif/cockroach that referenced this issue Jan 18, 2022
@irfansharif
spanconfig: support rangefeeds for dynamic system tables
eb6ff6b 
Fixes cockroachdb#73045.

We're running out of system table IDs (see cockroachdb#57531), and as a result
we're now introducing the notion of dynamic system IDs throughout the
system. Previously KV baked-in the assumption of static system IDs at
two points:
- When deciding to allow rangefeeds on a given range;
- When enforcing strict GC TTL;

It did so by decoding the range's key span and comparing against the
hard-coded maximum system ID, all to determine whether the range in
question contained system tables. If so, we allowed rangefeeds to be
declared over it, and also did not enforce strict GC TTL (only really
applies to user tables). This way of doing things does not compose with
dynamically allocated system table IDs. With arbitrary, possibly
non-contiguous IDs, we don't have the convenient key-comparison
properties to rely on.

To that end, we use the span configs infrastructure to to delegate
control of whether rangefeeds are enabled over a given range and whether
strict GC is enforced. This scheme allows SQL code to declare "system
table configs" over arbitrary schemas, and have KV still respect it.
This PR does not expose these span config settings as part of zone
configs -- there's no need to (though we could in the future). To
account for the asynchronous nature of the span configs infra, we need
to ensure that ranges without an available config default to enabling
rangefeeds.

Release note: None
craig bot pushed a commit that referenced this issue Jan 31, 2022
@irfansharif @ajwerner
@knz @srosenberg
Merge #74555 #75698 #75724 #75743
6a0bdec 
74555: spanconfig: support rangefeeds for dynamic system tables r=irfansharif a=irfansharif

Fixes #73045.

We're running out of system table IDs (see #57531), and as a result
we're now introducing the notion of dynamic system IDs throughout the
system. Previously KV baked-in the assumption of static system IDs at
two points:
- When deciding to allow rangefeeds on a given range;
- When enforcing strict GC TTL;

It did so by decoding the range's key span and comparing against the
hard-coded maximum system ID, all to determine whether the range in
question contained system tables. If so, we allowed rangefeeds to be
declared over it, and also did not enforce strict GC TTL (only really
applies to user tables). This way of doing things does not compose with
dynamically allocated system table IDs. With arbitrary, possibly
non-contiguous IDs, we don't have the convenient key-comparison
properties to rely on.

To that end, we use the span configs infrastructure to to delegate
control of whether rangefeeds are enabled over a given range and whether
strict GC is enforced. This scheme allows SQL code to declare "system
table configs" over arbitrary schemas, and have KV still respect it.
This PR does not expose these span config settings as part of zone
configs -- there's no need to (though we could in the future). To
account for the asynchronous nature of the span configs infra, we need
to ensure that ranges without an available config default to enabling
rangefeeds.

Release note: None

75698: sql/catalog/descs: move direct KV access methods under an interface r=ajwerner a=ajwerner

These things are both generally frowned upon and have different contracts
than the collection itself. Separate them with an interface layer of
indirection to make it clearer that there's a trap lurking around these
methods.

Release note: None

75724: server.go: misc refactors r=tbg a=knz

This moves code out of server.go into distinct files, to make the code easier to navigate/explore.



75743: roachprod: add cockroach-roachstress GCE project to GC cronjob r=srosenberg a=srosenberg

Roachtest Stress runs nightly in TC; to reduce the chance of
resource exhaustion, these tests are executed in cockroach-roachstress
project whereas roachtests are normally executed in cockroach-ephemeral.
Thus, to ensure all resources are eventually GCed, we add
cockroach-roachstress to the list of currently GCed projects.

Release note: None

Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Stan Rosenberg <[email protected]>
@craig craig bot closed this as completed in 4b8258c Jan 31, 2022
@ajwerner ajwerner mentioned this issue Feb 8, 2022
Open
@healthy-pod healthy-pod added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-zone-configs C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

spanconfig: support rangefeeds for dynamic system tables irfansharif/cockroach
3 participants
@ajwerner @irfansharif @healthy-pod