From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: yuzuko <yuzukohosoya@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: David Steele <david@pgmasters.net>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Greg Stark <stark@mit.edu>
Subject: Re: Autovacuum on partitioned table (autoanalyze)
Date: Thu, 8 Apr 2021 02:04:08 -0400
Message-ID: <20210408060408.GA31695@alvherre.pgsql> (raw)
In-Reply-To: <20210408032235.GA6842@alvherre.pgsql>
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
Attachments:
[text/x-diff] partitioned-reltuples.patch (2.5K, ../20210408060408.GA31695@alvherre.pgsql/2-partitioned-reltuples.patch)
download | inline diff:
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.cindex 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.cindex 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;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, yuzukohosoya@gmail.com, tomas.vondra@enterprisedb.com, david@pgmasters.net, horikyota.ntt@gmail.com, pryzby@telsasoft.com, daniel@yesql.se, amitlangote09@gmail.com, masahiko.sawada@2ndquadrant.com, laurenz.albe@cybertec.at, pgsql-hackers@lists.postgresql.org, stark@mit.edu
Subject: Re: Autovacuum on partitioned table (autoanalyze)
In-Reply-To: <20210408060408.GA31695@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox