Re: Autovacuum on partitioned table (autoanalyze)

On Fri, Oct 23, 2020 at 03:12:51PM +0900, yuzuko wrote:
> 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.

+                * 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.

+                       /* Find all members of inheritance set taking AccessShareLock */
+                       children = find_all_inheritors(relid, AccessShareLock, NULL);

=> Do you know that returns the table itself ?  And in pg14dev, each
partitioned table has reltuples = -1, not zero...

+                               /* Skip foreign partitions */
+                               if (childclass->relkind == RELKIND_FOREIGN_TABLE)
+                                       continue;

=> Michael's suggrestion is to use RELKIND_HAS_STORAGE to skip both foreign and
partitioned tables.

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

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

-- 
Justin