Date: Thu, 27 Feb 2020 23:25:45 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: yuzuko <yuzukohosoya@gmail.com>
Cc: Amit Langote <amitlangote09@gmail.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
Message-ID: <20200228022545.GA19686@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAKkQ509Gn8bOEZNmcudBYj=g97qG4PcNC2pbGxC4Ci_pPtQrEQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hello Yuzuko,

> +	 * Report ANALYZE to the stats collector, too.  If the table is a
> +	 * partition, report changes_since_analyze of its parent because
> +	 * autovacuum process for partitioned tables needs it.  Reset the
> +	 * changes_since_analyze counter only if we analyzed all columns;
> +	 * otherwise, there is still work for auto-analyze to do.
>  	 */
> -	if (!inh)
> -		pgstat_report_analyze(onerel, totalrows, totaldeadrows,
> -							  (va_cols == NIL));
> +	if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +	pgstat_report_analyze(onerel, totalrows, totaldeadrows,
> +						  (va_cols == NIL));
 
Hmm, I think the comment has a bug: it says "report ... of its parent"
but the report is of the same rel.  (The pgstat_report_analyze line is
mis-indented also).


>  	/*
> +	 * If the relation is a partitioned table, we must add up children's
> +	 * reltuples.
> +	 */
> +	if (classForm->relkind == RELKIND_PARTITIONED_TABLE)
> +	{
> +		List     *children;
> +		ListCell *lc;
> +
> +		reltuples = 0;
> +
> +		/* Find all members of inheritance set taking AccessShareLock */
> +		children = find_all_inheritors(relid, AccessShareLock, NULL);
> +
> +		foreach(lc, children)
> +		{
> +			Oid        childOID = lfirst_oid(lc);
> +			HeapTuple  childtuple;
> +			Form_pg_class childclass;
> +
> +			/* Ignore the parent table */
> +			if (childOID == relid)
> +				continue;

I think this loop counts partitioned partitions multiple times, because
we add up reltuples for all levels, no?  (If I'm wrong, that is, if
a partitioned rel does not have reltuples, then why skip the parent?)
 
> +	/*
> +	 * If the table is a leaf partition, tell the stats collector its parent's
> +	 * changes_since_analyze for auto analyze
> +	 */
> +	if (IsAutoVacuumWorkerProcess() &&
> +		rel->rd_rel->relispartition &&
> +		!(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))

I'm not sure I understand why we do this only on autovac. Why not all
analyzes?

> +	{
> +		Oid      parentoid;
> +		Relation parentrel;
> +		PgStat_StatDBEntry *dbentry;
> +		PgStat_StatTabEntry *tabentry;
> +
> +		/* Get its parent table's Oid and relation */
> +		parentoid = get_partition_parent(RelationGetRelid(rel));
> +		parentrel = table_open(parentoid, AccessShareLock);

Climbing up the partitioning hierarchy acquiring locks on ancestor
relations opens up for deadlocks.  It's better to avoid that.  (As a
test, you could try what happens if you lock the topmost relation with
access-exclusive and leave a transaction open, then have autoanalyze
run).  At the same time, I wonder if it's sensible to move one level up
here, and also have pgstat_report_partanalyze move more levels up.

> + * pgstat_report_partanalyze() -
> + *
> + *	Tell the collector about the parent table of which partition just analyzed.
> + *
> + * Caller must provide a child's changes_since_analyze as a parents.

I'm not sure what the last line is trying to say.


Thanks,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services