Re: a verbose option for autovacuum

On Wed, Mar 17, 2021 at 08:50:26AM -0300, Euler Taveira wrote:
> Since commit 5f8727f5a6, this patch doesn't apply anymore. Fortunately, it is
> just a small hunk. I reviewed this patch and it looks good to me. There is just
> a small issue (double space after 'if') that I fixed in the attached version.

No major objections to what you are proposing here.

>  	/* and log the action if appropriate */
> -	if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0)
> +	if (IsAutoVacuumWorkerProcess())
>  	{
> -		TimestampTz endtime = GetCurrentTimestamp();
> +		TimestampTz endtime = 0;
> +		int i;
>  
> -		if (params->log_min_duration == 0 ||
> -			TimestampDifferenceExceeds(starttime, endtime,
> -									   params->log_min_duration))
> +		if (params->log_min_duration >= 0)
> +			endtime = GetCurrentTimestamp();
> +
> +		if (endtime > 0 &&
> +			(params->log_min_duration == 0 ||
> +			 TimestampDifferenceExceeds(starttime, endtime,

Why is there any need to actually change this part?  If I am following
the patch correctly, the reason why you are doing things this way is
to free the set of N statistics all the time for autovacuum.  However,
we could make that much simpler, and your patch is already half-way
through that by adding the stats of the N indexes to LVRelStats.  Here
is the idea:
- Allocation of the N items for IndexBulkDeleteResult at the beginning
of heap_vacuum_rel().  It seems to me that we are going to need the N
index names within LVRelStats, to be able to still call
vac_close_indexes() *before* logging the stats.
- No need to pass down indstats for the two callers of
lazy_vacuum_all_indexes().
- Clean up of vacrelstats once heap_vacuum_rel() is done with it.
--
Michael