Re: Push down more full joins in postgres_fdw

On 2016/12/05 20:20, Ashutosh Bapat wrote:
> On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2016/11/24 21:45, Etsuro Fujita wrote:
>>> * I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo
>>> and added a new member "is_subquery_rel" to fpinfo, as a flag on whether
>>> to deparse the relation as a subquery.

> Replacing make_outerrel_subquery/make_innerrel_subquery with is_subquery_rel
> seems to be a bad idea. Whether a relation needs to be deparsed as a subquery
> or not depends upon the relation which joins it with another relation. Take for
> example a case when a join ABC can pull up the conditions on AB, but ABD can
> not. In this case we will mark that AB in ABD needs to be deparsed as a
> subquery but will not mark so in ABC. So, if we choose to join ABCD as (ABC)D
> we don't need AB to be deparsed as a subquery. If we choose to join ABCD as
> (ABD)C, we will need AB to deparsed as a subquery.

This is not true; since (1) currently, a relation needs to be deparsed
as a subquery only when the relation is full-joined with any other
relation, and (2) the join ordering for full joins is preserved, we
wouldn't have such an example. (You might think we would have that when
extending the patch to handle PHVs, but the answer would be no, because
the patch for PHVs would determine whether a relation emitting PHVs
needs to be deparsed as a subquery, depending on the relation itself.
See the patch for PHVs.) I like is_subquery_rel more than
make_outerrel_subquery/make_innerrel_subquery, because it reduces the
number of members in fpinfo. But the choice would be arbitrary, so I
wouldn't object to going back to
make_outerrel_subquery/make_innerrel_subquery.

> Some more comments
> 1. The output of the following query
> +SELECT 1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t1 FULL
> JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t2 ON (TRUE);
> produces 21 rows all containing a "1". That's not quite good use of expected
> output space, given that all that the test tests is the empty
> targetlists are deparsed
> correctly. You might want to either just test EXPLAIN output or make "between 50
> and 60" condition more stringent to reduce the number of rows output.

Will do.

> 2. Got lost here. I guess, all you need to say is "test deparsing FOR UPDATE
> clause with subqueries.". Anyway, the sentence is very hard to read and needs
> simplification.
> +-- d. test deparsing the remote queries for simple foreign table scans in
> +-- an EPQ subplan of a foreign join in which the foreign tables are full
> +-- joined and in the remote join query the foreign tables are deparsed as
> +-- subqueries

Will do.

> 3. Why is foreignrel variable changed to rel?
> -extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
> - RelOptInfo *foreignrel, List *tlist,
> - List *remote_conds, List *pathkeys,
> - List **retrieved_attrs, List **params_list);
> +extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo
> *root, RelOptInfo *rel,
> + List *tlist, List *remote_conds, List *pathkeys,
> + bool is_subquery, List **retrieved_attrs,
> + List **params_list);

I changed the name that way to match with the function definition in
deparse.c.

Thanks for the comments!

Best regards,
Etsuro Fujita