Re: Autovacuum on partitioned table - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: Autovacuum on partitioned table |
Date | |
Msg-id | CA+HiwqFrLkaYW8G8hXz6XeMYUi-NHzbQ=Dn8CwEJuH+ahwbRZA@mail.gmail.com Whole thread Raw |
In response to | Re: Autovacuum on partitioned table (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Responses |
Re: Autovacuum on partitioned table
|
List | pgsql-hackers |
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
pgsql-hackers by date: