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.

diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index ed275d85aad96a89b077e687be017bc975eb5748..47290930ea0dc94a36f16dae3f36f8dfe0d42a6f 100644 (file)
--- 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 1d6301bac163ca131f55c67864da04b4a979e19f..a9ddb2c2807ee2cd4d7059961369961c0df015ed 100644 (file)
--- 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 4509cdab90045b37a136c15b20eccc759ea6c6b4..1e8ad2b671682ad21c372f845ad5454f5998ed84 100644 (file)
--- 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 0e8bbe0929f3542c09189fde00eee7d73fc3ce96..333e30326d5c35e81daa2ecdd93bb9292ba0eb87 100644 (file)
--- 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,