On 09/10/2020 11:01, Amit Langote wrote:
On Thu, Oct 8, 2020 at 9:35 PM Amit Langote <amitlangot...@gmail.com> wrote:
On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
On 07/10/2020 12:50, Amit Langote wrote:
I have thought about something like this before. An idea I had is to
make es_result_relations array indexable by plain RT indexes, then we
don't need to maintain separate indexes that we do today for result
relations.
That sounds like a good idea. es_result_relations is currently an array
of ResultRelInfos, so that would leave a lot of unfilled structs in the
array. But in on of your other threads, you proposed turning
es_result_relations into an array of pointers anyway
(https://www.postgresql.org/message-id/CA+HiwqE4k1Q2TLmCAvekw+8_NXepbnfUOamOeX=kphrdtfs...@mail.gmail.com).
Okay, I am reorganizing the patches around that idea and will post an
update soon.
Attached updated patches.
0001 makes es_result_relations an RTI-indexable array, which allows to
get rid of all "result relation index" fields across the code.
Thanks! A couple small things I wanted to check with you before committing:
1. We have many different cleanup/close routines now:
ExecCloseResultRelations, ExecCloseRangeTableRelations, and
ExecCleanUpTriggerState. Do we need them all? It seems to me that we
could merge ExecCloseRangeTableRelations() and
ExecCleanUpTriggerState(), they seem to do roughly the same thing: close
relations that were opened for ResultRelInfos. They are always called
together, except in afterTriggerInvokeEvents(). And in
afterTriggerInvokeEvents() too, there would be no harm in doing both,
even though we know there aren't any entries in the es_result_relations
array at that point.
2. The way this is handled in worker.c is a bit funny. In
create_estate_for_relation(), you create a ResultRelInfo, but you
*don't* put it in the es_opened_result_relations list. That's
surprising, but I'm also surprised there are no
ExecCloseResultRelations() calls before the FreeExecutorState() calls in
worker.c. It's not needed because the
apply_handle_insert/update/delete_internal() functions call
ExecCloseIndices() directly, so they don't rely on the
ExecCloseResultRelations() function for cleanup. That works too, but
it's a bit surprising because it's different from how it's done in
copy.c and nodeModifyTable.c. It would feel natural to rely on
ExecCloseResultRelations() in worker.c as well, but on the other hand,
it also calls ExecOpenIndices() in a more lazy fashion, and it makes
sense to call ExecCloseIndices() in the same functions that
ExecOpenIndices() is called. So I'm not sure if changing that would be
an improvement overall. What do you think? Did you consider doing that?
Attached is your original patch v13, and a patch on top of it that
merges ExecCloseResultRelations() and ExecCleanUpTriggerState(), and
makes some minor comment changes. I didn't do anything about the
worker.c business, aside from adding a comment about it.
- Heikki
>From 17e22c13509941c561346f386d53fd4f4ff16ef8 Mon Sep 17 00:00:00 2001
From: amitlan <amitlangot...@gmail.com>
Date: Fri, 9 Oct 2020 13:52:29 +0900
Subject: [PATCH 1/2] Make es_result_relations array indexable by RT index
This allows us to get rid of the need to keep around a separate set
of indexes for each result relation.
---
src/backend/commands/copy.c | 20 +--
src/backend/commands/explain.c | 17 +-
src/backend/commands/tablecmds.c | 12 +-
src/backend/executor/execMain.c | 189 ++++++-----------------
src/backend/executor/execUtils.c | 73 ++++++---
src/backend/executor/nodeModifyTable.c | 24 ++-
src/backend/nodes/copyfuncs.c | 2 -
src/backend/nodes/outfuncs.c | 2 -
src/backend/nodes/readfuncs.c | 2 -
src/backend/optimizer/plan/createplan.c | 2 -
src/backend/optimizer/plan/setrefs.c | 10 +-
src/backend/replication/logical/worker.c | 4 +-
src/include/executor/executor.h | 5 +
src/include/nodes/execnodes.h | 21 ++-
src/include/nodes/plannodes.h | 2 -
15 files changed, 162 insertions(+), 223 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3c7dbad27a..6948214334 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2829,25 +2829,18 @@ CopyFrom(CopyState cstate)
* index-entry-making machinery. (There used to be a huge amount of code
* here that basically duplicated execUtils.c ...)
*/
- resultRelInfo = makeNode(ResultRelInfo);
- InitResultRelInfo(resultRelInfo,
- cstate->rel,
- 1, /* must match rel's position in range_table */
- NULL,
- 0);
- target_resultRelInfo = resultRelInfo;
+ ExecInitRangeTable(estate, cstate->range_table);
+ ExecInitResultRelationsArray(estate);
+ resultRelInfo = target_resultRelInfo = makeNode(ResultRelInfo);
+ ExecInitResultRelation(estate, resultRelInfo, 1);
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
ExecOpenIndices(resultRelInfo, false);
- estate->es_result_relations = resultRelInfo;
- estate->es_num_result_relations = 1;
estate->es_result_relation_info = resultRelInfo;
- ExecInitRangeTable(estate, cstate->range_table);
-
/*
* Set up a ModifyTableState so we can let FDW(s) init themselves for
* foreign-table result relation(s).
@@ -2856,7 +2849,7 @@ CopyFrom(CopyState cstate)
mtstate->ps.plan = NULL;
mtstate->ps.state = estate;
mtstate->operation = CMD_INSERT;
- mtstate->resultRelInfo = estate->es_result_relations;
+ mtstate->resultRelInfo = resultRelInfo;
if (resultRelInfo->ri_FdwRoutine != NULL &&
resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
@@ -3359,7 +3352,8 @@ CopyFrom(CopyState cstate)
if (insertMethod != CIM_SINGLE)
CopyMultiInsertInfoCleanup(&multiInsertInfo);
- ExecCloseIndices(target_resultRelInfo);
+ ExecCloseResultRelations(estate);
+ ExecCloseRangeTableRelations(estate);
/* Close all the partitioned tables, leaf partitions, and their indices */
if (proute)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c98c9b5547..3210f90bd1 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -769,13 +769,14 @@ ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc)
{
ResultRelInfo *rInfo;
bool show_relname;
- int numrels = queryDesc->estate->es_num_result_relations;
- int numrootrels = queryDesc->estate->es_num_root_result_relations;
+ int numrels = list_length(queryDesc->plannedstmt->resultRelations);
+ int numrootrels = list_length(queryDesc->plannedstmt->rootResultRelations);
+ List *resultrels;
List *routerels;
List *targrels;
- int nr;
ListCell *l;
+ resultrels = queryDesc->estate->es_opened_result_relations;
routerels = queryDesc->estate->es_tuple_routing_result_relations;
targrels = queryDesc->estate->es_trig_target_relations;
@@ -783,13 +784,11 @@ ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc)
show_relname = (numrels > 1 || numrootrels > 0 ||
routerels != NIL || targrels != NIL);
- rInfo = queryDesc->estate->es_result_relations;
- for (nr = 0; nr < numrels; rInfo++, nr++)
- report_triggers(rInfo, show_relname, es);
-
- rInfo = queryDesc->estate->es_root_result_relations;
- for (nr = 0; nr < numrootrels; rInfo++, nr++)
+ foreach(l, resultrels)
+ {
+ rInfo = lfirst(l);
report_triggers(rInfo, show_relname, es);
+ }
foreach(l, routerels)
{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e0ac4e05e5..1cd8cfb41d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1693,6 +1693,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
SubTransactionId mySubid;
ListCell *cell;
Oid *logrelids;
+ int dummy_rti;
/*
* Check the explicitly-specified relations.
@@ -1792,19 +1793,24 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
resultRelInfos = (ResultRelInfo *)
palloc(list_length(rels) * sizeof(ResultRelInfo));
resultRelInfo = resultRelInfos;
+ estate->es_result_relations = (ResultRelInfo **)
+ palloc(list_length(rels) * sizeof(ResultRelInfo *));
+ dummy_rti = 1;
foreach(cell, rels)
{
Relation rel = (Relation) lfirst(cell);
InitResultRelInfo(resultRelInfo,
rel,
- 0, /* dummy rangetable index */
+ dummy_rti,
NULL,
0);
+ estate->es_result_relations[dummy_rti - 1] = resultRelInfo;
+ estate->es_opened_result_relations =
+ lappend(estate->es_opened_result_relations, resultRelInfo);
resultRelInfo++;
+ dummy_rti++;
}
- estate->es_result_relations = resultRelInfos;
- estate->es_num_result_relations = list_length(rels);
/*
* Process all BEFORE STATEMENT TRUNCATE triggers before we begin
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 2e27e26ba4..f62fd8f7aa 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -827,85 +827,12 @@ InitPlan(QueryDesc *queryDesc, int eflags)
estate->es_plannedstmt = plannedstmt;
- /*
- * Initialize ResultRelInfo data structures, and open the result rels.
- */
if (plannedstmt->resultRelations)
{
- List *resultRelations = plannedstmt->resultRelations;
- int numResultRelations = list_length(resultRelations);
- ResultRelInfo *resultRelInfos;
- ResultRelInfo *resultRelInfo;
-
- resultRelInfos = (ResultRelInfo *)
- palloc(numResultRelations * sizeof(ResultRelInfo));
- resultRelInfo = resultRelInfos;
- foreach(l, resultRelations)
- {
- Index resultRelationIndex = lfirst_int(l);
- Relation resultRelation;
-
- resultRelation = ExecGetRangeTableRelation(estate,
- resultRelationIndex);
- InitResultRelInfo(resultRelInfo,
- resultRelation,
- resultRelationIndex,
- NULL,
- estate->es_instrument);
- resultRelInfo++;
- }
- estate->es_result_relations = resultRelInfos;
- estate->es_num_result_relations = numResultRelations;
+ ExecInitResultRelationsArray(estate);
/* es_result_relation_info is NULL except when within ModifyTable */
estate->es_result_relation_info = NULL;
-
- /*
- * In the partitioned result relation case, also build ResultRelInfos
- * for all the partitioned table roots, because we will need them to
- * fire statement-level triggers, if any.
- */
- if (plannedstmt->rootResultRelations)
- {
- int num_roots = list_length(plannedstmt->rootResultRelations);
-
- resultRelInfos = (ResultRelInfo *)
- palloc(num_roots * sizeof(ResultRelInfo));
- resultRelInfo = resultRelInfos;
- foreach(l, plannedstmt->rootResultRelations)
- {
- Index resultRelIndex = lfirst_int(l);
- Relation resultRelDesc;
-
- resultRelDesc = ExecGetRangeTableRelation(estate,
- resultRelIndex);
- InitResultRelInfo(resultRelInfo,
- resultRelDesc,
- resultRelIndex,
- NULL,
- estate->es_instrument);
- resultRelInfo++;
- }
-
- estate->es_root_result_relations = resultRelInfos;
- estate->es_num_root_result_relations = num_roots;
- }
- else
- {
- estate->es_root_result_relations = NULL;
- estate->es_num_root_result_relations = 0;
- }
- }
- else
- {
- /*
- * if no result relation, then set state appropriately
- */
- estate->es_result_relations = NULL;
- estate->es_num_result_relations = 0;
- estate->es_result_relation_info = NULL;
- estate->es_root_result_relations = NULL;
- estate->es_num_root_result_relations = 0;
}
/*
@@ -1334,8 +1261,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
*
* Most of the time, triggers are fired on one of the result relations of the
* query, and so we can just return a member of the es_result_relations array,
- * or the es_root_result_relations array (if any), or the
- * es_tuple_routing_result_relations list (if any). (Note: in self-join
+ * or the es_tuple_routing_result_relations list (if any). (Note: in self-join
* situations there might be multiple members with the same OID; if so it
* doesn't matter which one we pick.)
*
@@ -1352,30 +1278,16 @@ ResultRelInfo *
ExecGetTriggerResultRel(EState *estate, Oid relid)
{
ResultRelInfo *rInfo;
- int nr;
ListCell *l;
Relation rel;
MemoryContext oldcontext;
/* First, search through the query result relations */
- rInfo = estate->es_result_relations;
- nr = estate->es_num_result_relations;
- while (nr > 0)
- {
- if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
- return rInfo;
- rInfo++;
- nr--;
- }
- /* Second, search through the root result relations, if any */
- rInfo = estate->es_root_result_relations;
- nr = estate->es_num_root_result_relations;
- while (nr > 0)
+ foreach(l, estate->es_opened_result_relations)
{
+ rInfo = lfirst(l);
if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
return rInfo;
- rInfo++;
- nr--;
}
/*
@@ -1512,9 +1424,6 @@ ExecPostprocessPlan(EState *estate)
static void
ExecEndPlan(PlanState *planstate, EState *estate)
{
- ResultRelInfo *resultRelInfo;
- Index num_relations;
- Index i;
ListCell *l;
/*
@@ -1540,30 +1449,52 @@ ExecEndPlan(PlanState *planstate, EState *estate)
*/
ExecResetTupleTable(estate->es_tupleTable, false);
+ /* Close indexes of result relation(s), if any. */
+ ExecCloseResultRelations(estate);
+
+ /*
+ * close whatever rangetable Relations have been opened. We do not
+ * release any locks we might hold on those rels.
+ */
+ ExecCloseRangeTableRelations(estate);
+
+ /* likewise close any trigger target relations */
+ ExecCleanUpTriggerState(estate);
+}
+
+/*
+ * ExecCloseResultRelations
+ */
+void
+ExecCloseResultRelations(EState *estate)
+{
+ ListCell *l;
+
/*
* close indexes of result relation(s) if any. (Rels themselves get
* closed next.)
*/
- resultRelInfo = estate->es_result_relations;
- for (i = estate->es_num_result_relations; i > 0; i--)
+ foreach(l, estate->es_opened_result_relations)
{
+ ResultRelInfo *resultRelInfo = lfirst(l);
+
ExecCloseIndices(resultRelInfo);
- resultRelInfo++;
}
+}
- /*
- * close whatever rangetable Relations have been opened. We do not
- * release any locks we might hold on those rels.
- */
- num_relations = estate->es_range_table_size;
- for (i = 0; i < num_relations; i++)
+/*
+ * ExecCloseRangeTableRelations
+ */
+void
+ExecCloseRangeTableRelations(EState *estate)
+{
+ int i;
+
+ for (i = 0; i < estate->es_range_table_size; i++)
{
if (estate->es_relations[i])
table_close(estate->es_relations[i], NoLock);
}
-
- /* likewise close any trigger target relations */
- ExecCleanUpTriggerState(estate);
}
/* ----------------------------------------------------------------
@@ -2758,17 +2689,9 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
/*
* Child EPQ EStates share the parent's copy of unchanging state such as
- * the snapshot, rangetable, result-rel info, and external Param info.
+ * the snapshot, rangetable, and external Param info.
* They need their own copies of local state, including a tuple table,
- * es_param_exec_vals, etc.
- *
- * The ResultRelInfo array management is trickier than it looks. We
- * create fresh arrays for the child but copy all the content from the
- * parent. This is because it's okay for the child to share any
- * per-relation state the parent has already created --- but if the child
- * sets up any ResultRelInfo fields, such as its own junkfilter, that
- * state must *not* propagate back to the parent. (For one thing, the
- * pointed-to data is in a memory context that won't last long enough.)
+ * es_param_exec_vals, result-rel info, etc.
*/
rcestate->es_direction = ForwardScanDirection;
rcestate->es_snapshot = parentestate->es_snapshot;
@@ -2781,30 +2704,12 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
rcestate->es_plannedstmt = parentestate->es_plannedstmt;
rcestate->es_junkFilter = parentestate->es_junkFilter;
rcestate->es_output_cid = parentestate->es_output_cid;
- if (parentestate->es_num_result_relations > 0)
- {
- int numResultRelations = parentestate->es_num_result_relations;
- int numRootResultRels = parentestate->es_num_root_result_relations;
- ResultRelInfo *resultRelInfos;
-
- resultRelInfos = (ResultRelInfo *)
- palloc(numResultRelations * sizeof(ResultRelInfo));
- memcpy(resultRelInfos, parentestate->es_result_relations,
- numResultRelations * sizeof(ResultRelInfo));
- rcestate->es_result_relations = resultRelInfos;
- rcestate->es_num_result_relations = numResultRelations;
-
- /* Also transfer partitioned root result relations. */
- if (numRootResultRels > 0)
- {
- resultRelInfos = (ResultRelInfo *)
- palloc(numRootResultRels * sizeof(ResultRelInfo));
- memcpy(resultRelInfos, parentestate->es_root_result_relations,
- numRootResultRels * sizeof(ResultRelInfo));
- rcestate->es_root_result_relations = resultRelInfos;
- rcestate->es_num_root_result_relations = numRootResultRels;
- }
- }
+ /*
+ * ResultRelInfos needed by subplans are initialized from scratch when
+ * the subplans themselves are initialized.
+ */
+ if (parentestate->es_result_relations)
+ ExecInitResultRelationsArray(rcestate);
/* es_result_relation_info must NOT be copied */
/* es_trig_target_relations must NOT be copied */
rcestate->es_top_eflags = parentestate->es_top_eflags;
@@ -2954,6 +2859,8 @@ EvalPlanQualEnd(EPQState *epqstate)
ExecEndNode(subplanstate);
}
+ ExecCloseResultRelations(estate);
+
/* throw away the per-estate tuple table, some node may have used it */
ExecResetTupleTable(estate->es_tupleTable, false);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index d0e65b8647..169dd925ad 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -124,14 +124,8 @@ CreateExecutorState(void)
estate->es_output_cid = (CommandId) 0;
estate->es_result_relations = NULL;
- estate->es_num_result_relations = 0;
estate->es_result_relation_info = NULL;
-
- estate->es_root_result_relations = NULL;
- estate->es_num_root_result_relations = 0;
-
estate->es_tuple_routing_result_relations = NIL;
-
estate->es_trig_target_relations = NIL;
estate->es_param_list_info = NULL;
@@ -711,16 +705,10 @@ ExecCreateScanSlotFromOuterPlan(EState *estate,
bool
ExecRelationIsTargetRelation(EState *estate, Index scanrelid)
{
- ResultRelInfo *resultRelInfos;
- int i;
-
- resultRelInfos = estate->es_result_relations;
- for (i = 0; i < estate->es_num_result_relations; i++)
- {
- if (resultRelInfos[i].ri_RangeTableIndex == scanrelid)
- return true;
- }
- return false;
+ return list_member_int(estate->es_plannedstmt->resultRelations,
+ scanrelid) ||
+ list_member_int(estate->es_plannedstmt->rootResultRelations,
+ scanrelid);
}
/* ----------------------------------------------------------------
@@ -779,8 +767,8 @@ ExecInitRangeTable(EState *estate, List *rangeTable)
palloc0(estate->es_range_table_size * sizeof(Relation));
/*
- * es_rowmarks is also parallel to the es_range_table, but it's allocated
- * only if needed.
+ * es_result_relations and es_rowmarks are also parallel to es_range_table,
+ * but are only allocated if needed.
*/
estate->es_rowmarks = NULL;
}
@@ -835,6 +823,55 @@ ExecGetRangeTableRelation(EState *estate, Index rti)
return rel;
}
+/*
+ * ExecInitResultRelationsArray
+ * Allocate space to hold ResultRelInfo pointers of result relations
+ *
+ * Although not relations in the range table may be result relations, we
+ * allocate that many pointers, because that allows to access individual
+ * entries by RT index (minus 1 to be accurate), which is convenient.
+ */
+void
+ExecInitResultRelationsArray(EState *estate)
+{
+ /*
+ * Individual pointers are assigned when ExecInitResultRelation() is
+ * called one-by-one for each result relation.
+ */
+ estate->es_result_relations = (ResultRelInfo **)
+ palloc0(estate->es_range_table_size * sizeof(ResultRelInfo *));
+}
+
+/*
+ * ExecInitResultRelation
+ * Open result relation given by the passed-in RT index and fill its
+ * ResultRelInfo node
+ *
+ * Here, we also save the ResultRelInfo in estate->es_result_relations array
+ * such that it can be accessed later using the RT index.
+ */
+void
+ExecInitResultRelation(EState *estate, ResultRelInfo *resultRelInfo,
+ Index rti)
+{
+ Relation resultRelationDesc;
+
+ resultRelationDesc = ExecGetRangeTableRelation(estate, rti);
+ InitResultRelInfo(resultRelInfo,
+ resultRelationDesc,
+ rti,
+ NULL,
+ estate->es_instrument);
+ estate->es_result_relations[rti - 1] = resultRelInfo;
+
+ /*
+ * Saving in the list allows to avoid needlessly traversing the whole
+ * array when only a few of its entries are possibly non-NULL.
+ */
+ estate->es_opened_result_relations =
+ lappend(estate->es_opened_result_relations, resultRelInfo);
+}
+
/*
* UpdateChangedParamSet
* Add changed parameters to a plan node's chgParam set
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 9812089161..9b27d34ba5 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2301,7 +2301,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
ResultRelInfo *saved_resultRelInfo;
ResultRelInfo *resultRelInfo;
Plan *subplan;
- ListCell *l;
+ ListCell *l,
+ *l1;
int i;
Relation rel;
bool update_tuple_routing_needed = node->partColsUpdated;
@@ -2322,13 +2323,17 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate->mt_done = false;
mtstate->mt_plans = (PlanState **) palloc0(sizeof(PlanState *) * nplans);
- mtstate->resultRelInfo = estate->es_result_relations + node->resultRelIndex;
+ mtstate->resultRelInfo = (ResultRelInfo *)
+ palloc(nplans * sizeof(ResultRelInfo));
mtstate->mt_scans = (TupleTableSlot **) palloc0(sizeof(TupleTableSlot *) * nplans);
/* If modifying a partitioned table, initialize the root table info */
- if (node->rootResultRelIndex >= 0)
- mtstate->rootResultRelInfo = estate->es_root_result_relations +
- node->rootResultRelIndex;
+ if (node->rootRelation > 0)
+ {
+ mtstate->rootResultRelInfo = makeNode(ResultRelInfo);
+ ExecInitResultRelation(estate, mtstate->rootResultRelInfo,
+ node->rootRelation);
+ }
mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans);
mtstate->mt_nplans = nplans;
@@ -2351,9 +2356,14 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
resultRelInfo = mtstate->resultRelInfo;
i = 0;
- foreach(l, node->plans)
+ forboth(l, node->resultRelations, l1, node->plans)
{
- subplan = (Plan *) lfirst(l);
+ Index resultRelation = lfirst_int(l);
+
+ subplan = (Plan *) lfirst(l1);
+
+ /* This opens result relation and fills ResultRelInfo. */
+ ExecInitResultRelation(estate, resultRelInfo, resultRelation);
/* Initialize the usesFdwDirectModify flag */
resultRelInfo->ri_usesFdwDirectModify = bms_is_member(i,
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 0409a40b82..7974fa01e6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -207,8 +207,6 @@ _copyModifyTable(const ModifyTable *from)
COPY_SCALAR_FIELD(rootRelation);
COPY_SCALAR_FIELD(partColsUpdated);
COPY_NODE_FIELD(resultRelations);
- COPY_SCALAR_FIELD(resultRelIndex);
- COPY_SCALAR_FIELD(rootResultRelIndex);
COPY_NODE_FIELD(plans);
COPY_NODE_FIELD(withCheckOptionLists);
COPY_NODE_FIELD(returningLists);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f0386480ab..f35d2a04a7 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -408,8 +408,6 @@ _outModifyTable(StringInfo str, const ModifyTable *node)
WRITE_UINT_FIELD(rootRelation);
WRITE_BOOL_FIELD(partColsUpdated);
WRITE_NODE_FIELD(resultRelations);
- WRITE_INT_FIELD(resultRelIndex);
- WRITE_INT_FIELD(rootResultRelIndex);
WRITE_NODE_FIELD(plans);
WRITE_NODE_FIELD(withCheckOptionLists);
WRITE_NODE_FIELD(returningLists);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 42050ab719..73b8d9d7f6 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1639,8 +1639,6 @@ _readModifyTable(void)
READ_UINT_FIELD(rootRelation);
READ_BOOL_FIELD(partColsUpdated);
READ_NODE_FIELD(resultRelations);
- READ_INT_FIELD(resultRelIndex);
- READ_INT_FIELD(rootResultRelIndex);
READ_NODE_FIELD(plans);
READ_NODE_FIELD(withCheckOptionLists);
READ_NODE_FIELD(returningLists);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 3d7a4e373f..881eaf4813 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -6808,8 +6808,6 @@ make_modifytable(PlannerInfo *root,
node->rootRelation = rootRelation;
node->partColsUpdated = partColsUpdated;
node->resultRelations = resultRelations;
- node->resultRelIndex = -1; /* will be set correctly in setrefs.c */
- node->rootResultRelIndex = -1; /* will be set correctly in setrefs.c */
node->plans = subplans;
if (!onconflict)
{
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index dd8e2e966d..66372d30fe 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -975,24 +975,18 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
/*
* Append this ModifyTable node's final result relation RT
- * index(es) to the global list for the plan, and set its
- * resultRelIndex to reflect their starting position in the
- * global list.
+ * index(es) to the global list for the plan.
*/
- splan->resultRelIndex = list_length(root->glob->resultRelations);
root->glob->resultRelations =
list_concat(root->glob->resultRelations,
splan->resultRelations);
/*
* If the main target relation is a partitioned table, also
- * add the partition root's RT index to rootResultRelations,
- * and remember its index in that list in rootResultRelIndex.
+ * add the partition root's RT index to rootResultRelations.
*/
if (splan->rootRelation)
{
- splan->rootResultRelIndex =
- list_length(root->glob->rootResultRelations);
root->glob->rootResultRelations =
lappend_int(root->glob->rootResultRelations,
splan->rootRelation);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9c6fdeeb56..77f71e52d4 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -355,12 +355,12 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
rte->relkind = rel->localrel->rd_rel->relkind;
rte->rellockmode = AccessShareLock;
ExecInitRangeTable(estate, list_make1(rte));
+ ExecInitResultRelationsArray(estate);
resultRelInfo = makeNode(ResultRelInfo);
InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
- estate->es_result_relations = resultRelInfo;
- estate->es_num_result_relations = 1;
+ estate->es_result_relations[0] = resultRelInfo;
estate->es_result_relation_info = resultRelInfo;
estate->es_output_cid = GetCurrentCommandId(true);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 415e117407..54455e1446 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -538,6 +538,9 @@ extern bool ExecRelationIsTargetRelation(EState *estate, Index scanrelid);
extern Relation ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags);
extern void ExecInitRangeTable(EState *estate, List *rangeTable);
+extern void ExecInitResultRelationsArray(EState *estate);
+extern void ExecCloseRangeTableRelations(EState *estate);
+extern void ExecCloseResultRelations(EState *estate);
static inline RangeTblEntry *
exec_rt_fetch(Index rti, EState *estate)
@@ -546,6 +549,8 @@ exec_rt_fetch(Index rti, EState *estate)
}
extern Relation ExecGetRangeTableRelation(EState *estate, Index rti);
+extern void ExecInitResultRelation(EState *estate, ResultRelInfo *resultRelInfo,
+ Index rti);
extern int executor_errposition(EState *estate, int location);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index ef448d67c7..7eb5ca6a04 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -508,6 +508,14 @@ typedef struct EState
Index es_range_table_size; /* size of the range table arrays */
Relation *es_relations; /* Array of per-range-table-entry Relation
* pointers, or NULL if not yet opened */
+ ResultRelInfo **es_result_relations; /* Array of Per-range-table-entry
+ * ResultRelInfo pointers, or
+ * NULL if a given range table
+ * relation not a target
+ * table */
+ List *es_opened_result_relations; /* List of non-NULL entries
+ * in es_result_relations added
+ * in no specific order */
struct ExecRowMark **es_rowmarks; /* Array of per-range-table-entry
* ExecRowMarks, or NULL if none */
PlannedStmt *es_plannedstmt; /* link to top of plan tree */
@@ -518,24 +526,13 @@ typedef struct EState
/* If query can insert/delete tuples, the command ID to mark them with */
CommandId es_output_cid;
- /* Info about target table(s) for insert/update/delete queries: */
- ResultRelInfo *es_result_relations; /* array of ResultRelInfos */
- int es_num_result_relations; /* length of array */
ResultRelInfo *es_result_relation_info; /* currently active array elt */
- /*
- * Info about the partition root table(s) for insert/update/delete queries
- * targeting partitioned tables. Only leaf partitions are mentioned in
- * es_result_relations, but we need access to the roots for firing
- * triggers and for runtime tuple routing.
- */
- ResultRelInfo *es_root_result_relations; /* array of ResultRelInfos */
- int es_num_root_result_relations; /* length of the array */
PartitionDirectory es_partition_directory; /* for PartitionDesc lookup */
/*
* The following list contains ResultRelInfos created by the tuple routing
- * code for partitions that don't already have one.
+ * code for partitions that aren't found in es_result_relations_array.
*/
List *es_tuple_routing_result_relations;
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 83e01074ed..06665468a5 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -224,8 +224,6 @@ typedef struct ModifyTable
Index rootRelation; /* Root RT index, if target is partitioned */
bool partColsUpdated; /* some part key in hierarchy updated */
List *resultRelations; /* integer list of RT indexes */
- int resultRelIndex; /* index of first resultRel in plan's list */
- int rootResultRelIndex; /* index of the partitioned table root */
List *plans; /* plan(s) producing source data */
List *withCheckOptionLists; /* per-target-table WCO lists */
List *returningLists; /* per-target-table RETURNING tlists */
--
2.20.1
>From 807dde3d299a2d4ae89d96ea7422d0feb9c58e4b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 9 Oct 2020 22:09:08 +0300
Subject: [PATCH 2/2] Merge ExecCleanUpTriggerState() and
ExecCloseResultRelations().
And some other kibitzing.
---
src/backend/commands/copy.c | 9 ++-
src/backend/commands/explain.c | 2 +-
src/backend/commands/trigger.c | 2 +-
src/backend/executor/execMain.c | 83 ++++++++++--------------
src/backend/executor/execUtils.c | 7 +-
src/backend/replication/logical/worker.c | 6 +-
src/include/executor/executor.h | 1 -
src/include/nodes/execnodes.h | 17 ++---
8 files changed, 59 insertions(+), 68 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 6948214334..bfdd366139 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2727,6 +2727,7 @@ CopyFrom(CopyState cstate)
bool leafpart_use_multi_insert = false;
Assert(cstate->rel);
+ Assert(list_length(cstate->range_table) == 1);
/*
* The target must be a plain, foreign, or partitioned relation, or have
@@ -3352,15 +3353,13 @@ CopyFrom(CopyState cstate)
if (insertMethod != CIM_SINGLE)
CopyMultiInsertInfoCleanup(&multiInsertInfo);
- ExecCloseResultRelations(estate);
- ExecCloseRangeTableRelations(estate);
-
/* Close all the partitioned tables, leaf partitions, and their indices */
if (proute)
ExecCleanupTupleRouting(mtstate, proute);
- /* Close any trigger target relations */
- ExecCleanUpTriggerState(estate);
+ /* Close the result relations */
+ ExecCloseResultRelations(estate);
+ ExecCloseRangeTableRelations(estate);
FreeExecutorState(estate);
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 3210f90bd1..1cc47b0e77 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -786,7 +786,7 @@ ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc)
routerels != NIL || targrels != NIL);
foreach(l, resultrels)
{
- rInfo = lfirst(l);
+ rInfo = (ResultRelInfo *) lfirst(l);
report_triggers(rInfo, show_relname, es);
}
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 672fccff5b..3b4fbdadf4 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -4227,7 +4227,7 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events,
if (local_estate)
{
- ExecCleanUpTriggerState(estate);
+ ExecCloseResultRelations(estate);
ExecResetTupleTable(estate->es_tupleTable, false);
FreeExecutorState(estate);
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index f62fd8f7aa..403d62f2b6 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1340,35 +1340,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
return rInfo;
}
-/*
- * Close any relations that have been opened by ExecGetTriggerResultRel().
- */
-void
-ExecCleanUpTriggerState(EState *estate)
-{
- ListCell *l;
-
- foreach(l, estate->es_trig_target_relations)
- {
- ResultRelInfo *resultRelInfo = (ResultRelInfo *) lfirst(l);
-
- /*
- * Assert this is a "dummy" ResultRelInfo, see above. Otherwise we
- * might be issuing a duplicate close against a Relation opened by
- * ExecGetRangeTableRelation.
- */
- Assert(resultRelInfo->ri_RangeTableIndex == 0);
-
- /*
- * Since ExecGetTriggerResultRel doesn't call ExecOpenIndices for
- * these rels, we needn't call ExecCloseIndices either.
- */
- Assert(resultRelInfo->ri_NumIndices == 0);
-
- table_close(resultRelInfo->ri_RelationDesc, NoLock);
- }
-}
-
/* ----------------------------------------------------------------
* ExecPostprocessPlan
*
@@ -1449,7 +1420,7 @@ ExecEndPlan(PlanState *planstate, EState *estate)
*/
ExecResetTupleTable(estate->es_tupleTable, false);
- /* Close indexes of result relation(s), if any. */
+ /* Close result relation(s), if any. */
ExecCloseResultRelations(estate);
/*
@@ -1457,22 +1428,19 @@ ExecEndPlan(PlanState *planstate, EState *estate)
* release any locks we might hold on those rels.
*/
ExecCloseRangeTableRelations(estate);
-
- /* likewise close any trigger target relations */
- ExecCleanUpTriggerState(estate);
}
/*
- * ExecCloseResultRelations
+ * Close any relations that have been opened for ResultRelInfos.
*/
void
ExecCloseResultRelations(EState *estate)
{
- ListCell *l;
+ ListCell *l;
/*
- * close indexes of result relation(s) if any. (Rels themselves get
- * closed next.)
+ * close indexes of result relation(s) if any. (Rels themselves are
+ * closed in ExecCloseRangeTableRelations())
*/
foreach(l, estate->es_opened_result_relations)
{
@@ -1480,15 +1448,36 @@ ExecCloseResultRelations(EState *estate)
ExecCloseIndices(resultRelInfo);
}
+
+ /* Close any relations that have been opened by ExecGetTriggerResultRel(). */
+ foreach(l, estate->es_trig_target_relations)
+ {
+ ResultRelInfo *resultRelInfo = (ResultRelInfo *) lfirst(l);
+
+ /*
+ * Assert this is a "dummy" ResultRelInfo, see above. Otherwise we
+ * might be issuing a duplicate close against a Relation opened by
+ * ExecGetRangeTableRelation.
+ */
+ Assert(resultRelInfo->ri_RangeTableIndex == 0);
+
+ /*
+ * Since ExecGetTriggerResultRel doesn't call ExecOpenIndices for
+ * these rels, we needn't call ExecCloseIndices either.
+ */
+ Assert(resultRelInfo->ri_NumIndices == 0);
+
+ table_close(resultRelInfo->ri_RelationDesc, NoLock);
+ }
}
/*
- * ExecCloseRangeTableRelations
+ * Close all relations opened by ExecGetRangeTableRelation()
*/
void
ExecCloseRangeTableRelations(EState *estate)
{
- int i;
+ int i;
for (i = 0; i < estate->es_range_table_size; i++)
{
@@ -2689,9 +2678,9 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
/*
* Child EPQ EStates share the parent's copy of unchanging state such as
- * the snapshot, rangetable, and external Param info.
- * They need their own copies of local state, including a tuple table,
- * es_param_exec_vals, result-rel info, etc.
+ * the snapshot, rangetable, and external Param info. They need their own
+ * copies of local state, including a tuple table, es_param_exec_vals,
+ * result-rel info, etc.
*/
rcestate->es_direction = ForwardScanDirection;
rcestate->es_snapshot = parentestate->es_snapshot;
@@ -2704,9 +2693,10 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
rcestate->es_plannedstmt = parentestate->es_plannedstmt;
rcestate->es_junkFilter = parentestate->es_junkFilter;
rcestate->es_output_cid = parentestate->es_output_cid;
+
/*
- * ResultRelInfos needed by subplans are initialized from scratch when
- * the subplans themselves are initialized.
+ * ResultRelInfos needed by subplans are initialized from scratch when the
+ * subplans themselves are initialized.
*/
if (parentestate->es_result_relations)
ExecInitResultRelationsArray(rcestate);
@@ -2859,13 +2849,10 @@ EvalPlanQualEnd(EPQState *epqstate)
ExecEndNode(subplanstate);
}
- ExecCloseResultRelations(estate);
-
/* throw away the per-estate tuple table, some node may have used it */
ExecResetTupleTable(estate->es_tupleTable, false);
- /* close any trigger target relations attached to this EState */
- ExecCleanUpTriggerState(estate);
+ ExecCloseResultRelations(estate);
MemoryContextSwitchTo(oldcontext);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 169dd925ad..eda84bca45 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -770,6 +770,7 @@ ExecInitRangeTable(EState *estate, List *rangeTable)
* es_result_relations and es_rowmarks are also parallel to es_range_table,
* but are only allocated if needed.
*/
+ estate->es_result_relations = NULL;
estate->es_rowmarks = NULL;
}
@@ -827,9 +828,9 @@ ExecGetRangeTableRelation(EState *estate, Index rti)
* ExecInitResultRelationsArray
* Allocate space to hold ResultRelInfo pointers of result relations
*
- * Although not relations in the range table may be result relations, we
- * allocate that many pointers, because that allows to access individual
- * entries by RT index (minus 1 to be accurate), which is convenient.
+ * Usually, only some relations in the range table are result relations, but
+ * we allocate an array with the same size as the range table, so that we
+ * can index it by the RT index (minus 1 to be accurate).
*/
void
ExecInitResultRelationsArray(EState *estate)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 77f71e52d4..07889f7a7b 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -357,9 +357,13 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
ExecInitRangeTable(estate, list_make1(rte));
ExecInitResultRelationsArray(estate);
+ /*
+ * Initialize a ResultRelInfo for the target relation. Note that we
+ * intentionally don't add it to the es_opened_result_relations list,
+ * because we do our own cleanup and don't use ExecCloseResultRelations().
+ */
resultRelInfo = makeNode(ResultRelInfo);
InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
-
estate->es_result_relations[0] = resultRelInfo;
estate->es_result_relation_info = resultRelInfo;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 54455e1446..76ef5cd91c 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -191,7 +191,6 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
-extern void ExecCleanUpTriggerState(EState *estate);
extern void ExecConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
extern bool ExecPartitionCheck(ResultRelInfo *resultRelInfo,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 7eb5ca6a04..b6500abc24 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -508,14 +508,6 @@ typedef struct EState
Index es_range_table_size; /* size of the range table arrays */
Relation *es_relations; /* Array of per-range-table-entry Relation
* pointers, or NULL if not yet opened */
- ResultRelInfo **es_result_relations; /* Array of Per-range-table-entry
- * ResultRelInfo pointers, or
- * NULL if a given range table
- * relation not a target
- * table */
- List *es_opened_result_relations; /* List of non-NULL entries
- * in es_result_relations added
- * in no specific order */
struct ExecRowMark **es_rowmarks; /* Array of per-range-table-entry
* ExecRowMarks, or NULL if none */
PlannedStmt *es_plannedstmt; /* link to top of plan tree */
@@ -526,6 +518,15 @@ typedef struct EState
/* If query can insert/delete tuples, the command ID to mark them with */
CommandId es_output_cid;
+ /* Info about target table(s) for insert/update/delete queries: */
+ ResultRelInfo **es_result_relations; /* Array of Per-range-table-entry
+ * ResultRelInfo pointers, or
+ * NULL if a given range table
+ * relation not a target
+ * table */
+ List *es_opened_result_relations; /* List of non-NULL entries
+ * in es_result_relations added
+ * in no specific order */
ResultRelInfo *es_result_relation_info; /* currently active array elt */
PartitionDirectory es_partition_directory; /* for PartitionDesc lookup */
--
2.20.1
