Re: a verbose option for autovacuum

On Fri, Mar 19, 2021 at 3:14 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Mar 18, 2021 at 11:30:46PM +0900, Masahiko Sawada wrote:
> > Sorry, I attached the wrong version patch. So attached the right one.
>
> Thanks. I have been hacking aon that, and I think that we could do
> more in terms of integration of the index stats into LVRelStats to
> help with debugging issues, mainly, but also to open the door at
> allowing autovacuum to use the parallel path in the future.

Thank you for updating the patch!

> Hence,
> for consistency, I think that we should change the following things in
> LVRelStats:
> - Add the number of indexes. It looks rather unusual to not track
> down the number of indexes directly in the structure anyway, as the
> stats array gets added there.
> - Add all the index names, for parallel and non-parallel mode.

Agreed with those two changes.

> - Replace the index name in the error callback by an index number,
> pointing back to its location in indstats and indnames.

I like this idea but I'm not sure the approach that the patch took
improved the code. Please read the below my concern.

>
> As lazy_vacuum_index() requires the index number to be set internally
> to it, this means that we need to pass it down across
> vacuum_indexes_leader(), lazy_parallel_vacuum_indexes(), but that
> seems like an acceptable compromise to me for now. I think that it
> would be good to tighten a bit more the relationship between the index
> stats in the DSM for the parallel case and the ones in local memory,
> but what we have here looks enough to me so we could figure out that
> as a future step.
>
> Sawada-san, what do you think? Attached is the patch I have finished
> with.

With this idea, vacuum_one_index() will become:

static void
lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
LVDeadTuples *dead_tuples, double reltuples,
LVRelStats *vacrelstats, int indnum)

and the caller calls this function as follow:

for (idx = 0; idx < nindexes; idx++)
lazy_vacuum_index(Irel[idx], &(vacrelstats->indstats[idx]),
vacrelstats->dead_tuples,
vacrelstats->old_live_tuples, vacrelstats,
idx);

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.

BTW the patch led to a crash in my environment. The problem is here:

static void
-vacuum_one_index(Relation indrel, IndexBulkDeleteResult **stats,
+vacuum_one_index(Relation indrel,
LVShared *lvshared, LVSharedIndStats *shared_indstats,
- LVDeadTuples *dead_tuples, LVRelStats *vacrelstats)
+ LVDeadTuples *dead_tuples, LVRelStats *vacrelstats,
+ int indnum)
{
IndexBulkDeleteResult *bulkdelete_res = NULL;
+ IndexBulkDeleteResult *stats;

We need to initialize *stats with NULL here.

And while looking at the change of vacuum_one_index() I found another problem:

@@ -2349,17 +2400,20 @@ vacuum_one_index(Relation indrel,
IndexBulkDeleteResult **stats,
* Update the pointer to the corresponding
bulk-deletion result if
* someone has already updated it.
*/
- if (shared_indstats->updated && *stats == NULL)
- *stats = bulkdelete_res;
+ if (shared_indstats->updated)
+ stats = bulkdelete_res;
}
+ else
+ stats = vacrelstats->indstats[indnum];

/* Do vacuum or cleanup of the index */
if (lvshared->for_cleanup)
- lazy_cleanup_index(indrel, stats, lvshared->reltuples,
-
lvshared->estimated_count, vacrelstats);
+ lazy_cleanup_index(indrel, &stats, lvshared->reltuples,
+
lvshared->estimated_count, vacrelstats,
+ indnum);
else
- lazy_vacuum_index(indrel, stats, dead_tuples,
- lvshared->reltuples,
vacelstats);
+ lazy_vacuum_index(indrel, &stats, dead_tuples,
+ lvshared->reltuples,
vacrelstats, indnum);

/*
* Copy the index bulk-deletion result returned from ambulkdelete and

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.

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).

Regards,

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