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