Re: Autovacuum on partitioned table (autoanalyze)

Horiguchi-san,

Thank you for your comments.

On Fri, Oct 23, 2020 at 8:23 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> Thanks you for the new version.
>
> At Fri, 23 Oct 2020 15:12:51 +0900, yuzuko <yuzukohosoya(at)gmail(dot)com> wrote in
> > Hello,
> >
> > I reconsidered a way based on the v5 patch in line with
> > Horiguchi-san's comment.
> >
> > This approach is as follows:
> > - A partitioned table is checked whether it needs analyze like a plain
> > table in relation_needs_vacanalyze(). To do this, we should store
> > partitioned table's stats (changes_since_analyze).
> > - Partitioned table's changes_since_analyze is updated when
> > analyze a leaf partition by propagating its changes_since_analyze.
> > In the next scheduled analyze time, it is used in the above process.
> > That is, the partitioned table is analyzed behind leaf partitions.
> > - The propagation process differs between autoanalyze or plain analyze.
> > In autoanalyze, a leaf partition's changes_since_analyze is propagated
> > to *all* ancestors. Whereas, in plain analyze on an inheritance tree,
> > propagates to ancestors not included the tree to avoid needless counting.
> >
> > Attach the latest patch to this email.
> > Could you check it again please?
>
> + /*
> + * 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.
> + */
> + if (!OidIsValid(toprel_oid))
> + ancestors = get_partition_ancestors(RelationGetRelid(rel));
> + else
> + ancestors = get_partition_ancestors(toprel_oid);
>
> This comment doesn't explaining what the code intends but what the
> code does.
>
> The reason for the difference is that if we have a valid toprel_oid,
> we analyze all descendants of the relation this time, and if we
> propagate the number to the descendants of the top relation, the next
> analyze on the relations could happen shortly than expected.
>
I modified this comment according to your advice.

>
> - msg.m_live_tuples = livetuples;
> - msg.m_dead_tuples = deadtuples;
> + msg.m_live_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + ? 0 /* if this is a partitioned table, skip modifying */
> + : livetuples;
> + msg.m_dead_tuples = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + ? 0 /* if this is a partitioned table, skip modifying */
> + : deadtuples;
>
> Two successive branching with the same condition looks odd. And we
> need an explanation of *why* we don't set the values for partitioned
> tables.

I moved this part to the previous block that livetuples and deadtuples are set.
Actually, I think the reason why those counters are set 0 when the given
relation is a partitioned table is that such a table doesn't have any data.
About changes_since_analyze counter, we should support exceptionally
in order to check whether partitioned tables need auto analyze.
I added this description to the comment of this function.

>
> + foreach(lc, ancestors)
> + {
> + Oid parentOid = lfirst_oid(lc);
> + Relation parentrel;
> +
> + parentrel = table_open(parentOid, AccessShareLock);
>
> I'm not sure, but all of the ancestors always cannot be a parent (in
> other words, a parent of a parent of mine is not a parent of
> mine). Isn't just rel sufficient?
>
I changed 'parentrel' to 'rel'.

>
>
> - * Report ANALYZE to the stats collector, too. However, if doing
> - * inherited stats we shouldn't report, because the stats collector only
> - * tracks per-table stats. Reset the changes_since_analyze counter only
> - * if we analyzed all columns; otherwise, there is still work for
> - * auto-analyze to do.
> + * Report ANALYZE to the stats collector, too. Reset the
> + * changes_since_analyze counter only if we analyzed all columns;
> + * otherwise, there is still work for auto-analyze to do.
> */
> - if (!inh)
> + if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> pgstat_report_analyze(onerel, totalrows, totaldeadrows,
>
> This still rejects traditional inheritance nonleaf relations. But if
> we remove the description about that completely in the comment above,
> we should support traditional inheritance parents here. I think we
> can do that as far as we need to propagate only per-tuple stats (that
> mans not per-attribute) like changes_since_analyze.
>
Regarding manual ANALYZE, not auto ANALYZE, when analyzing declarative
partitioning, all children are also analyzed at the same time. However,
in the case of traditional inheritance, we need to run that command on
each child table individually. That is, they are not analyzed all together
by ANALYZE. So I tried to support auto analyze for declarative
partitioning for now.
Added that we only support declarative partitioning to that comment.

> Whichever way we take, do we need the description about the behavior
> in the documentation?
>
Added a description about this patch to the document.

I attach the latest patch to this email.
Could you please check it again?

--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center