From 9674053fd1e57a480d8a42585cb10421e2c76a70 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 2 Aug 2017 17:14:59 +0900 Subject: [PATCH 1/3] Add get_all_partition_oids and get_partition_oids Their respective counterparts find_all_inheritors() and find_inheritance_children() read the pg_inherits catalog directly and frame the result list in some order determined by the order of OIDs. get_all_partition_oids() and get_partition_oids() form their result by reading the partition OIDs from the PartitionDesc contained in the relcache. Hence, the order of OIDs in the resulting list is based on that of the partition bounds. In the case of get_all_partition_oids which traverses the whole-tree, the order is also determined by the fact that the tree is traversed in a breadth-first manner. --- contrib/sepgsql/dml.c | 4 +- src/backend/catalog/partition.c | 84 ++++++++++++++++++++++ src/backend/commands/analyze.c | 8 ++- src/backend/commands/lockcmds.c | 6 +- src/backend/commands/publicationcmds.c | 9 ++- src/backend/commands/tablecmds.c | 124 +++++++++++++++++++++++++-------- src/backend/commands/vacuum.c | 7 +- src/backend/optimizer/prep/prepunion.c | 6 +- src/include/catalog/partition.h | 3 + 9 files changed, 213 insertions(+), 38 deletions(-) diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c index b643720e36..62d6610c43 100644 --- a/contrib/sepgsql/dml.c +++ b/contrib/sepgsql/dml.c @@ -332,8 +332,10 @@ sepgsql_dml_privileges(List *rangeTabls, bool abort_on_violation) */ if (!rte->inh) tableIds = list_make1_oid(rte->relid); - else + else if (rte->relkind != RELKIND_PARTITIONED_TABLE) tableIds = find_all_inheritors(rte->relid, NoLock, NULL); + else + tableIds = get_all_partition_oids(rte->relid, NoLock); foreach(li, tableIds) { diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index dcc7f8af27..614b2f79f2 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1150,6 +1150,90 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode, return pd; } +/* + * get_all_partition_oids - returns the list of all partitions in the + * partition tree rooted at relid + * + * OIDs in the list are ordered canonically using the partition bound order, + * while the tree is being traversed in a breadth-first manner. Actually, + * this's just a wrapper on top of RelationGetPartitionDispatchInfo. + * + * All the partitions are locked with lockmode. We assume that relid has been + * locked by the caller with lockmode. + */ +List *get_all_partition_oids(Oid relid, int lockmode) +{ + List *result = NIL; + List *leaf_part_oids = NIL; + ListCell *lc; + Relation rel; + int num_parted; + PartitionDispatch *pds; + int i; + + /* caller should've locked already */ + rel = heap_open(relid, NoLock); + pds = RelationGetPartitionDispatchInfo(rel, lockmode, &num_parted, + &leaf_part_oids); + + /* + * First append the OIDs of all the partitions that are partitioned + * tables themselves, starting with relid itself. + */ + result = lappend_oid(result, relid); + for (i = 1; i < num_parted; i++) + { + result = lappend_oid(result, RelationGetRelid(pds[i]->reldesc)); + + /* + * To avoid leaking resources, release them. This is to work around + * the existing interface of RelationGetPartitionDispatchInfo() that + * acquires these resources at the mercy of the caller to release + * them. + */ + heap_close(pds[i]->reldesc, NoLock); + if (pds[i]->tupmap) + pfree(pds[i]->tupmap); + ExecDropSingleTupleTableSlot(pds[i]->tupslot); + } + heap_close(rel, NoLock); + + /* Leaf partitions were not locked; do so now. */ + foreach(lc, leaf_part_oids) + { + if (lockmode != NoLock) + LockRelationOid(lfirst_oid(lc), lockmode); + } + + /* Return after concatening the leaf partition OIDs. */ + return list_concat(result, leaf_part_oids); +} + +/* + * get_partition_oids - returns a list of OIDs of partitions of relid + * + * OIDs are returned from the PartitionDesc contained in the relcache, so they + * are ordered canonically using partition bound order. + */ +List *get_partition_oids(Oid relid, int lockmode) +{ + List *result = NIL; + Relation rel; + int i; + PartitionDesc partdesc; + + /* caller should've locked already */ + rel = heap_open(relid, NoLock); + partdesc = RelationGetPartitionDesc(rel); + for (i = 0; i < partdesc->nparts; i++) + { + result = lappend_oid(result, partdesc->oids[i]); + } + heap_close(rel, NoLock); + + return result; +} + /* Module-local functions */ /* diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 2b638271b3..f3c1893b12 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -1281,8 +1281,12 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, * Find all members of inheritance set. We only need AccessShareLock on * the children. */ - tableOIDs = - find_all_inheritors(RelationGetRelid(onerel), AccessShareLock, NULL); + if (onerel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + tableOIDs = find_all_inheritors(RelationGetRelid(onerel), + AccessShareLock, NULL); + else + tableOIDs = get_all_partition_oids(RelationGetRelid(onerel), + AccessShareLock); /* * Check that there's at least one descendant, else fail. This could diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 9fe9e022b0..29a9ef82b2 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -15,6 +15,7 @@ #include "postgres.h" #include "catalog/namespace.h" +#include "catalog/partition.h" #include "catalog/pg_inherits_fn.h" #include "commands/lockcmds.h" #include "miscadmin.h" @@ -112,7 +113,10 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) List *children; ListCell *lc; - children = find_inheritance_children(reloid, NoLock); + if (get_rel_relkind(reloid) != RELKIND_PARTITIONED_TABLE) + children = find_inheritance_children(reloid, NoLock); + else + children = get_partition_oids(reloid, NoLock); foreach(lc, children) { diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 610cb499d2..ab7423577f 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -515,8 +515,13 @@ OpenTableList(List *tables) ListCell *child; List *children; - children = find_all_inheritors(myrelid, ShareUpdateExclusiveLock, - NULL); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_all_inheritors(myrelid, + ShareUpdateExclusiveLock, + NULL); + else + children = get_all_partition_oids(myrelid, + ShareUpdateExclusiveLock); foreach(child, children) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7859ef13ac..332697c095 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1231,7 +1231,12 @@ ExecuteTruncate(TruncateStmt *stmt) ListCell *child; List *children; - children = find_all_inheritors(myrelid, AccessExclusiveLock, NULL); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_all_inheritors(myrelid, + AccessExclusiveLock, NULL); + else + children = get_all_partition_oids(myrelid, + AccessExclusiveLock); foreach(child, children) { @@ -2555,8 +2560,11 @@ renameatt_internal(Oid myrelid, * calls to renameatt() can determine whether there are any parents * outside the inheritance hierarchy being processed. */ - child_oids = find_all_inheritors(myrelid, AccessExclusiveLock, - &child_numparents); + if (targetrelation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + child_oids = find_all_inheritors(myrelid, AccessExclusiveLock, + &child_numparents); + else + child_oids = get_all_partition_oids(myrelid, AccessExclusiveLock); /* * find_all_inheritors does the recursive search of the inheritance @@ -2581,6 +2589,10 @@ renameatt_internal(Oid myrelid, * tables; else the rename would put them out of step. * * expected_parents will only be 0 if we are not already recursing. + * + * We don't bother to distinguish between find_inheritance_children's + * and get_partition_oids's results unlike in most other places, + * because we're not concerned about the order of OIDs here. */ if (expected_parents == 0 && find_inheritance_children(myrelid, NoLock) != NIL) @@ -2765,8 +2777,13 @@ rename_constraint_internal(Oid myrelid, ListCell *lo, *li; - child_oids = find_all_inheritors(myrelid, AccessExclusiveLock, - &child_numparents); + Assert(targetrelation != NULL); + if (targetrelation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + child_oids = find_all_inheritors(myrelid, AccessExclusiveLock, + &child_numparents); + else + child_oids = get_all_partition_oids(myrelid, + AccessExclusiveLock); forboth(lo, child_oids, li, child_numparents) { @@ -2781,6 +2798,12 @@ rename_constraint_internal(Oid myrelid, } else { + /* + * We don't bother to distinguish between + * find_inheritance_children's and get_partition_oids's results + * unlike in most other places, because we're not concerned about + * the order of OIDs here. + */ if (expected_parents == 0 && find_inheritance_children(myrelid, NoLock) != NIL) ereport(ERROR, @@ -4790,7 +4813,10 @@ ATSimpleRecursion(List **wqueue, Relation rel, ListCell *child; List *children; - children = find_all_inheritors(relid, lockmode, NULL); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_all_inheritors(relid, lockmode, NULL); + else + children = get_all_partition_oids(relid, lockmode); /* * find_all_inheritors does the recursive search of the inheritance @@ -5183,6 +5209,10 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, /* * Cannot add identity column if table has children, because identity does * not inherit. (Adding column and identity separately will work.) + * + * We don't bother to distinguish between find_inheritance_children's and + * get_partition_oids's results unlike in most other places, because we're + * not concerned about the order of OIDs here. */ if (colDef->identity && recurse && @@ -5390,9 +5420,12 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, /* * Propagate to children as appropriate. Unlike most other ALTER * routines, we have to do this one level of recursion at a time; we can't - * use find_all_inheritors to do it in one pass. + * use find_all_inheritors or get_all_partition_oids to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_inheritance_children(RelationGetRelid(rel), lockmode); + else + children = get_partition_oids(RelationGetRelid(rel), lockmode); /* * If we are told not to recurse, there had better not be any child @@ -6509,9 +6542,12 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, /* * Propagate to children as appropriate. Unlike most other ALTER * routines, we have to do this one level of recursion at a time; we can't - * use find_all_inheritors to do it in one pass. + * use find_all_inheritors or get_all_partition_oids to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_inheritance_children(RelationGetRelid(rel), lockmode); + else + children = get_partition_oids(RelationGetRelid(rel), lockmode); if (children) { @@ -6943,9 +6979,12 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, /* * Propagate to children as appropriate. Unlike most other ALTER * routines, we have to do this one level of recursion at a time; we can't - * use find_all_inheritors to do it in one pass. + * use find_all_inheritors or get_all_partition_oids to do it in one pass. */ - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_inheritance_children(RelationGetRelid(rel), lockmode); + else + children = get_partition_oids(RelationGetRelid(rel), lockmode); /* * Check if ONLY was specified with ALTER TABLE. If so, allow the @@ -7663,8 +7702,14 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, * shouldn't try to look for it in the children. */ if (!recursing && !con->connoinherit) - children = find_all_inheritors(RelationGetRelid(rel), - lockmode, NULL); + { + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_all_inheritors(RelationGetRelid(rel), + lockmode, NULL); + else + children = get_all_partition_oids(RelationGetRelid(rel), + lockmode); + } /* * For CHECK constraints, we must ensure that we only mark the @@ -8544,12 +8589,14 @@ ATExecDropConstraint(Relation rel, const char *constrName, /* * Propagate to children as appropriate. Unlike most other ALTER * routines, we have to do this one level of recursion at a time; we can't - * use find_all_inheritors to do it in one pass. + * use find_all_inheritors or get_all_partition_oids to do it in one pass. */ - if (!is_no_inherit_constraint) + if (is_no_inherit_constraint) + children = NIL; + else if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) children = find_inheritance_children(RelationGetRelid(rel), lockmode); else - children = NIL; + children = get_partition_oids(RelationGetRelid(rel), lockmode); /* * For a partitioned table, if partitions exist and we are told not to @@ -8836,7 +8883,10 @@ ATPrepAlterColumnType(List **wqueue, ListCell *child; List *children; - children = find_all_inheritors(relid, lockmode, NULL); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + children = find_all_inheritors(relid, lockmode, NULL); + else + children = get_all_partition_oids(relid, lockmode); /* * find_all_inheritors does the recursive search of the inheritance @@ -8886,6 +8936,11 @@ ATPrepAlterColumnType(List **wqueue, relation_close(childrel, NoLock); } } + /* + * We don't bother to distinguish between find_inheritance_children's and + * get_partition_oids's results unlike in most other places, because we're + * not concerned about the order of OIDs here. + */ else if (!recursing && find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) ereport(ERROR, @@ -10996,6 +11051,7 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode) * * We use weakest lock we can on child's children, namely AccessShareLock. */ + Assert(child_rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE); children = find_all_inheritors(RelationGetRelid(child_rel), AccessShareLock, NULL); @@ -13421,7 +13477,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) { Relation attachrel, catalog; - List *attachrel_children; + List *attachrel_children = NIL; TupleConstr *attachrel_constr; List *partConstraint, *existConstraint; @@ -13501,15 +13557,20 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) * table, nor its partitions. But we cannot risk a deadlock by taking a * weaker lock now and the stronger one only when needed. */ - attachrel_children = find_all_inheritors(RelationGetRelid(attachrel), - AccessExclusiveLock, NULL); - if (list_member_oid(attachrel_children, RelationGetRelid(rel))) - ereport(ERROR, - (errcode(ERRCODE_DUPLICATE_TABLE), - errmsg("circular inheritance not allowed"), - errdetail("\"%s\" is already a child of \"%s\".", - RelationGetRelationName(rel), - RelationGetRelationName(attachrel)))); + if (attachrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid attachrel_oid = RelationGetRelid(attachrel); + + attachrel_children = get_all_partition_oids(attachrel_oid, + AccessExclusiveLock); + if (list_member_oid(attachrel_children, RelationGetRelid(rel))) + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_TABLE), + errmsg("circular inheritance not allowed"), + errdetail("\"%s\" is already a child of \"%s\".", + RelationGetRelationName(rel), + RelationGetRelationName(attachrel)))); + } /* Temp parent cannot have a partition that is itself not a temp */ if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP && @@ -13707,6 +13768,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) /* Constraints proved insufficient, so we need to scan the table. */ ListCell *lc; + /* + * If attachrel isn't partitioned, attachrel_children would be empty. + * We still need to process attachrel itself, so initialize. + */ + if (attachrel_children == NIL) + attachrel_children = list_make1_oid(RelationGetRelid(attachrel)); + foreach(lc, attachrel_children) { AlteredTableInfo *tab; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index faa181207a..7bea95d9c5 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -31,6 +31,7 @@ #include "access/transam.h" #include "access/xact.h" #include "catalog/namespace.h" +#include "catalog/partition.h" #include "catalog/pg_database.h" #include "catalog/pg_inherits_fn.h" #include "catalog/pg_namespace.h" @@ -423,14 +424,14 @@ get_rel_oids(Oid relid, const RangeVar *vacrel) /* * Make relation list entries for this guy and its partitions, if any. - * Note that the list returned by find_all_inheritors() include the - * passed-in OID at its head. Also note that we did not request a + * Note that the list returned by get_all_partition_oids() includes + * the passed-in OID at its head. Also note that we did not request a * lock to be taken to match what would be done otherwise. */ oldcontext = MemoryContextSwitchTo(vac_context); if (include_parts) oid_list = list_concat(oid_list, - find_all_inheritors(relid, NoLock, NULL)); + get_all_partition_oids(relid, NoLock)); else oid_list = lappend_oid(oid_list, relid); MemoryContextSwitchTo(oldcontext); diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index cf46b74782..398bdd598a 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -33,6 +33,7 @@ #include "access/heapam.h" #include "access/htup_details.h" #include "access/sysattr.h" +#include "catalog/partition.h" #include "catalog/pg_inherits_fn.h" #include "catalog/pg_type.h" #include "miscadmin.h" @@ -1418,7 +1419,10 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) lockmode = AccessShareLock; /* Scan for all members of inheritance set, acquire needed locks */ - inhOIDs = find_all_inheritors(parentOID, lockmode, NULL); + if (rte->relkind != RELKIND_PARTITIONED_TABLE) + inhOIDs = find_all_inheritors(parentOID, lockmode, NULL); + else + inhOIDs = get_all_partition_oids(parentOID, lockmode); /* * Check that there's at least one descendant, else treat as no-child diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h index 434ded37d7..e6314fbaa2 100644 --- a/src/include/catalog/partition.h +++ b/src/include/catalog/partition.h @@ -85,6 +85,9 @@ extern List *map_partition_varattnos(List *expr, int target_varno, extern List *RelationGetPartitionQual(Relation rel); extern Expr *get_partition_qual_relid(Oid relid); +extern List *get_all_partition_oids(Oid relid, int lockmode); +extern List *get_partition_oids(Oid relid, int lockmode); + /* For tuple routing */ extern PartitionDispatch *RelationGetPartitionDispatchInfo(Relation rel, int lockmode, int *num_parted, -- 2.11.0