On Wed Jun 18, 2025 at 5:03 AM -03, Daniil Davydov wrote: > > Thanks for the review! Please, see v5 patch : > 1) GUC variable and field in autovacuum shmem are renamed > 2) ParallelAutoVacuumReleaseWorkers call moved from parallel.c to > vacuumparallel.c > 3) max_parallel_autovacuum_workers is now PGC_SIGHUP parameter > 4) Fix little bug (ParallelAutoVacuumReleaseWorkers in autovacuum.c:735) >
Thanks for the new version! The "autovacuum_max_parallel_workers" declared on guc_tables.c mention that is capped by "max_worker_process": + { + {"autovacuum_max_parallel_workers", PGC_SIGHUP, VACUUM_AUTOVACUUM, + gettext_noop("Maximum number of parallel autovacuum workers, that can be taken from bgworkers pool."), + gettext_noop("This parameter is capped by \"max_worker_processes\" (not by \"autovacuum_max_workers\"!)."), + }, + &autovacuum_max_parallel_workers, + 0, 0, MAX_BACKENDS, + check_autovacuum_max_parallel_workers, NULL, NULL + }, But the postgresql.conf.sample say that it is limited by max_parallel_workers: +#autovacuum_max_parallel_workers = 0 # disabled by default and limited by max_parallel_workers IIUC the code, it cap by "max_worker_process", but Masahiko has mention on [1] that it should be capped by max_parallel_workers. --- We actually capping the autovacuum_max_parallel_workers by max_worker_process-1, so we can't have 10 max_worker_process and 10 autovacuum_max_parallel_workers. Is that correct? +bool +check_autovacuum_max_parallel_workers(int *newval, void **extra, + GucSource source) +{ + if (*newval >= max_worker_processes) + return false; + return true; +} --- Locking unnecessary the AutovacuumLock if none if the if's is true can cause some performance issue here? I don't think that this would be a serious problem because this code will only be called if the configuration file is changed during the autovacuum execution right? But I could be wrong, so just sharing my thoughts on this (still learning about [auto]vacuum code). + +/* + * Make sure that number of available parallel workers corresponds to the + * autovacuum_max_parallel_workers parameter (after it was changed). + */ +static void +check_parallel_av_gucs(int prev_max_parallel_workers) +{ + LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE); + + if (AutoVacuumShmem->av_available_parallel_workers > + autovacuum_max_parallel_workers) + { + Assert(prev_max_parallel_workers > autovacuum_max_parallel_workers); + Typo on "exeed" + /* + * Number of available workers must not exeed limit. + * + * Note, that if some parallel autovacuum workers are running at this + * moment, available workers number will not exeed limit after releasing + * them (see ParallelAutoVacuumReleaseWorkers). + */ --- I'm not seeing an usage of this macro? +/* + * RelationGetParallelAutovacuumWorkers + * Returns the relation's parallel_autovacuum_workers reloption setting. + * Note multiple eval of argument! + */ +#define RelationGetParallelAutovacuumWorkers(relation, defaultpw) \ + ((relation)->rd_options ? \ + ((StdRdOptions *) (relation)->rd_options)->autovacuum.parallel_autovacuum_workers : \ + (defaultpw)) + --- Also pgindent is needed on some files. --- I've made some tests and I can confirm that is working correctly for what I can see. I think that would be to start include the documentation changes, what do you think? [1] https://www.postgresql.org/message-id/CAD21AoAxTkpkLtJDgrH9dXg_h%2ByzOZpOZj3B-4FjW1Mr4qEdbQ%40mail.gmail.com -- Matheus Alcantara