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

sql: migrate schema object removals and GC to DeleteRange #70427

Closed
erikgrinaker opened this issue Sep 20, 2021 · 2 comments · Fixed by #85878
Closed

sql: migrate schema object removals and GC to DeleteRange #70427

erikgrinaker opened this issue Sep 20, 2021 · 2 comments · Fixed by #85878
Assignees
@ajwerner
Labels
A-disaster-recovery A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Sep 20, 2021 •
edited by cockroach-jira-scripts
Loading

Currently, removal of schema objects such as tables, indexes, and tenants are done by changing the descriptor visibility and scheduling a job to remove the object data using ClearRange once the GC TTL expires. However, as described in #69380, when DeleteRange is changed to use MVCC range tombstones in #70415 we should use DeleteRange to remove these objects instead. This includes removing the schema GC jobs entirely, and relying on MVCC GC to remove the data when the TTL expires (requires #70414).

There is a much cheaper initial option in #76105.

Related to #31563.

Epic CRDB-2624

Jira issue: CRDB-10070
@erikgrinaker erikgrinaker added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-schema-changes T-disaster-recovery labels Sep 20, 2021
@erikgrinaker erikgrinaker mentioned this issue Sep 20, 2021
Closed
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Sep 21, 2021
@ajwerner
Copy link
Contributor

Reflecting on this a bit, the primary challenge will be the migration away from the GC job so that we can delete that code. My instinct is that the way to do it is to:

  1. Create a version gate that follows the new DelRange being enabled (VersionDrainGCJobs)
    • This version reversion also prevents the creation of GC Job.
  2. Add logic in the GC Job in 22.1 that detects the version activation and then issues the new DelRange ops
  3. Add a migration with this versions which waits until there are no GC jobs.
    • Probably need to be careful about having a barrier in terms of ongoing transactions and schema change jobs

This is still hand wavy.

The problem with this is that it may take 2x GC TTL for data to get removed. Seems fine.

@erikgrinaker
Copy link
Contributor Author

Will we use DeleteRange for tenant GC as well? I believe it currently uses its own GC system similar to schema GC, with ClearRange. Since we'll be populating tenants using non-MVCC methods (e.g. with streaming replication), I suppose we might be able to keep using ClearRange when GCing them. I think I'd prefer to stick with MVCC where possible though.

@ajwerner ajwerner mentioned this issue Oct 19, 2021
Closed
@ajwerner ajwerner mentioned this issue Nov 29, 2021
Closed
@erikgrinaker erikgrinaker mentioned this issue Feb 5, 2022
Closed
@erikgrinaker erikgrinaker mentioned this issue Feb 21, 2022
Merged
craig bot pushed a commit that referenced this issue Feb 21, 2022
@erikgrinaker
Merge #76203
9c0d08d 
76203: batcheval: add range tombstone support for `DeleteRange` r=aliher1911,dt a=erikgrinaker

This patch adds a parameter `UseExperimentalRangeTombstone` for
`DeleteRange`, which deletes the span using an MVCC range tombstone.
This must only be called after checking
`storage.CanUseExperimentalMVCCRangeTombstones()`, which ensures the
`ExperimentalMVCCRangeTombstones` version gate and
`COCKROACH_EXPERIMENTAL_MVCC_RANGE_TOMBSTONES` environment variable are
set.

Did I mention that MVCC range tombstones are experimental? They are
currently under active development, and are not respected by the KV or
MVCC APIs, nor are they persisted. This patch simply sets up the
plumbing for it.

Resolves #70415.
Touches #70427.

Release note: None

Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>
@postamar postamar assigned postamar and unassigned chengxiong-ruan Feb 23, 2022
@postamar postamar removed their assignment Mar 11, 2022
@ajwerner ajwerner mentioned this issue Jul 12, 2022
Closed
This was referenced Aug 8, 2022
Closed
Closed
@ajwerner ajwerner self-assigned this Aug 8, 2022
This was referenced Aug 8, 2022
Closed
Closed
ajwerner added a commit to ajwerner/cockroach that referenced this issue Aug 10, 2022
@ajwerner
gcjob: issue DeleteRange tombstones and then wait for GC
f2a3ba4 
Note that this does not change anything about tenant GC.

Fixes cockroachdb#70427

Release note (sql change): The asynchronous garbage collection process has
been changed such that very soon after dropping a table, index, or database, or
after refreshing a materialized view, the system will issue range deletion
tombstones over the dropped data. These tombstones will result in the KV
statistics properly counting these bytes as garbage. Before this change, the
asynchronous "gc job" would wait out the TTL and then issue a lower-level
operation to clear out the data. That meant that while the job was waiting
out the TTL, the data would appear in the statistics to still be live. This
was confusing.
@ajwerner ajwerner mentioned this issue Aug 10, 2022
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue Aug 12, 2022
@ajwerner
gcjob: issue DeleteRange tombstones and then wait for GC
499b7bd 
Note that this does not change anything about tenant GC.

Fixes cockroachdb#70427

Release note (sql change): The asynchronous garbage collection process has
been changed such that very soon after dropping a table, index, or database, or
after refreshing a materialized view, the system will issue range deletion
tombstones over the dropped data. These tombstones will result in the KV
statistics properly counting these bytes as garbage. Before this change, the
asynchronous "gc job" would wait out the TTL and then issue a lower-level
operation to clear out the data. That meant that while the job was waiting
out the TTL, the data would appear in the statistics to still be live. This
was confusing.
craig bot pushed a commit that referenced this issue Aug 14, 2022
@arulajmani @ajwerner
Merge #85853 #85878
b6d1689 
85853: kv: ensure secondary tenants route follower reads to the closest replica r=arulajmani a=arulajmani

The dist sender uses node locality information to rank replicas of a
range by latency. Previously, this node locality information was read
off a node descriptor available in Gossip. Unfortunately, secondary
tenants do not have access to Gossip, and as such, would end up
randomizing this list of replicas. This manifested itself through
unpredictable latencies when running follower reads.

We're no longer susceptible to this hazard with this patch. This is done
by eschewing the need of a node descriptor from gossip in the
DistSender; instead, we now instantiate the DistSender with locality
information.

However, we do still use Gossip to get the current node's
ID when ranking replicas. This is done to ascertain if there is a local
replica, and if there is, to always route to it. Unfortunately, because
secondary tenants don't have access to Gossip, they can't conform to
these semantics. They're susceptible to a hazard where a request may
be routed to another replica in the same locality tier as the client
even though the client has a local replica as well. This shouldn't be
a concern in practice given the diversity heuristic.

Resolves #81000

Release note (bug fix): fix an issue where secondary tenants could
route follower reads to a random, far away replica instead of one
closer.

85878: gcjob: issue DeleteRange tombstones and then wait for GC r=ajwerner a=ajwerner

Note that this does not change anything about tenant GC.

Fixes #70427

Release note (sql change): The asynchronous garbage collection process has
been changed such that very soon after dropping a table, index, or database, or
after refreshing a materialized view, the system will issue range deletion
tombstones over the dropped data. These tombstones will result in the KV
statistics properly counting these bytes as garbage. Before this change, the
asynchronous "gc job" would wait out the TTL and then issue a lower-level
operation to clear out the data. That meant that while the job was waiting
out the TTL, the data would appear in the statistics to still be live. This
was confusing.

Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
@craig craig bot closed this as completed in 85590e2 Aug 14, 2022
@exalate-issue-sync exalate-issue-sync bot 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 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ajwerner ajwerner

Labels
A-disaster-recovery A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) 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.

gcjob: issue DeleteRange tombstones and then wait for GC ajwerner/cockroach
4 participants
@erikgrinaker @ajwerner @postamar @chengxiong-ruan