Re: Push down more full joins in postgres_fdw

On 2017/01/27 20:04, Ashutosh Bapat wrote:
> On Fri, Jan 13, 2017 at 12:30 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> A more clean way I'm thinking is: (1) in
>> postgresGetForeignJoinPaths(), create a tlist by build_tlist_to_deparse()
>> and save it in fpinfo->tlist before estimate_path_cost_size() if
>> use_remote_estimates=true, (2) in estimate_path_cost_size(), use the
>> fpinfo->tlist if use_remote_estimates=true, and (3) in
>> postgresGetForeignPlan(), use the fpinfo->tlist as the fdw_scan_tlist if
>> use_remote_estimates=true, otherwise create a tlist as the fdw_scan_tlist by
>> build_tlist_to_deparse(), like the attached.
>>
>> What do you think about that?
>>
>> Another change is: I simplified build_tlist_to_deparse() a bit and put that
>> in postgres_fdw.c because that is used only in postgres_fdw.c.
>>
>> I still think we should discuss this separately because this is an existing
>> issue and that makes it easy to review the patch. If the attached is the
>> right way to go, I'll update the join-pushdown patch on top of the patch.

> I don't think it's right to assume that the targetlist will be
> available only when use_remote_estimate is true; for grouping it's
> certainly not.

My explanation was not enough. Sorry about that. My proposal described
above was for join relations, not upper relations. We build the
targetlist of an upper relation during postgresGetForeignUpperPaths, so
for grouping I think we should use that targetlist in
estimate_path_cost_size and postgresGetForeignPlan. The patch was
created that way.

> But I don't see this discussion getting anywhere. I will leave it to
> the committer's judgement.

I'm fine with that.

> I think we should pick up your patch on
> 27th December, update the comment per your mail on 5th Jan. I will
> review it once and list down the things left to committer's judgement.

Sorry, I started thinking we went in the wrong direction. I added to
deparseSelectStmtForRel build_subquery_tlists, which creates a tlist for
each subquery present in a given join tree prior to deparsing a whole
remote query. But that's nothing but an overhead; we need to create a
tlist for the top-level query because we use it as fdw_scan_tlist, but
not for subqueries, and we need to create retrieved_attrs for the
top-level query while deparsing the targetlist in
deparseExplicitTargetList, but not for subqueries. So, we should need
some work to avoid such a useless overhead.

Best regards,
Etsuro Fujita