On Sat, Mar 20, 2021 at 3:40 PM Michael Paquier <mich...@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/
v6-index_stats_log.patch
Description: Binary data
