Re: Fixes inconsistent behavior in vacuum when it processes multiple relations

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Nathan Bossart <[email protected]>
To: shihao zhong <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fixes inconsistent behavior in vacuum when it processes multiple relations
Date: Thu, 19 Jun 2025 15:30:26 -0500
Message-ID: <aFRzYhOTZcRgKPLu@nathan> (raw)
In-Reply-To: <CAGRkXqTp9k_7X1OqqQSnVfoyGxAM3SQ083CcyaezBg9qQ8i+qg@mail.gmail.com>
References: <CAGRkXqTo+aK=GTy5pSc-9cy8H2F2TJvcrZ-zXEiNJj93np1UUw@mail.gmail.com>
	<aFLp9sMEHFHULOFx@nathan>
	<CAGRkXqTp9k_7X1OqqQSnVfoyGxAM3SQ083CcyaezBg9qQ8i+qg@mail.gmail.com>

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

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;
 


Attachments:

  [text/plain] vacuum_params_v3.patch (1.8K, 2-vacuum_params_v3.patch)
  download | inline diff:
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;

view thread (32+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Fixes inconsistent behavior in vacuum when it processes multiple relations
  In-Reply-To: <aFRzYhOTZcRgKPLu@nathan>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox