Skip to content

settings: simple refactors#111153

Merged
craig[bot] merged 10 commits intocockroachdb:masterfrom
knz:20230922-settings-tests
Sep 26, 2023
Merged

settings: simple refactors#111153
craig[bot] merged 10 commits intocockroachdb:masterfrom
knz:20230922-settings-tests

Conversation

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have a couple of nits on some earlier commits on other PRs.

:lgtm:

Reviewed 1 of 3 files at r1, 1 of 3 files at r2, 2 of 2 files at r3, 6 of 6 files at r4, 1 of 1 files at r5, 5 of 5 files at r6, 5 of 5 files at r7, 2 of 2 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, 1 of 1 files at r11, 3 of 3 files at r12, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)

@knz knz force-pushed the 20230922-settings-tests branch from bc4ef14 to 17f42ed Compare September 26, 2023 10:57
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Sep 26, 2023

I only have a couple of nits on some earlier commits on other PRs.

I have added 2 commits to this PR to process these nits.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Sep 26, 2023

bors r=yuzefovich,stevendanna

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 26, 2023

Build succeeded:

@craig craig bot merged commit c704759 into cockroachdb:master Sep 26, 2023
@knz knz deleted the 20230922-settings-tests branch September 26, 2023 13:28
craig bot pushed a commit that referenced this pull request Sep 29, 2023
110758: server,settings: properly cascade defaults for TenantReadOnly r=stevendanna,yuzefovich a=knz

Previous in sequence:
- [x] #110789 and #110947 
- [x] #110676
- [x] #111008
- [x] #111145
- [x] #111147
- [x]  #111149
- [x] #111150
- [x] #111153
- [x] #111475
- [x] #111210
- [x] #111212

Fixes #108677.
Fixes #85729.
Fixes #91825.
Completes the work described in the settings RFC.
Epic: CRDB-6671

TLDR: this patch ensures that virtual cluster servers observe changes
made to TenantReadOnly settings via SET CLUSTER SETTING in the system
interface, even when there is no override set via ALTER VIRTUAL
CLUSTER SET CLUSTER SETTING.

For example, after `SET CLUSTER SETTING
kv.closed_timestamp.target_duration = '10s'` in the system interface,
this value will show up via `SHOW CLUSTER SETTING` in a virtual
cluster SQL session.

This changes the way that settings are picked up in virtual cluster,
as follows:

1. if there is an override specifically for this tenant's ID
   (in `tenant_settings`), use that.
2. otherwise, if there is an override for the pseudo-ID 0
   (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER
   SETTING`), then use that.
3. **NEW**  otherwise, if the class is TenantReadOnly and there
   is a custom value in `system.settings`, set via a regular
   `SET CLUSTER SETTING` in the system tenant, then use that.
4. otherwise, use the global default set via the setting's
   `Register()` call.

----

Prior to this patch, TenantReadOnly settings as observed from virtual
clusters were defined as the following priority order:

1. if there is an override specifically for this tenant's ID
   (in `tenant_settings`), use that.
2. otherwise, if there is an override for the pseudo-ID 0
   (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER
   SETTING`), then use that.
3. otherwise, use the global default set via the setting's
   `Register()` call.

Remarkably, this did not pick up any changes made via a plain `SET
CLUSTER SETTING` statement via the system interface, which only
modifies this setting's value in `system.settings` (thus not
`tenant_settings`).

This situation was problematic in two ways.

To start, settings like `kv.closed_timestamp.target_duration` cannot
be set solely in `system.tenant_settings`; they are also used in the
storage layer and so must also be picked up from changes in
`system.settings`.

For these settings, it is common for operators to just issue the plain
`SET CLUSTER SETTING` statement (to update `system.settings`) and
simply forget to _also_ run `ALTER TENANT ALL SET CLUSTER SETTING`.

This mistake is nearly unavoidable and would result in incoherent
behavior, where the storage layer would use the customized value
and virtual clusters would use the registered global default.

The second problem is in mixed-version configurations, where
the storage layer runs version N+1 and the SQL service runs version N
of the executable. If the registered global default changes from
version N to N+1, the SQL service would not properly pick up
the new default defined in version N+1 of the storage layer.

This patch fixes both problems as follows:

- it integrates changes to TenantReadOnly settings observed
  in `system.settings`, to the watcher that tracks changes
  to `system.tenant_settings`. When a TenantReadOnly setting
  is present in the former but not the latter, a synthetic
  override is added.

- it also initializes synthetic overrides for all the
  TenantReadOnly settings upon server initialization,
  from the registered global default, so that virtual
  cluster servers always pick up the storage layer's
  default as override.


111383: *: simplify tests r=yuzefovich a=knz

All commits except the last are from #110758.
Epic: CRDB-6671

Now that "tenant-ro" settings take their default from the system tenant's value, we do not need `ALTER TENANT ALL` for them any more.

This patch simplifies test code accordingly.

Rcommended by `@yuzefovich` in the review for #110758. 

111440: cluster-ui: fix db page stories r=THardy98 a=THardy98

Epic: none

This change fixes the stories for the database pages.

Release note: None

111512: kv: correctly handle shared lock replays in KVNemesis r=nvanbenschoten a=arulajmani

Previously, we'd return an AssertionFailed error if a SKIP LOCKED request discovered another request from its own transaction waiting in a lock's wait queue. In SQL's use of KV, this can only happen if the SKIP LOCKED request is being replayed -- so returning an error here is fine. However, this tripped KVNemesis up.

This patch marks such errors, for the benefit of KVNemesis, and doesn't call them assertion failed errors.

Fixes #111426
Fixes #111506
Fixes #111513

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Thomas Hardy <thardy@cockroachlabs.com>
Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants