Thread: BUG #15669: Error with unnest in PG 11 (ERROR: 0A000)
The following bug has been logged on the website:
Bug reference: 15669
Logged by: Thibaut MADELAINE
Email address: thibaut.madelaine@dalibo.com
PostgreSQL version: 11.2
Operating system: Debian
Description:
Hello,
A client found a possible bug in version 11.2.
Trying to use "unnest" on an array record with the predicate "false" fails
with the message:
ERROR: set-valued function called in context that cannot accept a set
In PostgreSQL 10.7 and before, it is possible to run the following query:
==========
thibaut=# select version();
version
-------------------------------------------------------------------------------------------------------------------
PostgreSQL 10.7 (Debian 10.7-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled
by gcc (Debian 8.2.0-16) 8.2.0, 64-bit
(1 ligne)
thibaut=# WITH test AS ( SELECT array[1,2] AS intarr )
SELECT unnest(intarr) AS lot_id FROM test WHERE false;
lot_id
--------
(0 ligne)
==========
In version 11.2, the same query fails:
==========
thibaut=# select version();
version
-------------------------------------------------------------------------------------------------------------------
PostgreSQL 11.2 (Debian 11.2-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled
by gcc (Debian 8.2.0-16) 8.2.0, 64-bit
(1 ligne)
thibaut=# \set VERBOSITY verbose
thibaut=# WITH test AS ( SELECT array[1,2] AS intarr )
SELECT unnest(intarr) AS lot_id FROM test WHERE false;
ERROR: 0A000: set-valued function called in context that cannot accept a
set
LIGNE 2 : SELECT unnest(intarr) as lot_id FROM test where false;
^
EMPLACEMENT : ExecInitFunc, execExpr.c : 2212
==========
The same query with a false predicate that needs to be evaluated succeeds:
==========
thibaut=# with test as ( SELECT array[1,2] as intarr )
SELECT unnest(intarr) as lot_id FROM test where now()<'1996-01-01';
lot_id
--------
(0 ligne)
==========
PG Bug reporting form <noreply@postgresql.org> writes:
> In PostgreSQL 10.7 and before, it is possible to run the following query:
> thibaut=# WITH test AS ( SELECT array[1,2] AS intarr )
> SELECT unnest(intarr) AS lot_id FROM test WHERE false;
> In version 11.2, the same query fails:
> ERROR: 0A000: set-valued function called in context that cannot accept a set
Hmm, that's definitely a bug. It looks like we're forgetting to make
a ProjectSet plan node for the unnest() if we realize that the query
is a no-op; but I'm not sure why 10.x doesn't have the same issue.
Digging ...
regards, tom lane
On Tue, Mar 5, 2019 at 4:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > PG Bug reporting form <noreply@postgresql.org> writes: > > In PostgreSQL 10.7 and before, it is possible to run the following query: > > > thibaut=# WITH test AS ( SELECT array[1,2] AS intarr ) > > SELECT unnest(intarr) AS lot_id FROM test WHERE false; > > > In version 11.2, the same query fails: > > ERROR: 0A000: set-valued function called in context that cannot accept a set > > Hmm, that's definitely a bug. It looks like we're forgetting to make > a ProjectSet plan node for the unnest() if we realize that the query > is a no-op; but I'm not sure why 10.x doesn't have the same issue. > Digging ... It seems to be due to 11cf92f6e2e which bypass adjust_paths_for_srfs() in case of dummy rel. I'm not familiar with that this code, but attached patch seems to fix the issue without breaking regression tests.
Attachment
On Tue, Mar 5, 2019 at 5:34 PM Julien Rouhaud <rjuju123@gmail.com> wrote: > > On Tue, Mar 5, 2019 at 4:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > PG Bug reporting form <noreply@postgresql.org> writes: > > > In PostgreSQL 10.7 and before, it is possible to run the following query: > > > > > thibaut=# WITH test AS ( SELECT array[1,2] AS intarr ) > > > SELECT unnest(intarr) AS lot_id FROM test WHERE false; > > > > > In version 11.2, the same query fails: > > > ERROR: 0A000: set-valued function called in context that cannot accept a set > > > > Hmm, that's definitely a bug. It looks like we're forgetting to make > > a ProjectSet plan node for the unnest() if we realize that the query > > is a no-op; but I'm not sure why 10.x doesn't have the same issue. > > Digging ... > > It seems to be due to 11cf92f6e2e which bypass adjust_paths_for_srfs() > in case of dummy rel. I'm not familiar with that this code, but > attached patch seems to fix the issue without breaking regression > tests. I forgot to add a regression test, fixed in v2 with a simplified test case.
Attachment
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Tue, Mar 5, 2019 at 4:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmm, that's definitely a bug. It looks like we're forgetting to make
>> a ProjectSet plan node for the unnest() if we realize that the query
>> is a no-op; but I'm not sure why 10.x doesn't have the same issue.
>> Digging ...
> It seems to be due to 11cf92f6e2e which bypass adjust_paths_for_srfs()
> in case of dummy rel. I'm not familiar with that this code, but
> attached patch seems to fix the issue without breaking regression
> tests.
Hm, yeah, that function is definitely a few bricks shy of a load.
The amount of code it's bypassing for the dummy-rel case is pretty
scary. While it doesn't look like anything except the SRF case is
actively broken, there's a lot of room there for somebody to insert
new code and not notice that they need to fix the dummy-rel path
as well. Having to duplicate the adjust_paths_for_srfs call doesn't
bode well for the future.
I'm inclined to fix it by not having the early-return path, but rather
if (is_dummy_rel)
{
// minimum possible amount of code here
}
else
{
// minimum possible amount of code here, too
}
// maximum possible amount of code here
regards, tom lane
On Tue, Mar 5, 2019 at 6:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
>
> > It seems to be due to 11cf92f6e2e which bypass adjust_paths_for_srfs()
> > in case of dummy rel. I'm not familiar with that this code, but
> > attached patch seems to fix the issue without breaking regression
> > tests.
>
> Hm, yeah, that function is definitely a few bricks shy of a load.
> The amount of code it's bypassing for the dummy-rel case is pretty
> scary. While it doesn't look like anything except the SRF case is
> actively broken, there's a lot of room there for somebody to insert
> new code and not notice that they need to fix the dummy-rel path
> as well. Having to duplicate the adjust_paths_for_srfs call doesn't
> bode well for the future.
>
> I'm inclined to fix it by not having the early-return path, but rather
>
> if (is_dummy_rel)
> {
> // minimum possible amount of code here
> }
> else
> {
> // minimum possible amount of code here, too
> }
>
> // maximum possible amount of code here
That makes sense, I was also dubious of the previous fix. Attached v3
reorganizes the code this way. The adjust_paths_for_srfs call is now
done after everything else. AFAICT it shouldn't matter, and
regression tests still seem happy with it.
Attachment
So actually, there are more things wrong here than I thought.
1. This logic is pretty stupid, if not outright wrong, for partitioned
relations. It fixes all the existing paths to have new outputs, and then
goes and generates all-new Append paths which it just adds on to the
existing paths. There isn't any situation where that makes sense; we may
as well just throw away the existing paths and then generate the new ones.
Aside from being wasteful, I wonder whether it's even correct --- if any
of the existing paths have dependencies on the old reltargets of the
children, there'll be trouble, assuming they survive the cost comparisons.
2. If we have a dummy relation, and we stick a ProjectionPath atop the
existing dummy path, it stops looking like a dummy relation, as indeed
noted in the existing comment. It's possible that nothing after this
point cares, but I would not exactly bet on that --- I think it's more
likely that we just don't have test cases exercising combinations where
there are nontrivial processing steps remaining.
The most obvious fix for #2 is to change IS_DUMMY_REL() so that it
can still return true once we've inserted SRF path steps. In HEAD
I'd be inclined to do that by backing off the premature optimization of
equating a dummy rel with something that has a dummy path, and instead
adding a separate bool is_dummy_rel flag to RelOptInfo. That doesn't
seem real safe to back-patch into v11 unfortunately. To make matters
worse, since IS_DUMMY_REL() is a macro, it could be that this broken
test is actually compiled into some third-party extensions. There's
probably little we can do about that except hope that those extensions
are not dealing with SRF-with-dummy-input cases. What I suggest we do
in v11 is change IS_DUMMY_REL() to be a subroutine that drills down
through any ProjectionPaths to see if it can find a dummy path.
(Having done that, possibly apply_scanjoin_target_to_paths could be
simplified --- I think it might not need to have a separate code
path for dummy rels anymore.)
Thoughts?
regards, tom lane
I wrote:
> 2. If we have a dummy relation, and we stick a ProjectionPath atop the
> existing dummy path, it stops looking like a dummy relation, as indeed
> noted in the existing comment. It's possible that nothing after this
> point cares, but I would not exactly bet on that --- I think it's more
> likely that we just don't have test cases exercising combinations where
> there are nontrivial processing steps remaining.
Indeed, here's a test case, using some trivial regression-test tables:
explain verbose
select * from int4_tbl,
(select unnest(array[1,2]) from int8_tbl where false offset 0) ss;
In 9.6 we figure out that the entire query must be dummy:
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.00 rows=0 width=8)
Output: int4_tbl.f1, ss.unnest
One-Time Filter: false
(3 rows)
In v10 and later, not so much:
QUERY PLAN
---------------------------------------------------------------------
Nested Loop (cost=0.00..1.10 rows=5 width=8)
Output: int4_tbl.f1, (unnest('{1,2}'::integer[]))
-> Seq Scan on public.int4_tbl (cost=0.00..1.05 rows=5 width=4)
Output: int4_tbl.f1
-> ProjectSet (cost=0.00..0.00 rows=0 width=4)
Output: unnest('{1,2}'::integer[])
-> Result (cost=0.00..0.00 rows=0 width=0)
One-Time Filter: false
(8 rows)
but if you delete the "unnest()" it's okay again:
regression=# explain verbose
select * from int4_tbl,
(select (array[1,2]) from int8_tbl where false offset 0) ss;
QUERY PLAN
-------------------------------------------
Result (cost=0.00..0.00 rows=0 width=36)
Output: int4_tbl.f1, ss."array"
One-Time Filter: false
(3 rows)
The reason for that is that the outer query level understands that
the "ss" subselect is dummy only as long as there's not a
ProjectSetPath in the way. So really this is a bug induced by the
ProjectSet patches, and we need a fix back to v10.
I'm now thinking that my hesitance to back-patch a data structure
change was misguided. We should add the bool flag to RelOptInfo
(in the back branches, being careful that it goes into alignment
padding or at the end) and redefine
#define IS_DUMMY_REL(r) ((r)->is_dummy_rel)
thus preserving source-level API compatibility. We'll still maintain
the convention that there's one dummy path, but possibly with a
ProjectSetPath on top. This means that extensions that are calling
IS_DUMMY_REL will get the wrong answer in these cases until they're
recompiled. But that really can't be helped --- my other idea
certainly wasn't better from that standpoint.
Alternatively, we could teach IS_DUMMY_PATH to recurse through
ProjectSetPath and then the IS_DUMMY_REL macro could be left alone
(though it's still broken ABI-wise, if IS_DUMMY_PATH changes).
I'd rather not do it that way, on cost grounds. It'd only matter
for direct uses of IS_DUMMY_PATH, of which there are only two in
the core code. The one in inheritance_planner should become an
IS_DUMMY_REL test anyway (though it might well be impossible to see a
ProjectSetPath there). The one in is_projection_capable_path is OK
as-is. Question is whether anyone has duplicated the
inheritance_planner coding in extensions ...
regards, tom lane
I wrote:
> In v10 and later, not so much:
BTW, I forgot to clarify that in unpatched v11 and HEAD,
this test case appears to work, but that's only because
apply_scanjoin_target_to_paths is forgetting to install the
ProjectSetPath that should be there (and then, because the outer
query now realizes that the inner one is dummy, we don't construct
a plan with any SRF calls in it, and thus avoid getting the error
reported at the top of this thread). With the patch discussed
so far, we get the same crummy plan as in v10.
I spent a little bit of time musing over whether we could avoid
inserting a ProjectSetPath at all, which would be nice because
then we'd not emit a useless ProjectSet plan node. But that
seems like it'd require replacing the SRF call with something
else --- maybe a null constant of the right type --- and that
opens up a large can of worms in terms of getting setrefs.c
to not choke. I'm doubtful that it's worth the work at all,
and I certainly wouldn't risk back-patching it.
regards, tom lane
Sorry for late answer, On Wed, Mar 6, 2019 at 12:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > In v10 and later, not so much: Ouch, I should have followed more closely the comments and code in the dummy part. > BTW, I forgot to clarify that in unpatched v11 and HEAD, > this test case appears to work, but that's only because > apply_scanjoin_target_to_paths is forgetting to install the > ProjectSetPath that should be there (and then, because the outer > query now realizes that the inner one is dummy, we don't construct > a plan with any SRF calls in it, and thus avoid getting the error > reported at the top of this thread). I see. > With the patch discussed > so far, we get the same crummy plan as in v10. > > I spent a little bit of time musing over whether we could avoid > inserting a ProjectSetPath at all, which would be nice because > then we'd not emit a useless ProjectSet plan node. But that > seems like it'd require replacing the SRF call with something > else --- maybe a null constant of the right type --- and that > opens up a large can of worms in terms of getting setrefs.c > to not choke. I'm doubtful that it's worth the work at all, > and I certainly wouldn't risk back-patching it. I agree that the RelOptInfo new flag / IS_DUMMY_REL change solution is the best solution. Let's hope there won't be too many extensions relying on the old IS_DUMMY_REL() macro. I'll work on a new version of the patch to implement it. In the meantime I'll add an entry for this bug in the next commitfest to make sure we don't miss it.
Julien Rouhaud <rjuju123@gmail.com> writes:
> I agree that the RelOptInfo new flag / IS_DUMMY_REL change solution is
> the best solution. Let's hope there won't be too many extensions
> relying on the old IS_DUMMY_REL() macro.
I concluded that that actually doesn't work very well. The reason that
things are set up the way they are, I now remember, is that if we exclude
all children of an appendrel then what we get is a childless Append path.
With the current data structure, that means the appendrel is automatically
recognized as dummy. If we have a separate flag then we'd have to fix a
fair number of places to also set the flag. I'm afraid we'd miss some,
or even more likely that there'd be extensions that would be silently
broken because that doesn't work the same anymore.
So it's now looking like the right solution is to fix IS_DUMMY_REL to
be able to drill down into the path to see if it's a dummy append with
projection(s) on top. This is also better for back-patching, assuming
that we need to worry about extensions using IS_DUMMY_REL:
(1) if an old extension is loaded into a server with the fix, it won't
recognize these corner cases as dummy, but that's no worse than before.
(2) if a recompiled extension is loaded into a server predating the fix,
it will get a link failure due to is_dummy_rel() not being exported.
That's not ideal but at least it's a pretty obvious failure mode.
With the other approach, this would lead to never recognizing dummy rels,
even in cases that worked before, since the extension would be looking at
probably-zero pad space instead of a correctly populated field. Silent
regressions are not nice.
> I'll work on a new version
> of the patch to implement it. In the meantime I'll add an entry for
> this bug in the next commitfest to make sure we don't miss it.
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...
regards, tom lane
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
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 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.c
index 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.c
index 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.c
index 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.h
index 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.h
index 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.h
index 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.out
index 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.sql
index 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;
On Wed, Mar 6, 2019 at 7:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Julien Rouhaud <rjuju123@gmail.com> writes: > > I agree that the RelOptInfo new flag / IS_DUMMY_REL change solution is > > the best solution. Let's hope there won't be too many extensions > > relying on the old IS_DUMMY_REL() macro. > > I concluded that that actually doesn't work very well. The reason that > things are set up the way they are, I now remember, is that if we exclude > all children of an appendrel then what we get is a childless Append path. Yes, I also realized that on my first try with this approach. > With the current data structure, that means the appendrel is automatically > recognized as dummy. If we have a separate flag then we'd have to fix a > fair number of places to also set the flag. I'm afraid we'd miss some, > or even more likely that there'd be extensions that would be silently > broken because that doesn't work the same anymore. I was wondering if we couldn't transform IS_DUMMY_REL to something like #define IS_DUMMY_REL(r) \ - ((r)->cheapest_total_path != NULL && \ - IS_DUMMY_PATH((r)->cheapest_total_path)) + ((r)->is_dummy_rel == true || ((r)->cheapest_total_path != NULL && \ + IS_DUMMY_PATH((r)->cheapest_total_path))) This way, any empty AppendRel would still be recognized as a dummy rel until a projection is added. We would simply have to fix create_projection_path or any function adding a projection to first explicitly set the rel->is_dummy_rel flag if it's an empty AppendRel. We shouldn't miss any spot and it should also work with extensions?
On Wed, Mar 6, 2019 at 9:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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. This all looks good to me. I'm wondering about this chunk though: + bool rel_is_partitioned = (rel->part_scheme && rel->part_rels); IIUC it' safe for now (according to f069c91a579), but should we use IS_PARTITIONED_REL macro instead? If yes, probably create_ordinary_grouping_paths() should be updated too.
Julien Rouhaud <rjuju123@gmail.com> writes:
> This all looks good to me. I'm wondering about this chunk though:
> + bool rel_is_partitioned = (rel->part_scheme && rel->part_rels);
> IIUC it' safe for now (according to f069c91a579), but should we use
> IS_PARTITIONED_REL macro instead? If yes, probably
> create_ordinary_grouping_paths() should be updated too.
Hmm, I was just copying the existing test in that function, but I agree
that it seems pretty random to not be using the macro.
Also, given that IS_PARTITIONED_REL is explicitly testing IS_DUMMY_REL,
it doesn't really seem like we need the hack about forcing nparts etc
to 0. I'd transferred that out of apply_scanjoin_target_to_paths into
set_dummy_rel_pathlist and mark_dummy_rel, but that doesn't make it any
less of an ugly kluge. In particular, that's not consistent with the
idea that an appendrel automatically becomes dummy if we generate a
zero-child path for it. So I'm now thinking we should drop that bit
and instead make sure that everyplace that's testing for partitioned-ness
is using this macro.
One more thing --- after sleeping in it, I'm questioning my earlier
feeling that apply_scanjoin_target_to_paths should flush existing paths
for partitioned rels. Maybe it was done like that with the idea of
letting paths with tlist computations done above the Append compete
with paths doing the tlist computations below? That would be a fine
idea if we had any costing factors that would produce a meaningful
choice between the two; but I'm afraid that what we're probably getting
right now is a quasi-random choice between paths whose costs differ
only by rounding errors.
regards, tom lane
On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Julien Rouhaud <rjuju123@gmail.com> writes: > > This all looks good to me. I'm wondering about this chunk though: > > > + bool rel_is_partitioned = (rel->part_scheme && rel->part_rels); > > > IIUC it' safe for now (according to f069c91a579), but should we use > > IS_PARTITIONED_REL macro instead? If yes, probably > > create_ordinary_grouping_paths() should be updated too. > > Hmm, I was just copying the existing test in that function, but I agree > that it seems pretty random to not be using the macro. > > Also, given that IS_PARTITIONED_REL is explicitly testing IS_DUMMY_REL, > it doesn't really seem like we need the hack about forcing nparts etc > to 0. I'd transferred that out of apply_scanjoin_target_to_paths into > set_dummy_rel_pathlist and mark_dummy_rel, but that doesn't make it any > less of an ugly kluge. In particular, that's not consistent with the > idea that an appendrel automatically becomes dummy if we generate a > zero-child path for it. So I'm now thinking we should drop that bit > and instead make sure that everyplace that's testing for partitioned-ness > is using this macro. I did look for other usage yesterday, and AFAICT the only remaining one is in create_ordinary_grouping_paths(), though it's already checking for !IS_DUMMY_REL: if (extra->patype != PARTITIONWISE_AGGREGATE_NONE && input_rel->part_scheme && input_rel->part_rels && !IS_DUMMY_REL(input_rel)) > One more thing --- after sleeping in it, I'm questioning my earlier > feeling that apply_scanjoin_target_to_paths should flush existing paths > for partitioned rels. Maybe it was done like that with the idea of > letting paths with tlist computations done above the Append compete > with paths doing the tlist computations below? That would be a fine > idea if we had any costing factors that would produce a meaningful > choice between the two; but I'm afraid that what we're probably getting > right now is a quasi-random choice between paths whose costs differ > only by rounding errors. I don't know and the comments surely didn't mention that. But since we're trying hard to speed up everything for high number of partitions it seems like a good idea to avoid wasting cycles here if there's no clear benefit.
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> One more thing --- after sleeping in it, I'm questioning my earlier
>> feeling that apply_scanjoin_target_to_paths should flush existing paths
>> for partitioned rels. Maybe it was done like that with the idea of
>> letting paths with tlist computations done above the Append compete
>> with paths doing the tlist computations below? That would be a fine
>> idea if we had any costing factors that would produce a meaningful
>> choice between the two; but I'm afraid that what we're probably getting
>> right now is a quasi-random choice between paths whose costs differ
>> only by rounding errors.
> I don't know and the comments surely didn't mention that. But since
> we're trying hard to speed up everything for high number of partitions
> it seems like a good idea to avoid wasting cycles here if there's no
> clear benefit.
I stuck in some debugging printouts, and it seems that at least for
cases we exercise in the regression tests, the newly-made paths are
either the same cost or cheaper than the modified old ones. Probably
that corresponds exactly to whether or not an extra Result node is
needed to evaluate the tlist when we don't push it down, but I did
not try to verify that. Anyway it seems pointless to do the extra
work, so I left the logic as I had it, with some extra commentary
about this.
I could imagine that in future we might have some costing considerations
that would make this a more interesting choice, in which case we can
just delete a few lines and do the extra cost comparisons. (One thought
that comes to mind right away is the likely extra cost of JIT'ing multiple
copies of basically the same tlist expressions.)
It's also occurred to me to make is_dummy_rel look at
linitial(rel->pathlist) rather than assuming cheapest_total_path is
valid. In that way, it will give a correct answer whether or not
we've done set_cheapest() lately. It was already a pretty large
leap of faith for apply_scanjoin_target_to_paths to invoke a lot of
other code while it knew perfectly well that is_dummy_rel would be
giving a wrong answer. Someday, something in those other functions
might try to check that; or if we didn't blow it here, we would
someplace else, given that IS_PARTITIONED_REL() is likely to get
called in more and more places.
regards, tom lane
On Thu, Mar 7, 2019 at 7:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Julien Rouhaud <rjuju123@gmail.com> writes: > > On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> One more thing --- after sleeping in it, I'm questioning my earlier > >> feeling that apply_scanjoin_target_to_paths should flush existing paths > >> for partitioned rels. Maybe it was done like that with the idea of > >> letting paths with tlist computations done above the Append compete > >> with paths doing the tlist computations below? That would be a fine > >> idea if we had any costing factors that would produce a meaningful > >> choice between the two; but I'm afraid that what we're probably getting > >> right now is a quasi-random choice between paths whose costs differ > >> only by rounding errors. > > > I don't know and the comments surely didn't mention that. But since > > we're trying hard to speed up everything for high number of partitions > > it seems like a good idea to avoid wasting cycles here if there's no > > clear benefit. > > I stuck in some debugging printouts, and it seems that at least for > cases we exercise in the regression tests, the newly-made paths are > either the same cost or cheaper than the modified old ones. Probably > that corresponds exactly to whether or not an extra Result node is > needed to evaluate the tlist when we don't push it down, but I did > not try to verify that. Anyway it seems pointless to do the extra > work, so I left the logic as I had it, with some extra commentary > about this. > > I could imagine that in future we might have some costing considerations > that would make this a more interesting choice, in which case we can > just delete a few lines and do the extra cost comparisons. (One thought > that comes to mind right away is the likely extra cost of JIT'ing multiple > copies of basically the same tlist expressions.) > > It's also occurred to me to make is_dummy_rel look at > linitial(rel->pathlist) rather than assuming cheapest_total_path is > valid. In that way, it will give a correct answer whether or not > we've done set_cheapest() lately. It was already a pretty large > leap of faith for apply_scanjoin_target_to_paths to invoke a lot of > other code while it knew perfectly well that is_dummy_rel would be > giving a wrong answer. Someday, something in those other functions > might try to check that; or if we didn't blow it here, we would > someplace else, given that IS_PARTITIONED_REL() is likely to get > called in more and more places. Thanks!