Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 04 Jul 2025 11:21:52 -0300
Message-Id: <DB3C67FCRLOO.1R5NLYCNEA6BF@gmail.com>
Cc: "Sami Imseih" <samimseih@gmail.com>, "Maxim Orlov" <orlovmg@gmail.com>,
 "Postgres hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: POC: Parallel processing of indexes in autovacuum
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
To: "Daniil Davydov" <3danissimo@gmail.com>, "Masahiko Sawada"
 <sawada.mshk@gmail.com>
References: 
 <CACG=ezZOrNsuLoETLD1gAswZMuH2nGGq7Ogcc0QOE5hhWaw=cw@mail.gmail.com>
 <CAD21AoCdx5ZNS_cO7bYz1Zfb+Kw1kuJV2wtewrz7T1pPpjcWGw@mail.gmail.com>
 <CAJDiXgi6ZQOoSEqj9RyZMEh+HHBtmW0+PHD85UNPtKch8ubvdg@mail.gmail.com>
 <CAD21AoBcoA-i-pJ_=y+jg14R8_QaJA1iwktCnu5i-C=yXDFPdA@mail.gmail.com>
 <CAJDiXgjnUdE6Sk4M0unmT+9dULyFAxcum2txQKpWTuo4uQ_oXQ@mail.gmail.com>
 <CAD21AoBTZdVR93JBo620B=MX-K8cdm3VRbjrBr_Vcpngk3AjVw@mail.gmail.com>
 <CAA5RZ0vfBg=c_0Sa1Tpxv8tueeBk8C5qTf9TrxKBbXUqPc99Ag@mail.gmail.com>
 <CAD21AoBgvUeWS8ZsXBahA1XdYayK6DJ6dx49d6Xpii-iH+Hrwg@mail.gmail.com>
 <CAA5RZ0vF+Lr-jU1LAZWTGUjboUETk8oLvaNBbA5ozX6dau+how@mail.gmail.com>
 <CAJDiXggueLSGMNRmLshbmFRfbo4jzks0W8bLDfUSRZ-61fPVEQ@mail.gmail.com>
 <CAFY6G8cJ=DRTX75pOGerH6sk39dRt+7MSH+y_qppDdhPs=qdQA@mail.gmail.com>
 <CAJDiXgg1t6wk9NjyMUTm1iKqM9GtdQ_wrEchBtz3xjWBZM8W8A@mail.gmail.com>
 <CAD21AoAC0=Xi38RQcAO4A+vdmoXToZMoHfbS=KLT49fAOTH_gA@mail.gmail.com>
 <CAJDiXgiD+AZKhJSn-FSRVQxtDLmJd95wDu4wtKniQF5==1JcjQ@mail.gmail.com>
 <CAD21AoAM8KsqNhrZYJuf7odvxcTC0TumXazJc-r_wC5KnDFDPg@mail.gmail.com>
 <CAJDiXghbcOC9OOj3ampxuyqXH0geggnosnrYUHGygkpss-RtxA@mail.gmail.com>
 <CAD21AoAPnq0vrcGgeN++r1GoL8Kza7jaGL=TNzuBn6+MkR=rUQ@mail.gmail.com>
 <CAJDiXghmsbTmnm--9B5bbuZXa1OL7SZ0HYppX3tx9XsdwfJBhA@mail.gmail.com>
In-Reply-To: 
 <CAJDiXghmsbTmnm--9B5bbuZXa1OL7SZ0HYppX3tx9XsdwfJBhA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DB3C67FCRLOO.1R5NLYCNEA6BF%40gmail.com>
Precedence: bulk

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 b=
e 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 =3D 0	# disabled by default and limited b=
y 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 >=3D 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_autovacuu=
m_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%2ByzOZ=
pOZj3B-4FjW1Mr4qEdbQ%40mail.gmail.com

--
Matheus Alcantara