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

On 2016/08/02 22:02, Kouhei Kaigai wrote:

I wrote:
>> I removed the Relations line. Here is an updated version of the patch.
>>
>> * As I said upthread, I left the upper-relation handling for another
>> patch. Currently, the patch prints "Foreign Scan" with no relations in
>> that case.
>>
>> * I kept custom joins as-is. We would need discussions about how to
>> choose relations we print in EXPLAIN, so I'd also like to leave that for
>> yet another patch.

> Please don't rely on fs_relids bitmap to pick up relations to be printed.
> It just hold a set of underlying relations, but it does not mean all of
> these relations are actually scanned inside of the ForeignScan.

Firstly, I'd like to discuss about what the relations printed after
"Foreign join" would mean. I think they would mean the relations
involved in that foreign join (in other words, the ones that participate
in that foreign join) rather than the relations to be scanned by a
Foreign Scan representing that foreign join. Wouldn't that make sense?

In that sense I think it's reasonable to print all relations specified
in fs_relids after "Foreign Join". (And in that sense I was thinking we
could handle the custom join case the same way as the foreign join case.)

> You didn't answer the following scenario I pointed out in the upthread.
>
> | Please assume an enhancement of postgres_fdw that reads a small local table (tbl_1)
> | and parse them as VALUES() clause within a remote query to execute remote join
> | with foreign tables (ftbl_2, ftbl_3).
> | This ForeignScan node has three underlying relations; tbl_1, ftbl_2 and ftbl_3.
> | Likely, tbl_1 will be scanned by SeqScan, not ForeignScan itself.
> | In this case, which relations shall be printed around ForeignScan?
> | Is it possible to choose proper relation names without hint by the extension?
> | ^^^^^^^^^^^^
>
> To care about these FDW usage, you should add an alternative bitmap rather
> than fs_relids/custom_relids. My suggestion is, having explain_relids for
> the purpose.

We currently don't allow such a join to be performed as a foreign join,
because we allow a join to be performed so if the relations of the join
are all foreign tables belonging to the same remote server, as you know.

So, as I said upthread, I think it would make sense to introduce such a
bitmap when we extend the existing foreign-join pushdown infrastructure
so that we can do such a thing as proposed by you. I guess that that
would need to extend the concept of foreign joins, though.

> Also, the logic to print "Foreign (Scan|Insert|Update|Delete)" is different
> from what I suggested. I'm suggesting to allow extension giving a label
> to fill up "Foreign %s" format.
>
> Please explain why your choice is better than my proposition.

No, I haven't done anything about that yet. I kept the behavior as-is.

> At least, my proposition is available to apply on both of foreign-scan and
> custom-scan, and no need to future maintenance if and when FDW gets support
> remote Aggregation, Sort or others.

I'd like to discuss this issue separately, maybe in a new thread.

Best regards,
Etsuro Fujita