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

*: stop gossiping the system config span #70560

Closed
irfansharif opened this issue Sep 22, 2021 · 4 comments · Fixed by #76279
Closed

*: stop gossiping the system config span #70560

irfansharif opened this issue Sep 22, 2021 · 4 comments · Fixed by #76279
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 Sep 22, 2021

And start allowing writes before DDLs in transactions. This is tracking issue for what should be the last piece of #66348 -- work we've been doing so that we no longer need to gossip the system config span. Once all the PRs in #67679 have settled, we should be able to flip the envvar introduced in #69658, and soon after decide to stop gossiping the system config span entirely.

Epic CRDB-10489

@irfansharif irfansharif added A-zone-configs C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Sep 22, 2021
@ajwerner
Copy link
Contributor

ajwerner commented Sep 22, 2021

This will solve #54477.

Another blocker relates to the system.settings propagation. I have an early attempt to deal with settings in the system tenant in #69269 and filed the issue as #70566.

@irfansharif
Copy link
Contributor Author

It should also put a plug on our most pressing schema scalability (ala #63206). It looks like we're exploring integrations with http://prisma.io, and the # of tables created in their test-suite run into the O(N^2) properties of our system config span. @tbg, @nvanbenschoten -- I realize our 22.1 planning surveys have closed out and I'm curious to see what comes of it, but I think we should seriously try to schedule this work over the next release.

@exalate-issue-sync exalate-issue-sync bot added the T-kv KV Team label Oct 11, 2021
@irfansharif
Copy link
Contributor Author

I lied above, after #67679 we still have one dependency on the system config span: our replication reports (#70557).

tbg added a commit to tbg/cockroach that referenced this issue Nov 4, 2021
These tests work today because the store they're operating under
is strung together and is missing lots of moving parts. Once we
use a more fully initialized stores these tests are prime candidates
for being flaky.

The real solution would be to to add testing knobs so that we could
investigate, in the test, the reasons for gossip, and to make sure
they are all valid reasons. But we are unlikely to break anything
in this area and are going to remove gossip of the system config
altogether in cockroachdb#70560.

So just remove these tests, which prevents them from getting in the
way of refactors of the test harness.

Release note: None
@irfansharif
Copy link
Contributor Author

A SQL dependency on the system config span that we'd also want to get rid of: #70558.

craig bot pushed a commit that referenced this issue Dec 23, 2021
71910: sql: add a cluster setting to avoid system config triggers r=ajwerner a=ajwerner

This is intended as a short-term workaround to improve performance in
situations of repeated schema changes, like ORM tests.

We have a plan to disable the system config trigger altogether in 22.1 with
#70560. 

This PR provides a cluster setting which allows schema change transactions
to bypass triggerring an update to the system config span. These updates
currently drive only the propagation of zone configs to KV and cluster
settings. The cluster setting behavior is retained until we address #70566.

We have a history of these sorts of unsafe settings in
`kv.raft_log.disable_synchronization_unsafe`.

Release note: None

Co-authored-by: Andrew Werner <awerner32@gmail.com>
craig bot pushed a commit that referenced this issue Jan 3, 2022
71910: sql: add a cluster setting to avoid system config triggers r=ajwerner a=ajwerner

This is intended as a short-term workaround to improve performance in
situations of repeated schema changes, like ORM tests.

We have a plan to disable the system config trigger altogether in 22.1 with
#70560. 

This PR provides a cluster setting which allows schema change transactions
to bypass triggerring an update to the system config span. These updates
currently drive only the propagation of zone configs to KV and cluster
settings. The cluster setting behavior is retained until we address #70566.

We have a history of these sorts of unsafe settings in
`kv.raft_log.disable_synchronization_unsafe`.

Release note: None

74188: sql: fix InternalExecutor bug r=ajwerner a=ajwerner

Any time we use WithSyntheticDescriptors, it has to be on an unshared internal
executor. The move in #71246 to not have an internal executor hanging around
for the current session hurts here.

Fixes #73788

Release note: None

Co-authored-by: Andrew Werner <awerner32@gmail.com>
craig bot pushed a commit that referenced this issue Feb 2, 2022
74612: rangefeedcache,settingswatcher,systemcfgwatcher: lose gossip deps r=ajwerner a=ajwerner

This is a pile of commits which supersedes #69269 and pretty much puts in place all of the pieces to move on #70560. 

75726: sql: refactor pg_has_role to remove privilege.GRANT usage r=RichardJCai a=ecwall

refs #73129

Also combines some layers of privilege checking code.

Release note: None

75770: vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division r=nvanbenschoten a=nvanbenschoten

Picks up two PRs that improved the performance of `Quo`, `Sqrt`, `Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`:
- cockroachdb/apd#114
- cockroachdb/apd#115

Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. To verify that this noise wasn't hiding any correctness regressions caused by the rewrite of `Context.Quo` in the first PR, I created #75757, which only includes the first PR. #75757 passes CI with minimal testing changes. The testing changes that PR did require all have to do with trailing zeros, and most of them are replaced in this PR.

Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`.

----

### Speedup on TPC-DS dataset

The TPC-DS dataset is full of decimal columns, so it's a good playground to test this change. Unfortunately, the variance in the runtime performance of the TPC-DS queries themselves is high (many queries varied by 30-40% per attempt), so it was hard to get signal out of them. Instead, I imported the TPC-DS dataset with a scale factor of 10 and ran some custom aggregation queries against the largest table (web_sales, row count = 7,197,566):

```sql
# q1
select sum(ws_wholesale_cost / ws_ext_list_price) from web_sales;

# q2
select sum(ws_wholesale_cost / ws_ext_list_price - sqrt(ws_net_paid_inc_tax)) from web_sales;
```

Here's the difference in runtime of these two queries before and after this change on an `n2-standard-8` instance:

```
name              old s/op   new s/op   delta
TPC-DS/custom/q1  22.4 ± 0%   8.6 ± 0%  -61.33%  (p=0.002 n=6+6)
TPC-DS/custom/q2  91.4 ± 0%  32.1 ± 0%  -64.85%  (p=0.008 n=5+5)
```

75771: colexec: close the ordered sync used by the external sorter r=yuzefovich a=yuzefovich

**colexec: close the ordered sync used by the external sorter**

Previously, we forgot to close the ordered synchronizer that is used by
the external sorter to merge already sorted partitions. This could
result in some tracing spans never being finished and is now fixed.

Release note: None

**colexec: return an error rather than logging it on Close in some cases**

This error eventually will be logged anyway, but we should try to
propagate it up the stack as much as possible.

Release note: None

75807: ui: Add CircleFilled component r=ericharmeling a=ericharmeling

Previously, there was no component for a filled circle icon in the `ui` package.
Recent product designs have requested this icon for the DB Console (see #67510, #73463).
This PR adds a `CircleFilled` component to the `ui` package.

Release note: None

75813: sql: fix flakey TestShowRangesMultipleStores r=ajwerner a=ajwerner

Fixes #75699

Release note: None

75836: dev: add generate protobuf r=ajwerner a=ajwerner

Convenient, fast.

Release note: None

Co-authored-by: Andrew Werner <awerner32@gmail.com>
Co-authored-by: Evan Wall <wall@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Eric Harmeling <eric.harmeling@cockroachlabs.com>
@ajwerner ajwerner assigned ajwerner and unassigned irfansharif Feb 4, 2022
@craig craig bot closed this as completed in #76279 Feb 16, 2022
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.

2 participants