On 2018/04/13 14:38, Amit Langote wrote:
> About the specific relation_open(.., NoLock) under question, I think there
> might be a way to address this by opening the tables with the appropriate
> lock mode in partitioned_rels list in ExecLockNonleafAppendTables
That may have sounded a bit confusing:
I meant to say: "by opening the tables in partitioned_rels list with the
appropriate lock mode in ExecLockNonleafAppendTables"
> Attached a PoC patch.
There were a couple of unnecessary hunks, which removed in the attached.
Thanks,
Amit
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 11139f743d..e3b3911945 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1244,7 +1244,8 @@ adjust_partition_tlist(List *tlist, TupleConversionMap
*map)
* PartitionPruneInfo.
*/
PartitionPruneState *
-ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
+ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo,
+ Relation
*partitioned_rels)
{
PartitionPruningData *prunedata;
PartitionPruneState *prunestate;
@@ -1303,10 +1304,11 @@ ExecSetupPartitionPruneState(PlanState *planstate, List
*partitionpruneinfo)
pprune->subpart_map = pinfo->subpart_map;
/*
- * Grab some info from the table's relcache; lock was already
obtained
- * by ExecLockNonLeafAppendTables.
+ * ExecLockNonLeafAppendTables already opened the relation for
us.
*/
- rel = relation_open(pinfo->reloid, NoLock);
+ Assert(partitioned_rels[i] != NULL);
+ rel = partitioned_rels[i];
+ Assert(RelationGetRelid(rel) == pinfo->reloid);
partkey = RelationGetPartitionKey(rel);
partdesc = RelationGetPartitionDesc(rel);
@@ -1336,8 +1338,6 @@ ExecSetupPartitionPruneState(PlanState *planstate, List
*partitionpruneinfo)
prunestate->execparams = bms_add_members(prunestate->execparams,
pinfo->execparams);
- relation_close(rel, NoLock);
-
i++;
}
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index b963cae730..58a0961eb1 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -858,22 +858,49 @@ ShutdownExprContext(ExprContext *econtext, bool isCommit)
/*
* ExecLockNonLeafAppendTables
*
- * Locks, if necessary, the tables indicated by the RT indexes contained in
- * the partitioned_rels list. These are the non-leaf tables in the partition
- * tree controlled by a given Append or MergeAppend node.
+ * Opens and/or locks, if necessary, the tables indicated by the RT indexes
+ * contained in the partitioned_rels list. These are the non-leaf tables in
+ * the partition tree controlled by a given Append or MergeAppend node.
*/
void
-ExecLockNonLeafAppendTables(List *partitioned_rels, EState *estate)
+ExecLockNonLeafAppendTables(PlanState *planstate,
+ EState *estate,
+ List *partitioned_rels)
{
PlannedStmt *stmt = estate->es_plannedstmt;
ListCell *lc;
+ int i;
+ /*
+ * If we're going to be performing pruning, allocate space for Relation
+ * pointers to be used later when setting up partition pruning state in
+ * ExecSetupPartitionPruneState.
+ */
+ if (IsA(planstate, AppendState))
+ {
+ AppendState *appendstate = (AppendState *) planstate;
+ Append *node = (Append *) planstate->plan;
+
+ if (node->part_prune_infos != NIL)
+ {
+ Assert(list_length(node->part_prune_infos) ==
+ list_length(partitioned_rels));
+ appendstate->partitioned_rels = (Relation *)
+
palloc(sizeof(Relation) *
+
list_length(node->part_prune_infos));
+ appendstate->num_partitioned_rels =
+
list_length(node->part_prune_infos);
+ }
+ }
+
+ i = 0;
foreach(lc, partitioned_rels)
{
ListCell *l;
Index rti = lfirst_int(lc);
bool is_result_rel = false;
Oid relid = getrelid(rti,
estate->es_range_table);
+ int lockmode;
/* If this is a result relation, already locked in InitPlan */
foreach(l, stmt->nonleafResultRelations)
@@ -903,9 +930,37 @@ ExecLockNonLeafAppendTables(List *partitioned_rels, EState
*estate)
}
if (rc && RowMarkRequiresRowShareLock(rc->markType))
- LockRelationOid(relid, RowShareLock);
+ lockmode = RowShareLock;
else
- LockRelationOid(relid, AccessShareLock);
+ lockmode = AccessShareLock;
+ switch (nodeTag(planstate))
+ {
+ /*
+ * For Append, we may need to store the
Relation pointers to
+ * be used later when setting up partition
pruning state.
+ */
+ case T_AppendState:
+ {
+ AppendState *appendstate =
(AppendState *) planstate;
+
+ if
(appendstate->partitioned_rels)
+
appendstate->partitioned_rels[i] =
+
heap_open(relid, lockmode);
+ else
+ LockRelationOid(relid,
lockmode);
+ i++;
+ }
+
+ /* Just lock here; there is no pruning support.
*/
+ case T_MergeAppendState:
+ LockRelationOid(relid, lockmode);
+ break;
+
+ default:
+ elog(ERROR, "invalid PlanState node:
%d",
+ nodeTag(planstate));
+ break;
+ }
}
}
}
diff --git a/src/backend/executor/nodeAppend.c
b/src/backend/executor/nodeAppend.c
index d062cfddac..0799bbc919 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -112,18 +112,19 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
Assert(!(eflags & EXEC_FLAG_MARK));
/*
- * Lock the non-leaf tables in the partition tree controlled by this
node.
- * It's a no-op for non-partitioned parent tables.
- */
- ExecLockNonLeafAppendTables(node->partitioned_rels, estate);
-
- /*
* create new AppendState for our append node
*/
appendstate->ps.plan = (Plan *) node;
appendstate->ps.state = estate;
appendstate->ps.ExecProcNode = ExecAppend;
+ /*
+ * Lock the non-leaf tables in the partition tree controlled by this
node.
+ * It's a no-op for non-partitioned parent tables.
+ */
+ ExecLockNonLeafAppendTables((PlanState *) appendstate, estate,
+
node->partitioned_rels);
+
/* Let choose_next_subplan_* function handle setting the first subplan
*/
appendstate->as_whichplan = INVALID_SUBPLAN_INDEX;
@@ -134,8 +135,11 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
ExecAssignExprContext(estate, &appendstate->ps);
+ /* ExecLockNonLeafAppendTables must have set this up. */
+ Assert(appendstate->partitioned_rels != NULL);
prunestate = ExecSetupPartitionPruneState(&appendstate->ps,
-
node->part_prune_infos);
+
node->part_prune_infos,
+
appendstate->partitioned_rels);
/*
* When there are external params matching the partition key we
may be
@@ -309,6 +313,7 @@ ExecEndAppend(AppendState *node)
PlanState **appendplans;
int nplans;
int i;
+ int num_partitioned_rels;
/*
* get information from the node
@@ -317,6 +322,15 @@ ExecEndAppend(AppendState *node)
nplans = node->as_nplans;
/*
+ * Close partitioned rels that we may have opened for partition
+ * pruning.
+ */
+ num_partitioned_rels = node->num_partitioned_rels;
+ Assert(node->partitioned_rels != NULL || num_partitioned_rels == 0);
+ for (i = 0; i < num_partitioned_rels; i++)
+ heap_close(node->partitioned_rels[i], NoLock);
+
+ /*
* shut down each of the subscans
*/
for (i = 0; i < nplans; i++)
diff --git a/src/backend/executor/nodeMergeAppend.c
b/src/backend/executor/nodeMergeAppend.c
index 118f4ef07d..6f28002ce2 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -76,7 +76,8 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int
eflags)
* Lock the non-leaf tables in the partition tree controlled by this
node.
* It's a no-op for non-partitioned parent tables.
*/
- ExecLockNonLeafAppendTables(node->partitioned_rels, estate);
+ ExecLockNonLeafAppendTables((PlanState *) mergestate, estate,
+
node->partitioned_rels);
/*
* Set up empty vector of subplan states
diff --git a/src/include/executor/execPartition.h
b/src/include/executor/execPartition.h
index 0c36c8be30..134eb9290f 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -118,6 +118,7 @@ typedef struct PartitionTupleRouting
* bypass certain subnodes when we have proofs that indicate that no tuple
* matching the 'pruning_steps' will be found within.
*
+ * parent RelationData pointer of
the parent
* subnode_map An array containing the subnode
index which
* matches this
partition index, or -1 if the
* subnode has
been pruned already.
@@ -137,6 +138,7 @@ typedef struct PartitionTupleRouting
*/
typedef struct PartitionPruningData
{
+ Relation parent;
int *subnode_map;
int *subpart_map;
Bitmapset *present_parts;
@@ -205,7 +207,8 @@ extern HeapTuple
ConvertPartitionTupleSlot(TupleConversionMap *map,
extern void ExecCleanupTupleRouting(ModifyTableState *mtstate,
PartitionTupleRouting *proute);
extern PartitionPruneState *ExecSetupPartitionPruneState(PlanState *planstate,
- List *partitionpruneinfo);
+ List *partitionpruneinfo,
+ Relation *partitioned_rels);
extern Bitmapset *ExecFindMatchingSubPlans(PartitionPruneState *prunestate);
extern Bitmapset *ExecFindInitialMatchingSubPlans(PartitionPruneState
*prunestate,
int nsubnodes);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 45a077a949..6300397099 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -526,7 +526,9 @@ extern void UnregisterExprContextCallback(ExprContext
*econtext,
ExprContextCallbackFunction function,
Datum arg);
-extern void ExecLockNonLeafAppendTables(List *partitioned_rels, EState
*estate);
+extern void ExecLockNonLeafAppendTables(PlanState *planstate,
+ EState *estate,
+ List *partitioned_rels);
extern Datum GetAttributeByName(HeapTupleHeader tuple, const char *attname,
bool *isNull);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 9fe0b79095..b6188f8458 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1089,6 +1089,8 @@ struct AppendState
int as_whichplan;
ParallelAppendState *as_pstate; /* parallel coordination info */
Size pstate_len; /* size of parallel
coordination info */
+ Relation *partitioned_rels;
+ int num_partitioned_rels; /* number of entries in
above array */
struct PartitionPruneState *as_prune_state;
Bitmapset *as_valid_subplans;
bool (*choose_next_subplan) (AppendState *);
