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.

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

spanconfig,kv: enable spanconfig.storage_coalesce_adjacent by default #81008

Closed
Tracked by #81009
irfansharif opened this issue May 4, 2022 · 4 comments · Fixed by #98820
Closed
Tracked by #81009

spanconfig,kv: enable spanconfig.storage_coalesce_adjacent by default #81008

irfansharif opened this issue May 4, 2022 · 4 comments · Fixed by #98820
Assignees
Labels
A-zone-configs C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@irfansharif
Copy link
Contributor

irfansharif commented May 4, 2022

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

For 22.2 it’d be great to enable the pack-tables-into-ranges optimization for the host tenant by default. A concern around flipping this default is that when clusters upgrade to 22.2, since table boundaries are no longer hard split points, we might see a rapid onset of possible range merges, which would cause rebalancing activity (replicas on both halves of the range merge need to be colocated on the same stores) which isn’t great both networking cost wise, and possibly its affect on foreground latencies.

Describe the solution you'd like

Strawman: enable the optimization for freshly created clusters (flipping the default value for spanconfig.host_coalesce_adjacent.enabled), and for existing clusters we could disable it. It’s possible to do this through a migration that we’d run on the lead up to 22.2 finalization where we explicitly SET CLUSTER SETTING spanconfig.host_coalesce_adjacent.enabled = false). This doesn’t fully answer the question of how we could prevent foreground traffic impact as a result of this setting being toggled, but that seems kind of difficult (and not clear to me how we could do it well). Since this is a hidden cluster setting, maybe that's fine -- you're on your own. If we want to dampen the effects of toggling this thing back and forth, let's do that as a separate thing — maybe using different rate limits for these merges (also splits). Some internal discussion here.

Jira issue: CRDB-15400

Epic CRDB-28121

@irfansharif irfansharif added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-zone-configs E-quick-win Likely to be a quick win for someone experienced. labels May 4, 2022
@irfansharif irfansharif changed the title spanconfig,kv: spanconfig.host_coalesce_adjacent.enabled = true for fresh clusters spanconfig,kv: enable spanconfig.host_coalesce_adjacent for fresh clusters May 4, 2022
@irfansharif irfansharif changed the title spanconfig,kv: enable spanconfig.host_coalesce_adjacent for fresh clusters spanconfig,kv: enable spanconfig.host_coalesce_adjacent by default May 4, 2022
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
@blathers-crl blathers-crl bot added the T-kv KV Team label Jul 8, 2022
@ajwerner
Copy link
Contributor

ajwerner commented Jul 8, 2022

This turns out to be problematic because of console pages which aggregate range data for table sizes. See #84105.

@nvanbenschoten nvanbenschoten removed the E-quick-win Likely to be a quick win for someone experienced. label Jul 11, 2022
@irfansharif
Copy link
Contributor Author

This setting breaks SHOW RANGES (and the underlying vtables) (#93617) but that's something @knz has a PR out for: #93644.

@knz
Copy link
Contributor

knz commented Dec 16, 2022

Can we also rename it? We don't use the word "host" elsewhere.

@irfansharif
Copy link
Contributor Author

irfansharif commented Jan 17, 2023

Now that #93617 is fixed, I'm only aware of one meaningful blocker for this work (this == enabling coalesced ranges by default for the system tenant). It's #84105 which in turn currently blocks https://cockroachlabs.atlassian.net/browse/CRDB-16704 as part of the over-arching https://cockroachlabs.atlassian.net/browse/CRDB-22711. As I understand it, we're working on all of those things as part of our UA work in 23.1. Which makes this (== enabling coalesced ranges by default for the system tenant) possible to also do in 23.1. There are concerns about upgrades, and them inducing excessive merge queue activity. I'm completely ok with only enabling this setting in clusters that start off in 23.1, leaving existing clusters as is. Also keeping the "uncoalesced" form for clusters that are being restored from 22.2 and earlier images. This is similar in spirit to what we did with lowering the default GC TTL: #89233.

I'm especially interested in enabling for the system tenant since it's a relatively low hanging fruit, very meaningfully advances CRDB scalability WRT to the # of schema objects (tables, indexes, partitions) that clusters can run with. We also have direct evidence that this magical, hidden-by-default setting is something POCs have reached for and found utility for. So far they've had to forego SHOW RANGES support, or the UI accommodating the notion of coalesced ranges, but neither of things are going to apply shortly.

@irfansharif irfansharif changed the title spanconfig,kv: enable spanconfig.host_coalesce_adjacent by default spanconfig,kv: enable spanconfig.storage_coalesce_adjacent by default Mar 16, 2023
zachlite added a commit to zachlite/cockroach that referenced this issue Mar 21, 2023
This is a stop-gap commit that enables the DataDistribution endpoint
to handle the parts of the key space belonging to secondary tenants
without error.

Despite no error, the result returned for secondary tenants is not correct.
The DataDistribution endpoint was written before cockroachdb#79700, and therefore
doesn't know that multiple tables can exist within a range.

Additionally, the results for the system tenant will be incorrect soon
because cockroachdb#81008 is in progress.

Fixes: cockroachdb#97993
Release note: None
craig bot pushed a commit that referenced this issue Mar 30, 2023
98689: workload: jitter the teardown of connections to prevent thundering herd r=sean- a=sean-

workload: jitter the teardown of connections to prevent thundering herd

This change upgrades workload's use of pgx from v4 to v5 in order to allow jittering the teardown of connections.  This change sets a max connection age of 5min and jitters the teardown by 30s.  Upgrading to pgx v5 also adds non-blocking pgxpool connection acquisition.

workload: add flags to manage the age and lifecycle of connection pool

Add flags to all workload types to specify:

* the max connection age: `--max-conn-lifetime duration`
* the max connection age jitter: `--max-conn-lifetime-jitter duration`
* the max connection idle time: `--max-conn-idle-time duration`
* the connection health check interval: `--conn-healthcheck-period duration`
* the min number of connections in the pool: `--min-conns int`

workload: add support for remaining pgx query modes

Add support for `pgx.QueryExecModeCacheDescribe` and `pgx.QueryExecModeDescribeExec`.  Previously, only three of the five query modes were available.

workload: fix race condition when recording histogram data

Release note (cli change): workload jitters teardown of connections to prevent thundering herd impacting P99 latency results.

Release note (cli change): workload utility now has flags to tune the connection pool used for testing.  See `--conn-healthcheck-period`, `--min-conns`, and the `--max-conn-*` flags for details.

Release note (cli change): workload now supports every [PostgreSQL query mode](https://github.com/jackc/pgx/blob/fa5fbed497bc75acee05c1667a8760ce0d634cba/conn.go#L167-L182) available via the underlying pgx driver.

99142: server: fix `DataDistribution` server error when creating a tenant r=zachlite a=zachlite

This is a stop-gap commit that enables the DataDistribution endpoint to handle the parts of the key space belonging to secondary tenants without error.

Despite no error, the result returned for secondary tenants is not correct. The DataDistribution endpoint was written before #79700, and therefore doesn't know that multiple tables can exist within a range. 

Additionally, the results for the system tenant will be incorrect soon because #81008 is in progress.
Improvements are tracked by #97942

Fixes: #97993
Release note: None

99494: changefeedccl: Do not require rangefeed when running initial scan only. r=miretskiy a=miretskiy

Fixes #99470

Release note: None

Co-authored-by: Sean Chittenden <sean@chittenden.org>
Co-authored-by: zachlite <zachlite@gmail.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Mar 30, 2023
This is a stop-gap commit that enables the DataDistribution endpoint
to handle the parts of the key space belonging to secondary tenants
without error.

Despite no error, the result returned for secondary tenants is not correct.
The DataDistribution endpoint was written before #79700, and therefore
doesn't know that multiple tables can exist within a range.

Additionally, the results for the system tenant will be incorrect soon
because #81008 is in progress.

Fixes: #97993
Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this issue May 7, 2023
Fixes cockroachdb#81008.

We built the basic infrastructure to coalesce ranges across table
boundaries back in 22.2 as part of cockroachdb#66063. We've enabled this
optimization for secondary tenants since then, but hadn't for the system
tenant because of two primary blockers:

- cockroachdb#93617: SHOW RANGES was broken by coalesced ranges.
- cockroachdb#84105: APIs to compute sizes for schema objects (used in our UI) was
  broken by coalesced ranges.

In both these cases we baked in assumptions about there being a minimum
of one-{table,index,partition}-per-range. These blockers didn't apply to
secondary tenants at the time since they didn't have access to SHOW
RANGES, nor the UI pages where these schema statistics were displayed.

We've addressed both these blockers in the 23.1 cycle as part of
bridging the compatibility between secondary tenants and yesteryear's
system tenant.
- cockroachdb#93644 revised SHOW RANGES and crdb_internal.ranges{,_no_leases}, both
  internally and its external UX, to accommodate ranges straddling
  table/database boundaries.
- cockroachdb#96223 re-worked our SpanStats API to work in the face of coalesced
  ranges, addressing cockroachdb#84105.

Release note (general change): CockroachDB would previously use separate
ranges for each table, index, or partition. This is no longer true --
it's possible now to have multiple tables, indexes, and partitions to
get packed into the same range. For users with many such "schema
objects", this will reduce the total range count in their clusters. This
is especially true if individual tables, indexes, or partitions are
smaller than the default configured maximum range size (controlled using
zone configs, specifically the range_max_bytes parameter).
We made this change to improve scalability with respect to the number of
schema objects, since the underlying range count now no longer a
bottleneck. Users upgrading from 22.2, once finalizing their upgrade,
may observe a round of range merges and snapshot transfers (to power
said range merges) as a result of this change. If users want to opt-out
of this optimization, they can use the following cluster setting:

  SET CLUSTER SETTING spanconfig.storage_coalesce_adjacent.enabled = false;
craig bot pushed a commit that referenced this issue May 8, 2023
98820: spanconfig: enable range coalescing by default r=irfansharif a=irfansharif

Fixes #81008.

We built the basic infrastructure to coalesce ranges across table boundaries back in 22.2 as part of #66063. We've enabled this optimization for secondary tenants since then, but hadn't for the system tenant because of two primary blockers:

- #93617: SHOW RANGES was broken by coalesced ranges.
- #84105: APIs to compute sizes for schema objects (used in our UI) was broken by coalesced ranges.

In both these cases we baked in assumptions about there being a minimum of one-{table,index,partition}-per-range. These blockers didn't apply to secondary tenants at the time since they didn't have access to SHOW RANGES, nor the UI pages where these schema statistics were displayed.

We've addressed both these blockers in the 23.1 cycle as part of bridging the compatibility between secondary tenants and yesteryear's system tenant.
- #93644 revised SHOW RANGES and crdb_internal.ranges{,_no_leases}, both internally and its external UX, to accommodate ranges straddling table/database boundaries.
- #96223 re-worked our SpanStats API to work in the face of coalesced ranges, addressing #84105.

Release note (general change): CockroachDB would previously use separate ranges for each table, index, or partition. This is no longer true -- it's possible now to have multiple tables, indexes, and partitions to get packed into the same range. For users with many such "schema objects", this will reduce the total range count in their clusters. This is especially true if individual tables, indexes, or partitions are smaller than the default configured maximum range size (controlled using zone configs, specifically the range_max_bytes parameter). We made this change to improve scalability with respect to the number of schema objects, since the underlying range count now no longer a bottleneck. Users upgrading from 22.2, once finalizing their upgrade, may observe a round of range merges and snapshot transfers (to power said range merges) as a result of this change. If users want to opt-out of this optimization, they can use the following cluster setting:
```
  SET CLUSTER SETTING spanconfig.storage_coalesce_adjacent.enabled = false; 
```

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
@craig craig bot closed this as completed in 7a57e7e May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-zone-configs C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants