On Fri, 8 Apr 2022 at 17:49, Amit Langote <amitlangot...@gmail.com> wrote:
> Attached updated patch with these changes.
Thanks for making the changes. I started looking over this patch but
really feel like it needs quite a few more iterations of what we've
just been doing to get it into proper committable shape. There seems
to be only about 40 mins to go before the freeze, so it seems very
unrealistic that it could be made to work.
I started trying to take a serious look at it this evening, but I feel
like I just failed to get into it deep enough to make any meaningful
improvements. I'd need more time to study the problem before I could
build up a proper opinion on how exactly I think it should work.
Anyway. I've attached a small patch that's just a few things I
adjusted or questions while reading over your v13 patch. Some of
these are just me questioning your code (See XXX comments) and some I
think are improvements. Feel free to take the hunks that you see fit
and drop anything you don't.
David
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 05cc99df8f..5ee978937d 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -121,6 +121,8 @@ static void EvalPlanQualStart(EPQState *epqstate, Plan
*planTree);
*
* Note: Partitioned tables mentioned in PartitionedRelPruneInfo nodes that
* drive the pruning will be locked before doing the pruning.
+ *
+ * ----------------------------------------------------------------
*/
PartitionPruneResult *
ExecutorDoInitialPruning(PlannedStmt *plannedstmt, ParamListInfo params)
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 3037742b8d..e9ca6bc55f 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1707,6 +1707,7 @@ ExecInitPartitionPruning(PlanState *planstate,
Assert(n_total_subplans > 0);
*initially_valid_subplans = bms_add_range(NULL, 0,
n_total_subplans - 1);
+ return prunestate;
}
/*
@@ -1714,14 +1715,15 @@ ExecInitPartitionPruning(PlanState *planstate,
* that were removed above due to initial pruning. No need to do this
if
* no steps were removed.
*/
- if (bms_num_members(*initially_valid_subplans) < n_total_subplans)
+ if (prunestate &&
+ bms_num_members(*initially_valid_subplans) < n_total_subplans)
{
/*
* We can safely skip this when !do_exec_prune, even though that
* leaves invalid data in prunestate, because that data won't be
* consulted again (cf initial Assert in
ExecFindMatchingSubPlans).
*/
- if (prunestate && prunestate->do_exec_prune)
+ if (prunestate->do_exec_prune)
PartitionPruneFixSubPlanMap(prunestate,
*initially_valid_subplans,
n_total_subplans);
@@ -1751,7 +1753,8 @@ ExecPartitionDoInitialPruning(PlannedStmt *plannedstmt,
ParamListInfo params,
Bitmapset *valid_subplan_offs;
/*
- * A temporary context to allocate stuff needded to run the pruning
steps.
+ * A temporary context to for memory allocations required while
execution
+ * partition pruning steps.
*/
tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
"initial pruning working data",
@@ -1765,11 +1768,12 @@ ExecPartitionDoInitialPruning(PlannedStmt *plannedstmt,
ParamListInfo params,
pdir = CreatePartitionDirectory(CurrentMemoryContext, false);
/*
- * We don't yet have a PlanState for the parent plan node, so must
create
- * a standalone ExprContext to evaluate pruning expressions, equipped
with
- * the information about the EXTERN parameters that the caller passed
us.
- * Note that that's okay because the initial pruning steps do not
contain
- * anything that requires the execution to have started.
+ * We don't yet have a PlanState for the parent plan node, so we must
+ * create a standalone ExprContext to evaluate pruning expressions,
+ * equipped with the information about the EXTERN parameters that the
+ * caller passed us. Note that that's okay because the initial pruning
+ * steps do not contain anything that requires the execution to have
+ * started.
*/
econtext = CreateStandaloneExprContext();
econtext->ecxt_param_list_info = params;
@@ -1874,7 +1878,6 @@ CreatePartitionPruneState(PlanState *planstate,
PartitionedRelPruneInfo *pinfo =
lfirst_node(PartitionedRelPruneInfo, lc2);
PartitionedRelPruningData *pprune =
&prunedata->partrelprunedata[j];
Relation partrel;
- bool close_partrel = false;
PartitionDesc partdesc;
PartitionKey partkey;
@@ -1894,7 +1897,6 @@ CreatePartitionPruneState(PlanState *planstate,
int lockmode = (j == 0) ? NoLock :
rte->rellockmode;
partrel = table_open(rte->relid, lockmode);
- close_partrel = true;
}
else
partrel = ExecGetRangeTableRelation(estate,
pinfo->rtindex);
@@ -1914,7 +1916,7 @@ CreatePartitionPruneState(PlanState *planstate,
* Must close partrel, keeping the lock taken, if we're
not using
* EState's entry.
*/
- if (close_partrel)
+ if (estate == NULL)
table_close(partrel, NoLock);
/*
@@ -2367,6 +2369,8 @@ find_matching_subplans_recurse(PartitionPruningData
*prunedata,
{
*validsubplans = bms_add_member(*validsubplans,
pprune->subplan_map[i]);
+ /* XXX why would pprune->rti_map[i] ever be zero
here??? */
+ Assert(pprune->rti_map[i] > 0);
if (scan_leafpart_rtis && pprune->rti_map[i] > 0)
*scan_leafpart_rtis =
bms_add_member(*scan_leafpart_rtis,
pprune->rti_map[i]);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 639145abe9..f9c7976ff2 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -119,6 +119,7 @@ CreateExecutorState(void)
estate->es_relations = NULL;
estate->es_rowmarks = NULL;
estate->es_plannedstmt = NULL;
+ estate->es_part_prune_infos = NIL;
estate->es_part_prune_result = NULL;
estate->es_junkFilter = NULL;
diff --git a/src/backend/executor/nodeAppend.c
b/src/backend/executor/nodeAppend.c
index 09f26658e2..96880e122a 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -94,7 +94,6 @@ static bool ExecAppendAsyncRequest(AppendState *node,
TupleTableSlot **result);
static void ExecAppendAsyncEventWait(AppendState *node);
static void classify_matching_subplans(AppendState *node);
-
/* ----------------------------------------------------------------
* ExecInitAppend
*
diff --git a/src/backend/optimizer/plan/createplan.c
b/src/backend/optimizer/plan/createplan.c
index ec6b1f1fc0..fe0df2f1d1 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1184,7 +1184,6 @@ create_append_plan(PlannerInfo *root, AppendPath
*best_path, int flags)
ListCell *subpaths;
int nasyncplans = 0;
RelOptInfo *rel = best_path->path.parent;
- int part_prune_index = -1;
int nodenumsortkeys = 0;
AttrNumber *nodeSortColIdx = NULL;
Oid *nodeSortOperators = NULL;
@@ -1335,6 +1334,9 @@ create_append_plan(PlannerInfo *root, AppendPath
*best_path, int flags)
subplans = lappend(subplans, subplan);
}
+ /* Set below if we find quals that we can use to run-time prune */
+ plan->part_prune_index = -1;
+
/*
* If any quals exist, they may be useful to perform further partition
* pruning during execution. Gather information needed by the executor
to
@@ -1358,18 +1360,15 @@ create_append_plan(PlannerInfo *root, AppendPath
*best_path, int flags)
}
if (prunequal != NIL)
- part_prune_index= make_partition_pruneinfo(root, rel,
-
best_path->subpaths,
-
prunequal);
+ plan->part_prune_index = make_partition_pruneinfo(root,
rel,
+
best_path->subpaths,
+
prunequal);
}
plan->appendplans = subplans;
plan->nasyncplans = nasyncplans;
plan->first_partial_plan = best_path->first_partial_path;
- /* Will be updated later in set_plan_references(). */
- plan->part_prune_index = part_prune_index;
-
copy_generic_path_info(&plan->plan, (Path *) best_path);
/*
@@ -1408,7 +1407,6 @@ create_merge_append_plan(PlannerInfo *root,
MergeAppendPath *best_path,
List *subplans = NIL;
ListCell *subpaths;
RelOptInfo *rel = best_path->path.parent;
- int part_prune_index = -1;
/*
* We don't have the actual creation of the MergeAppend node split out
@@ -1501,6 +1499,9 @@ create_merge_append_plan(PlannerInfo *root,
MergeAppendPath *best_path,
subplans = lappend(subplans, subplan);
}
+ /* Set below if we find quals that we can use to run-time prune */
+ node->part_prune_index = -1;
+
/*
* If any quals exist, they may be useful to perform further partition
* pruning during execution. Gather information needed by the executor
to
@@ -1524,15 +1525,13 @@ create_merge_append_plan(PlannerInfo *root,
MergeAppendPath *best_path,
}
if (prunequal != NIL)
- part_prune_index= make_partition_pruneinfo(root, rel,
-
best_path->subpaths,
-
prunequal);
+ node->part_prune_index = make_partition_pruneinfo(root,
rel,
+
best_path->subpaths,
+
prunequal);
}
node->mergeplans = subplans;
- /* Will be updated later in set_plan_references(). */
- node->part_prune_index = part_prune_index;
/*
* If prepare_sort_from_pathkeys added sort columns, but we were told to
diff --git a/src/backend/optimizer/plan/setrefs.c
b/src/backend/optimizer/plan/setrefs.c
index c88e5bacac..63ec8a98fc 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -408,6 +408,13 @@ set_plan_references(PlannerInfo *root, Plan *plan)
glob->partPruneInfos = lappend(glob->partPruneInfos, pruneinfo);
}
+ /*
+ * XXX is it worth doing a bms_copy() on glob->minLockRelids if
+ * glob->containsInitialPruning is true?. I'm slighly worried that the
+ * Bitmapset could have a very long empty tail resulting in excessive
+ * looping during AcquireExecutorLocks().
+ */
+
return result;
}
diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index 5a5f5dee46..952c5b8327 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -212,12 +212,12 @@ static void partkey_datum_from_expr(PartitionPruneContext
*context,
/*
* make_partition_pruneinfo
* Checks if the given set of quals can be used to build pruning
steps
- * that the executor will use to prune useless ones from given set
of
- * child paths, and if so builds a PartitionPruneInfo that will
allow the
- * executor to do do and append it to root->partPruneInfos.
+ * that the executor can use to prune away unneeded partitions. If
+ * suitable quals are found then a PartitionPruneInfo is built and
tagged
+ * onto the PlannerInfo's partPruneInfos list.
*
- * Return value is 0-based index of the added PartitionPruneInfo or -1 if one
- * was not built after all.
+ * The return value is the 0-based index of the item added to the
+ * partPruneInfos list or -1 if nothing was added.
*
* 'parentrel' is the RelOptInfo for an appendrel, and 'subpaths' is the list
* of scan paths for its child rels.
@@ -335,10 +335,9 @@ make_partition_pruneinfo(PlannerInfo *root, RelOptInfo
*parentrel,
allmatchedsubplans = bms_join(matchedsubplans,
allmatchedsubplans);
}
- if (!needs_init_pruning)
- needs_init_pruning = partrel_needs_init_pruning;
- if (!needs_exec_pruning)
- needs_exec_pruning = partrel_needs_exec_pruning;
+
+ needs_init_pruning |= partrel_needs_init_pruning;
+ needs_exec_pruning |= partrel_needs_exec_pruning;
}
pfree(relid_subplan_map);
@@ -570,7 +569,7 @@ make_partitionedrel_pruneinfo(PlannerInfo *root, RelOptInfo
*parentrel,
* that would require per-scan pruning.
*
* In the first pass, we note whether the 2nd pass is necessary
by
- * by noting the presence of EXEC parameters.
+ * noting the presence of EXEC parameters.
*/
gen_partprune_steps(subpart, partprunequal, PARTTARGET_INITIAL,
&context);
@@ -645,10 +644,11 @@ make_partitionedrel_pruneinfo(PlannerInfo *root,
RelOptInfo *parentrel,
pinfo->execparamids = execparamids;
/* Remaining fields will be filled in the next loop */
- if (!*needs_init_pruning)
- *needs_init_pruning = (initial_pruning_steps != NIL);
- if (!*needs_exec_pruning)
- *needs_exec_pruning = (exec_pruning_steps != NIL);
+ /* record which types of pruning steps we've seen so far */
+ if (initial_pruning_steps != NIL)
+ *needs_init_pruning = true;
+ if (exec_pruning_steps != NIL)
+ *needs_exec_pruning = true;
pinfolist = lappend(pinfolist, pinfo);
}
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index a627448a5a..8cc2e2162d 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1204,7 +1204,6 @@ PortalRunMulti(Portal portal,
{
bool active_snapshot_set = false;
ListCell *stmtlist_item;
- int i;
/*
* If the destination is DestRemoteExecute, change to DestNone. The
@@ -1225,15 +1224,9 @@ PortalRunMulti(Portal portal,
* Loop to handle the individual queries generated from a single
parsetree
* by analysis and rewrite.
*/
- i = 0;
foreach(stmtlist_item, portal->stmts)
{
PlannedStmt *pstmt = lfirst_node(PlannedStmt, stmtlist_item);
- PartitionPruneResult *part_prune_result =
portal->part_prune_results ?
-
list_nth(portal->part_prune_results, i) :
- NULL;
-
- i++;
/*
* If we got a cancel signal in prior command, quit
@@ -1242,6 +1235,8 @@ PortalRunMulti(Portal portal,
if (pstmt->utilityStmt == NULL)
{
+ PartitionPruneResult *part_prune_result = NULL;
+
/*
* process a plannable query.
*/
@@ -1288,6 +1283,14 @@ PortalRunMulti(Portal portal,
else
UpdateActiveSnapshotCommandId();
+ /*
+ * Determine if there's a corresponding
PartitionPruneResult for
+ * this PlannedStmt.
+ */
+ if (portal->part_prune_results != NIL)
+ part_prune_result =
list_nth(portal->part_prune_results,
+
foreach_current_index(stmtlist_item));
+
if (pstmt->canSetTag)
{
/* statement can set tag string */
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 34975c69ee..bbc8c42d88 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -87,7 +87,8 @@ extern void ExplainOneUtility(Node *utilityStmt, IntoClause
*into,
ExplainState *es,
const char *queryString,
ParamListInfo params,
QueryEnvironment *queryEnv);
-extern void ExplainOnePlan(PlannedStmt *plannedstmt, PartitionPruneResult
*part_prune_resul,
+extern void ExplainOnePlan(PlannedStmt *plannedstmt,
+ PartitionPruneResult
*part_prune_result,
IntoClause *into,
ExplainState *es, const char
*queryString,
ParamListInfo params,
QueryEnvironment *queryEnv,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 43bd293433..a8bf908d63 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1000,11 +1000,11 @@ typedef TupleTableSlot *(*ExecProcNodeMtd) (struct
PlanState *pstate);
*
* This is used by GetCachedPlan() to inform its callers of the pruning
* decisions made when performing AcquireExecutorLocks() on a given cached
- * PlannedStmt, which the callers then pass that on to the executor. The
- * executor refers to this node when made available when initializing the plan
- * nodes to which those PartitionPruneInfos apply so that the same set of
- * qualifying subplans are initialized, rather than deriving that set again by
- * redoing initial pruning.
+ * PlannedStmt, which the callers then pass onto the executor. The executor
+ * refers to this node when made available when initializing the plan nodes to
+ * which those PartitionPruneInfos apply so that the same set of qualifying
+ * subplans are initialized, rather than deriving that set again by redoing
+ * initial pruning.
*/
typedef struct PartitionPruneResult
{
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 550308147d..f8f3971f44 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -274,11 +274,7 @@ typedef struct Append
*/
int first_partial_plan;
- /*
- * Index of this plan's PartitionPruneInfo in PlannedStmt.partPruneInfos
- * to be used for run-time subplan pruning; -1 if run-time pruning is
- * not needed.
- */
+ /* Index to PlannerInfo.partPruneInfos or -1 if no run-time pruning */
int part_prune_index;
} Append;
@@ -299,11 +295,7 @@ typedef struct MergeAppend
Oid *collations; /* OIDs of collations */
bool *nullsFirst; /* NULLS FIRST/LAST directions */
- /*
- * Index of this plan's PartitionPruneInfo in PlannedStmt.partPruneInfos
- * to be used for run-time subplan pruning; -1 if run-time pruning is
- * not needed.
- */
+ /* Index to PlannerInfo.partPruneInfos or -1 if no run-time pruning */
int part_prune_index;
} MergeAppend;
