Re: a verbose option for autovacuum

On Thu, Mar 18, 2021 at 3:41 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Okay, I've updated the patch accordingly. If we add
IndexBulkDeleteResult to LVRelStats I think we can remove
IndexBulkDeleteResult function argument also from some other functions
such as lazy_parallel_vacuum_indexes() and vacuum_indexes_leader().
Please review the attached patch.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/