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

backupccl: backup does not check tenant active column #89997

Closed
knz opened this issue Oct 14, 2022 · 2 comments · Fixed by #96679
Closed

backupccl: backup does not check tenant active column #89997

knz opened this issue Oct 14, 2022 · 2 comments · Fixed by #96679
Assignees
@dt
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery

Comments

@knz
Copy link
Contributor

knz commented Oct 14, 2022 •
edited by exalate-issue-sync bot
Loading

Found while working on #89966.

This code in the backup planning points to a bug:

  rows, ⊙ ≔ ie.QueryBuffered(
    ⋄, "backupccl.retrieveAllTenantsMetadata", txn,
    // XXX Should we add a `WHERE active`? We require the tenant to be active
    // when it is specified

We should test the active column.

cc @adityamaru @dt for triage.

Jira issue: CRDB-20539

Epic CRDB-14582
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery T-disaster-recovery labels Oct 14, 2022
@blathers-crl
Copy link

blathers-crl bot commented Oct 14, 2022

cc @cockroachdb/disaster-recovery

@knz
Copy link
Contributor Author

knz commented Oct 14, 2022

comment from david:

I wonder we should really commit to working with KV on add span admin op locks to make these higher level span handling bugs less silently unsafe

@dt dt mentioned this issue Feb 6, 2023
Merged
craig bot pushed a commit that referenced this issue Feb 7, 2023
@pav-kv @knz
@nvanbenschoten @dt
Merge #96424 #96553 #96565 #96679
75eb88d 
96424: kvserver: factor out replica loading phase r=tbg a=pavelkalinnikov

This commit factors out Replica state loading and invariant checking so that it
is more consolidated rather than interleaved into Replica creation. Replica
creation path was loading this state, to post-factum assert that the in-memory
state matches the state in storage. Now this is a pre-requisite and input into
creating the Replica, and turning it from uninitialized to initialized state.

This change also eliminates the concept of "unloaded" Replica. Now the Replica
is created in a valid state (either initialized or uninitialized).

Touches #93898

96553: rpc,server: use node certs for shared-process tenant RPCs r=stevendanna a=knz

TLDR: This PR makes it possible to use regular node certs with shared-process multi-tenancy (i.e. skip creating client tenant certs). I think this will help `@aadityasondhi` 's work on `debug zip`.

Fixes #96215.
Epic: CRDB-23559

The PR also contains a few cleanup commits; it's a short change away from fixing  #87141 and #77173, which I intend to do in a separate PR.




96565: kv/logstore: avoid heap allocations around non-blocking sync waiter callback r=nvanbenschoten a=nvanbenschoten

This commit structures the non-blocking sync callback provided to the Raft log `SyncWaiterLoop` as a struct with a method that satisfies an interface (i.e. a functor) instead of an anonymous function. This provides more control over the memory layout of the callback and prevents individual fields from escaping to the heap. The change also provides the opportunity to pool the callback to avoid an additional heap allocation.

The change has the following effect on microbenchmarks:
```
name                                                 old time/op    new time/op    delta
ReplicaProposal/bytes=1.0_KiB,withFollower=false-10    40.1µs ± 3%    39.1µs ± 1%   -2.51%  (p=0.000 n=10+9)
ReplicaProposal/bytes=512_B,withFollower=false-10      38.2µs ± 2%    37.3µs ± 4%   -2.48%  (p=0.015 n=10+10)
ReplicaProposal/bytes=256_B,withFollower=false-10      37.1µs ± 1%    36.2µs ± 3%   -2.32%  (p=0.000 n=10+10)
ReplicaProposal/bytes=256_B,withFollower=true-10       52.2µs ± 1%    51.2µs ± 1%   -1.91%  (p=0.000 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=true-10     58.5µs ± 2%    57.5µs ± 2%   -1.79%  (p=0.001 n=10+10)
ReplicaProposal/bytes=512_B,withFollower=true-10       53.8µs ± 2%    52.8µs ± 1%   -1.74%  (p=0.000 n=10+10)

name                                                 old speed      new speed      delta
ReplicaProposal/bytes=512_B,withFollower=false-10    13.4MB/s ± 2%  13.7MB/s ± 4%   +2.57%  (p=0.016 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=false-10  25.5MB/s ± 2%  26.2MB/s ± 1%   +2.57%  (p=0.000 n=10+9)
ReplicaProposal/bytes=256_B,withFollower=false-10    6.91MB/s ± 1%  7.07MB/s ± 3%   +2.36%  (p=0.000 n=10+10)
ReplicaProposal/bytes=256_B,withFollower=true-10     4.90MB/s ± 1%  5.00MB/s ± 1%   +1.96%  (p=0.000 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=true-10   17.5MB/s ± 2%  17.8MB/s ± 1%   +1.82%  (p=0.001 n=10+10)
ReplicaProposal/bytes=512_B,withFollower=true-10     9.52MB/s ± 2%  9.69MB/s ± 1%   +1.76%  (p=0.000 n=10+10)

name                                                 old alloc/op   new alloc/op   delta
ReplicaProposal/bytes=256_B,withFollower=false-10      14.6kB ± 0%    12.8kB ± 0%  -12.73%  (p=0.000 n=10+10)
ReplicaProposal/bytes=256_B,withFollower=true-10       35.0kB ± 1%    30.6kB ± 1%  -12.59%  (p=0.000 n=10+10)
ReplicaProposal/bytes=512_B,withFollower=true-10       42.9kB ± 0%    38.6kB ± 1%  -10.04%  (p=0.000 n=8+10)
ReplicaProposal/bytes=512_B,withFollower=false-10      18.5kB ± 2%    16.8kB ± 1%   -9.19%  (p=0.000 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=true-10     60.6kB ± 1%    55.9kB ± 1%   -7.76%  (p=0.000 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=false-10    27.5kB ± 2%    25.6kB ± 2%   -7.06%  (p=0.000 n=10+10)

name                                                 old allocs/op  new allocs/op  delta
ReplicaProposal/bytes=512_B,withFollower=false-10        70.0 ± 0%      61.6 ± 1%  -12.00%  (p=0.000 n=10+10)
ReplicaProposal/bytes=256_B,withFollower=false-10        69.0 ± 0%      61.0 ± 0%  -11.59%  (p=0.000 n=10+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=false-10      73.0 ± 0%      65.0 ± 0%  -10.96%  (p=0.002 n=8+10)
ReplicaProposal/bytes=256_B,withFollower=true-10          179 ± 0%       161 ± 0%  -10.21%  (p=0.000 n=10+7)
ReplicaProposal/bytes=512_B,withFollower=true-10          181 ± 1%       162 ± 0%  -10.11%  (p=0.000 n=9+10)
ReplicaProposal/bytes=1.0_KiB,withFollower=true-10        186 ± 0%       168 ± 0%   -9.84%  (p=0.000 n=9+10)
```

Release note: None
Epic: None

96679: backupccl: only backup active tenants r=dt a=dt

Release note: none.
Epic: none.
Fixes: #89997.

Co-authored-by: Pavel Kalinnikov <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: David Taylor <[email protected]>
@craig craig bot closed this as completed in 36b4329 Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dt dt

Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

backupccl: only backup active tenants dt/cockroach
2 participants
@dt @knz