On Thu, Feb 20, 2020 at 5:32 PM Amit Langote <amitlangot...@gmail.com> wrote:
> On Thu, Feb 20, 2020 at 4:50 PM Amit Langote <amitlangot...@gmail.com> wrote:
> > * I may be missing something, but why doesn't do_autovacuum() fetch a
> > partitioned table's entry from pgstat instead of fetching that for
> > individual children and adding? That is, why do we need to do the
> > following:
> >
> > + /*
> > + * If the relation is a partitioned table, we check it
> > using reltuples
> > + * added up childrens' and changes_since_analyze tracked
> > by stats collector.
>
> Oh, it's only adding up children's pg_class.reltuple, not pgstat
> stats. We need to do that because a partitioned table's
> pg_class.reltuples is always 0 and correctly so. Sorry for not
> reading the patch properly.
Having read the relevant diffs again, I think this could be done
without duplicating code too much. You seem to have added the same
logic in two places: do_autovacuum() and table_recheck_autovac().
More importantly, part of the logic of relation_needs_vacanalyze() is
duplicated in both of the aforementioned places, which I think is
unnecessary and undesirable if you consider maintainability. I think
we could just add the logic to compute reltuples for partitioned
tables at the beginning of relation_needs_vacanalyze() and be done. I
have attached a delta patch to show what I mean. Please check and
tell what you think.
Thanks,
Amit
diff --git a/src/backend/postmaster/autovacuum.c
b/src/backend/postmaster/autovacuum.c
index 7d0a5ce30d..ca6996e448 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2089,103 +2089,19 @@ do_autovacuum(void)
continue;
}
- if (classForm->relkind == RELKIND_RELATION ||
- classForm->relkind == RELKIND_MATVIEW)
- {
- /* Fetch reloptions and the pgstat entry for this table
*/
- relopts = extract_autovac_opts(tuple, pg_class_desc);
- tabentry = get_pgstat_tabentry_relid(relid,
classForm->relisshared,
-
shared, dbentry);
-
- /* Check if it needs vacuum or analyze */
- relation_needs_vacanalyze(relid, relopts, classForm,
tabentry,
-
effective_multixact_freeze_max_age,
-
&dovacuum, &doanalyze, &wraparound);
-
- /* Relations that need work are added to table_oids */
- if (dovacuum || doanalyze)
- table_oids = lappend_oid(table_oids, relid);
- }
- else
- {
- /*
- * If the relation is a partitioned table, we check it
using reltuples
- * added up childrens' and changes_since_analyze
tracked by stats collector.
- * We check only auto analyze because partitioned
tables don't need to vacuum.
- */
- List *tableOIDs;
- ListCell *lc;
- bool av_enabled;
- int anl_base_thresh;
- float4 all_reltuples = 0,
- anl_scale_factor,
- anlthresh,
- reltuples,
- anltuples;
-
- /* Find all members of inheritance set taking
AccessShareLock */
- tableOIDs = find_all_inheritors(relid, AccessShareLock,
NULL);
-
- foreach(lc, tableOIDs)
- {
- Oid childOID = lfirst_oid(lc);
- HeapTuple childtuple;
- Form_pg_class childclassForm;
-
- /* Ignore the parent table */
- if (childOID == relid)
- continue;
-
- childtuple = SearchSysCacheCopy1(RELOID,
ObjectIdGetDatum(childOID));
- childclassForm = (Form_pg_class)
GETSTRUCT(childtuple);
-
- /* Skip foreign partitions */
- if (childclassForm->relkind ==
RELKIND_FOREIGN_TABLE)
- continue;
-
- /* Sum up the child's reltuples for its parent
table */
- all_reltuples += childclassForm->reltuples;
- elog(NOTICE, "[parent:%s] child%s has %.0f
tuples", NameStr(classForm->relname),NameStr(childclassForm->relname),
childclassForm->reltuples);
- }
-
-
- /* Fetch reloptions and the pgstat entry for the
partitioned table */
- relopts = extract_autovac_opts(tuple, pg_class_desc);
- tabentry = get_pgstat_tabentry_relid(relid,
-
classForm->relisshared,
-
shared, dbentry);
-
- /* Check if it needs auto analyze */
- av_enabled = (relopts ? relopts->enabled : true);
-
- if (av_enabled)
- {
- anl_scale_factor = (relopts &&
relopts->analyze_scale_factor >= 0)
- ? relopts->analyze_scale_factor
- : autovacuum_anl_scale;
-
- anl_base_thresh = (relopts &&
relopts->analyze_threshold >= 0)
- ? relopts->analyze_threshold
- : autovacuum_anl_thresh;
-
- elog(NOTICE, "[parent:%s] has %.0f tuples",
NameStr(classForm->relname), all_reltuples);
- if (PointerIsValid(tabentry))
- {
- reltuples = all_reltuples;
- anltuples =
tabentry->changes_since_analyze;
- anlthresh = (float4) anl_base_thresh +
anl_scale_factor * reltuples;
+ /* Fetch reloptions and the pgstat entry for this table */
+ relopts = extract_autovac_opts(tuple, pg_class_desc);
+ tabentry = get_pgstat_tabentry_relid(relid,
classForm->relisshared,
+
shared, dbentry);
- elog(DEBUG3, "%s: anl: %.0f (threshold
%.0f)",
- NameStr(classForm->relname),
- anltuples, anlthresh);
+ /* Check if it needs vacuum or analyze */
+ relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
+
effective_multixact_freeze_max_age,
+ &dovacuum,
&doanalyze, &wraparound);
- /* Determine if this table needs
analyze. */
- doanalyze = (anltuples > anlthresh);
- }
- if (doanalyze)
- table_oids = lappend_oid(table_oids,
relid);
- }
- }
+ /* Relations that need work are added to table_oids */
+ if (dovacuum || doanalyze)
+ table_oids = lappend_oid(table_oids, relid);
/*
* Remember TOAST associations for the second pass. Note: we
must do
@@ -2880,105 +2796,33 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
return NULL;
classForm = (Form_pg_class) GETSTRUCT(classTup);
- if (classForm->relkind != RELKIND_PARTITIONED_TABLE)
+ /*
+ * Get the applicable reloptions. If it is a TOAST table, try to get
the
+ * main table reloptions if the toast table itself doesn't have.
+ */
+ avopts = extract_autovac_opts(classTup, pg_class_desc);
+ if (classForm->relkind == RELKIND_TOASTVALUE &&
+ avopts == NULL && table_toast_map != NULL)
{
- /*
- * Get the applicable reloptions. If it is a TOAST table, try
to get the
- * main table reloptions if the toast table itself doesn't have.
- */
- avopts = extract_autovac_opts(classTup, pg_class_desc);
- if (classForm->relkind == RELKIND_TOASTVALUE &&
- avopts == NULL && table_toast_map != NULL)
- {
- av_relation *hentry;
- bool found;
-
- hentry = hash_search(table_toast_map, &relid,
HASH_FIND, &found);
- if (found && hentry->ar_hasrelopts)
- avopts = &hentry->ar_reloptions;
- }
-
- /* fetch the pgstat table entry */
- tabentry = get_pgstat_tabentry_relid(relid,
classForm->relisshared,
-
shared, dbentry);
-
- relation_needs_vacanalyze(relid, avopts, classForm, tabentry,
-
effective_multixact_freeze_max_age,
- &dovacuum,
&doanalyze, &wraparound);
+ av_relation *hentry;
+ bool found;
- /* ignore ANALYZE for toast tables */
- if (classForm->relkind == RELKIND_TOASTVALUE)
- doanalyze = false;
+ hentry = hash_search(table_toast_map, &relid, HASH_FIND,
&found);
+ if (found && hentry->ar_hasrelopts)
+ avopts = &hentry->ar_reloptions;
}
- else
- {
- List *tableOIDs;
- ListCell *lc;
- bool av_enabled;
- int anl_base_thresh;
- float4 all_reltuples,
- anl_scale_factor,
- anlthresh,
- reltuples,
- anltuples;
- /* Find all members of inheritance set taking AccessShareLock */
- tableOIDs = find_all_inheritors(relid, AccessShareLock, NULL);
+ /* fetch the pgstat table entry */
+ tabentry = get_pgstat_tabentry_relid(relid, classForm->relisshared,
+
shared, dbentry);
- foreach(lc, tableOIDs)
- {
- Oid childOID = lfirst_oid(lc);
- HeapTuple childtuple;
- Form_pg_class childclassForm;
+ relation_needs_vacanalyze(relid, avopts, classForm, tabentry,
+
effective_multixact_freeze_max_age,
+ &dovacuum,
&doanalyze, &wraparound);
- /* Ignore the parent table */
- if (childOID == relid)
- continue;
-
- childtuple = SearchSysCacheCopy1(RELOID,
ObjectIdGetDatum(childOID));
- childclassForm = (Form_pg_class) GETSTRUCT(childtuple);
-
- /* Skip foreign partitions */
- if (childclassForm->relkind == RELKIND_FOREIGN_TABLE)
- continue;
-
- /* Sum up the child's reltuples for the partitioned
table */
- all_reltuples += childclassForm->reltuples;
- }
-
- /* Fetch reloptions and the pgstat entry for the partitioned
table */
- avopts = extract_autovac_opts(classTup, pg_class_desc);
- tabentry = get_pgstat_tabentry_relid(relid,
-
classForm->relisshared,
-
shared, dbentry);
-
- /* Check if it needs auto analyze */
- av_enabled = (avopts ? avopts->enabled : true);
-
- if (av_enabled)
- {
- anl_scale_factor = (avopts &&
avopts->analyze_scale_factor >= 0)
- ? avopts->analyze_scale_factor
- : autovacuum_anl_scale;
-
- anl_base_thresh = (avopts && avopts->analyze_threshold
>= 0)
- ? avopts->analyze_threshold
- : autovacuum_anl_thresh;
-
- if (PointerIsValid(tabentry))
- {
- reltuples = all_reltuples;
- anltuples = tabentry->changes_since_analyze;
- anlthresh = (float4) anl_base_thresh +
anl_scale_factor * reltuples;
-
- elog(DEBUG3, "%s: anl: %.0f (threshold %.0f)",
- NameStr(classForm->relname),
- anltuples, anlthresh);
-
- doanalyze = (anltuples > anlthresh);
- }
- }
- }
+ /* ignore ANALYZE for toast tables */
+ if (classForm->relkind == RELKIND_TOASTVALUE)
+ doanalyze = false;
/* OK, it needs something done */
if (doanalyze || dovacuum)
@@ -3148,6 +2992,42 @@ relation_needs_vacanalyze(Oid relid,
AssertArg(classForm != NULL);
AssertArg(OidIsValid(relid));
+ /*
+ * 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;
+
+ childtuple = SearchSysCacheCopy1(RELOID,
ObjectIdGetDatum(childOID));
+ childclass = (Form_pg_class) GETSTRUCT(childtuple);
+
+ /* Skip foreign partitions */
+ if (childclass->relkind == RELKIND_FOREIGN_TABLE)
+ continue;
+
+ /* Sum up the child's reltuples for its parent table */
+ reltuples += childclass->reltuples;
+ }
+ }
+
/*
* Determine vacuum/analyze equation parameters. We have two possible
* sources: the passed reloptions (which could be a main table or a
toast
