On Mon, Jun 30, 2025 at 9:41 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > I've rebased the patches to the current HEAD.
Hi Sawada-san, I've started reviewing this. I initially meant to review the eager scan part, but I think it would be easier to do that if it were in a separate patch on top of the rest of the parallel heap vacuuming patches -- just during review. I had a hard time telling which parts of the design (and code) were necessitated by the eager scanning feature. I started reviewing the rest of the code and this is some initial feedback on it: LV --- I don't think we should use "LV" and "lazy vacuum" anymore. Let's call it what it is: "heap vacuuming". We don't need to rename vacuumlazy.c right now, but if you are making new structs or substantially altering old ones, I wouldn't use "LV". table AM callbacks ------------------------- I think there is a fundamental tension here related to whether or not vacuumparallel.c should be table-AM agnostic. All of its code is invoked below heap_vacuum_rel(). I would argue that vacuumparallel.c is index AM-agnostic but heap-specific. I think this is what makes your table AM callbacks feel odd. Other table AM callbacks are invoked from general code -- for example from executor code that is agnostic to the underlying table AMs. But, all of the functions you added and the associated callbacks are invoked below heap_vacuum_rel() (as is all of vacuumparallel.c). duplicated information across structs ----------------------------------------------- I think a useful preliminary patch to this set would be to refactor LVRelState into multiple structs that each represent the static input, working state, and output of vacuum. You start to do this in one of your patches with LVScanData but it doesn't go far enough. In fact, I think there are members of all the parallel data structures you introduced and of those that are already present that overlap with members of the LVRelState and we could start putting these smaller structs. For my examples, I am referring both to existing code and code you added. For example, relnamespace, relname, and indname in LVRelState and ParallelVacuumState and ParallelVacuumState->heaprel and LVRelState->rel. Seems like you could have a smaller struct that is accessible to the vacuum error callback and also to the users in LVRelState. (Side note: In your patch, I find it confusing that these members of LVRelState are initialized in heap_parallel_vacuum_collect_dead_items() instead of heap_parallel_vacuum_initialize_worker().) And even for cases where the information has to be passed from the leader to the worker, there is no reason you can't have the same struct but in shared memory for the parallel case and local memory for the serial case. For example with the struct members "aggressive", "skipwithvm", and "cutoffs" in LVRelState and ParallelLVShared. Or ParallelLVScanDesc->nblocks and LVRelState->rel_pages. Ideally the entire part of the LVRelState that is an unchanging input/reference data is in a struct which is in local memory for the serial and local parallel case and a single read-only location in the shared parallel case. Or, for the shared case, if there is a reason not to read from the shared memory, we copy them over to a local instance of the same struct. Maybe it is called HeapVacInput or similar. There are a bunch of other instances like this. For example, the leader initializes LVScanData->NewRelfrozenXid from vacrel->cutoffs.OldestXmin, then uses this to set ParallelLVShared->NewRelfrozenXid. Then the worker copies it from ParallelLVShared->NewRelfrozenXid back to LVScanData->NewRelfrozenXid. But the worker has access to ParallelLVShared->cutoffs. So you could either get it from there or allow the workers to access the first LVScanData and have that always belong to the leader. Either way, I don't think you should need NewRelfrozenXid in ParallelLVShared. Another related concern: I don't understand why ParallelLVScanWorkerData->nallocated has to be in shared memory. I get that each worker has to keep track for itself how many blocks it has "processed" so far (either skipped or scanned) and I get that there has to be some coordination variable that keeps track of whether or not all blocks have been processed and where the next chunk is. But why does the worker's nallocated have to be in shared memory? It doesn't seem like the leader accesses it. (Side note: I'm rather confused by why ParallelLVScanDesc is its own thing [instead of part of ParallelLVShared] -- not to mention its chunk_size member appears to be unused.) Separately, I think the fact that PVShared and ParallelVacuumState stayed almost untouched (and continue to have general, parallel-sounding names) with your patch but now mostly deal with stuff about indexes while most other parallel vacuuming content is in other structs is confusing. I think we need to consider which members in PVShared and ParallelVacuumState are phase II only and put that in an appropriately named structure or get more of the heap vacuuming specific members in those generally named structures. - Melanie
