Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Fix CommandCounterIncrement in partition-related DDL > > Hmm, Prion seems unhappy about this. Looking
Here's a patch that seems to fix the problem, and generally looks sane to me. The previous arrangements to avoid CCI seem brittle: apparently we were forced to do StorePartitionBounds() and immediately update_default_partition_oid() without CCI in between, or various things went nuts ("unexpected partdefid"). With this patch, what we do is we call update_default_partition_oid *within* StorePartitionBounds without an intervening CCI, which seems to achieve the same result -- namely that when the relcache entry is rebuilt, it doesn't end up incomplete. The two places that previously called update_default_partition_oid with a non-zero default partition OID no longer call it, since they were storing bounds. The only places that remain outside of StorePartitionBounds call it with InvalidOid (during relation drop, and during partition detach). I also remove a crock in RelationBuildPartitionDesc to return empty when "key is null" (i.e. pg_class entry was there but not pg_partitioned_table), which makes no sense -- except if you're building the cache untimely, which apparently is what was happening. If you make sure the pg_partitioned_table entry is present before making the pg_class entry visible, then this should not be necessary. heap_drop_with_catalog contains a copy-paste failure, fixed also in this patch. I wonder about adding a syscache callback so that when an item in pg_partitioned_table is invalidated, the relcache entry for partrelid entry in pg_class is invalidated also. I can't find any precedent for anything similar, though, and there doesn't seem to be any convenient place to do it, either. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 0497332e9d..a049234390 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -1769,7 +1769,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)) @@ -3247,6 +3248,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. @@ -3298,7 +3302,17 @@ 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. This must happen before CCI, to avoid risk + * of constructing a partition descriptor lacking the default partition + * OID. + */ + 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 8f83aa4675..c4cf0abadc 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -859,10 +859,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); } @@ -14020,11 +14016,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 -- 2.11.0