Re: Parallel heap vacuum

On Wed, Aug 27, 2025 at 2:30 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Aug 26, 2025 at 8:55 AM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
>
> > If you do parallel worker setup below heap_vacuum_rel(), then how are
> > you supposed to use those workers to do non-heap table vacuuming?
>
> IIUC non-heap tables can call parallel_vacuum_init() in its
> relation_vacuum table AM callback implementation in order to
> initialize parallel table vacuum, parallel index vacuum, or both.

Yep, it's just that that is more like a library helper function.
Currently, in master, the flow is:

leader does:
heap_vacuum_rel()
dead_items_alloc()
parallel_vacuum_init()
passes parallel_vacuum_main() to parallel context
lazy_scan_heap()
...
parallel_vacuum_process_all_indexes()
LaunchParallelWorkers()
...
parallel_vacuum_process_[un]safe_indexes()

For the leader, I don't remember where your patch moves
LaunchParallelWorkers() -- since it would have to move to before the
first heap vacuuming phase. When I was imagining what made the most
sense to make parallelism agnostic of heap, it seemed like it would be
moving LaunchParallelWorkers() above heap_vacuum_rel(). But I
recognize that is a pretty big change, and I haven't tried it, so I
don't know all the complications. (sounds like you mention some
below).

Basically, right now in master, we expose some functions for parallel
index vacuuming that people can call in their own table implementation
of table vacuuming, but we don't invoke any of them in table-AM
agnostic code. That may work fine for just index vacuuming, but now
that we are trying to make parts of the table vacuuming itself
extensible, the interface feels more awkward.

I won't push the idea further because I'm not even sure if it works,
but the thing that felt the most natural to me was pushing parallelism
above the table AM.

> > All the code in vacuumparallel.c is invoked from below
> > lazy_scan_heap(), so I don't see how having a
> > vacuumparallel.c-specific callback struct solves the layering
> > violation.
>
> I think the layering problem we discussed is about where the callbacks
> are declared; it seems odd that we add new table AM callbacks that are
> invoked only by another table AM callback. IIUC we invoke all codes in
> vacuumparallel.c in vacuumlazy.c even today. If we think we think this
> design has a problem in terms of layering of functions, we can
> refactor it as a separate patch.

Sure, but we are doubling down on it. I think parallel heap vacuuming
would have a completely different interface if that layering worked
differently. But, again, I don't want to put this additional,
extremely complex barrier in the way. I brought it up because it's
what occurred to me when reviewing this. But, I think it's fine to
look for an achievable compromise.

> > It seems like parallel index vacuuming setup would have to be done in
> > vacuum_rel() if we want to reuse the same parallel workers for the
> > table vacuuming and index vacuuming phases and allow for different
> > table AMs to vacuum the tables in their own way using these parallel
> > workers.
>
> Hmm, let me clarify your idea as I'm confused. If the parallel
> context used for both table vacuuming and index vacuuming is set up in
> vacuum_rel(), its DSM would need to have some data too required by
> table AM to do parallel table vacuuming. In order to do that, table
> AMs somehow need to tell the necessary DSM size at least. How do table
> AMs tell that to parallel vacuum initialization function (e.g.,
> parallel_vacuum_init()) in vacuum_rel()?

Right, you'd have to gather more information sooner (number of indexes, etc).

> Also, if we set up the parallel context in vacuum_rel(), we would
> somehow need to pass it to the relation_vacuum table AM callback so
> that they can use it during their own vacuum operation. Do you mean to
> pass it via table_relation_vacuum()?

Yes, something like that. But, that would require introducing more
knowledge of index vacuuming into that layer.

I chatted with Andres about this a bit off-list and he suggested
passing a callback for vacuuming the table to parallel_vacuum_init().
I don't know exactly how this would play with parallel_vacuum_main()'s
current functionality, though.

I think this is a similar approach to what you mentioned in your earlier comment

> Based on our previous discussions, I have contemplated modifying the
> patch to define the callback functions for parallel table vacuum
> within a structure in vacuumparallel.c instead of using table AM's
> callbacks. This approach would require the leader to pass both the
> library name and handler function name to vacuumparallel.c, enabling
> workers to locate the handler function via load_external_function()
> and access the callback functions. Although it's technically feasible,
> I'm not sure that the design is elegant; while table AM seeking to use
> parallel index vacuuming can simply invoke parallel_vacuum_init(),
> those requiring parallel table vacuum would need to both provide the
> handler function and supply the library and handler function names.

Are there other drawbacks to requiring the library and handler function names?

I'd find it a bit awkward to pass ParallelWorkerMain as the
bgw_libary/function name in LaunchParallelWorkers and then have
another point where you pass other library and function names, but I'm
not really sure how to resolve that.

In your patch, for the workers, I know you added a state machine to
parallel_vacuum_main() so that it would do table and index vacuuming.
That makes sense to me. But doing it this way seems like it is
limiting extensibility. But finding somewhere to pass the callback
that doesn't feel like an awkward addition to the existing function
passed to parallel workers is also hard.

- Melanie