Re: Parallel heap vacuum - Mailing list pgsql-hackers

Hi Sawada-san.

I was revisiting this thread after a long time. I found most of my
previous review comments from v11-0001 were not yet addressed. I can't
tell if they are deliberately left out, or if they are accidentally
overlooked. Please see the details below.

On Mon, Mar 10, 2025 at 3:05 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ======
> src/backend/access/table/tableamapi.c
>
> 2.
>   Assert(routine->relation_vacuum != NULL);
> + Assert(routine->parallel_vacuum_compute_workers != NULL);
>   Assert(routine->scan_analyze_next_block != NULL);
>
> Is it better to keep these Asserts in the same order that the
> TableAmRoutine fields are assigned (in heapam_handler.c)?
>

Not yet addressed?

> ~~~
>
> 3.
> + /*
> + * Callbacks for parallel vacuum are also optional (except for
> + * parallel_vacuum_compute_workers). But one callback implies presence of
> + * the others.
> + */
> + Assert(((((routine->parallel_vacuum_estimate == NULL) ==
> +   (routine->parallel_vacuum_initialize == NULL)) ==
> + (routine->parallel_vacuum_initialize_worker == NULL)) ==
> + (routine->parallel_vacuum_collect_dead_items == NULL)));
>
> /also optional/optional/
>

Not yet addressed?

> ======
> src/include/access/heapam.h
>
> +extern int heap_parallel_vacuum_compute_workers(Relation rel, int
> nworkers_requested);
>
> 4.
> wrong tab/space after 'int'.

Not yet addressed?

>
> ======
> src/include/access/tableam.h
>
> 5.
> + /*
> + * Compute the number of parallel workers for parallel table vacuum. The
> + * parallel degree for parallel vacuum is further limited by
> + * max_parallel_maintenance_workers. The function must return 0 to disable
> + * parallel table vacuum.
> + *
> + * 'nworkers_requested' is a >=0 number and the requested number of
> + * workers. This comes from the PARALLEL option. 0 means to choose the
> + * parallel degree based on the table AM specific factors such as table
> + * size.
> + */
> + int (*parallel_vacuum_compute_workers) (Relation rel,
> + int nworkers_requested);
>
> The comment here says "This comes from the PARALLEL option." and "0
> means to choose the parallel degree...". But, the PG docs [1] says "To
> disable this feature, one can use PARALLEL option and specify parallel
> workers as zero.".
>
> These two descriptions "disable this feature" (PG docs) and letting
> the system "choose the parallel degree" (code comment) don't sound the
> same. Should this 0001 patch update the PG documentation for the
> effect of setting PARALLEL value zero?

Not yet addressed?

>
> ~~~
>
> 6.
> + /*
> + * Initialize DSM space for parallel table vacuum.
> + *
> + * Not called if parallel table vacuum is disabled.
> + *
> + * Optional callback, but either all other parallel vacuum callbacks need
> + * to exist, or neither.
> + */
>
> "or neither"?
>
> Also, saying "all other" seems incorrect because
> parallel_vacuum_compute_workers callback must exist event if
> parallel_vacuum_initialize does not exist.
>
> IMO you meant to say "all optional", and "or none".
>
> SUGGESTION:
> Optional callback. Either all optional parallel vacuum callbacks need
> to exist, or none.
>
> (this same issue is repeated in multiple places).

Not yet addressed?

======
> [1] https://www.postgresql.org/docs/devel/sql-vacuum.html
Kind Regards,
Peter Smith.
Fujitsu Australia