From: Tom Lane <tgl@sss.pgh.pa.us>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: thibaut.madelaine@dalibo.com, Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15669: Error with unnest in PG 11 (ERROR: 0A000)
Date: Wed, 06 Mar 2019 15:12:15 -0500
Message-ID: <1610.1551903135@sss.pgh.pa.us> (raw)
In-Reply-To: <2250.1551896629@sss.pgh.pa.us>
References: <15669-02fb3296cca26203@postgresql.org>
<19353.1551800347@sss.pgh.pa.us>
<CAOBaU_YPJj95_9oskbobWxinOmfu3zJrNAnFD77yuzFTb6tmbA@mail.gmail.com>
<21482.1551806937@sss.pgh.pa.us>
<CAOBaU_ZE6+9YBL8htVC=hvV7Wv=Dd3i_Ze+NzW+ESuWpSsjoOw@mail.gmail.com>
<20477.1551819776@sss.pgh.pa.us>
<22437.1551824516@sss.pgh.pa.us>
<23541.1551826799@sss.pgh.pa.us>
<CAOBaU_bs7hbXxtvncLmfX34E4V8KQia6M8KSt4Lf_hapZKQtQQ@mail.gmail.com>
<2250.1551896629@sss.pgh.pa.us>
I wrote:
> I've already got a mostly-working patch. It's causing one plan change
> in select_parallel that I've not quite figured out the reason for, or
> I should say that it's not obvious why the existing code appears to
> work...
And here 'tis. I spent some time improving the existing comments, because
it's not very clear what some of this is doing or why it has to be done
that way.
I renamed IS_DUMMY_PATH to IS_DUMMY_APPEND, because almost anything that
might've been using it is probably wrong now; there's only one valid
use-case left in the core code, other than is_dummy_rel itself.
I noticed that there's no reason any more for set_dummy_rel_pathlist to
be global: it's not called from outside allpaths.c, and anything that
might want to call it would be better off using mark_dummy_rel anyway.
Also, there's no reason for is_dummy_plan to exist at all: that's a
leftover from days when recursing into a subquery produced a completed
Plan rather than some Paths. So this patch includes those cleanups,
but I don't propose back-patching those changes.
I have to leave for today, but unless somebody complains, I'm intending
to push this tomorrow.
regards, tom lane
Attachments:fix-interaction-of-dummy-paths-and-SRFs-1.patch (text/x-diff, 19.8K) downloaddiff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.cindex 0debac7..f1eed32 100644--- a/src/backend/optimizer/path/allpaths.c+++ b/src/backend/optimizer/path/allpaths.c@@ -105,6 +105,7 @@ static Path *get_cheapest_parameterized_child_path(PlannerInfo *root,
Relids required_outer);
static void accumulate_append_subpath(Path *path,
List **subpaths, List **special_subpaths);
+static void set_dummy_rel_pathlist(RelOptInfo *rel);
static void set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
Index rti, RangeTblEntry *rte);
static void set_function_pathlist(PlannerInfo *root, RelOptInfo *rel,
@@ -1557,7 +1558,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
* Consider an append of unordered, unparameterized partial paths. Make
* it parallel-aware if possible.
*/
- if (partial_subpaths_valid)+ if (partial_subpaths_valid && partial_subpaths != NIL)
{
AppendPath *appendpath;
ListCell *lc;
@@ -1954,11 +1955,16 @@ accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths)
* Build a dummy path for a relation that's been excluded by constraints
*
* Rather than inventing a special "dummy" path type, we represent this as an
- * AppendPath with no members (see also IS_DUMMY_PATH/IS_DUMMY_REL macros).- *- * This is exported because inheritance_planner() has need for it.+ * AppendPath with no members (see also IS_DUMMY_APPEND/IS_DUMMY_REL macros).+ * This is a convenient representation because it means that when we build+ * an appendrel and find that all its children have been excluded, no extra+ * action is needed to recognize the rel as dummy.+ *+ * (See also mark_dummy_rel, which does basically the same thing, but is+ * typically used to change a rel into dummy state after we already made+ * paths for it.)
*/
-void+static void
set_dummy_rel_pathlist(RelOptInfo *rel)
{
/* Set dummy size estimates --- we leave attr_widths[] as zeroes */
@@ -1969,6 +1975,12 @@ set_dummy_rel_pathlist(RelOptInfo *rel)
rel->pathlist = NIL;
rel->partial_pathlist = NIL;
+ /* Forget about child partitions, too, since they must all be dummy */+ rel->nparts = 0;+ rel->boundinfo = NULL;+ rel->part_rels = NULL;++ /* Set up the dummy path */
add_path(rel, (Path *) create_append_path(NULL, rel, NIL, NIL, NULL,
0, false, NIL, -1));
@@ -3532,12 +3544,12 @@ generate_partitionwise_join_paths(PlannerInfo *root, RelOptInfo *rel)
/* Add partitionwise join paths for partitioned child-joins. */
generate_partitionwise_join_paths(root, child_rel);
+ set_cheapest(child_rel);+
/* Dummy children will not be scanned, so ignore those. */
if (IS_DUMMY_REL(child_rel))
continue;
- set_cheapest(child_rel);-
#ifdef OPTIMIZER_DEBUG
debug_print_rel(root, child_rel);
#endif
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.cindex dfbbfda..36536bf 100644--- a/src/backend/optimizer/path/joinrels.c+++ b/src/backend/optimizer/path/joinrels.c@@ -34,7 +34,6 @@ static void make_rels_by_clauseless_joins(PlannerInfo *root,
ListCell *other_rels);
static bool has_join_restriction(PlannerInfo *root, RelOptInfo *rel);
static bool has_legal_joinclause(PlannerInfo *root, RelOptInfo *rel);
-static bool is_dummy_rel(RelOptInfo *rel);
static bool restriction_is_constant_false(List *restrictlist,
RelOptInfo *joinrel,
bool only_pushed_down);
@@ -1196,10 +1195,31 @@ have_dangerous_phv(PlannerInfo *root,
/*
* is_dummy_rel --- has relation been proven empty?
*/
-static bool+bool
is_dummy_rel(RelOptInfo *rel)
{
- return IS_DUMMY_REL(rel);+ Path *path = rel->cheapest_total_path;++ /*+ * Initially, a rel that is known dummy will have a childless Append path.+ * But in later planning stages we might stick a ProjectSetPath on top,+ * and it seems possible that a ProjectionPath could end up atop that+ * (since ProjectSetPath does not accept further layers of projection).+ * Rather than make assumptions about exactly what combinations can occur,+ * just descend through whatever we find.+ */+ while (path)+ {+ if (IsA(path, ProjectionPath))+ path = ((ProjectionPath *) path)->subpath;+ else if (IsA(path, ProjectSetPath))+ path = ((ProjectSetPath *) path)->subpath;+ else+ break;+ }+ if (path && IS_DUMMY_APPEND(path))+ return true;+ return false;
}
/*
@@ -1236,6 +1256,11 @@ mark_dummy_rel(RelOptInfo *rel)
rel->pathlist = NIL;
rel->partial_pathlist = NIL;
+ /* Forget about child partitions, too, since they must all be dummy */+ rel->nparts = 0;+ rel->boundinfo = NULL;+ rel->part_rels = NULL;+
/* Set up the dummy path */
add_path(rel, (Path *) create_append_path(NULL, rel, NIL, NIL, NULL,
0, false, NIL, -1));
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.cindex 236f506..9fbe5b2 100644--- a/src/backend/optimizer/plan/createplan.c+++ b/src/backend/optimizer/plan/createplan.c@@ -1072,7 +1072,7 @@ create_append_plan(PlannerInfo *root, AppendPath *best_path)
*
* Note that an AppendPath with no members is also generated in certain
* cases where there was no appending construct at all, but we know the
- * relation is empty (see set_dummy_rel_pathlist).+ * relation is empty (see set_dummy_rel_pathlist and mark_dummy_rel).
*/
if (best_path->subpaths == NIL)
{
@@ -6585,12 +6585,11 @@ is_projection_capable_path(Path *path)
case T_Append:
/*
- * Append can't project, but if it's being used to represent a- * dummy path, claim that it can project. This prevents us from- * converting a rel from dummy to non-dummy status by applying a- * projection to its dummy path.+ * Append can't project, but if an AppendPath is being used to+ * represent a dummy path, what will actually be generated is a+ * Result which can project.
*/
- return IS_DUMMY_PATH(path);+ return IS_DUMMY_APPEND(path);
case T_ProjectSet:
/*
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.cindex bc81535..df9c398 100644--- a/src/backend/optimizer/plan/planner.c+++ b/src/backend/optimizer/plan/planner.c@@ -1517,7 +1517,7 @@ inheritance_planner(PlannerInfo *root)
* If this child rel was excluded by constraint exclusion, exclude it
* from the result plan.
*/
- if (IS_DUMMY_PATH(subpath))+ if (IS_DUMMY_REL(sub_final_rel))
continue;
/*
@@ -2508,38 +2508,6 @@ remap_to_groupclause_idx(List *groupClause,
}
--/*- * Detect whether a plan node is a "dummy" plan created when a relation- * is deemed not to need scanning due to constraint exclusion.- *- * Currently, such dummy plans are Result nodes with constant FALSE- * filter quals (see set_dummy_rel_pathlist and create_append_plan).- *- * XXX this probably ought to be somewhere else, but not clear where.- */-bool-is_dummy_plan(Plan *plan)-{- if (IsA(plan, Result))- {- List *rcqual = (List *) ((Result *) plan)->resconstantqual;-- if (list_length(rcqual) == 1)- {- Const *constqual = (Const *) linitial(rcqual);-- if (constqual && IsA(constqual, Const))- {- if (!constqual->constisnull &&- !DatumGetBool(constqual->constvalue))- return true;- }- }- }- return false;-}-
/*
* preprocess_rowmarks - set up PlanRowMarks if needed
*/
@@ -6901,27 +6869,44 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
bool scanjoin_target_parallel_safe,
bool tlist_same_exprs)
{
- ListCell *lc;+ bool rel_is_partitioned = (rel->part_scheme && rel->part_rels);
PathTarget *scanjoin_target;
- bool is_dummy_rel = IS_DUMMY_REL(rel);+ ListCell *lc;+ /* This recurses, so be paranoid. */
check_stack_depth();
/*
+ * If the rel is partitioned, we're going to generate all-new paths here,+ * and there's no point in doing work on the existing paths. So we want+ * to drop those paths. Care is needed, though, because we have to allow+ * generate_gather_paths to see the old partial paths in the next stanza.+ * Hence, zap the main pathlist here, then allow generate_gather_paths to+ * add path(s) to the main list, and finally zap the partial pathlist.+ */+ if (rel_is_partitioned)+ rel->pathlist = NIL;++ /*
* If the scan/join target is not parallel-safe, partial paths cannot
* generate it.
*/
if (!scanjoin_target_parallel_safe)
{
/*
- * Since we can't generate the final scan/join target, this is our- * last opportunity to use any partial paths that exist. We don't do- * this if the case where the target is parallel-safe, since we will- * be able to generate superior paths by doing it after the final- * scan/join target has been applied.+ * Since we can't generate the final scan/join target in parallel+ * workers, this is our last opportunity to use any partial paths that+ * exist; so build Gather path(s) that use them and emit whatever the+ * current reltarget is. We don't do this in the case where the+ * target is parallel-safe, since we will be able to generate superior+ * paths by doing it after the final scan/join target has been+ * applied.
*
* Note that this may invalidate rel->cheapest_total_path, so we must
- * not rely on it after this point without first calling set_cheapest.+ * not rely on that path being valid until we call set_cheapest. In+ * particular, this means that IS_DUMMY_REL(rel) might not work during+ * this routine. We don't currently need that, but if in future we+ * do, save its value at the start of the function.
*/
generate_gather_paths(root, rel, false);
@@ -6930,61 +6915,27 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
rel->consider_parallel = false;
}
- /*- * Update the reltarget. This may not be strictly necessary in all cases,- * but it is at least necessary when create_append_path() gets called- * below directly or indirectly, since that function uses the reltarget as- * the pathtarget for the resulting path. It seems like a good idea to do- * it unconditionally.- */- rel->reltarget = llast_node(PathTarget, scanjoin_targets);-- /* Special case: handle dummy relations separately. */- if (is_dummy_rel)- {- /*- * Since this is a dummy rel, it's got a single Append path with no- * child paths. Replace it with a new path having the final scan/join- * target. (Note that since Append is not projection-capable, it- * would be bad to handle this using the general purpose code below;- * we'd end up putting a ProjectionPath on top of the existing Append- * node, which would cause this relation to stop appearing to be a- * dummy rel.)- */- rel->pathlist = list_make1(create_append_path(root, rel, NIL, NIL,- NULL, 0, false, NIL,- -1));+ /* Finish dropping old paths for a partitioned rel, per comment above */+ if (rel_is_partitioned)
rel->partial_pathlist = NIL;
- set_cheapest(rel);- Assert(IS_DUMMY_REL(rel));-- /*- * Forget about any child relations. There's no point in adjusting- * them and no point in using them for later planning stages (in- * particular, partitionwise aggregate).- */- rel->nparts = 0;- rel->part_rels = NULL;- rel->boundinfo = NULL;-- return;- }
/* Extract SRF-free scan/join target. */
scanjoin_target = linitial_node(PathTarget, scanjoin_targets);
/*
- * Adjust each input path. If the tlist exprs are the same, we can just- * inject the sortgroupref information into the existing pathtarget.- * Otherwise, replace each path with a projection path that generates the- * SRF-free scan/join target. This can't change the ordering of paths- * within rel->pathlist, so we just modify the list in place.+ * Apply the SRF-free scan/join target to each existing path.+ *+ * If the tlist exprs are the same, we can just inject the sortgroupref+ * information into the existing pathtargets. Otherwise, replace each+ * path with a projection path that generates the SRF-free scan/join+ * target. This can't change the ordering of paths within rel->pathlist,+ * so we just modify the list in place.
*/
foreach(lc, rel->pathlist)
{
Path *subpath = (Path *) lfirst(lc);
- Path *newpath;+ /* Shouldn't have any parameterized paths anymore */
Assert(subpath->param_info == NULL);
if (tlist_same_exprs)
@@ -6992,17 +6943,18 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
scanjoin_target->sortgrouprefs;
else
{
+ Path *newpath;+
newpath = (Path *) create_projection_path(root, rel, subpath,
scanjoin_target);
lfirst(lc) = newpath;
}
}
- /* Same for partial paths. */+ /* Likewise adjust the targets for any partial paths. */
foreach(lc, rel->partial_pathlist)
{
Path *subpath = (Path *) lfirst(lc);
- Path *newpath;
/* Shouldn't have any parameterized paths anymore */
Assert(subpath->param_info == NULL);
@@ -7012,39 +6964,53 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
scanjoin_target->sortgrouprefs;
else
{
- newpath = (Path *) create_projection_path(root,- rel,- subpath,+ Path *newpath;++ newpath = (Path *) create_projection_path(root, rel, subpath,
scanjoin_target);
lfirst(lc) = newpath;
}
}
- /* Now fix things up if scan/join target contains SRFs */+ /*+ * Now, if final scan/join target contains SRFs, insert ProjectSetPath(s)+ * atop each existing path. (Note that this function doesn't look at the+ * cheapest-path fields, which is a good thing because they're bogus right+ * now.)+ */
if (root->parse->hasTargetSRFs)
adjust_paths_for_srfs(root, rel,
scanjoin_targets,
scanjoin_targets_contain_srfs);
/*
- * If the relation is partitioned, recursively apply the same changes to- * all partitions and generate new Append paths. Since Append is not- * projection-capable, that might save a separate Result node, and it also- * is important for partitionwise aggregate.+ * Update the rel's target to be the final (with SRFs) scan/join target.+ * This now matches the actual output of all the paths, and we'll get+ * confused in createplan.c if they don't agree. We must do this now so+ * that any append paths made in the next part will use the correct+ * pathtarget (cf. create_append_path).+ */+ rel->reltarget = llast_node(PathTarget, scanjoin_targets);++ /*+ * If the relation is partitioned, recursively apply the scan/join target+ * to all partitions and generate brand-new Append paths. Since Append is+ * not projection-capable, that might save a separate Result node, and it+ * also is important for partitionwise aggregate.
*/
- if (rel->part_scheme && rel->part_rels)+ if (rel_is_partitioned)
{
- int partition_idx;
List *live_children = NIL;
+ int partition_idx;
/* Adjust each partition. */
for (partition_idx = 0; partition_idx < rel->nparts; partition_idx++)
{
RelOptInfo *child_rel = rel->part_rels[partition_idx];
- ListCell *lc;
AppendRelInfo **appinfos;
int nappinfos;
List *child_scanjoin_targets = NIL;
+ ListCell *lc;
/* Translate scan/join targets for this child. */
appinfos = find_appinfos_by_relids(root, child_rel->relids,
@@ -7076,8 +7042,7 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
}
/* Build new paths for this relation by appending child paths. */
- if (live_children != NIL)- add_paths_to_append_rel(root, rel, live_children);+ add_paths_to_append_rel(root, rel, live_children);
}
/*
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.hindex a008ae0..7a8c3f7 100644--- a/src/include/nodes/pathnodes.h+++ b/src/include/nodes/pathnodes.h@@ -1361,13 +1361,16 @@ typedef struct AppendPath
int first_partial_path;
} AppendPath;
-#define IS_DUMMY_PATH(p) \+#define IS_DUMMY_APPEND(p) \
(IsA((p), AppendPath) && ((AppendPath *) (p))->subpaths == NIL)
-/* A relation that's been proven empty will have one path that is dummy */-#define IS_DUMMY_REL(r) \- ((r)->cheapest_total_path != NULL && \- IS_DUMMY_PATH((r)->cheapest_total_path))+/*+ * A relation that's been proven empty will have one path that is dummy+ * (but might have projection paths on top). For historical reasons,+ * this is provided as a macro that wraps is_dummy_rel().+ */+#define IS_DUMMY_REL(r) is_dummy_rel(r)+extern bool is_dummy_rel(RelOptInfo *rel);
/*
* MergeAppendPath represents a MergeAppend plan, ie, the merging of sorted
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.hindex 040335a..36d12bc 100644--- a/src/include/optimizer/paths.h+++ b/src/include/optimizer/paths.h@@ -49,7 +49,6 @@ extern PGDLLIMPORT join_search_hook_type join_search_hook;
extern RelOptInfo *make_one_rel(PlannerInfo *root, List *joinlist);
-extern void set_dummy_rel_pathlist(RelOptInfo *rel);
extern RelOptInfo *standard_join_search(PlannerInfo *root, int levels_needed,
List *initial_rels);
diff --git a/src/include/optimizer/planner.h b/src/include/optimizer/planner.hindex cb41e40..830557e 100644--- a/src/include/optimizer/planner.h+++ b/src/include/optimizer/planner.h@@ -44,8 +44,6 @@ extern PlannerInfo *subquery_planner(PlannerGlobal *glob, Query *parse,
PlannerInfo *parent_root,
bool hasRecursion, double tuple_fraction);
-extern bool is_dummy_plan(Plan *plan);-
extern RowMarkType select_rowmark_type(RangeTblEntry *rte,
LockClauseStrength strength);
diff --git a/src/test/regress/expected/tsrf.out b/src/test/regress/expected/tsrf.outindex 25be6b9..d47b5f6 100644--- a/src/test/regress/expected/tsrf.out+++ b/src/test/regress/expected/tsrf.out@@ -83,6 +83,39 @@ SELECT generate_series(1, generate_series(1, 3)), generate_series(2, 4);
CREATE TABLE few(id int, dataa text, datab text);
INSERT INTO few VALUES(1, 'a', 'foo'),(2, 'a', 'bar'),(3, 'b', 'bar');
+-- SRF with a provably-dummy relation+explain (verbose, costs off)+SELECT unnest(ARRAY[1, 2]) FROM few WHERE false;+ QUERY PLAN +--------------------------------------+ ProjectSet+ Output: unnest('{1,2}'::integer[])+ -> Result+ One-Time Filter: false+(4 rows)++SELECT unnest(ARRAY[1, 2]) FROM few WHERE false;+ unnest +--------+(0 rows)++-- SRF shouldn't prevent upper query from recognizing lower as dummy+explain (verbose, costs off)+SELECT * FROM few f1,+ (SELECT unnest(ARRAY[1,2]) FROM few f2 WHERE false OFFSET 0) ss;+ QUERY PLAN +------------------------------------------------+ Result+ Output: f1.id, f1.dataa, f1.datab, ss.unnest+ One-Time Filter: false+(3 rows)++SELECT * FROM few f1,+ (SELECT unnest(ARRAY[1,2]) FROM few f2 WHERE false OFFSET 0) ss;+ id | dataa | datab | unnest +----+-------+-------+--------+(0 rows)+
-- SRF output order of sorting is maintained, if SRF is not referenced
SELECT few.id, generate_series(1,3) g FROM few ORDER BY id DESC;
id | g
diff --git a/src/test/regress/sql/tsrf.sql b/src/test/regress/sql/tsrf.sqlindex 0a1e8e5..7c22529 100644--- a/src/test/regress/sql/tsrf.sql+++ b/src/test/regress/sql/tsrf.sql@@ -28,6 +28,18 @@ SELECT generate_series(1, generate_series(1, 3)), generate_series(2, 4);
CREATE TABLE few(id int, dataa text, datab text);
INSERT INTO few VALUES(1, 'a', 'foo'),(2, 'a', 'bar'),(3, 'b', 'bar');
+-- SRF with a provably-dummy relation+explain (verbose, costs off)+SELECT unnest(ARRAY[1, 2]) FROM few WHERE false;+SELECT unnest(ARRAY[1, 2]) FROM few WHERE false;++-- SRF shouldn't prevent upper query from recognizing lower as dummy+explain (verbose, costs off)+SELECT * FROM few f1,+ (SELECT unnest(ARRAY[1,2]) FROM few f2 WHERE false OFFSET 0) ss;+SELECT * FROM few f1,+ (SELECT unnest(ARRAY[1,2]) FROM few f2 WHERE false OFFSET 0) ss;+
-- SRF output order of sorting is maintained, if SRF is not referenced
SELECT few.id, generate_series(1,3) g FROM few ORDER BY id DESC;
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, rjuju123@gmail.com, andres@anarazel.de, robertmhaas@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15669: Error with unnest in PG 11 (ERROR: 0A000)
In-Reply-To: <1610.1551903135@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox