Re: Autovacuum on partitioned table

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