materialize icon indicating copy to clipboard operation
materialize copied to clipboard

persist: consolidate pending gc requests per handle

Open pH14 opened this issue 1 year ago • 0 comments

There are a few scenarios in which we expect to build up significant work for GC under normal usage - while a reader is consuming a snapshot, or when a reader dies and its seqno capability lingers until the reader is expired. There are also unexpected opportunities for GC work to accumulate, like from all the bugs we may or may not introduce 😁

No matter the scenario, a challenge with the current GC design is that a single process can duplicate work with itself. ex. if a handle gets assigned GC work from seqno [0, 5), nothing stops it from also being assigned [0, 10) a short while later if it could not complete the initial work in time. This is especially problematic because GC work is most likely to be duplicated exactly when the work has fallen behind, because that's when there's more to catch up on!


This PR introduces a single GC task per handle that consumes GC requests from a channel, batching them up when possible, rather than spinning up a task per request. This ensures GC is happening in a single place, and once combined with https://github.com/MaterializeInc/materialize/pull/13961, we'll be able to ensure GC is always working over a bounded number of states at a given time.

This PR does not make any effort to deduplicate GC requests across different processes. ([tangent] though as I'm writing this, once this is in, maybe we can implement that fairly easily using SELECT FOR UPDATE SKIP LOCKED queries? that way a process can grab as many non-locked rows as it needs to up to new_seqno_since, and if a process dies after claiming certain rows, the row locks will expire and a future gc req will get to claim them)

Motivation

  • This PR fixes a recognized bug.

Tips for reviewer

The Rust async changes/type signatures are kinda grisly, I don't know what I'm doing there

Checklist

pH14 avatar Aug 04 '22 12:08 pH14

Debating whether I want to dedicate time to write a test for the consolidating... it's tricky because it's in an async task. I think it could be done through something like a LatchedConsensus impl that only allows a request to go through when the test harness releases the latch. This way we could submit a gc req --> background tasks starts working on it, gets stuck on consensus op --> we submit many gc reqs that overlap --> release latch (first gc req finishes) --> release latch (verify all other gc reqs got merged together and did the right work)

And now in writing that out, I think that'll take enough time where it's worth spending time elsewhere for right now. Will add to my backlog list of persist tests

pH14 avatar Aug 17 '22 15:08 pH14