From 56163004b8b2151db279744b77138d4d90e2d5cb Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 21 Mar 2018 12:03:35 -0300 Subject: [PATCH] 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 --- src/backend/catalog/heap.c | 16 ++++++++++++++-- src/backend/catalog/partition.c | 7 ------- src/backend/commands/tablecmds.c | 25 +++++++------------------ 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ca2c2f9952..5ed4654875 100644 --- 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 786c05df73..53855f5088 100644 --- 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 2ec99f99f9..f5c744b9f5 100644 --- 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 */