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



Reply via email to