Date: Mon, 26 Oct 2020 22:22:50 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: yuzuko <yuzukohosoya@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	Amit Langote <amitlangote09@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Laurenz Albe <laurenz.albe@cybertec.at>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Greg Stark <stark@mit.edu>
Subject: Re: Autovacuum on partitioned table (autoanalyze)
Message-ID: <20201027032249.GE9241@telsasoft.com>
References: <A7A3BAC3-4540-4946-8DAE-1D48370E2849@yesql.se>
 <CAKkQ50_+AXKTS4JUuoD4HDboLddeXgmDZfhJ+oahirnZGKOxbw@mail.gmail.com>
 <ACE6950A-7C00-4BFB-90DA-DEBFF870565C@yesql.se>
 <CAKkQ50_DWOiyxTFRaiprYE4UqHmxL4JDPNfxzmti6s6nz0i1uw@mail.gmail.com>
 <20200915.190135.1065752215377064117.horikyota.ntt@gmail.com>
 <CAKkQ50_D=1LbYLD9qtWaKxPvRq3T_qGWfQov5eH5rycbMEq-pA@mail.gmail.com>
 <CAKkQ508g4zNhw36rZ5Cjt9ANnrerGujP5cHfN2nrwO6JywDVhg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAKkQ508g4zNhw36rZ5Cjt9ANnrerGujP5cHfN2nrwO6JywDVhg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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