Re: Extracting only the columns needed for a query
Melanie Plageman <[email protected]>
| Newsgroups | gmane.comp.db.postgresql.devel.general |
|---|---|
| Message-ID | <CAAKRu_Z355CurEzBJ1Mq3RS_dYqx3x0eUHhoOsQa+RQwHrje-w@mail.gmail.com> |
On Tue, Dec 17, 2019 at 2:57 AM Dmitry Dolgov <[email protected]> wrote: > > Thanks for the patch! If I understand correctly from this thread, > approach B is more preferable, so I've concentrated on the patch 0001 > and have a few commentaries/questions: > Thanks so much for the review! > > * The idea is to collect columns that have being used for selects/updates > (where it makes sense for columnar storage to avoid extra work), do I > see it > right? If it's the case, then scanCols could be a bit misleading, since > it > gives an impression that it's only about reads. > The "scanCols" columns are only what will need to be scanned in order to execute a query, so, even if a column is being "used", it may not be set in "scanCols" if it is not required to scan it. For example, a column which does not need to be scanned but is "used" -- e.g. in UPDATE x SET col = 2; "col" will not be in "scanCols" because it is known that it will be 2. That makes me think that maybe the function name, extract_used_columns() is bad, though. Maybe extract_scan_columns()? I tried this in the attached, updated patch. > > * After a quick experiment, it seems that extract_used_columns is invoked > for > updates, but collects too many colums, e.g: > > create table test (id int primary key, a text, b text, c text); > update test set a = 'something' where id = 1; > > collects into scanCols all columns (varattno from 1 to 4) + again the > first > column from baserestrictinfo. Is it correct? > For UPDATE, we need all of the columns in the table because of the table_lock() API's current expectation that the slot has all of the columns populated. If we want UPDATE to only need to insert the column values which have changed, table_tuple_lock() will have to change. Collecting columns from the baserestrictinfo is important for when that column isn't present in another part of the query, but it won't double count it in the bitmap (when it is already present). > > * Not sure if it supposed to be covered by this functionality, but if we do > > insert ... on conflict (uniq_col) do update set other_col = 'something' > > and actually have to perform an update, extract_used_columns is not > called. > > For UPSERT, you are correct that it will not extract scan columns. It wasn't by design. It is because that UPDATE is planned as part of an INSERT. For an INSERT, in query_planner(), because the jointree has only one relation and that relation is an RTE_RESULT planner does not continue on to make_one_rel() and thus doesn't extract scan columns. This means that for INSERT ON CONFLICT DO UPDATE, "scanCols" is not populated, however, since UPDATE needs to scan all of the columns anyway, I don't think populating "scanCols" would have any impact. This does mean that that bitmap would be different for a regular UPDATE vs an UPSERT, however, I don't think that doing the extra work to populate it makes sense if it won't be used. What do you think? > * Probably it also makes sense to check IS_DUMMY_REL in > extract_used_columns? > > I am wondering, when IS_DUMMY_REL is true for a relation, do we reference the associated RTE later? It seems like if it is a dummy rel, we wouldn't scan it. It still makes sense to add it to extract_used_columns(), I think, to avoid any wasted loops through the rel's expressions. Thanks for the idea! -- Melanie Plageman
v2-0001-Plan-time-extraction-of-scan-cols.patch
(application/octet-stream, 9.8 KB)
From dfdc22b56bc28c520ad4af9461533f3d7a5421be Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 2 Jan 2020 16:59:29 -0800 Subject: [PATCH v2] 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 | 63 ++++++++++++++++++++++++++- 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, 96 insertions(+), 1 deletion(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 8034d5a51c..84e4058aab 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2397,6 +2397,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 9c8070c640..d9749d9cc5 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2681,6 +2681,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 a53d47371b..69d52ed75f 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -3119,6 +3119,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 81e7b94b9b..a26b99ec58 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1458,6 +1458,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 8286d9cf34..10b64121e1 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_scan_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_scan_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,65 @@ make_one_rel(PlannerInfo *root, List *joinlist) return rel; } +static void +extract_scan_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; + + if (IS_DUMMY_REL(rel)) + 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 d6f2153593..40fa7a11d3 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1486,6 +1486,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 @@ -1690,6 +1693,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 ceed0ceb48..64bb9d8d28 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -1460,6 +1460,7 @@ addRangeTableEntry(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -1548,6 +1549,7 @@ addRangeTableEntryForRelation(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -1645,6 +1647,7 @@ addRangeTableEntryForSubquery(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -1921,6 +1924,7 @@ addRangeTableEntryForFunction(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -1992,6 +1996,7 @@ addRangeTableEntryForTableFunc(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -2079,6 +2084,7 @@ addRangeTableEntryForValues(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -2162,6 +2168,7 @@ addRangeTableEntryForJoin(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -2281,6 +2288,7 @@ addRangeTableEntryForCTE(ParseState *pstate, rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; + rte->scanCols = NULL; rte->extraUpdatedCols = NULL; /* @@ -2395,6 +2403,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, so that we know its diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index e9fefec8b3..f0197fa0bb 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1641,6 +1641,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 @@ -1749,6 +1750,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 f67bd9fad5..fca4b3a47c 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.20.1 (Apple Git-117)