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(dot)madelaine(at)dalibo(dot)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(at)postgresql(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> PG Bug reporting form <noreply(at)postgresql(dot)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.

On Tue, Mar 5, 2019 at 5:34 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Tue, Mar 5, 2019 at 4:39 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > PG Bug reporting form <noreply(at)postgresql(dot)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.

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> On Tue, Mar 5, 2019 at 4:39 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)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.

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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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

On Wed, Mar 6, 2019 at 7:23 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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!