Re: POC: Parallel processing of indexes in autovacuum - Mailing list pgsql-hackers

Thanks for the patches!

I have only reviewed the v8-0001-Parallel-index-autovacuum.patch so far and
have a few comments from my initial pass.

1/ Please run pgindent.

2/ Documentation is missing. There may be more, but here are the places I
found that likely need updates for the new behavior, reloptions, GUC, etc.
Including docs in the patch early would help clarify expected behavior.

https://www.postgresql.org/docs/current/routine-vacuuming.html#VACUUM-BASICS
https://www.postgresql.org/docs/current/routine-vacuuming.html#AUTOVACUUM
https://www.postgresql.org/docs/current/runtime-config-autovacuum.html
https://www.postgresql.org/docs/current/sql-createtable.html
https://www.postgresql.org/docs/current/sql-altertable.html
https://www.postgresql.org/docs/current/runtime-config-resource.html#GUC-MAX-WORKER-PROCESSES
https://www.postgresql.org/docs/current/runtime-config-resource.html#GUC-MAX-PARALLEL-MAINTENANCE-WORKERS

One thing I am unclear on is the interaction between max_worker_processes,
max_parallel_workers, and max_parallel_maintenance_workers. For example, does
the following change mean that manual VACUUM PARALLEL is no longer capped by
max_parallel_maintenance_workers?

@@ -597,8 +614,8 @@ parallel_vacuum_compute_workers(Relation *indrels,
int nindexes, int nrequested,
        parallel_workers = (nrequested > 0) ?
                Min(nrequested, nindexes_parallel) : nindexes_parallel;

-       /* Cap by max_parallel_maintenance_workers */
-       parallel_workers = Min(parallel_workers,
max_parallel_maintenance_workers);
+       /* Cap by GUC variable */
+       parallel_workers = Min(parallel_workers, max_parallel_workers);


3/ Shouldn't this be "max_parallel_workers" instead of "bgworkers pool" ?

+                       "autovacuum_parallel_workers",
+                       "Maximum number of parallel autovacuum workers
that can be taken from bgworkers pool for processing this table. "

4/ The comment "When parallel autovacuum worker die" suggests an abnormal
exit. "Terminates" seems clearer, since this applies to both normal and
abnormal exits.

instead of:
+ * When parallel autovacuum worker die,

how about this:
* When parallel autovacuum worker terminates,


5/ Any reason AutoVacuumReleaseParallelWorkers cannot be called before
DestroyParallelContext?

+       nlaunched_workers = pvs->pcxt->nworkers_launched; /* remember
this value */
        DestroyParallelContext(pvs->pcxt);
+
+       /* Release all launched (i.e. reserved) parallel autovacuum workers. */
+       if (AmAutoVacuumWorkerProcess())
+               AutoVacuumReleaseParallelWorkers(nlaunched_workers);


6/ Also, would it be cleaner to move AmAutoVacuumWorkerProcess() inside
AutoVacuumReleaseParallelWorkers()?

if (!AmAutoVacuumWorkerProcess())
        return;

7/ It looks like the psql tab completion for autovacuum_parallel_workers is
missing:

test=# alter table t set (autovacuum_
autovacuum_analyze_scale_factor
autovacuum_analyze_threshold
autovacuum_enabled
autovacuum_freeze_max_age
autovacuum_freeze_min_age
autovacuum_freeze_table_age
autovacuum_multixact_freeze_max_age
autovacuum_multixact_freeze_min_age
autovacuum_multixact_freeze_table_age
autovacuum_vacuum_cost_delay
autovacuum_vacuum_cost_limit
autovacuum_vacuum_insert_scale_factor
autovacuum_vacuum_insert_threshold
autovacuum_vacuum_max_threshold
autovacuum_vacuum_scale_factor
autovacuum_vacuum_threshold

--

Sami Imseih
Amazon Web Services (AWS)