Date: Mon, 6 Mar 2023 13:13:37 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, vignesh C <vignesh21@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: add PROCESS_MAIN to VACUUM
Message-ID: <20230306211337.GA3076909@nathanxps13>
References: <20230119190807.GA4062411@nathanxps13>
 <Y/7xVESfLgw3E/F/@paquier.xyz>
 <20230301172637.GA1456203@nathanxps13>
 <ZAA5vt7jUTRpPDmL@paquier.xyz>
 <20230302065359.GA2140777@nathanxps13>
 <ZAWbkiZSdFZOTS7F@paquier.xyz>
 <20230306173723.GA2934573@nathanxps13>
 <20230306194009.5cn6sp3wjotd36nu@liskov>
 <20230306202734.GA3033389@nathanxps13>
 <CAAKRu_ZJMFrO_tspzoX43h9NyEFS1nS438MoS3Yv7JxK3=jFPw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="liOOAslEiF7prFVr"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAKRu_ZJMFrO_tspzoX43h9NyEFS1nS438MoS3Yv7JxK3=jFPw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230306211337.GA3076909%40nathanxps13>
Precedence: bulk


--liOOAslEiF7prFVr
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Mon, Mar 06, 2023 at 03:48:28PM -0500, Melanie Plageman wrote:
> On Mon, Mar 6, 2023 at 3:27 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Mon, Mar 06, 2023 at 02:40:09PM -0500, Melanie Plageman wrote:
>> > I noticed in vacuum_rel() in vacuum.c where table_relation_vacuum() is
>> > called, 4211fbd84 changes the else into an else if [1]. I understand
>> > after reading the commit and re-reading the code why that is now, but I
>> > was initially confused. I was thinking it might be nice to have a
>> > comment mentioning why there is no else case here (i.e. that the main
>> > table relation will be vacuumed on the else if branch).
>>
>> This was a hack to avoid another level of indentation for that whole block
>> of code, but based on your comment, it might be better to just surround
>> this entire section with an "if (params->options & VACOPT_PROCESS_MAIN)"
>> check.  WDYT?
> 
> I think that would be clearer.

Here's a patch.  Thanks for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--liOOAslEiF7prFVr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="process_main_readability_fix.patch"

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 580f966499..fb1ef28fa9 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2060,23 +2060,25 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, bool skip_privs)
 	/*
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
-	if ((params->options & VACOPT_FULL) &&
-		(params->options & VACOPT_PROCESS_MAIN))
+	if (params->options & VACOPT_PROCESS_MAIN)
 	{
-		ClusterParams cluster_params = {0};
+		if (params->options & VACOPT_FULL)
+		{
+			ClusterParams cluster_params = {0};
 
-		/* close relation before vacuuming, but hold lock until commit */
-		relation_close(rel, NoLock);
-		rel = NULL;
+			/* close relation before vacuuming, but hold lock until commit */
+			relation_close(rel, NoLock);
+			rel = NULL;
 
-		if ((params->options & VACOPT_VERBOSE) != 0)
-			cluster_params.options |= CLUOPT_VERBOSE;
+			if ((params->options & VACOPT_VERBOSE) != 0)
+				cluster_params.options |= CLUOPT_VERBOSE;
 
-		/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
-		cluster_rel(relid, InvalidOid, &cluster_params);
+			/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
+			cluster_rel(relid, InvalidOid, &cluster_params);
+		}
+		else
+			table_relation_vacuum(rel, params, vac_strategy);
 	}
-	else if (params->options & VACOPT_PROCESS_MAIN)
-		table_relation_vacuum(rel, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);

--liOOAslEiF7prFVr--