On Sat, Mar 20, 2021 at 3:40 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Mar 20, 2021 at 01:06:51PM +0900, Masahiko Sawada wrote:
> > It's not bad but it seems redundant a bit to me. We pass the idx in
> > spite of passing also Irel[idx] and &(vacrelstats->indstats[idx]). I
> > think your first idea that is done in v4 patch (saving index names at
> > the beginning of heap_vacuum_rel() for autovacuum logging purpose
> > only) and the idea of deferring to close indexes until the end of
> > heap_vacuum_rel() so that we can refer index name at autovacuum
> > logging are more simple.
>
> Okay.
>
> > We need to initialize *stats with NULL here.
>
> Right. I am wondering why I did not get any complain here.
>
> > If shared_indstats is NULL (e.g., we do " stats =
> > vacrelstats->indstats[indnum];"), vacrelstats->indstats[indnum] is not
> > updated since we pass &stats. I think we should pass
> > &(vacrelstats->indstats[indnum]) instead in this case.
>
> If we get rid completely of this idea around indnum, that I don't
> disagree with so let's keep just indname, you mean to keep the second
> argument IndexBulkDeleteResult of vacuum_one_index() and pass down
> &(vacrelstats->indstats[indnum]) as argument. No objections from me
> to just do that.
>
> > Previously, we update the element of the pointer array of index
> > statistics to the pointer pointing to either the local memory or DSM.
> > But with the above change, we do that only when the index statistics
> > are in the local memory. In other words, vacrelstats->indstats[i] is
> > never updated if the corresponding index supports parallel indexes. I
> > think this is not relevant with the change that we'd like to do here
> > (i.e., passing indnum down).
>
> Yeah, that looks like just some over-engineering design on my side.
> Would you like to update the patch with what you think is most
> adapted?
I've updated the patch. I saved the index names at the beginning of
heap_vacuum_rel() for autovacuum logging, and add indstats and
nindexes to LVRelStats. Some functions still have nindexes as a
function argument but it seems to make sense since it corresponds the
list of index relations (*Irel). Please review the patch.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
