Re: Oddity in EXPLAIN for foreign/custom join pushdown plans

On 2016/08/05 21:47, Robert Haas wrote:
> On Tue, Jul 26, 2016 at 11:20 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I noticed that currently the core doesn't show any information on the target
>> relations involved in a foreign/custom join in EXPLAIN, by itself.

> I think that's a feature, not a bug.

I agree with you. I'd leave that for 10.0.

>> postgres_fdw shows the target relations in the Relations line, as shown
>> above, but I think that the core should show such information independently
>> of FDWs; in the above example replace "Foreign Scan" with "Foreign Join on
>> public.ft1 t1, public.ft2 t2".

> I disagree with that. Currently, when we say that something is a join
> (Merge Join, Hash Join, Nested Loop) we mean that the executor is
> performing a join, but that's not the case here. The executor is
> performing a scan. The remote side, we suppose, is performing a join
> for us, but we are not performing a join: we are performing a scan.
> So I think the fact that it shows up in the plan as "Foreign Scan" is
> exactly right. We are scanning some foreign thing, and that thing may
> internally be doing other stuff, like joins and aggregates, but all
> we're doing is scanning it.

Hmm. One thing I'm concerned about would be the case where direct
modification is implemented by using GetForeignUpperPaths, not
PlanDirectModify. In that case, the way things are now, we would have
"Foreign Scan" followed by an INSERT/UPDATE/DELETE query, but that seems
odd to me.

> Also, I don't really see the point of moving this from postgres_fdw to
> core. If, at some point in time, there are many FDWs that implement
> sophisticated pushdown operations and we figure out that they are all
> duplicating the code to do the EXPLAIN printout, and they're all
> printing basically the same thing, perhaps not in an entirely
> consistent way, then we could try to unify all of them into one
> implementation in core. But that's certainly not where we are right
> now. I don't see any harm at all in leaving this under the control of
> the FDW, and in fact, I think it's better. Neither the postgres_fdw
> format nor what you want to replace it with are so unambiguously
> awesome that some other FDW author might not come up with something
> better.
>
> I think we should leave this the way it is.

One thing we need to do to leave that as is would be to fix a bug that I
pointed out upthred. Let me explain about that again. The EXPLAIN
command selects relation aliases to be used in printing a query so that
each selected alias is unique, but postgres_fdw wouldn't consider the
uniqueness. Here is an example:

postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1,
ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2
where t1.a = t2.a) as t(t1a, t2a);
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
Unique (cost=204.12..204.13 rows=2 width=8)
Output: t1.a, t2.a
-> Sort (cost=204.12..204.12 rows=2 width=8)
Output: t1.a, t2.a
Sort Key: t1.a, t2.a
-> Append (cost=100.00..204.11 rows=2 width=8)
-> Foreign Scan (cost=100.00..102.04 rows=1 width=8)
Output: t1.a, t2.a
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
INNER JOIN public.t2 r2 ON (((r1.a = r2.a))))
-> Foreign Scan (cost=100.00..102.04 rows=1 width=8)
Output: t1_1.a, t2_1.a
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
INNER JOIN public.t2 r2 ON (((r1.a = r2.a))))
(14 rows)

The relation aliases in the Relations line in the second Foreign Scan,
t1 and t2 for ft1 and ft2, are not unique; they should be t1_1 and t2_1
(compare the aliases in the Relations line with ones in the Output line
directly above that, created by core). The reason for that is because
postgres_fdw has created the Relations info by using
rte->eref->aliasname as relation aliases as is at path-creation time.
Probably it would be a little bit too early for postgers_fdw to do that.
Couldn't postgres_fdw create that info after query planning, for
example, during ExplainForeignScan?

Best regards,
Etsuro Fujita