Date: Thu, 19 Jun 2025 15:30:26 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: shihao zhong <zhong950419@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Fixes inconsistent behavior in vacuum when it processes multiple
 relations
Message-ID: <aFRzYhOTZcRgKPLu@nathan>
References: 
 <CAGRkXqTo+aK=GTy5pSc-9cy8H2F2TJvcrZ-zXEiNJj93np1UUw@mail.gmail.com>
 <aFLp9sMEHFHULOFx@nathan>
 <CAGRkXqTp9k_7X1OqqQSnVfoyGxAM3SQ083CcyaezBg9qQ8i+qg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="kspI2SpvKnsc4o9Q"
Content-Disposition: inline
In-Reply-To: 
 <CAGRkXqTp9k_7X1OqqQSnVfoyGxAM3SQ083CcyaezBg9qQ8i+qg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aFRzYhOTZcRgKPLu%40nathan>
Precedence: bulk


--kspI2SpvKnsc4o9Q
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jun 18, 2025 at 02:48:16PM -0400, shihao zhong wrote:
>>> That leads me to think (1) might be the better option, although I'm not too
>>> wild about the subtlety of the fix.
> 
> Thanks for your feedback. New patch is attached. I also updated the
> signature of do_analyze_rel for the same reason.

After thinking about this some more, I'm wondering if it would be better to
pursue option (2) because it's a little less invasive (which is important
because this will need to be back-patched).  In any case, we have a similar
problem when recursing to the TOAST table, which can be fixed by copying
the params at the top of vacuum_rel().

While testing out the attached patch, I noticed a couple of other
interesting problems in this area [0].

[0] https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan

-- 
nathan

--kspI2SpvKnsc4o9Q
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=vacuum_params_v3.patch

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 33a33bf6b1c..a43f090ee17 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -634,7 +634,15 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
 			if (params->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
+				VacuumParams params_copy;
+
+				/*
+				 * vacuum_rel() scribbles on the parameters, so give it a copy
+				 * to avoid affecting other relations.
+				 */
+				memcpy(&params_copy, params, sizeof(VacuumParams));
+
+				if (!vacuum_rel(vrel->oid, vrel->relation, &params_copy, bstrategy))
 					continue;
 			}
 
@@ -2008,9 +2016,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	Oid			save_userid;
 	int			save_sec_context;
 	int			save_nestlevel;
+	VacuumParams toast_vacuum_params;
 
 	Assert(params != NULL);
 
+	/*
+	 * This function scribbles on the parameters, so make a copy early to
+	 * avoid affecting the TOAST table (if we do end up recursing to it).
+	 */
+	memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
+
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
 
@@ -2299,15 +2314,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
 	if (toast_relid != InvalidOid)
 	{
-		VacuumParams toast_vacuum_params;
-
 		/*
 		 * Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it.  Likewise,
 		 * set toast_parent so that the privilege checks are done on the main
 		 * relation.  NB: This is only safe to do because we hold a session
 		 * lock on the main relation that prevents concurrent deletion.
 		 */
-		memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
 		toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
 		toast_vacuum_params.toast_parent = relid;
 

--kspI2SpvKnsc4o9Q--