Re: Autovacuum on partitioned table

On Fri, Feb 28, 2020 at 11:25 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> >       /*
> > +      * 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?)

+1, no need to skip partitioned tables here a their reltuples is always 0.

> > +     /*
> > +      * If the table is a leaf partition, tell the stats collector its parent's
> > +      * changes_since_analyze for auto analyze

Maybe write:

For a leaf partition, add its current changes_since_analyze into its
ancestors' counts.  This must be done before sending the ANALYZE
message as it resets the partition's changes_since_analyze counter.

> > +      */
> > +     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?

+1.  If there is a reason, it should at least be documented in the
comment above.

> > +     {
> > +             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.

Maybe fetch all ancestors here and process from the top.  But as we'd
have locked the leaf partition long before we got here, maybe we
should lock ancestors even before we start analyzing the leaf
partition? AccessShareLock should be enough on the ancestors because
we're not actually analyzing them.

(It appears get_partition_ancestors() returns a list where the root
parent is the last element, so need to be careful with that.)

Thanks,
Amit