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/
