Re: Parallel heap vacuum

Hi Sawada-San,

Here are some review comments for patch v11-0001.

======
src/backend/access/heap/vacuumlazy.c

1.
+/*
+ * Compute the number of workers for parallel heap vacuum.
+ *
+ * Return 0 to disable parallel vacuum so far.
+ */
+int
+heap_parallel_vacuum_compute_workers(Relation rel, int nworkers_requested)

You don't need to say "so far".

======
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)?

~~~

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/

======
src/include/access/heapam.h

+extern int heap_parallel_vacuum_compute_workers(Relation rel, int
nworkers_requested);

4.
wrong tab/space after 'int'.

======
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?

~~~

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).

======
[1] https://www.postgresql.org/docs/devel/sql-vacuum.html

Kind Regards,
Peter Smith.
Fujitsu Australia