Re: Autovacuum on partitioned table (autoanalyze)

At Thu, 5 Nov 2020 16:03:12 +0900, yuzuko <yuzukohosoya@gmail.com> wrote in 
> Hi Justin,
> 
> Thank you for your comments.
> I attached the latest patch(v11) to the previous email.
> 
> >
> > +                * Get its all ancestors to propagate changes_since_analyze count.
> > +                * However, when ANALYZE inheritance tree, we get ancestors of
> > +                * toprel_oid to avoid needless counting.
> >
> > => I don't understand that comment.
> >
> I fixed that comment.

+		 * Get its all ancestors to propagate changes_since_analyze count.
+		 * However, when we have a valid toprel_oid, that is ANALYZE inheritance
+		 * tree, if we propagate the number to all ancestors, the next analyze
+		 * on partitioned tables in the tree could happen shortly expected.
+		 * So we get ancestors of toprel_oid which are not analyzed this time.

In second thought about the reason for the "toprel_oid". It is perhaps
to avoid "wrongly" propagated values to ancestors after a manual
ANALYZE on a partitioned table.  But the same happens after an
autoanalyze iteration if some of the ancestors of a leaf relation are
analyzed before the leaf relation in a autoanalyze iteration.  That
can trigger an unnecessary analyzing for some of the ancestors.
So we need to do a similar thing for autovacuum, However..

  [1(root):analzye]-[2:DONT analyze]-[3:analyze]-[leaf]

In this case topre_oid is invalid (since it's autoanalyze) but we
should avoid propagating the count to 1 and 3 if it is processed
*before* the leaf, but should propagate to 2. toprel_oid doesn't work
in that case.

So, to propagate the count properly, we need to analyze relations
leaf-to-root order, or propagate the counter only to anscestors that
haven't been processed in the current iteration.  It seems a bit too
complex to sort analyze relations in that order.  The latter would be
relatively simple.  See the attached for how it looks like.

Anyway, either way we take, it is not pgstat.c's responsibility to do
that since the former need to heavily reliant to what analyze does,
and the latter need to know what anlyze is doing.


> > Also, you called SearchSysCacheCopy1, but didn't free the tuple.  I don't think
> > you need to copy it anyway - just call ReleaseSysCache().
> >
> Fixed it.

Mmm. Unfortunately, that fix leaks cache reference when
!RELKIND_HAS_STORAGE.

> > Regarding the counters in pg_stat_all_tables: maybe some of these should be
> > null rather than zero ?  Or else you should make an 0001 patch to fully
> > implement this view, with all relevant counters, not just n_mod_since_analyze,
> > last_*analyze, and *analyze_count.  These are specifically misleading:
> >
> > last_vacuum         |
> > last_autovacuum     |
> > n_ins_since_vacuum  | 0
> > vacuum_count        | 0
> > autovacuum_count    | 0
> >
> I haven't modified this part yet, but you meant that we should set
> null to counters
> about vacuum because partitioned tables are not vacuumed?

Perhaps bacause partitioned tables *cannot* be vacuumed.  I'm not sure
what is the best way here.  Showing null seems reasonable but I'm not
sure that doesn't break anything.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center