diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 8f242ae..a143998 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -812,7 +812,7 @@ DefineIndex(Oid relationId, */ if (partitioned && (stmt->unique || stmt->primary)) { - PartitionKey key = rel->rd_partkey; + PartitionKey key = RelationGetPartitionKey(rel); int i; /* diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c index d160d6f..a010b3e 100644 --- a/src/backend/partitioning/partbounds.c +++ b/src/backend/partitioning/partbounds.c @@ -775,6 +775,9 @@ partition_bounds_equal(int partnatts, int16 *parttyplen, bool *parttypbyval, /* * Return a copy of given PartitionBoundInfo structure. The data types of bounds * are described by given partition key specification. + * + * Note: it's critical that this function and its callees not do any + * catalog access, because we use it to copy data into the relcache. */ PartitionBoundInfo partition_bounds_copy(PartitionBoundInfo src, diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c index 6ede084..d0e253d 100644 --- a/src/backend/partitioning/partdesc.c +++ b/src/backend/partitioning/partdesc.c @@ -65,10 +65,12 @@ RelationBuildPartitionDesc(Relation rel) List *inhoids; PartitionBoundSpec **boundspecs = NULL; Oid *oids = NULL; + bool *is_leaf = NULL; ListCell *cell; int i, nparts; PartitionKey key = RelationGetPartitionKey(rel); + MemoryContext new_pdcxt; MemoryContext oldcxt; int *mapping; @@ -81,10 +83,11 @@ RelationBuildPartitionDesc(Relation rel) inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock); nparts = list_length(inhoids); - /* Allocate arrays for OIDs and boundspecs. */ + /* Allocate arrays for OIDs, leaf flags, and boundspecs. */ if (nparts > 0) { - oids = palloc(nparts * sizeof(Oid)); + oids = (Oid *) palloc(nparts * sizeof(Oid)); + is_leaf = (bool *) palloc(nparts * sizeof(bool)); boundspecs = palloc(nparts * sizeof(PartitionBoundSpec *)); } @@ -172,63 +175,75 @@ RelationBuildPartitionDesc(Relation rel) /* Save results. */ oids[i] = inhrelid; + is_leaf[i] = (get_rel_relkind(inhrelid) != RELKIND_PARTITIONED_TABLE); boundspecs[i] = boundspec; ++i; } - /* Assert we aren't about to leak any old data structure */ - Assert(rel->rd_pdcxt == NULL); - Assert(rel->rd_partdesc == NULL); + /* + * Create PartitionBoundInfo, working in the caller's context. This could + * fail, but we haven't done any damage if so. + */ + if (nparts > 0) + boundinfo = partition_bounds_create(boundspecs, nparts, key, &mapping); /* * Now build the actual relcache partition descriptor. Note that the * order of operations here is fairly critical. If we fail partway - * through this code, we won't have leaked memory because the rd_pdcxt is - * attached to the relcache entry immediately, so it'll be freed whenever - * the entry is rebuilt or destroyed. However, we don't assign to - * rd_partdesc until the cached data structure is fully complete and - * valid, so that no other code might try to use it. + * through the data-copying code, we won't have leaked memory because the + * rd_pdcxt is attached to the relcache entry immediately, so it'll be + * freed whenever the entry is rebuilt or destroyed. However, we don't + * assign to rd_partdesc until the cached data structure is fully complete + * and valid, so that no other code might try to use it. */ - rel->rd_pdcxt = AllocSetContextCreate(CacheMemoryContext, - "partition descriptor", - ALLOCSET_SMALL_SIZES); - MemoryContextCopyAndSetIdentifier(rel->rd_pdcxt, + new_pdcxt = AllocSetContextCreate(CacheMemoryContext, + "partition descriptor", + ALLOCSET_SMALL_SIZES); + MemoryContextCopyAndSetIdentifier(new_pdcxt, RelationGetRelationName(rel)); + /* + * This is a hack and a half: if there's an old rd_pdcxt, it might contain + * an old partition descriptor that is still being referenced somewhere. + * Preserve it, while not leaking it, by reattaching it as a child context + * of the new rd_pdcxt. Eventually somebody will clean it up. See + * related hack in RelationClearRelation. + */ + rel->rd_partdesc = NULL; /* let's just make sure of this */ + if (rel->rd_pdcxt != NULL) + MemoryContextSetParent(rel->rd_pdcxt, new_pdcxt); + rel->rd_pdcxt = new_pdcxt; + + /* + * Now copy everything into new_pdcxt. Note it's critical that none of + * this code do any catalog access, else we could end up recursing here. + */ partdesc = (PartitionDescData *) - MemoryContextAllocZero(rel->rd_pdcxt, sizeof(PartitionDescData)); + MemoryContextAllocZero(new_pdcxt, sizeof(PartitionDescData)); partdesc->nparts = nparts; /* If there are no partitions, the rest of the partdesc can stay zero */ if (nparts > 0) { - /* Create PartitionBoundInfo, using the caller's context. */ - boundinfo = partition_bounds_create(boundspecs, nparts, key, &mapping); - - /* Now copy all info into relcache's partdesc. */ - oldcxt = MemoryContextSwitchTo(rel->rd_pdcxt); + oldcxt = MemoryContextSwitchTo(new_pdcxt); partdesc->boundinfo = partition_bounds_copy(boundinfo, key); partdesc->oids = (Oid *) palloc(nparts * sizeof(Oid)); partdesc->is_leaf = (bool *) palloc(nparts * sizeof(bool)); - MemoryContextSwitchTo(oldcxt); /* * Assign OIDs from the original array into mapped indexes of the * result array. The order of OIDs in the former is defined by the * catalog scan that retrieved them, whereas that in the latter is * defined by canonicalized representation of the partition bounds. - * - * Also record leaf-ness of each partition. For this we use - * get_rel_relkind() which may leak memory, so be sure to run it in - * the caller's context. + * Also save leaf-ness of each partition. */ for (i = 0; i < nparts; i++) { int index = mapping[i]; partdesc->oids[index] = oids[i]; - partdesc->is_leaf[index] = - (get_rel_relkind(oids[i]) != RELKIND_PARTITIONED_TABLE); + partdesc->is_leaf[index] = is_leaf[i]; } + MemoryContextSwitchTo(oldcxt); } rel->rd_partdesc = partdesc; diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 50f8912..0a693fef 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -1165,20 +1165,11 @@ RelationBuildDesc(Oid targetRelId, bool insertIt) relation->rd_fkeylist = NIL; relation->rd_fkeyvalid = false; - /* if a partitioned table, initialize key and partition descriptor info */ - if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - { - RelationBuildPartitionKey(relation); - RelationBuildPartitionDesc(relation); - } - else - { - relation->rd_partkey = NULL; - relation->rd_partkeycxt = NULL; - relation->rd_partdesc = NULL; - relation->rd_pdcxt = NULL; - } - /* ... but partcheck is not loaded till asked for */ + /* partitioning data is not loaded till asked for */ + relation->rd_partkey = NULL; + relation->rd_partkeycxt = NULL; + relation->rd_partdesc = NULL; + relation->rd_pdcxt = NULL; relation->rd_partcheck = NIL; relation->rd_partcheckvalid = false; relation->rd_partcheckcxt = NULL; @@ -2527,7 +2518,6 @@ RelationClearRelation(Relation relation, bool rebuild) bool keep_rules; bool keep_policies; bool keep_partkey; - bool keep_partdesc; /* Build temporary entry, but don't link it into hashtable */ newrel = RelationBuildDesc(save_relid, false); @@ -2560,9 +2550,6 @@ RelationClearRelation(Relation relation, bool rebuild) keep_policies = equalRSDesc(relation->rd_rsdesc, newrel->rd_rsdesc); /* partkey is immutable once set up, so we can always keep it */ keep_partkey = (relation->rd_partkey != NULL); - keep_partdesc = equalPartitionDescs(relation->rd_partkey, - relation->rd_partdesc, - newrel->rd_partdesc); /* * Perform swapping of the relcache entry contents. Within this @@ -2617,34 +2604,46 @@ RelationClearRelation(Relation relation, bool rebuild) SWAPFIELD(Oid, rd_toastoid); /* pgstat_info must be preserved */ SWAPFIELD(struct PgStat_TableStatus *, pgstat_info); - /* preserve old partitioning info if no logical change */ + /* preserve old partition key if we have one */ if (keep_partkey) { SWAPFIELD(PartitionKey, rd_partkey); SWAPFIELD(MemoryContext, rd_partkeycxt); } - if (keep_partdesc) - { - SWAPFIELD(PartitionDesc, rd_partdesc); - SWAPFIELD(MemoryContext, rd_pdcxt); - } - else if (rebuild && newrel->rd_pdcxt != NULL) + if (rebuild && newrel->rd_pdcxt != NULL) { /* * We are rebuilding a partitioned relation with a non-zero * reference count, so keep the old partition descriptor around, * in case there's a PartitionDirectory with a pointer to it. - * Attach it to the new rd_pdcxt so that it gets cleaned up - * eventually. In the case where the reference count is 0, this - * code is not reached, which should be OK because in that case - * there should be no PartitionDirectory with a pointer to the old - * entry. + * (This is necessary because RelationGetPartitionDesc hands out + * direct pointers to the relcache's data structure, unlike our + * usual practice which is to hand out copies. We'd have the same + * problem with rd_partkey, except that we always preserve that + * once created.) + * + * To ensure that it's not leaked completely, re-attach it to the + * new reldesc, or make it a child of the new reldesc's rd_pdcxt + * in the unlikely event that there is one already. (See hack in + * RelationBuildPartitionDesc.) Currently, it'll stick around + * until such time as we destroy the reldesc, or rebuild it while + * it has zero refcount. That's pretty undesirable and should get + * fixed. + * + * In the case where the reference count is 0, this code is not + * reached, which should be OK because in that case there should + * be no PartitionDirectory with a pointer to the old entry. * * Note that newrel and relation have already been swapped, so the * "old" partition descriptor is actually the one hanging off of * newrel. */ - MemoryContextSetParent(newrel->rd_pdcxt, relation->rd_pdcxt); + if (relation->rd_pdcxt != NULL) /* probably never happens */ + MemoryContextSetParent(newrel->rd_pdcxt, relation->rd_pdcxt); + else + relation->rd_pdcxt = newrel->rd_pdcxt; + relation->rd_partdesc = NULL; /* just to be sure */ + /* drop newrel's pointers so we don't destroy it below */ newrel->rd_partdesc = NULL; newrel->rd_pdcxt = NULL; } @@ -3907,27 +3906,7 @@ RelationCacheInitializePhase3(void) restart = true; } - /* - * Reload the partition key and descriptor for a partitioned table. - */ - if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && - relation->rd_partkey == NULL) - { - RelationBuildPartitionKey(relation); - Assert(relation->rd_partkey != NULL); - - restart = true; - } - - if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && - relation->rd_partdesc == NULL) - { - RelationBuildPartitionDesc(relation); - Assert(relation->rd_partdesc != NULL); - - restart = true; - } - + /* Reload tableam data if needed */ if (relation->rd_tableam == NULL && (relation->rd_rel->relkind == RELKIND_RELATION || relation->rd_rel->relkind == RELKIND_SEQUENCE || @@ -5133,6 +5112,36 @@ RelationGetExclusionInfo(Relation indexRelation, } /* + * RelationGetPartitionKey -- get partition key, if relation is partitioned + */ +struct PartitionKeyData * +RelationGetPartitionKey(Relation rel) +{ + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + return NULL; + + if (rel->rd_partkey == NULL) + RelationBuildPartitionKey(rel); + + return rel->rd_partkey; +} + +/* + * RelationGetPartitionDesc -- get partition descriptor, if relation is partitioned + */ +struct PartitionDescData * +RelationGetPartitionDesc(Relation rel) +{ + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + return NULL; + + if (rel->rd_partdesc == NULL) + RelationBuildPartitionDesc(rel); + + return rel->rd_partdesc; +} + +/* * Get publication actions for the given relation. */ struct PublicationActions * diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 31d8a1a..aa8c625 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -596,20 +596,10 @@ typedef struct ViewOptions RelationNeedsWAL(relation) && \ !IsCatalogRelation(relation)) -/* - * RelationGetPartitionKey - * Returns the PartitionKey of a relation - */ -#define RelationGetPartitionKey(relation) ((relation)->rd_partkey) - -/* - * RelationGetPartitionDesc - * Returns partition descriptor for a relation. - */ -#define RelationGetPartitionDesc(relation) ((relation)->rd_partdesc) - /* routines in utils/cache/relcache.c */ extern void RelationIncrementReferenceCount(Relation rel); extern void RelationDecrementReferenceCount(Relation rel); +extern struct PartitionKeyData *RelationGetPartitionKey(Relation rel); +extern struct PartitionDescData *RelationGetPartitionDesc(Relation rel); #endif /* REL_H */ diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index f630168..9d6d1ba 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -501,6 +501,22 @@ Partition of: partitioned2 FOR VALUES FROM ('-1', 'aaaaa') TO (100, 'ccccc') Partition constraint: (((a + 1) IS NOT NULL) AND (substr(b, 1, 5) IS NOT NULL) AND (((a + 1) > '-1'::integer) OR (((a + 1) = '-1'::integer) AND (substr(b, 1, 5) >= 'aaaaa'::text))) AND (((a + 1) < 100) OR (((a + 1) = 100) AND (substr(b, 1, 5) < 'ccccc'::text)))) DROP TABLE partitioned, partitioned2; +-- check reference to partitioned table's rowtype in partition descriptor +create table partitioned (a int, b int) + partition by list ((row(a, b)::partitioned)); +create table partitioned1 + partition of partitioned for values in ('(1,2)'::partitioned); +create table partitioned2 + partition of partitioned for values in ('(2,4)'::partitioned); +explain (costs off) +select * from partitioned where row(a,b)::partitioned = '(1,2)'::partitioned; + QUERY PLAN +----------------------------------------------------------- + Seq Scan on partitioned1 partitioned + Filter: (ROW(a, b)::partitioned = '(1,2)'::partitioned) +(2 rows) + +drop table partitioned; -- check that dependencies of partition columns are handled correctly create domain intdom1 as int; create table partitioned ( diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index e835b65..7ef646a 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -449,6 +449,17 @@ CREATE TABLE part2_1 PARTITION OF partitioned2 FOR VALUES FROM (-1, 'aaaaa') TO DROP TABLE partitioned, partitioned2; +-- check reference to partitioned table's rowtype in partition descriptor +create table partitioned (a int, b int) + partition by list ((row(a, b)::partitioned)); +create table partitioned1 + partition of partitioned for values in ('(1,2)'::partitioned); +create table partitioned2 + partition of partitioned for values in ('(2,4)'::partitioned); +explain (costs off) +select * from partitioned where row(a,b)::partitioned = '(1,2)'::partitioned; +drop table partitioned; + -- check that dependencies of partition columns are handled correctly create domain intdom1 as int;