From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: yuzuko <yuzukohosoya@gmail.com>
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
Date: Thu, 27 Feb 2020 23:25:45 -0300
Message-ID: <20200228022545.GA19686@alvherre.pgsql> (raw)
In-Reply-To: <CAKkQ509Gn8bOEZNmcudBYj=g97qG4PcNC2pbGxC4Ci_pPtQrEQ@mail.gmail.com>
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
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@2ndquadrant.com, yuzukohosoya@gmail.com, 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
In-Reply-To: <20200228022545.GA19686@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