Re: add PROCESS_MAIN to VACUUM

On Mon, Mar 06, 2023 at 01:13:37PM -0800, Nathan Bossart wrote:
> 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.

> 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)

I would move this comment inside of the outer if statement since it is
distinguishing between the two branches of the inner if statement.

Also, I would still consider putting a comment above that reminds us that
VACOPT_PROCESS_MAIN is the default and will vacuum the main relation.

>  	/*
>  	 * 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);


- Melanie