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
fbcf4b92), 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

@@ -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.

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;
 	}
 

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));

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,