postgrespro.git / summary / log / commit / refs
commit f4b6341d5fcb627d51b3fc89527aa5eaa659fb2e
Author: Robert Haas <rhaas@postgresql.org>
Date: Tue Feb 26 17:22:57 2019 +0000
Change lock acquisition order in expand_inherited_rtentry.
Previously, this function acquired locks in the order using
find_all_inheritors(), which locks the children of each table that it
processes in ascending OID order, and which processes the inheritance
hierarchy as a whole in a breadth-first fashion. Now, it processes
the inheritance hierarchy in a depth-first fashion, and at each level
it proceeds in the order in which tables appear in the PartitionDesc.
If table inheritance rather than table partitioning is used, the old
order is preserved.
This change moves the locking of any given partition much closer to
the code that actually expands that partition. This seems essential
if we ever want to allow concurrent DDL to add or remove partitions,
because if the set of partitions can change, we must use the same data
to decide which partitions to lock as we do to decide which partitions
to expand; otherwise, we might expand a partition that we haven't
locked. It should hopefully also facilitate efforts to postpone
inheritance expansion or locking for performance reasons, because
there's really no way to postpone locking some partitions if
we're blindly locking them all using find_all_inheritors().
The only downside of this change which is known to me is that it
further deviates from the principle that we should always lock the
inheritance hierarchy in find_all_inheritors() order to avoid deadlock
risk. However, we've already crossed that bridge in commit
9eefba181f7782d27d85d7e94e6028371e7ab2d7 and there are futher patches
pending that make similar changes, so this isn't really giving up
anything that we haven't surrendered already -- and it seems entirely
worth it, given the performance benefits some of those changes seem
likely to bring.
Patch by me; thanks to David Rowley for discussion of these issues.
Discussion: http://postgr.es/m/CAKJS1f_eEYVEq5tM8sm1k-HOwG0AyCPwX54XG9x4w0zy_N4Q_Q@mail.gmail.com
Discussion: http://postgr.es/m/CA+TgmoZUwPf_uanjF==gTGBMJrn8uCq52XYvAEorNkLrUdoawg@mail.gmail.com
src/backend/optimizer/util/inherit.c | 59 +++++++++++++++++-------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
index faba493200..a014a12060 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -124,28 +124,15 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
/*
* The rewriter should already have obtained an appropriate lock on each
- * relation named in the query. However, for each child relation we add
- * to the query, we must obtain an appropriate lock, because this will be
- * the first use of those relations in the parse/rewrite/plan pipeline.
- * Child rels should use the same lockmode as their parent.
+ * relation named in the query, so we can open the parent relation without
+ * locking it. However, for each child relation we add to the query, we
+ * must obtain an appropriate lock, because this will be the first use of
+ * those relations in the parse/rewrite/plan pipeline. Child rels should
+ * use the same lockmode as their parent.
*/
+ oldrelation = table_open(parentOID, NoLock);
lockmode = rte->rellockmode;
- /* Scan for all members of inheritance set, acquire needed locks */
- inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
-
- /*
- * Check that there's at least one descendant, else treat as no-child
- * case. This could happen despite above has_subclass() check, if table
- * once had a child but no longer does.
- */
- if (list_length(inhOIDs) < 2)
- {
- /* Clear flag before returning */
- rte->inh = false;
- return;
- }
-
/*
* If parent relation is selected FOR UPDATE/SHARE, we need to mark its
* PlanRowMark as isParent = true, and generate a new PlanRowMark for each
@@ -155,21 +142,15 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
if (oldrc)
oldrc->isParent = true;
- /*
- * Must open the parent relation to examine its tupdesc. We need not lock
- * it; we assume the rewriter already did.
- */
- oldrelation = table_open(parentOID, NoLock);
-
/* Scan the inheritance set and expand it */
- if (RelationGetPartitionDesc(oldrelation) != NULL)
+ if (oldrelation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
Assert(rte->relkind == RELKIND_PARTITIONED_TABLE);
/*
- * If this table has partitions, recursively expand them in the order
- * in which they appear in the PartitionDesc. While at it, also
- * extract the partition key columns of all the partitioned tables.
+ * If this table has partitions, recursively expand and lock them.
+ * While at it, also extract the partition key columns of all the
+ * partitioned tables.
*/
expand_partitioned_rtentry(root, rte, rti, oldrelation, oldrc,
lockmode, &root->append_rel_list);
@@ -180,6 +161,22 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
RangeTblEntry *childrte;
Index childRTindex;
+ /* Scan for all members of inheritance set, acquire needed locks */
+ inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+
+ /*
+ * Check that there's at least one descendant, else treat as no-child
+ * case. This could happen despite above has_subclass() check, if the
+ * table once had a child but no longer does.
+ */
+ if (list_length(inhOIDs) < 2)
+ {
+ /* Clear flag before returning */
+ rte->inh = false;
+ heap_close(oldrelation, NoLock);
+ return;
+ }
+
/*
* This table has no partitions. Expand any plain inheritance
* children in the order the OIDs were returned by
@@ -289,8 +286,8 @@ expand_partitioned_rtentry(PlannerInfo *root, RangeTblEntry *parentrte,
Oid childOID = partdesc->oids[i];
Relation childrel;
- /* Open rel; we already have required locks */
- childrel = table_open(childOID, NoLock);
+ /* Open rel, acquiring required locks */
+ childrel = table_open(childOID, lockmode);
/*
* Temporary partitions belonging to other sessions should have been
[parent: 42ccbe4351e4]