From 5b562644fec696977df4a82790064e8287927891 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 2 Sep 2011 14:29:31 -0400 Subject: [PATCH] Teach ANALYZE to clear pg_class.relhassubclass when appropriate. In the past, relhassubclass always remained true if a relation had ever had child relations, even if the last subclass was long gone. While this had only marginal performance implications in most cases, it was annoying, and I'm now considering some planner changes that would raise the cost of a false positive. It was previously impractical to fix this because of race condition concerns. However, given the recent change that made tablecmds.c take ShareExclusiveLock on relations that are gaining a child (commit fbcf4b92aa64d4577bcf25925b055316b978744a), we can now allow ANALYZE to clear the flag when it's no longer relevant. There is no additional locking cost to do so, since ANALYZE takes ShareExclusiveLock anyway. --- src/backend/catalog/pg_inherits.c | 7 ++++--- src/backend/commands/analyze.c | 12 +++++++----- src/backend/commands/tablecmds.c | 28 +++++++++++++++++++--------- src/include/commands/tablecmds.h | 2 ++ 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c index ed275d85aa..47290930ea 100644 --- a/src/backend/catalog/pg_inherits.c +++ b/src/backend/catalog/pg_inherits.c @@ -228,11 +228,12 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents) * In the current implementation, has_subclass returns whether a * particular class *might* have a subclass. It will not return the * correct result if a class had a subclass which was later dropped. - * This is because relhassubclass in pg_class is not updated when a - * subclass is dropped, primarily because of concurrency concerns. + * This is because relhassubclass in pg_class is not updated immediately + * when a subclass is dropped, primarily because of concurrency concerns. * * Currently has_subclass is only used as an efficiency hack to skip - * unnecessary inheritance searches, so this is OK. + * unnecessary inheritance searches, so this is OK. Note that ANALYZE + * on a childless table will clean up the obsolete relhassubclass flag. * * Although this doesn't actually touch pg_inherits, it seems reasonable * to keep it here since it's normally used with the other routines here. diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 1d6301bac1..a9ddb2c280 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -26,6 +26,7 @@ #include "catalog/pg_inherits_fn.h" #include "catalog/pg_namespace.h" #include "commands/dbcommands.h" +#include "commands/tablecmds.h" #include "commands/vacuum.h" #include "executor/executor.h" #include "miscadmin.h" @@ -1408,14 +1409,15 @@ acquire_inherited_sample_rows(Relation onerel, HeapTuple *rows, int targrows, /* * Check that there's at least one descendant, else fail. This could * happen despite analyze_rel's relhassubclass check, if table once had a - * child but no longer does. + * child but no longer does. In that case, we can clear the + * relhassubclass field so as not to make the same mistake again later. + * (This is safe because we hold ShareUpdateExclusiveLock.) */ if (list_length(tableOIDs) < 2) { - /* - * XXX It would be desirable to clear relhassubclass here, but we - * don't have adequate lock to do that safely. - */ + /* CCI because we already updated the pg_class row in this command */ + CommandCounterIncrement(); + SetRelationHasSubclass(RelationGetRelid(onerel), false); return 0; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4509cdab90..1e8ad2b671 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -253,7 +253,6 @@ static void StoreCatalogInheritance(Oid relationId, List *supers); static void StoreCatalogInheritance1(Oid relationId, Oid parentOid, int16 seqNumber, Relation inhRelation); static int findAttrByName(const char *attributeName, List *schema); -static void setRelhassubclassInRelation(Oid relationId, bool relhassubclass); static void AlterIndexNamespaces(Relation classRel, Relation rel, Oid oldNspOid, Oid newNspOid); static void AlterSeqNamespaces(Relation classRel, Relation rel, @@ -1359,7 +1358,10 @@ MergeAttributes(List *schema, List *supers, char relpersistence, * add children to the same parent simultaneously, and that parent has * no pre-existing children, then both will attempt to update the * parent's relhassubclass field, leading to a "tuple concurrently - * updated" error. + * updated" error. Also, this interlocks against a concurrent ANALYZE + * on the parent table, which might otherwise be attempting to clear + * the parent's relhassubclass field, if its previous children were + * recently dropped. */ relation = heap_openrv(parent, ShareUpdateExclusiveLock); @@ -1958,7 +1960,7 @@ StoreCatalogInheritance1(Oid relationId, Oid parentOid, /* * Mark the parent as having subclasses. */ - setRelhassubclassInRelation(parentOid, true); + SetRelationHasSubclass(parentOid, true); } /* @@ -1985,11 +1987,22 @@ findAttrByName(const char *attributeName, List *schema) return 0; } + /* - * Update a relation's pg_class.relhassubclass entry to the given value + * SetRelationHasSubclass + * Set the value of the relation's relhassubclass field in pg_class. + * + * NOTE: caller must be holding an appropriate lock on the relation. + * ShareUpdateExclusiveLock is sufficient. + * + * NOTE: an important side-effect of this operation is that an SI invalidation + * message is sent out to all backends --- including me --- causing plans + * referencing the relation to be rebuilt with the new list of children. + * This must happen even if we find that no change is needed in the pg_class + * row. */ -static void -setRelhassubclassInRelation(Oid relationId, bool relhassubclass) +void +SetRelationHasSubclass(Oid relationId, bool relhassubclass) { Relation relationRelation; HeapTuple tuple; @@ -1997,9 +2010,6 @@ setRelhassubclassInRelation(Oid relationId, bool relhassubclass) /* * Fetch a modifiable copy of the tuple, modify it, update pg_class. - * - * If the tuple already has the right relhassubclass setting, we don't - * need to update it, but we still need to issue an SI inval message. */ relationRelation = heap_open(RelationRelationId, RowExclusiveLock); tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relationId)); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 0e8bbe0929..333e30326d 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -43,6 +43,8 @@ extern void CheckTableNotInUse(Relation rel, const char *stmt); extern void ExecuteTruncate(TruncateStmt *stmt); +extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass); + extern void renameatt(Oid myrelid, RenameStmt *stmt); extern void RenameRelation(Oid myrelid, -- GitLab