Date: Tue, 17 Aug 2021 06:30:18 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: =?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>,
	Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	yuzuko <yuzukohosoya@gmail.com>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	David Steele <david@pgmasters.net>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	Amit Langote <amitlangote09@gmail.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Laurenz Albe <laurenz.albe@cybertec.at>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	Greg Stark <stark@mit.edu>
Subject: Re: Autovacuum on partitioned table (autoanalyze)
Message-ID: <20210817113018.GG10479@telsasoft.com>
References: <202108161546.3zuouk76skr4@alvherre.pgsql>
 <202108162142.crk2myigb2ad@alvherre.pgsql>
 <20210816222810.GE10479@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="/Uq4LBwYP4y1W6pO"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210816222810.GE10479@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210817113018.GG10479%40telsasoft.com>
Precedence: bulk


--/Uq4LBwYP4y1W6pO
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Mon, Aug 16, 2021 at 05:28:10PM -0500, Justin Pryzby wrote:
> On Mon, Aug 16, 2021 at 05:42:48PM -0400, Álvaro Herrera wrote:
> > On 2021-Aug-16, Álvaro Herrera wrote:
> > 
> > > Here's the reversal patch for the 14 branch.  (It applies cleanly to
> > > master, but the unused member of PgStat_StatTabEntry needs to be
> > > removed and catversion bumped).
> > 
> > I have pushed this to both branches.  (I did not remove the item from
> > the release notes in the 14 branch.)
> 
> |    I retained the addition of relkind 'p' to tables included by
> |    pg_stat_user_tables, because reverting that would require a catversion
> |    bump.
> 
> Right now, on v15dev, it shows 0, which is misleading.
> Shouldn't it be null ?
> 
> analyze_count       | 0
> 
> Note that having analyze_count and last_analyze would be an an independently
> useful change.  Since parent tables aren't analyzed automatically, I have a
> script to periodically process them if they weren't processed recently.  Right
> now, for partitioned tables, the best I could find is to check its partitions:
> | MIN(last_analyzed) FROM pg_stat_all_tables psat JOIN pg_inherits i ON psat.relid=i.inhrelid
> 
> In 20200418050815.GE26953@telsasoft.com I wrote:
> |This patch includes partitioned tables in pg_stat_*_tables, which is great; I
> |complained awhile ago that they were missing [0].  It might be useful if that
> |part was split out into a separate 0001 patch (?).
> | [0] https://www.postgresql.org/message-id/20180601221428.GU5164%40telsasoft.com

I suggest the attached (which partially reverts the revert), to allow showing
correct data for analyze_count and last_analyzed.

Arguably these should be reported as null in v14 for partitioned tables, since
they're not "known to be zero", but rather "currently unpopulated".

n_mod_since_analyze | 0
n_ins_since_vacuum  | 0

Justin

--/Uq4LBwYP4y1W6pO
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Report-last_analyze-and-analyze_count-of-partitioned.patch"

From 0d0e149727d89115803b4528e15f5b3c04bd816b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 16 Aug 2021 22:55:06 -0500
Subject: [PATCH] Report last_analyze and analyze_count of partitioned tables..

In v14, partitioned tables are included, but these fields are being reported as
zero, which is misleading.
---
 src/backend/commands/analyze.c  | 36 ++++++++++++++++++++++-----------
 src/backend/postmaster/pgstat.c | 27 +++++++++++++++++--------
 2 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 8d7b38d170..0050df08f6 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -626,8 +626,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 								 PROGRESS_ANALYZE_PHASE_FINALIZE_ANALYZE);
 
 	/*
-	 * Update pages/tuples stats in pg_class, and report ANALYZE to the stats
-	 * collector ... but not if we're doing inherited stats.
+	 * Update pages/tuples stats in pg_class ... but not if we're doing
+	 * inherited stats.
 	 *
 	 * We assume that VACUUM hasn't set pg_class.reltuples already, even
 	 * during a VACUUM ANALYZE.  Although VACUUM often updates pg_class,
@@ -668,20 +668,32 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 								InvalidMultiXactId,
 								in_outer_xact);
 		}
-
+	}
+	else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
 		/*
-		 * Now report ANALYZE to the stats collector.
-		 *
-		 * We deliberately don't report to the stats collector when doing
-		 * inherited stats, 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.
+		 * Partitioned tables don't have storage, so we don't set any fields
+		 * in their pg_class entries except for reltuples, which is necessary
+		 * for auto-analyze to work properly, and relhasindex.
 		 */
+		vac_update_relstats(onerel, -1, totalrows,
+							0, hasindex, InvalidTransactionId,
+							InvalidMultiXactId,
+							in_outer_xact);
+	}
+
+	/*
+	 * Now report ANALYZE to the stats collector.  For regular tables, we do
+	 * it only if not doing inherited stats.  For partitioned tables, we only
+	 * do it for inherited stats. (We're never called for not-inherited stats
+	 * on partitioned tables anyway.)
+	 *
+	 * Reset the changes_since_analyze counter only if we analyzed all
+	 * columns; otherwise, there is still work for auto-analyze to do.
+	 */
+	if (!inh || onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		pgstat_report_analyze(onerel, totalrows, totaldeadrows,
 							  (va_cols == NIL));
-	}
 
 	/*
 	 * If this isn't part of VACUUM ANALYZE, let index AMs do cleanup.
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a3c35bdf60..2a9673154b 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -1632,21 +1632,31 @@ pgstat_report_analyze(Relation rel,
 	 * be double-counted after commit.  (This approach also ensures that the
 	 * collector ends up with the right numbers if we abort instead of
 	 * committing.)
+	 *
+	 * For partitioned tables, we don't report live and dead tuples, because
+	 * such tables don't have any data.
 	 */
 	if (rel->pgstat_info != NULL)
 	{
 		PgStat_TableXactStatus *trans;
 
-		for (trans = rel->pgstat_info->trans; trans; trans = trans->upper)
+		if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+			/* If this rel is partitioned, skip modifying */
+			livetuples = deadtuples = 0;
+		else
 		{
-			livetuples -= trans->tuples_inserted - trans->tuples_deleted;
-			deadtuples -= trans->tuples_updated + trans->tuples_deleted;
+			for (trans = rel->pgstat_info->trans; trans; trans = trans->upper)
+			{
+				livetuples -= trans->tuples_inserted - trans->tuples_deleted;
+				deadtuples -= trans->tuples_updated + trans->tuples_deleted;
+			}
+			/* count stuff inserted by already-aborted subxacts, too */
+			deadtuples -= rel->pgstat_info->t_counts.t_delta_dead_tuples;
+			/* Since ANALYZE's counts are estimates, we could have underflowed */
+			livetuples = Max(livetuples, 0);
+			deadtuples = Max(deadtuples, 0);
 		}
-		/* count stuff inserted by already-aborted subxacts, too */
-		deadtuples -= rel->pgstat_info->t_counts.t_delta_dead_tuples;
-		/* Since ANALYZE's counts are estimates, we could have underflowed */
-		livetuples = Max(livetuples, 0);
-		deadtuples = Max(deadtuples, 0);
+
 	}
 
 	pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_ANALYZE);
@@ -1999,6 +2009,7 @@ pgstat_initstats(Relation rel)
 
 	/* We only count stats for things that have storage */
 	if (!RELKIND_HAS_STORAGE(relkind))
+// relkind != RELKIND_PARTITIONED_TABLE)
 	{
 		rel->pgstat_info = NULL;
 		return;
-- 
2.17.0


--/Uq4LBwYP4y1W6pO--