We implemented Approach B in the attached patch set (patch 0001) and
then implemented Approach A (patch 0002) to sanity check the pruned
list of columns to scan we were getting at plan-time.
We emit a notice in SeqNext() if the two column sets differ.
Currently, for all of the queries in the regression suite, no
differences are found.
We did notice that none of the current regression tests for triggers
are showing a difference between columns that can be extracted at plan
time and those that can be extracted at execution time--though we
haven't dug into this at all.
In our implementation of Approach B, we extract the columns to scan in
make_one_rel() after set_base_rel_sizes() and before
set_base_rel_pathlists() so that the scan cols can be used in costing.
We do it after calling set_base_rel_sizes() because it eventually
calls set_append_rel_size() which adds PathTarget exprs for the
partitions with Vars having the correct varattno and varno.
We added the scanCols to RangeTblEntries because it seemed like the
easiest way to make sure the information was available at scan time
(as suggested by David).
A quirk in the patch for Approach B is that, in inheritance_planner(),
we copy over the scanCols which we populated in each subroot's rtable
to the final rtable.
The rtables for all of the subroots seem to be basically unused after
finishing with inheritance_planner(). That is, many other members of
the modified child PlannerInfos are copied over to the root
PlannerInfo, but the rtables seem to be an exception.
If we want to get at them later, we would have had to go rooting
around in the pathlist of the RelOptInfo, which seemed very complex.
One note: we did not add any logic to make the extraction of scan
columns conditional on the AM. We have code to do it conditionally in
the zedstore patch, but we made it generic here.
While we were implementing this, we found that in the columns
extracted at plan-time, there were excess columns for
UPDATE/DELETE...RETURNING on partition tables.
Vars for these columns are being added to the targetlist in
preprocess_targetlist(). Eventually targetlist items are added to
RelOptInfo->reltarget exprs.
However, when result_relation is 0, this means all of the vars from
the returningList will be added to the targetlist, which seems
incorrect. We included a patch (0003) to only add the vars if
result_relation is not 0.
Adding the scanCols in RangeTblEntry, we noticed that a few other
members of RangeTblEntry are consistently initialized to NULL whenever
a new RangeTblEntry is being made.
This is confusing because makeNode() for a RangeTblEntry does
palloc0() anyway, so, why is this necessary?
If it is necessary, why not create some kind of generic initialization
function to do this?
Thanks,
Ashwin & Melanie
From 3b8515aaea164174d18ef2450b19478de8e64298 Mon Sep 17 00:00:00 2001
From: csteam <mplage...@pivotal.io>
Date: Tue, 16 Jul 2019 16:19:49 -0700
Subject: [PATCH v1 2/3] Execution-time scan col extraction and comparison with
plan-time cols
Extract the columns needed to scan directly before scanning from the
ScanState and compare this set of columns with those extracted at
plan-time.
In regress, there are two main queries where there is a difference in
the columns extracted at plan time vs execution time. They are both due
to the fact that UPDATE/DELETE on partition tables adds the contents of
the returningList to the PathTargets in the RelOptInfos. This manifests
as a difference in the column sets.
---
src/backend/executor/execScan.c | 63 ++++++++++++++++++++++++++++++
src/backend/executor/nodeSeqscan.c | 24 ++++++++++++
src/include/executor/executor.h | 4 ++
3 files changed, 91 insertions(+)
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index c0e4a5376c..89146693a3 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -20,6 +20,7 @@
#include "executor/executor.h"
#include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
#include "utils/memutils.h"
@@ -300,3 +301,65 @@ ExecScanReScan(ScanState *node)
}
}
}
+
+typedef struct neededColumnContext
+{
+ Bitmapset **mask;
+ int n;
+} neededColumnContext;
+
+static bool
+neededColumnContextWalker(Node *node, neededColumnContext *c)
+{
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, Var))
+ {
+ Var *var = (Var *)node;
+
+ if (var->varattno >= 0)
+ {
+ Assert(var->varattno <= c->n);
+ *(c->mask) = bms_add_member(*(c->mask), var->varattno);
+ }
+
+ return false;
+ }
+ return expression_tree_walker(node, neededColumnContextWalker, (void * )c);
+}
+
+/*
+ * n specifies the number of allowed entries in mask: we use
+ * it for bounds-checking in the walker above.
+ */
+void
+PopulateNeededColumnsForNode(Node *expr, int n, Bitmapset **scanCols)
+{
+ neededColumnContext c;
+
+ c.mask = scanCols;
+ c.n = n;
+
+ neededColumnContextWalker(expr, &c);
+}
+
+Bitmapset *
+PopulateNeededColumnsForScan(ScanState *scanstate, int ncol)
+{
+ Bitmapset *result = NULL;
+ Plan *plan = scanstate->ps.plan;
+
+ PopulateNeededColumnsForNode((Node *) plan->targetlist, ncol, &result);
+ PopulateNeededColumnsForNode((Node *) plan->qual, ncol, &result);
+
+ if (IsA(plan, IndexScan))
+ {
+ PopulateNeededColumnsForNode((Node *) ((IndexScan *) plan)->indexqualorig, ncol, &result);
+ PopulateNeededColumnsForNode((Node *) ((IndexScan *) plan)->indexorderbyorig, ncol, &result);
+ }
+ else if (IsA(plan, BitmapHeapScan))
+ PopulateNeededColumnsForNode((Node *) ((BitmapHeapScan *) plan)->bitmapqualorig, ncol, &result);
+
+ return result;
+}
diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index 436b43f8ca..c4b6a35554 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -64,6 +64,30 @@ SeqNext(SeqScanState *node)
if (scandesc == NULL)
{
+ /*
+ * Print used cols extracted during planning as well as
+ * used cols extracted now from the ScanState
+ */
+ int plan_col_num;
+ Bitmapset *execution_cols = NULL;
+ Scan *planNode = (Scan *)node->ss.ps.plan;
+ int ncols = node->ss.ss_currentRelation->rd_att->natts;
+ int rti = planNode->scanrelid;
+ RangeTblEntry *rangeTblEntry = list_nth(estate->es_plannedstmt->rtable, rti - 1);
+
+ Bitmapset *plan_cols = rangeTblEntry->scanCols;
+#ifdef USE_ASSERT_CHECKING
+ while ((plan_col_num = bms_next_member(plan_cols, ncols)) >= 0)
+ Assert(plan_col_num <= ncols);
+#endif
+ execution_cols = PopulateNeededColumnsForScan(&node->ss, ncols);
+
+ if (bms_is_empty(bms_difference(plan_cols, execution_cols)) == false)
+ elog(NOTICE, "table: %s.\n exec-time cols: %s\n plan-time cols: %s",
+ RelationGetRelationName(node->ss.ss_currentRelation),
+ bmsToString(execution_cols),
+ bmsToString(plan_cols));
+
/*
* We reach here if the scan is not parallel, or if we're serially
* executing a scan that was planned to be parallel.
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 1fb28b4596..310fa2c1bb 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -596,5 +596,9 @@ extern void CheckCmdReplicaIdentity(Relation rel, CmdType cmd);
extern void CheckSubscriptionRelkind(char relkind, const char *nspname,
const char *relname);
+extern void
+PopulateNeededColumnsForNode(Node *expr, int n, Bitmapset **scanCols);
+extern Bitmapset *
+PopulateNeededColumnsForScan(ScanState *scanstate, int ncol);
#endif /* EXECUTOR_H */
--
2.22.0
From 09c9bc8262df05ad178ac6b213ea215e73e3a881 Mon Sep 17 00:00:00 2001
From: csteam <mplage...@pivotal.io>
Date: Fri, 7 Jun 2019 17:26:29 -0700
Subject: [PATCH v1 1/3] Plan-time extraction of scan cols
Extract columns from query and save in the RangeTblEntry which
corresponds to the relation that will eventually be scanned.
Do this directly before costing so that this pruned list of columns
could potentially be used in costing calculations
Note that this patch does not use the scanCols.
---
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c | 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/path/allpaths.c | 60 ++++++++++++++++++++++++++-
src/backend/optimizer/plan/planner.c | 12 ++++++
src/backend/parser/parse_relation.c | 9 ++++
src/backend/rewrite/rewriteHandler.c | 2 +
src/include/nodes/parsenodes.h | 7 ++++
9 files changed, 93 insertions(+), 1 deletion(-)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 6414aded0e..2b97ba687a 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2388,6 +2388,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_BITMAPSET_FIELD(insertedCols);
COPY_BITMAPSET_FIELD(updatedCols);
COPY_BITMAPSET_FIELD(extraUpdatedCols);
+ COPY_BITMAPSET_FIELD(scanCols);
COPY_NODE_FIELD(securityQuals);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 4f2ebe5118..a57788f60a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2668,6 +2668,7 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b)
COMPARE_BITMAPSET_FIELD(insertedCols);
COMPARE_BITMAPSET_FIELD(updatedCols);
COMPARE_BITMAPSET_FIELD(extraUpdatedCols);
+ COMPARE_BITMAPSET_FIELD(scanCols);
COMPARE_NODE_FIELD(securityQuals);
return true;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8e31fae47f..e015388612 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3097,6 +3097,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_BITMAPSET_FIELD(insertedCols);
WRITE_BITMAPSET_FIELD(updatedCols);
WRITE_BITMAPSET_FIELD(extraUpdatedCols);
+ WRITE_BITMAPSET_FIELD(scanCols);
WRITE_NODE_FIELD(securityQuals);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 6c2626ee62..3ac9d67e7a 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1431,6 +1431,7 @@ _readRangeTblEntry(void)
READ_BITMAPSET_FIELD(insertedCols);
READ_BITMAPSET_FIELD(updatedCols);
READ_BITMAPSET_FIELD(extraUpdatedCols);
+ READ_BITMAPSET_FIELD(scanCols);
READ_NODE_FIELD(securityQuals);
READ_DONE();
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index e9ee32b7f4..4c437e2da1 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -142,7 +142,7 @@ static void recurse_push_qual(Node *setOp, Query *topquery,
RangeTblEntry *rte, Index rti, Node *qual);
static void remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel);
-
+static void extract_used_columns(PlannerInfo *root);
/*
* make_one_rel
* Finds all possible access paths for executing a query, returning a
@@ -184,6 +184,8 @@ make_one_rel(PlannerInfo *root, List *joinlist)
*/
set_base_rel_sizes(root);
+ extract_used_columns(root);
+
/*
* We should now have size estimates for every actual table involved in
* the query, and we also know which if any have been deleted from the
@@ -234,6 +236,62 @@ make_one_rel(PlannerInfo *root, List *joinlist)
return rel;
}
+static void
+extract_used_columns(PlannerInfo *root)
+{
+ for (int i = 1; i < root->simple_rel_array_size; i++)
+ {
+ ListCell *lc;
+ RangeTblEntry *rte = root->simple_rte_array[i];
+ RelOptInfo *rel = root->simple_rel_array[i];
+
+ if (rte == NULL)
+ continue;
+
+ if (rel == NULL)
+ continue;
+
+ rte->scanCols = NULL;
+
+ foreach(lc, rel->reltarget->exprs)
+ {
+ Node *node;
+ List *vars;
+ ListCell *lc1;
+ node = lfirst(lc);
+ /*
+ * TODO: suggest a default for vars_only to make maintenance less burdensome
+ */
+ vars = pull_var_clause(node,
+ PVC_RECURSE_AGGREGATES |
+ PVC_RECURSE_WINDOWFUNCS |
+ PVC_RECURSE_PLACEHOLDERS);
+ foreach(lc1, vars)
+ {
+ Var *var = lfirst(lc1);
+ if (var->varno == i && var->varattno >= 0)
+ rte->scanCols = bms_add_member(rte->scanCols, var->varattno);
+ }
+ }
+
+ foreach(lc, rel->baserestrictinfo)
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+ List *vars = pull_var_clause((Node *)rinfo->clause,
+ PVC_RECURSE_AGGREGATES |
+ PVC_RECURSE_WINDOWFUNCS |
+ PVC_RECURSE_PLACEHOLDERS);
+ ListCell *lc1;
+ foreach(lc1, vars)
+ {
+ Var *var = lfirst(lc1);
+ if (var->varno == i && var->varattno >= 0)
+ rte->scanCols = bms_add_member(rte->scanCols, var->varattno);
+ }
+ }
+ }
+}
+
/*
* set_base_rel_consider_startup
* Set the consider_[param_]startup flags for each base-relation entry.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index ca3b7f29e1..4f9e319d38 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1477,6 +1477,9 @@ inheritance_planner(PlannerInfo *root)
RelOptInfo *sub_final_rel;
Path *subpath;
+ ListCell *listCell;
+ int rti;
+
/*
* expand_inherited_rtentry() always processes a parent before any of
* that parent's children, so the parent query for this relation
@@ -1680,6 +1683,15 @@ inheritance_planner(PlannerInfo *root)
/* Build list of modified subroots, too */
subroots = lappend(subroots, subroot);
+ rti = 0;
+ foreach(listCell, subroot->parse->rtable)
+ {
+ RangeTblEntry *subroot_rte = lfirst(listCell);
+ RangeTblEntry *finalroot_rte = list_nth(final_rtable, rti);
+ if (finalroot_rte != subroot_rte)
+ finalroot_rte->scanCols = bms_union(finalroot_rte->scanCols, subroot_rte->scanCols);
+ rti++;
+ }
/* Build list of target-relation RT indexes */
resultRelations = lappend_int(resultRelations, appinfo->child_relid);
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 4dd81507a7..a6c51083b6 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1271,6 +1271,7 @@ addRangeTableEntry(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1343,6 +1344,7 @@ addRangeTableEntryForRelation(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1423,6 +1425,7 @@ addRangeTableEntryForSubquery(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1687,6 +1690,7 @@ addRangeTableEntryForFunction(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1751,6 +1755,7 @@ addRangeTableEntryForTableFunc(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1830,6 +1835,7 @@ addRangeTableEntryForValues(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -1901,6 +1907,7 @@ addRangeTableEntryForJoin(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -2004,6 +2011,7 @@ addRangeTableEntryForCTE(ParseState *pstate,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
rte->extraUpdatedCols = NULL;
/*
@@ -2110,6 +2118,7 @@ addRangeTableEntryForENR(ParseState *pstate,
rte->requiredPerms = 0;
rte->checkAsUser = InvalidOid;
rte->selectedCols = NULL;
+ rte->scanCols = NULL;
/*
* Add completed RTE to pstate's range table list, but not to join list
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 5b047d1662..d17dd49446 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1640,6 +1640,7 @@ ApplyRetrieveRule(Query *parsetree,
rte->selectedCols = NULL;
rte->insertedCols = NULL;
rte->updatedCols = NULL;
+ rte->scanCols = NULL;
/*
* For the most part, Vars referencing the view should remain as
@@ -1748,6 +1749,7 @@ ApplyRetrieveRule(Query *parsetree,
rte->insertedCols = NULL;
rte->updatedCols = NULL;
rte->extraUpdatedCols = NULL;
+ rte->scanCols = NULL;
return parsetree;
}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 94ded3c135..84f2d2250c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1099,7 +1099,14 @@ typedef struct RangeTblEntry
Bitmapset *insertedCols; /* columns needing INSERT permission */
Bitmapset *updatedCols; /* columns needing UPDATE permission */
Bitmapset *extraUpdatedCols; /* generated columns being updated */
+ /*
+ * Columns to be scanned.
+ * If the 0th element is set due to varattno == 0
+ * that means all columns must be scanned, so handle this at scan-time
+ */
+ Bitmapset *scanCols;
List *securityQuals; /* security barrier quals to apply, if any */
+
} RangeTblEntry;
/*
--
2.22.0
From b565386600cbd84087dc43b0ea56dc038b2562fe Mon Sep 17 00:00:00 2001
From: csteam <mplage...@pivotal.io>
Date: Tue, 16 Jul 2019 18:12:29 -0700
Subject: [PATCH v1 3/3] Avoid adding returningList for invalid result_relation
number
When result_relation is 0, this loop will add all Vars from the
returningList to the targetlist, which seems incorrect.
---
src/backend/optimizer/prep/preptlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 792ae393d9..24da3bfb6c 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -192,7 +192,7 @@ preprocess_targetlist(PlannerInfo *root)
* belong to the result rel don't need to be added, because they will be
* made to refer to the actual heap tuple.
*/
- if (parse->returningList && list_length(parse->rtable) > 1)
+ if (result_relation && parse->returningList && list_length(parse->rtable) > 1)
{
List *vars;
ListCell *l;
--
2.22.0
