Fix relcache handling of the 'default' partition

My commit 4dba331cb3dc that moved around CommandCounterIncrement calls
in partitioning DDL code unearthed a problem with the relcache handling
for the 'default' partition: the construction of a correct relcache
entry for the partitioned table was at the mercy of lack of CCI calls in
non-trivial amounts of code.  This was prone to creating problems later
on, as the code develops.  This was visible as a test failure in a
compile with RELCACHE_FORCE_RELASE (buildfarm member prion).

The problem is that after the mentioned commit it was possible to create
a relcache entry that had incomplete information regarding the default
partition because I introduced a CCI between adding the catalog entries
for the default partition (StorePartitionBound) and the update of
pg_partitioned_table entry for its parent partitioned table
(update_default_partition_oid).  It seems the best fix is to move the
latter so that it occurs inside the former; the purposeful lack of
intervening CCI should be more obvious, and harder to break.

I also remove a check in RelationBuildPartitionDesc that returns NULL if
the key is not set.  I couldn't find any place that needs this hack
anymore; probably it was required because of bugs that have since been
fixed.

Fix a few typos I noticed while reviewing the code involved.

Discussion: https://postgr.es/m/20180320182659.nyzn3vqtjbbtfgwq@alvherre.pgsql

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ca2c2f9952066f228a6981b2669e8c66f1722555..5ed4654875eb31e6d91b00e0b186fb952f7b240c 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1773,7 +1773,8 @@ heap_drop_with_catalog(Oid relid)
     * could attempt to access the just-dropped relation as its partition. We
     * must therefore take a table lock strong enough to prevent all queries
     * on the table from proceeding until we commit and send out a
-    * shared-cache-inval notice that will make them update their index lists.
+    * shared-cache-inval notice that will make them update their partition
+    * descriptors.
     */
    tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
    if (!HeapTupleIsValid(tuple))
@@ -3251,6 +3252,9 @@ RemovePartitionKeyByRelId(Oid relid)
  *     Update pg_class tuple of rel to store the partition bound and set
  *     relispartition to true
  *
+ * If this is the default partition, also update the default partition OID in
+ * pg_partitioned_table.
+ *
  * Also, invalidate the parent's relcache, so that the next rebuild will load
  * the new partition's info into its partition descriptor.  If there is a
  * default partition, we must invalidate its relcache entry as well.
@@ -3302,7 +3306,15 @@ StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound)
    heap_freetuple(newtuple);
    heap_close(classRel, RowExclusiveLock);
 
-   /* Make update visible */
+   /*
+    * If we're storing bounds for the default partition, update
+    * pg_partitioned_table too.
+    */
+   if (bound->is_default)
+       update_default_partition_oid(RelationGetRelid(parent),
+                                    RelationGetRelid(rel));
+
+   /* Make these updates visible */
    CommandCounterIncrement();
 
    /*

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 786c05df7371fc87a628dd5345814cb691710779..53855f5088b10b27c690e0195f0cd3930633cc10 100644 (file)
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -227,13 +227,6 @@ RelationBuildPartitionDesc(Relation rel)
    /* Range partitioning specific */
    PartitionRangeBound **rbounds = NULL;
 
-   /*
-    * The following could happen in situations where rel has a pg_class entry
-    * but not the pg_partitioned_table entry yet.
-    */
-   if (key == NULL)
-       return;
-
    /* Get partition oids from pg_inherits */
    inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock);
 

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2ec99f99f935d9e77133e4e878c33636836a6aea..f5c744b9f5a41f20918f3f06863dc42c40e818d4 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -860,10 +860,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
        /* Update the pg_class entry. */
        StorePartitionBound(rel, parent, bound);
 
-       /* Update the default partition oid */
-       if (bound->is_default)
-           update_default_partition_oid(RelationGetRelid(parent), relationId);
-
        heap_close(parent, NoLock);
    }
 
@@ -14021,11 +14017,6 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
    /* OK to create inheritance.  Rest of the checks performed there */
    CreateInheritance(attachrel, rel);
 
-   /* Update the default partition oid */
-   if (cmd->bound->is_default)
-       update_default_partition_oid(RelationGetRelid(rel),
-                                    RelationGetRelid(attachrel));
-
    /*
     * Check that the new partition's bound is valid and does not overlap any
     * of existing partitions of the parent - note that it does not return on
@@ -14286,7 +14277,7 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 
    /*
     * We must lock the default partition, because detaching this partition
-    * will changing its partition constrant.
+    * will change its partition constraint.
     */
    defaultPartOid =
        get_default_oid_from_partdesc(RelationGetPartitionDesc(rel));
@@ -14329,19 +14320,17 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
    if (OidIsValid(defaultPartOid))
    {
        /*
-        * If the detach relation is the default partition itself, invalidate
-        * its entry in pg_partitioned_table.
+        * If the relation being detached is the default partition itself,
+        * remove it from the parent's pg_partitioned_table entry.
+        *
+        * If not, we must invalidate default partition's relcache entry, as
+        * in StorePartitionBound: its partition constraint depends on every
+        * other partition's partition constraint.
         */
        if (RelationGetRelid(partRel) == defaultPartOid)
            update_default_partition_oid(RelationGetRelid(rel), InvalidOid);
        else
-       {
-           /*
-            * We must invalidate default partition's relcache, for the same
-            * reasons explained in StorePartitionBound().
-            */
            CacheInvalidateRelcacheByRelid(defaultPartOid);
-       }
    }
 
    /* detach indexes too */