Date: Thu, 8 Apr 2021 02:04:08 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: yuzuko <yuzukohosoya@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	David Steele <david@pgmasters.net>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	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 (autoanalyze)
Message-ID: <20210408060408.GA31695@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="17pEHd4RhPHOinZp"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210408032235.GA6842@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210408060408.GA31695%40alvherre.pgsql>
Precedence: bulk


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

On 2021-Apr-07, Alvaro Herrera wrote:

> However, I just noticed there is a huge problem, which is that the new
> code in relation_needs_vacanalyze() is doing find_all_inheritors(), and
> we don't necessarily have a snapshot that lets us do that.  While adding
> a snapshot acquisition at that spot is a very easy fix, I hesitate to
> fix it that way, because the whole idea there seems quite wasteful: we
> have to look up, open and lock every single partition, on every single
> autovacuum iteration through the database.  That seems bad.  I'm
> inclined to think that a better idea may be to store reltuples for the
> partitioned table in pg_class.reltuples, instead of having to add up the
> reltuples of each partition.  I haven't checked if this is likely to
> break anything.

I forgot to comment on this aspect.  First, I was obviously mistaken
about there not being an active snapshot.  I mean, it's correct that
there isn't.  The issue is that it's really a bug to require that there
is one; it just hasn't failed before because partially detached
partitions aren't very common.  So I patched that as a bug in a
preliminary patch.

Next, the idea of storing the number of tuples in pg_class.reltuples is
a nice one, and I think we should consider it in the long run.  However,
while it can be done as a quick job (shown in the attached, which AFAICT
works fine) there are side-effects -- for example, TRUNCATE doesn't
clear the value, which is surely wrong.  I suspect that if I try to
handle it in this way, it would blow up in some corner case I forgot to
consider.  So, I decided not to go that way, at least for now.

-- 
Álvaro Herrera       Valdivia, Chile

--17pEHd4RhPHOinZp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="partitioned-reltuples.patch"

commit 5ddb7c00e5f1d63eb251d334afce738919a772c0
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Wed Apr 7 23:52:33 2021 -0400
CommitDate: Wed Apr 7 23:52:33 2021 -0400

    set pg_class.reltuples for partitioned tables

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 0789117bb8..36e35722bc 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -656,6 +656,17 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 								in_outer_xact);
 		}
 	}
+	else if (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		/*
+		 * Partitioned tables don't have storage, so we don't set any of these
+		 * value in their pg_class entries.  However, reltuples is necessary
+		 * in order for auto-analyze to work properly, so update that.
+		 */
+		vac_update_relstats(onerel, 0, totalrows,
+							0, false, InvalidTransactionId, InvalidMultiXactId,
+							in_outer_xact);
+	}
 
 	/*
 	 * Now report ANALYZE to the stats collector.  For regular tables, we do
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 5517836be6..48c1bf048f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3208,44 +3208,7 @@ relation_needs_vacanalyze(Oid relid,
 	 */
 	if (PointerIsValid(tabentry) && AutoVacuumingActive())
 	{
-		if (classForm->relkind != RELKIND_PARTITIONED_TABLE)
-		{
-			reltuples = classForm->reltuples;
-		}
-		else
-		{
-			/*
-			 * If the relation is a partitioned table, we must add up
-			 * children's reltuples.
-			 */
-			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;
-
-				childtuple = SearchSysCache1(RELOID, ObjectIdGetDatum(childOID));
-				childclass = (Form_pg_class) GETSTRUCT(childtuple);
-
-				/* Skip a partitioned table and foreign partitions */
-				if (RELKIND_HAS_STORAGE(childclass->relkind))
-				{
-					/* Sum up the child's reltuples for its parent table */
-					reltuples += childclass->reltuples;
-				}
-				ReleaseSysCache(childtuple);
-			}
-
-			list_free(children);
-		}
+		reltuples = classForm->reltuples;
 		vactuples = tabentry->n_dead_tuples;
 		instuples = tabentry->inserts_since_vacuum;
 		anltuples = tabentry->changes_since_analyze;

--17pEHd4RhPHOinZp--