gcjob: issue DeleteRange tombstones and then wait for GC#85878
gcjob: issue DeleteRange tombstones and then wait for GC#85878craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
chengxiong-ruan
left a comment
There was a problem hiding this comment.
LGTM, just very few nits.
| } | ||
|
|
||
| // performGC GCs any schema elements that are in the DELETING state and returns | ||
| // a bool indicating if it GC'd any elements. |
There was a problem hiding this comment.
looks like it does not return a bool.
| return err | ||
| }); err != nil { | ||
| if errors.Is(err, catalog.ErrDescriptorNotFound) { | ||
| // This can happen if another GC job created for the same table got to |
There was a problem hiding this comment.
a table can be dropped twice?
There was a problem hiding this comment.
We had bugs in the past where we'd create extra GC jobs.
|
|
||
| // TODO(ajwerner): How does this happen? | ||
| if !table.Dropped() { | ||
| // We shouldn't drop this table yet. |
There was a problem hiding this comment.
should we error out if this can't happen?
There was a problem hiding this comment.
Erroring out in the GC job just leads to it retrying. I don't know how we get here. I think I'll log it.
fcd2a8d to
3f9904f
Compare
5d2889c to
875c130
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Only reviewed this at a high level, but LGTM. Thanks!
| // of interest. A case where one will show up in a changefeed is when | ||
| // the primary index changes while we're watching it and then the old | ||
| // primary index is dropped. In this case, we'll get a schema event to | ||
| // restart into the new primary index, but the DeleteRange may come | ||
| // through before the schema event. |
There was a problem hiding this comment.
Something tells me this will be an unwelcome surprise once we start using MVCC range tombstones for other stuff too, but we can cross that bridge when we get there.
| if err != nil || len(jobIDs) == 0 { | ||
| return err | ||
| } | ||
| log.Infof(ctx, "waiting for %d GC jobs to adopt the new protocol: %v", len(jobIDs), jobIDs) |
There was a problem hiding this comment.
If any of these jobs are in backoff due to errors, can we end up waiting for hours here? Or are they accounted for by nonTerminalStatusTupleString? If yes, would it be safe to ignore these since they would adopt the new protocol once they resume?
There was a problem hiding this comment.
This was a good suggestion! Thanks.
acd4357 to
ad75d48
Compare
We're going to need some new language to capture the states which correspond to ClearRange so we can differentiate them from the DelRange states. Release note: None
ad75d48 to
4530f4b
Compare
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.
…upgrade Release note: None
4530f4b to
de12cee
Compare
|
TFTR! bors r+ |
|
Build failed: |
|
The test failure was in bors r+ |
|
Build succeeded: |
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.