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 */